| Message ID | 1522250043-8065-4-git-send-email-tharvey@gateworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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@roeck-us.net> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > 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. > include/linux/platform_data/gsc_hwmon.h | 43 ++++ > 4 files changed, 421 insertions(+) > create mode 100644 drivers/hwmon/gsc-hwmon.c > create mode 100644 include/linux/platform_data/gsc_hwmon.h > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 7ad0176..85d9aa3 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -475,6 +475,15 @@ config SENSORS_F75375S > This driver can also be built as a module. If so, the module > will be called f75375s. > > +config SENSORS_GSC > + tristate "Gateworks System Controller ADC" > + depends on MFD_GATEWORKS_GSC > + help > + Support for the Gateworks System Controller A/D converters. > + > + To compile this driver as a module, choose M here: > + the module will be called gsc-hwmon. > + > config SENSORS_MC13783_ADC > tristate "Freescale MC13783/MC13892 ADC" > depends on MFD_MC13XXX > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index 0fe489f..835a536 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A) += g760a.o > obj-$(CONFIG_SENSORS_G762) += g762.o > obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o > obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o > +obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o > obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o > obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o > obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o > diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c > new file mode 100644 > index 0000000..10da46f > --- /dev/null > +++ b/drivers/hwmon/gsc-hwmon.c > @@ -0,0 +1,368 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (C) 2018 Gateworks Corporation > + * > + * This driver registers Linux HWMON attributes for GSC ADC's > + */ > +#include <linux/hwmon.h> > +#include <linux/hwmon-sysfs.h> > +#include <linux/mfd/gsc.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#include <linux/platform_data/gsc_hwmon.h> > + > +#define GSC_HWMON_MAX_TEMP_CH 16 > +#define GSC_HWMON_MAX_IN_CH 16 > +#define GSC_HWMON_MAX_FAN_CH 6 > + > +struct gsc_hwmon_data { > + struct gsc_dev *gsc; > + struct device *dev; > + struct gsc_hwmon_platform_data *pdata; > + const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH]; > + const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH]; > + const struct gsc_hwmon_channel *fan_ch[GSC_HWMON_MAX_FAN_CH]; > + u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1]; > + u32 in_config[GSC_HWMON_MAX_IN_CH + 1]; > + u32 fan_config[GSC_HWMON_MAX_FAN_CH + 1]; > + struct hwmon_channel_info temp_info; > + struct hwmon_channel_info in_info; > + struct hwmon_channel_info fan_info; > + const struct hwmon_channel_info *info[4]; > + struct hwmon_chip_info chip; > + int vreference; > + int resolution; > +}; > + > +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. > + > + switch (ch->type) { > + case type_temperature: > + if ((type == hwmon_temp) && *val > 0x8000) Please no unnecessary ( ). Is there ever a situation where ch->type == type_temperature and type != hwmon_temp ? Wouldn't that be a bug ? > + *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 ? 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. > + 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. > + } > + > + 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. > + 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. > + 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 ? > + 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. > + 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. > + > + /* 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@gateworks.com>"); > +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 ? > + 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 ? > + */ > +struct gsc_hwmon_platform_data { > + const struct gsc_hwmon_channel *channels; > + int nchannels; > + unsigned int resolution; > + unsigned int vreference; > +}; > + > +#endif > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 28, 2018 at 10:00 AM, Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> --- >> 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@gateworks.com>"); >> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 28, 2018 at 01:23:59PM -0700, Tim Harvey wrote: > On Wed, Mar 28, 2018 at 10:00 AM, Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net> > >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >> --- > >> 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? > As mentioned in the other thread, pwm[1-*]_auto_point[1-*]_pwm pwm[1-*]_auto_point[1-*]_temp seems to be the best fit. Downside is that we don't have those supported in the new ABI. You have two options: add the attributes to the ABI and use them, or define your own sysfs group for it. Either way is fine with me (I would prefer to have it added to the API, though). > > > >> + 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? > Again, please discuss with Rob. > > > >> + 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@gateworks.com>"); > >> +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 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 7ad0176..85d9aa3 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -475,6 +475,15 @@ config SENSORS_F75375S This driver can also be built as a module. If so, the module will be called f75375s. +config SENSORS_GSC + tristate "Gateworks System Controller ADC" + depends on MFD_GATEWORKS_GSC + help + Support for the Gateworks System Controller A/D converters. + + To compile this driver as a module, choose M here: + the module will be called gsc-hwmon. + config SENSORS_MC13783_ADC tristate "Freescale MC13783/MC13892 ADC" depends on MFD_MC13XXX diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 0fe489f..835a536 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A) += g760a.o obj-$(CONFIG_SENSORS_G762) += g762.o obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o +obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c new file mode 100644 index 0000000..10da46f --- /dev/null +++ b/drivers/hwmon/gsc-hwmon.c @@ -0,0 +1,368 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (C) 2018 Gateworks Corporation + * + * This driver registers Linux HWMON attributes for GSC ADC's + */ +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/mfd/gsc.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#include <linux/platform_data/gsc_hwmon.h> + +#define GSC_HWMON_MAX_TEMP_CH 16 +#define GSC_HWMON_MAX_IN_CH 16 +#define GSC_HWMON_MAX_FAN_CH 6 + +struct gsc_hwmon_data { + struct gsc_dev *gsc; + struct device *dev; + struct gsc_hwmon_platform_data *pdata; + const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH]; + const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH]; + const struct gsc_hwmon_channel *fan_ch[GSC_HWMON_MAX_FAN_CH]; + u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1]; + u32 in_config[GSC_HWMON_MAX_IN_CH + 1]; + u32 fan_config[GSC_HWMON_MAX_FAN_CH + 1]; + struct hwmon_channel_info temp_info; + struct hwmon_channel_info in_info; + struct hwmon_channel_info fan_info; + const struct hwmon_channel_info *info[4]; + struct hwmon_chip_info chip; + int vreference; + int resolution; +}; + +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)); + + switch (ch->type) { + case type_temperature: + if ((type == hwmon_temp) && *val > 0x8000) + *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]; + } + /* adjust by offset */ + *val += ch->voffset; + break; + } + + 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); + 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); + 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; + mode = S_IRUGO; + if (attr == hwmon_fan_input) + mode |= S_IWUSR; + break; + case hwmon_temp: + if (ch >= GSC_HWMON_MAX_TEMP_CH) + return -EOPNOTSUPP; + mode = S_IRUGO; + break; + case hwmon_in: + if (ch >= GSC_HWMON_MAX_IN_CH) + return -EOPNOTSUPP; + 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); + 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; + + /* 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@gateworks.com>"); +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; + 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 + */ +struct gsc_hwmon_platform_data { + const struct gsc_hwmon_channel *channels; + int nchannels; + unsigned int resolution; + unsigned int vreference; +}; + +#endif
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@roeck-us.net> Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- 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 ++++++++++++++++++++++++++++++++ include/linux/platform_data/gsc_hwmon.h | 43 ++++ 4 files changed, 421 insertions(+) create mode 100644 drivers/hwmon/gsc-hwmon.c create mode 100644 include/linux/platform_data/gsc_hwmon.h