Re: [PATCH v3 3/4] hwmon: add Gateworks System Controller support

[Tim Harvey]

On Wed, Mar 28, 2018 at 10:00 AM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On Wed, Mar 28, 2018 at 08:14:02AM -0700, Tim Harvey wrote:
>> The Gateworks System Controller has a hwmon sub-component that exposes
>> up to 16 ADC's, some of which are temperature sensors, others which are
>> voltage inputs. The ADC configuration (register mapping and name) is
>> configured via device-tree and varies board to board.
>>
>> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
>> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx>
>> ---
>> v3:
>> - add voltage_raw input type and supporting fields
>> - add channel validation to is_visible function
>> - remove unnecessary channel validation from read/write functions
>>
>> v2:
>> - change license comment style
>> - remove DEBUG
>> - simplify regmap_bulk_read err check
>> - remove break after returns in switch statement
>> - fix fan setpoint buffer address
>> - remove unnecessary parens
>> - consistently use struct device *dev pointer
>> - change license/comment block
>> - add validation for hwmon child node props
>> - move parsing of of to own function
>> - use strlcpy to ensure null termination
>> - fix static array sizes and removed unnecessary initializers
>> - dynamically allocate channels
>> - fix fan input label
>> - support platform data
>> - fixed whitespace issues
>>
>>  drivers/hwmon/Kconfig                   |   9 +
>>  drivers/hwmon/Makefile                  |   1 +
>>  drivers/hwmon/gsc-hwmon.c               | 368 ++++++++++++++++++++++++++++++++
>
> This will require a matching Documentation/hwmon/gsc-hwmon to explain supported
> attributes.

ok - will add in next submission

>
<snip>
>>
>> +static int
>> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +            int channel, long *val)
>> +{
>> +     struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +     struct gsc_hwmon_platform_data *pdata = hwmon->pdata;
>> +     const struct gsc_hwmon_channel *ch;
>> +     int sz, ret;
>> +     u8 buf[3];
>> +
>> +     dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> +             channel);
>> +     switch (type) {
>> +     case hwmon_in:
>> +             ch = hwmon->in_ch[channel];
>> +             break;
>> +     case hwmon_temp:
>> +             ch = hwmon->temp_ch[channel];
>> +             break;
>> +     case hwmon_fan:
>> +             ch = hwmon->fan_ch[channel];
>> +             break;
>> +     default:
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     sz = (ch->type == type_voltage) ? 3 : 2;
>> +     ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *val = 0;
>> +     while (sz-- > 0)
>> +             *val |= (buf[sz] << (8*sz));
>
> Please use spaces before and after operators.

ok

>
>> +
>> +     switch (ch->type) {
>> +     case type_temperature:
>> +             if ((type == hwmon_temp) && *val > 0x8000)
>
> Please no unnecessary ( ).
>

ok

> Is there ever a situation where ch->type == type_temperature and type !=
> hwmon_temp ? Wouldn't that be a bug ?

I should not have been checking (type == hwmon_temp) there... will remove that.

>
>> +                     *val -= 0xffff;
>> +             break;
>> +     case type_voltage_raw:
>> +             /* scale based on ref voltage and resolution */
>> +             if (pdata->vreference && pdata->resolution) {
>> +                     *val *= pdata->vreference;
>> +                     *val /= pdata->resolution;
>> +             }
>> +             /* scale based on optional voltage divider */
>> +             if (ch->vdiv[0] && ch->vdiv[1]) {
>> +                     *val *= (ch->vdiv[0] + ch->vdiv[1]);
>> +                     *val /= ch->vdiv[1];
>> +             }
>
> This accepts both types of scaling. Is that intentional ?

yes that is intentional. One version of the GSC reports cooked
pre-scaled values that won't fall into either of these. Another
version of the GSC will report raw ADC values which will need the
vref/res scaling and those may have an optional voltage divider.

>
> I don't see any protection against overflows. What if pdata->vreference is
> larger than 256 on a system with sizeof(long) == 4 and the raw voltage
> as reported by the chip is 0xffffff ?
>
>> +             /* adjust by offset */
>> +             *val += ch->voffset;
>
> Similar to the above, this can result in an overflow on systems with
> sizeof(long) == 4.
>

true - I will add some overflow checking

>> +             break;
>
> There should be a default case as well as 'case type_voltage:'
> with a break; statement and a comment indicating that no adjustment
> is needed.
>

ok

>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int
>> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>> +                   u32 attr, int channel, const char **buf)
>> +{
>> +     struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +
>> +     dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> +             channel);
>
> I seriously wonder if all those dev_dbg() statements add value.

only to me while I was writing/testing; I will remove them

>
>> +     switch (type) {
>> +     case hwmon_in:
>> +             *buf = hwmon->in_ch[channel]->name;
>> +             break;
>> +     case hwmon_temp:
>> +             *buf = hwmon->temp_ch[channel]->name;
>> +             break;
>> +     case hwmon_fan:
>> +             *buf = hwmon->fan_ch[channel]->name;
>> +             break;
>> +     default:
>> +             return -ENOTSUPP;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int
>> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +             int channel, long val)
>> +{
>> +     struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +     u8 buf[2];
>> +
>> +     dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> +             channel);
>> +     switch (type) {
>> +     case hwmon_fan:
>> +             buf[0] = val & 0xff;
>> +             buf[1] = (val >> 8) & 0xff;
>> +             return regmap_bulk_write(hwmon->gsc->regmap_hwmon,
>> +                                      hwmon->fan_ch[channel]->reg, buf, 2);
>
> fanX_input reports the fan speed. By its nature, a fan speed is not writeable.
> I have no idea what this is supposed to achieve, but whatever it is, it is wrong.
>
> Please stick with the ABI.

ok, I see that now. Do you have any recommendation of what I should
use for these temperature setpoints that control when the fan pwm is
adjusted?

>
>> +     default:
>> +             break;
>> +     }
>> +
>> +     return -EOPNOTSUPP;
>> +}
>> +
>> +static umode_t
>> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
>> +                  int ch)
>> +{
>> +     const struct gsc_hwmon_data *hwmon = _data;
>> +     struct device *dev = hwmon->gsc->dev;
>> +     umode_t mode = 0;
>> +
>> +     switch (type) {
>> +     case hwmon_fan:
>> +             if (ch >= GSC_HWMON_MAX_FAN_CH)
>> +                     return -EOPNOTSUPP;
>
> Can that ever happen ?

No, I suppose not because the ch input comes from a hwmon node that
was created by the driver registering the entities and a check was
done there to avoid overflow. I will remove these.

>
>> +             mode = S_IRUGO;
>> +             if (attr == hwmon_fan_input)
>> +                     mode |= S_IWUSR;
>
> fanX_input is a read-only attribute per ABI.
>
>> +             break;
>> +     case hwmon_temp:
>> +             if (ch >= GSC_HWMON_MAX_TEMP_CH)
>> +                     return -EOPNOTSUPP;
>
> Can that ever happen ?
>
>> +             mode = S_IRUGO;
>> +             break;
>> +     case hwmon_in:
>> +             if (ch >= GSC_HWMON_MAX_IN_CH)
>> +                     return -EOPNOTSUPP;
>
> Can that ever happen ?
>
>> +             mode = S_IRUGO;
>> +             break;
>> +     default:
>> +             return -EOPNOTSUPP;
>> +     }
>> +     dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
>> +             attr, ch, mode);
>> +
>> +     return mode;
>> +}
>> +
>> +static const struct hwmon_ops gsc_hwmon_ops = {
>> +     .is_visible = gsc_hwmon_is_visible,
>> +     .read = gsc_hwmon_read,
>> +     .read_string = gsc_hwmon_read_string,
>> +     .write = gsc_hwmon_write,
>> +};
>> +
>> +static struct gsc_hwmon_platform_data *
>> +gsc_hwmon_get_devtree_pdata(struct device *dev)
>> +{
>> +     struct gsc_hwmon_platform_data *pdata;
>> +     struct gsc_hwmon_channel *ch;
>> +     struct fwnode_handle *child;
>> +     const char *type;
>> +     int nchannels;
>> +
>> +     nchannels = device_get_child_node_count(dev);
>> +     dev_dbg(dev, "channels=%d\n", nchannels);
>> +     if (nchannels == 0)
>> +             return ERR_PTR(-ENODEV);
>> +
>> +     pdata = devm_kzalloc(dev,
>> +                          sizeof(*pdata) + nchannels * sizeof(*ch),
>> +                          GFP_KERNEL);
>> +     if (!pdata)
>> +             return ERR_PTR(-ENOMEM);
>> +     ch = (struct gsc_hwmon_channel *)(pdata + 1);
>> +     pdata->channels = ch;
>> +     pdata->nchannels = nchannels;
>> +
>> +     device_property_read_u32(dev, "gw,reference-voltage",
>> +                              &pdata->vreference);
>> +     device_property_read_u32(dev, "gw,resolution", &pdata->resolution);
>> +
>> +     /* allocate structures for channels and count instances of each type */
>> +     device_for_each_child_node(dev, child) {
>> +             if (fwnode_property_read_string(child, "label", &ch->name)) {
>> +                     dev_err(dev, "channel without label\n");
>> +                     fwnode_handle_put(child);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>> +             if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
>> +                     dev_err(dev, "channel without reg\n");
>> +                     fwnode_handle_put(child);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>> +             if (fwnode_property_read_string(child, "type", &type)) {
>> +                     dev_err(dev, "channel without type\n");
>> +                     fwnode_handle_put(child);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>> +             if (!strcasecmp(type, "gw,hwmon-temperature"))
>> +                     ch->type = type_temperature;
>> +             else if (!strcasecmp(type, "gw,hwmon-voltage"))
>> +                     ch->type = type_voltage;
>> +             else if (!strcasecmp(type, "gw,hwmon-voltage-raw"))
>> +                     ch->type = type_voltage_raw;
>> +             else if (!strcasecmp(type, "gw,hwmon-fan"))
>> +                     ch->type = type_fan;
>> +             else {
>> +                     dev_err(dev, "channel without type\n");
>> +                     fwnode_handle_put(child);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>> +
>> +             fwnode_property_read_u32(child, "gw,voltage-offset",
>> +                     &ch->voffset);
>
> Note that while it is technically ok to keep voltages internally in mV,
> devicetree will likely require specificayion in uV. For accuracy, it might be
> better to perform any calculations on that base and convert to mV for display
> purposes.

I'm happy to change them if that really is the standard but it doesn't
look like it is a standard?

>
>> +             fwnode_property_read_u32_array(child, "gw,voltage-divider",
>> +                     ch->vdiv, ARRAY_SIZE(ch->vdiv));
>> +             dev_dbg(dev, "of: reg=0x%02x type=%d %s\n", ch->reg, ch->type,
>> +                     ch->name);
>> +             ch++;
>> +     }
>> +
>> +     return pdata;
>> +}
>> +
>> +static int gsc_hwmon_probe(struct platform_device *pdev)
>> +{
>> +     struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> +     struct device *dev = &pdev->dev;
>> +     struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
>> +     struct gsc_hwmon_data *hwmon;
>> +     int i, i_in, i_temp, i_fan;
>> +
>> +     if (!pdata) {
>> +             pdata = gsc_hwmon_get_devtree_pdata(dev);
>> +             if (IS_ERR(pdata))
>> +                     return PTR_ERR(pdata);
>> +     }
>> +
>> +     hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
>> +     if (!hwmon)
>> +             return -ENOMEM;
>> +     hwmon->gsc = gsc;
>> +     hwmon->pdata = pdata;
>> +
>> +     for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
>> +          i < hwmon->pdata->nchannels; i++) {
>> +             const struct gsc_hwmon_channel *ch = &pdata->channels[i];
>> +
>> +             if (ch->reg > GSC_HWMON_MAX_REG) {
>> +                     dev_err(dev, "invalid reg: 0x%02x\n", ch->reg);
>> +                     return -EINVAL;
>> +             }
>> +             switch (ch->type) {
>> +             case type_temperature:
>> +                     if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
>> +                             dev_err(dev, "too many temp channels\n");
>> +                             return -EINVAL;
>> +                     }
>> +                     hwmon->temp_ch[i_temp] = ch;
>> +                     hwmon->temp_config[i_temp] = HWMON_T_INPUT |
>> +                                                  HWMON_T_LABEL;
>> +                     i_temp++;
>> +                     break;
>> +             case type_voltage:
>> +             case type_voltage_raw:
>> +                     if (i_in == GSC_HWMON_MAX_IN_CH) {
>> +                             dev_err(dev, "too many voltage channels\n");
>> +                             return -EINVAL;
>> +                     }
>> +                     hwmon->in_ch[i_in] = ch;
>> +                     hwmon->in_config[i_in] =
>> +                             HWMON_I_INPUT | HWMON_I_LABEL;
>> +                     i_in++;
>> +                     break;
>> +             case type_fan:
>> +                     if (i_fan == GSC_HWMON_MAX_FAN_CH) {
>> +                             dev_err(dev, "too many voltage channels\n");
>> +                             return -EINVAL;
>> +                     }
>> +                     hwmon->fan_ch[i_fan] = ch;
>> +                     hwmon->fan_config[i_fan] =
>> +                             HWMON_F_INPUT | HWMON_F_LABEL;
>> +                     i_fan++;
>> +                     break;
>> +             default:
>> +                     dev_err(dev, "invalid type: %d\n", ch->type);
>> +                     return -EINVAL;
>> +             }
>> +             dev_dbg(dev, "pdata: reg=0x%02x type=%d %s\n", ch->reg,
>> +                     ch->type, ch->name);
>> +     }
>> +
>> +     /* terminate channel config lists */
>> +     hwmon->temp_config[i_temp] = 0;
>> +     hwmon->in_config[i_in] = 0;
>> +     hwmon->fan_config[i_fan] = 0;
>
> 'hwmon' was alocated with devm_kzalloc(). Initializing any of its members with 0
> is unnecessary.

right - will remove

>
>> +
>> +     /* setup config structures */
>> +     hwmon->chip.ops = &gsc_hwmon_ops;
>> +     hwmon->chip.info = hwmon->info;
>> +     hwmon->info[0] = &hwmon->temp_info;
>> +     hwmon->info[1] = &hwmon->in_info;
>> +     hwmon->info[2] = &hwmon->fan_info;
>> +     hwmon->temp_info.type = hwmon_temp;
>> +     hwmon->temp_info.config = hwmon->temp_config;
>> +     hwmon->in_info.type = hwmon_in;
>> +     hwmon->in_info.config = hwmon->in_config;
>> +     hwmon->fan_info.type = hwmon_fan;
>> +     hwmon->fan_info.config = hwmon->fan_config;
>> +
>> +     hwmon->dev = devm_hwmon_device_register_with_info(dev,
>> +                                                       KBUILD_MODNAME, hwmon,
>> +                                                       &hwmon->chip, NULL);
>> +     return PTR_ERR_OR_ZERO(hwmon->dev);
>> +}
>> +
>> +static const struct of_device_id gsc_hwmon_of_match[] = {
>> +     { .compatible = "gw,gsc-hwmon", },
>> +     {}
>> +};
>> +
>> +static struct platform_driver gsc_hwmon_driver = {
>> +     .driver = {
>> +             .name = KBUILD_MODNAME,
>> +             .of_match_table = gsc_hwmon_of_match,
>> +     },
>> +     .probe = gsc_hwmon_probe,
>> +};
>> +
>> +module_platform_driver(gsc_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Tim Harvey <tharvey@xxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("GSC hardware monitor driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
>> new file mode 100644
>> index 0000000..5e59846
>> --- /dev/null
>> +++ b/include/linux/platform_data/gsc_hwmon.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _GSC_HWMON_H
>> +#define _GSC_HWMON_H
>> +
>> +enum gsc_hwmon_type {
>> +     type_temperature,
>> +     type_voltage,
>> +     type_voltage_raw,
>> +     type_fan,
>> +};
>> +
>> +/**
>> + * struct gsc_hwmon_channel - configuration parameters
>> + * @reg:  I2C register offset
>> + * @type: channel type
>> + * @name: channel name
>> + * @voffset: voltage offset (mV)
>> + * @vdiv: voltage divider array (2 resistor values in ohms)
>> + */
>> +struct gsc_hwmon_channel {
>> +     unsigned int reg;
>> +     unsigned int type;
>> +     const char *name;
>> +     unsigned int voffset;
>
> It is interesting that only positive offset values are supported.
> Is that intentional ?

yes, they are only used for voltage rails that have a diode drop to
adjust for and will only be positive. They are not used to fine tune
offsets.

>
>> +     unsigned int vdiv[2];
>> +};
>> +
>> +/**
>> + * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
>> + * @channels:        pointer to array of gsc_hwmon_channel structures
>> + *           describing channels
>> + * @nchannels:       number of elements in @channels array
>> + * @vreference: voltage reference (mV)
>> + * @resolution: ADC resolution
>
> Resolution in what ?

That's the bit-resolution of the ADC so I'll switch to that notation
and document it as such. I just realized that the vref/resolution can
technically change per ADC rail so I'll leave them in but move them
down into the child nodes and switch resolution to be in bits.

Thanks for the review!

Tim