| Message ID | 1519780874-8558-4-git-send-email-tharvey@gateworks.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 02/27/2018 05:21 PM, 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. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > drivers/hwmon/Kconfig | 6 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/gsc-hwmon.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 306 insertions(+) > create mode 100644 drivers/hwmon/gsc-hwmon.c > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 7ad0176..9cdc3cb 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -475,6 +475,12 @@ 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_GSC > + help > + Support for the Gateworks System Controller A/D converters. > + > 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..3e14bea > --- /dev/null > +++ b/drivers/hwmon/gsc-hwmon.c > @@ -0,0 +1,299 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Gateworks Corporation > + */ > +#define DEBUG Please no. > + > +#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> > + > +/* map channel to channel info */ > +struct gsc_hwmon_ch { > + u8 reg; > + char name[32]; > +}; > +static struct gsc_hwmon_ch gsc_temp_ch[16]; 16 temperature channels ... > +static struct gsc_hwmon_ch gsc_in_ch[16]; > +static struct gsc_hwmon_ch gsc_fan_ch[5]; > + > +static int > +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > + int ch, long *val) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(dev); > + int sz, ret; > + u8 reg; > + u8 buf[3]; > + > + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); > + switch (type) { > + case hwmon_in: > + sz = 3; > + reg = gsc_in_ch[ch].reg; > + break; > + case hwmon_temp: > + sz = 2; > + reg = gsc_temp_ch[ch].reg; > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz); > + if (!ret) { > + *val = 0; > + while (sz-- > 0) > + *val |= (buf[sz] << (8*sz)); > + if ((type == hwmon_temp) && *val > 0x8000) Excessive [and inconsistent] ( ) Please make this if (ret) return ret; ... return 0; > + *val -= 0xffff; > + } > + > + return ret; > +} > + > +static int > +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int ch, const char **buf) > +{ > + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); > + switch (type) { > + case hwmon_in: > + case hwmon_temp: > + case hwmon_fan: > + switch (attr) { > + case hwmon_in_label: > + *buf = gsc_in_ch[ch].name; > + return 0; > + break; > + case hwmon_temp_label: > + *buf = gsc_temp_ch[ch].name; > + return 0; > + break; > + case hwmon_fan_label: > + *buf = gsc_fan_ch[ch].name; > + return 0; > + break; return followed by break doesn't make sense. > + default: > + break; > + } > + break; > + default: > + break; > + } > + > + return -ENOTSUPP; > +} > + > +static int > +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, > + int ch, long val) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(dev); > + int ret; > + u8 reg; > + u8 buf[3]; > + > + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); > + switch (type) { > + case hwmon_fan: > + buf[0] = val & 0xff; > + buf[1] = (val >> 8) & 0xff; > + reg = gsc_fan_ch[ch].reg; > + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2); &buf -> buf > + break; > + default: > + ret = -EOPNOTSUPP; > + break; > + } > + > + return ret; > +} > + > +static umode_t > +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr, > + int ch) > +{ > + const struct gsc_dev *gsc = _data; > + struct device *dev = gsc->dev; > + umode_t mode = 0; > + > + switch (type) { > + case hwmon_fan: > + if (attr == hwmon_fan_input) > + mode = (S_IRUGO | S_IWUSR); Unnecessary ( ) > + break; > + case hwmon_temp: > + case hwmon_in: > + mode = S_IRUGO; > + break; > + default: > + break; > + } > + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type, > + attr, ch, mode); > + > + return mode; > +} > + > +static u32 gsc_in_config[] = { > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + HWMON_I_INPUT, > + 0 > +}; > +static const struct hwmon_channel_info gsc_in = { > + .type = hwmon_in, > + .config = gsc_in_config, > +}; > + > +static u32 gsc_temp_config[] = { > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + HWMON_T_INPUT, > + 0 ... but this array only has 8+1 elements. This seems inconsistent. How about using some defines for array sizes ? Also, why initialize those arrays ? You are overwriting them below. You could just use a static size array instead. I assume it is guaranteed that there is only exactly one instance of this device in the system. Have you tried what happens if you declare two instances anyway ? The result must be interesting, with all those static variables. > +}; > +static const struct hwmon_channel_info gsc_temp = { > + .type = hwmon_temp, > + .config = gsc_temp_config, > +}; > + > +static u32 gsc_fan_config[] = { > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + HWMON_F_INPUT, > + 0, > +}; The matching gsc_fan_ch has only 5 entries. > +static const struct hwmon_channel_info gsc_fan = { > + .type = hwmon_fan, > + .config = gsc_fan_config, > +}; > + > +static const struct hwmon_channel_info *gsc_info[] = { > + &gsc_temp, > + &gsc_in, > + &gsc_fan, > + NULL > +}; > + > +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 const struct hwmon_chip_info gsc_chip_info = { > + .ops = &gsc_hwmon_ops, > + .info = gsc_info, > +}; > + > +static int gsc_hwmon_probe(struct platform_device *pdev) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); > + struct device_node *np; > + struct device *hwmon; > + int temp_count = 0; > + int in_count = 0; > + int fan_count = 0; > + int ret; > + > + dev_dbg(&pdev->dev, "%s\n", __func__); You declare local 'dev' variables all over the place, except here, where it would actually be used multiple times. Please either declare one here as well, or drop all the others. > + np = of_get_next_child(pdev->dev.of_node, NULL); > + while (np) { > + u32 reg, type; > + const char *label; > + > + of_property_read_u32(np, "reg", ®); > + of_property_read_u32(np, "type", &type); > + label = of_get_property(np, "label", NULL); It must be interesting to see what happens if no 'label' property is provided. Have you tried ? Also, no validation of 'reg' and 'type' ? Are you sure ? > + switch(type) { > + case 0: /* temperature sensor */ > + gsc_temp_config[temp_count] = HWMON_T_INPUT | > + HWMON_T_LABEL; > + gsc_temp_ch[temp_count].reg = reg; > + strncpy(gsc_temp_ch[temp_count].name, label, 32); This leaves the string unterminated if it is too long. Have you tested what happens in this situation ? Consider using strlcpy instead. Also please use sizeof() instead of '32'. > + if (temp_count < ARRAY_SIZE(gsc_temp_config)) > + temp_count++; I would suggest to abort with EINVAL if this happens. Otherwise the last entry is overwritten, which doesn't make much sense. Also, this accepts up to ARRAY_SIZE() entries, leaving no termination. > + break; > + case 1: /* voltage sensor */ > + gsc_in_config[in_count] = HWMON_I_INPUT | > + HWMON_I_LABEL; > + gsc_in_ch[in_count].reg = reg; So a reg value of 0xXXyy is auto-converted to 0xYY ? > + strncpy(gsc_in_ch[in_count].name, label, 32); > + if (in_count < ARRAY_SIZE(gsc_in_config)) > + in_count++; > + break; > + case 2: /* fan controller setpoint */ > + gsc_fan_config[fan_count] = HWMON_F_INPUT | > + HWMON_F_LABEL; > + gsc_fan_ch[fan_count].reg = reg; It is going to be interesting to see what happens if there are more than 5 such entries. > + strncpy(gsc_fan_ch[fan_count].name, label, 32); > + if (fan_count < ARRAY_SIZE(gsc_fan_config)) > + fan_count++; > + break; All other types are silently ignored ? > + } > + np = of_get_next_child(pdev->dev.of_node, np); > + } > + /* terminate list */ > + gsc_in_config[in_count] = 0; > + gsc_temp_config[temp_count] = 0; > + gsc_fan_config[fan_count] = 0; > + I would suggest to move above code into a separate function. > + hwmon = devm_hwmon_device_register_with_info(&pdev->dev, > + KBUILD_MODNAME, gsc, > + &gsc_chip_info, NULL); > + if (IS_ERR(hwmon)) { > + ret = PTR_ERR(hwmon); > + dev_err(&pdev->dev, "Unable to register hwmon device: %d\n", > + ret); > + return ret; > + } The error would be ENOMEM. Is it necessary to report that again ? > + > + return 0; > +} > + > +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"); > -- 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 Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote: > On 02/27/2018 05:21 PM, 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. >> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> --- >> drivers/hwmon/Kconfig | 6 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/gsc-hwmon.c | 299 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 306 insertions(+) >> create mode 100644 drivers/hwmon/gsc-hwmon.c >> >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 7ad0176..9cdc3cb 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -475,6 +475,12 @@ 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_GSC >> + help >> + Support for the Gateworks System Controller A/D converters. >> + >> 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..3e14bea >> --- /dev/null >> +++ b/drivers/hwmon/gsc-hwmon.c >> @@ -0,0 +1,299 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2018 Gateworks Corporation >> + */ >> +#define DEBUG Guenter, Thanks for the review! > > > Please no. oops - left that in by mistake. > >> + >> +#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> >> + >> +/* map channel to channel info */ >> +struct gsc_hwmon_ch { >> + u8 reg; >> + char name[32]; >> +}; >> +static struct gsc_hwmon_ch gsc_temp_ch[16]; > > > 16 temperature channels ... It has 16x ADC channels where some can be temperatures and others can be voltage inputs (based on device tree). > > >> +static struct gsc_hwmon_ch gsc_in_ch[16]; >> +static struct gsc_hwmon_ch gsc_fan_ch[5]; >> + >> +static int >> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 >> attr, >> + int ch, long *val) >> +{ >> + struct gsc_dev *gsc = dev_get_drvdata(dev); >> + int sz, ret; >> + u8 reg; >> + u8 buf[3]; >> + >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, >> ch); >> + switch (type) { >> + case hwmon_in: >> + sz = 3; >> + reg = gsc_in_ch[ch].reg; >> + break; >> + case hwmon_temp: >> + sz = 2; >> + reg = gsc_temp_ch[ch].reg; >> + break; >> + default: >> + return -EOPNOTSUPP; >> + } >> + >> + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz); >> + if (!ret) { >> + *val = 0; >> + while (sz-- > 0) >> + *val |= (buf[sz] << (8*sz)); >> + if ((type == hwmon_temp) && *val > 0x8000) > > > Excessive [and inconsistent] ( ) > > Please make this > if (ret) > return ret; > ... > return 0; understood - a much cleaner pattern > > >> + *val -= 0xffff; >> + } >> + >> + return ret; >> +} >> + >> +static int >> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, >> + u32 attr, int ch, const char **buf) >> +{ >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, >> ch); >> + switch (type) { >> + case hwmon_in: >> + case hwmon_temp: >> + case hwmon_fan: >> + switch (attr) { >> + case hwmon_in_label: >> + *buf = gsc_in_ch[ch].name; >> + return 0; >> + break; >> + case hwmon_temp_label: >> + *buf = gsc_temp_ch[ch].name; >> + return 0; >> + break; >> + case hwmon_fan_label: >> + *buf = gsc_fan_ch[ch].name; >> + return 0; >> + break; > > > return followed by break doesn't make sense. right - removed > > >> + default: >> + break; >> + } >> + break; >> + default: >> + break; >> + } >> + >> + return -ENOTSUPP; >> +} >> + >> +static int >> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 >> attr, >> + int ch, long val) >> +{ >> + struct gsc_dev *gsc = dev_get_drvdata(dev); >> + int ret; >> + u8 reg; >> + u8 buf[3]; >> + >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, >> ch); >> + switch (type) { >> + case hwmon_fan: >> + buf[0] = val & 0xff; >> + buf[1] = (val >> 8) & 0xff; >> + reg = gsc_fan_ch[ch].reg; >> + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2); > > > &buf -> buf yikes - thanks for catching that > >> + break; >> + default: >> + ret = -EOPNOTSUPP; >> + break; >> + } >> + >> + return ret; >> +} >> + >> +static umode_t >> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 >> attr, >> + int ch) >> +{ >> + const struct gsc_dev *gsc = _data; >> + struct device *dev = gsc->dev; >> + umode_t mode = 0; >> + >> + switch (type) { >> + case hwmon_fan: >> + if (attr == hwmon_fan_input) >> + mode = (S_IRUGO | S_IWUSR); > > > Unnecessary ( ) ok > > >> + break; >> + case hwmon_temp: >> + case hwmon_in: >> + mode = S_IRUGO; >> + break; >> + default: >> + break; >> + } >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, >> type, >> + attr, ch, mode); >> + >> + return mode; >> +} >> + >> +static u32 gsc_in_config[] = { >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + HWMON_I_INPUT, >> + 0 >> +}; >> +static const struct hwmon_channel_info gsc_in = { >> + .type = hwmon_in, >> + .config = gsc_in_config, >> +}; >> + >> +static u32 gsc_temp_config[] = { >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + HWMON_T_INPUT, >> + 0 > > > ... but this array only has 8+1 elements. This seems inconsistent. > How about using some defines for array sizes ? > > Also, why initialize those arrays ? You are overwriting them below. > You could just use a static size array instead. > > I assume it is guaranteed that there is only exactly one instance > of this device in the system. Have you tried what happens if you > declare two instances anyway ? The result must be interesting, > with all those static variables. yes, that static arrays are not very forward-thinking and yes my arrays are not consistent. I'll convert to dynamically allocating the channels for v2 > >> +}; >> +static const struct hwmon_channel_info gsc_temp = { >> + .type = hwmon_temp, >> + .config = gsc_temp_config, >> +}; >> + >> +static u32 gsc_fan_config[] = { >> + HWMON_F_INPUT, >> + HWMON_F_INPUT, >> + HWMON_F_INPUT, >> + HWMON_F_INPUT, >> + HWMON_F_INPUT, >> + HWMON_F_INPUT, >> + 0, >> +}; > > > The matching gsc_fan_ch has only 5 entries. right - certainly an issue > > >> +static const struct hwmon_channel_info gsc_fan = { >> + .type = hwmon_fan, >> + .config = gsc_fan_config, >> +}; >> + >> +static const struct hwmon_channel_info *gsc_info[] = { >> + &gsc_temp, >> + &gsc_in, >> + &gsc_fan, >> + NULL >> +}; >> + >> +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 const struct hwmon_chip_info gsc_chip_info = { >> + .ops = &gsc_hwmon_ops, >> + .info = gsc_info, >> +}; >> + >> +static int gsc_hwmon_probe(struct platform_device *pdev) >> +{ >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); >> + struct device_node *np; >> + struct device *hwmon; >> + int temp_count = 0; >> + int in_count = 0; >> + int fan_count = 0; >> + int ret; >> + >> + dev_dbg(&pdev->dev, "%s\n", __func__); > > > You declare local 'dev' variables all over the place, except here, > where it would actually be used multiple times. > > Please either declare one here as well, or drop all the others. will do > >> + np = of_get_next_child(pdev->dev.of_node, NULL); >> + while (np) { >> + u32 reg, type; >> + const char *label; >> + >> + of_property_read_u32(np, "reg", ®); >> + of_property_read_u32(np, "type", &type); >> + label = of_get_property(np, "label", NULL); > > > It must be interesting to see what happens if no 'label' property > is provided. Have you tried ? Also, no validation of 'reg' and 'type' ? > Are you sure ? will add validation > >> + switch(type) { >> + case 0: /* temperature sensor */ >> + gsc_temp_config[temp_count] = HWMON_T_INPUT | >> + HWMON_T_LABEL; >> + gsc_temp_ch[temp_count].reg = reg; >> + strncpy(gsc_temp_ch[temp_count].name, label, 32); > > > This leaves the string unterminated if it is too long. Have you tested > what happens in this situation ? Consider using strlcpy instead. > > Also please use sizeof() instead of '32'. ok > >> + if (temp_count < ARRAY_SIZE(gsc_temp_config)) >> + temp_count++; > > > I would suggest to abort with EINVAL if this happens. Otherwise the last > entry > is overwritten, which doesn't make much sense. Also, this accepts up to > ARRAY_SIZE() > entries, leaving no termination. > >> + break; >> + case 1: /* voltage sensor */ >> + gsc_in_config[in_count] = HWMON_I_INPUT | >> + HWMON_I_LABEL; >> + gsc_in_ch[in_count].reg = reg; > > > So a reg value of 0xXXyy is auto-converted to 0xYY ? Do you mean stuffing a u32 into a u8? > >> + strncpy(gsc_in_ch[in_count].name, label, 32); >> + if (in_count < ARRAY_SIZE(gsc_in_config)) >> + in_count++; >> + break; >> + case 2: /* fan controller setpoint */ >> + gsc_fan_config[fan_count] = HWMON_F_INPUT | >> + HWMON_F_LABEL; >> + gsc_fan_ch[fan_count].reg = reg; > > > It is going to be interesting to see what happens if there are more than > 5 such entries. will fix > >> + strncpy(gsc_fan_ch[fan_count].name, label, 32); >> + if (fan_count < ARRAY_SIZE(gsc_fan_config)) >> + fan_count++; >> + break; > > > All other types are silently ignored ? will fix > >> + } >> + np = of_get_next_child(pdev->dev.of_node, np); >> + } >> + /* terminate list */ >> + gsc_in_config[in_count] = 0; >> + gsc_temp_config[temp_count] = 0; >> + gsc_fan_config[fan_count] = 0; >> + > > I would suggest to move above code into a separate function. will do > >> + hwmon = devm_hwmon_device_register_with_info(&pdev->dev, >> + KBUILD_MODNAME, gsc, >> + &gsc_chip_info, >> NULL); >> + if (IS_ERR(hwmon)) { >> + ret = PTR_ERR(hwmon); >> + dev_err(&pdev->dev, "Unable to register hwmon device: >> %d\n", >> + ret); >> + return ret; >> + } > > > The error would be ENOMEM. Is it necessary to report that again ? could also return -EINVAL but not with the args I'm passing in so I'll change it to: return PTR_ERR_OR_ZERO(hwmon); Thanks! 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, Feb 28, 2018 at 01:44:36PM -0800, Tim Harvey wrote: > On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@roeck-us.net> wrote: > > On 02/27/2018 05:21 PM, 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. > >> > >> Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >> --- > >> drivers/hwmon/Kconfig | 6 + > >> drivers/hwmon/Makefile | 1 + > >> drivers/hwmon/gsc-hwmon.c | 299 > >> ++++++++++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 306 insertions(+) > >> create mode 100644 drivers/hwmon/gsc-hwmon.c > >> > >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > >> index 7ad0176..9cdc3cb 100644 > >> --- a/drivers/hwmon/Kconfig > >> +++ b/drivers/hwmon/Kconfig > >> @@ -475,6 +475,12 @@ 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_GSC > >> + help > >> + Support for the Gateworks System Controller A/D converters. > >> + > >> 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..3e14bea > >> --- /dev/null > >> +++ b/drivers/hwmon/gsc-hwmon.c > >> @@ -0,0 +1,299 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (C) 2018 Gateworks Corporation > >> + */ > >> +#define DEBUG > > Guenter, > > Thanks for the review! > > > > > > > Please no. > > oops - left that in by mistake. > > > > >> + > >> +#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> > >> + > >> +/* map channel to channel info */ > >> +struct gsc_hwmon_ch { > >> + u8 reg; > >> + char name[32]; > >> +}; > >> +static struct gsc_hwmon_ch gsc_temp_ch[16]; > > > > > > 16 temperature channels ... > > It has 16x ADC channels where some can be temperatures and others can > be voltage inputs (based on device tree). > That was just to point out that you use smaller arrays later on. > > > > > >> +static struct gsc_hwmon_ch gsc_in_ch[16]; > >> +static struct gsc_hwmon_ch gsc_fan_ch[5]; > >> + > >> +static int > >> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 > >> attr, > >> + int ch, long *val) > >> +{ > >> + struct gsc_dev *gsc = dev_get_drvdata(dev); > >> + int sz, ret; > >> + u8 reg; > >> + u8 buf[3]; > >> + > >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, > >> ch); > >> + switch (type) { > >> + case hwmon_in: > >> + sz = 3; > >> + reg = gsc_in_ch[ch].reg; > >> + break; > >> + case hwmon_temp: > >> + sz = 2; > >> + reg = gsc_temp_ch[ch].reg; > >> + break; > >> + default: > >> + return -EOPNOTSUPP; > >> + } > >> + > >> + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz); > >> + if (!ret) { > >> + *val = 0; > >> + while (sz-- > 0) > >> + *val |= (buf[sz] << (8*sz)); > >> + if ((type == hwmon_temp) && *val > 0x8000) > > > > > > Excessive [and inconsistent] ( ) > > > > Please make this > > if (ret) > > return ret; > > ... > > return 0; > > understood - a much cleaner pattern > > > > > > >> + *val -= 0xffff; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static int > >> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > >> + u32 attr, int ch, const char **buf) > >> +{ > >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, > >> ch); > >> + switch (type) { > >> + case hwmon_in: > >> + case hwmon_temp: > >> + case hwmon_fan: > >> + switch (attr) { > >> + case hwmon_in_label: > >> + *buf = gsc_in_ch[ch].name; > >> + return 0; > >> + break; > >> + case hwmon_temp_label: > >> + *buf = gsc_temp_ch[ch].name; > >> + return 0; > >> + break; > >> + case hwmon_fan_label: > >> + *buf = gsc_fan_ch[ch].name; > >> + return 0; > >> + break; > > > > > > return followed by break doesn't make sense. > > right - removed > > > > > > >> + default: > >> + break; > >> + } > >> + break; > >> + default: > >> + break; > >> + } > >> + > >> + return -ENOTSUPP; > >> +} > >> + > >> +static int > >> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 > >> attr, > >> + int ch, long val) > >> +{ > >> + struct gsc_dev *gsc = dev_get_drvdata(dev); > >> + int ret; > >> + u8 reg; > >> + u8 buf[3]; > >> + > >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, > >> ch); > >> + switch (type) { > >> + case hwmon_fan: > >> + buf[0] = val & 0xff; > >> + buf[1] = (val >> 8) & 0xff; > >> + reg = gsc_fan_ch[ch].reg; > >> + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2); > > > > > > &buf -> buf > > yikes - thanks for catching that > > > > >> + break; > >> + default: > >> + ret = -EOPNOTSUPP; > >> + break; > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +static umode_t > >> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 > >> attr, > >> + int ch) > >> +{ > >> + const struct gsc_dev *gsc = _data; > >> + struct device *dev = gsc->dev; > >> + umode_t mode = 0; > >> + > >> + switch (type) { > >> + case hwmon_fan: > >> + if (attr == hwmon_fan_input) > >> + mode = (S_IRUGO | S_IWUSR); > > > > > > Unnecessary ( ) > > ok > > > > > > >> + break; > >> + case hwmon_temp: > >> + case hwmon_in: > >> + mode = S_IRUGO; > >> + break; > >> + default: > >> + break; > >> + } > >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, > >> type, > >> + attr, ch, mode); > >> + > >> + return mode; > >> +} > >> + > >> +static u32 gsc_in_config[] = { > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + HWMON_I_INPUT, > >> + 0 > >> +}; > >> +static const struct hwmon_channel_info gsc_in = { > >> + .type = hwmon_in, > >> + .config = gsc_in_config, > >> +}; > >> + > >> +static u32 gsc_temp_config[] = { > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + HWMON_T_INPUT, > >> + 0 > > > > > > ... but this array only has 8+1 elements. This seems inconsistent. > > How about using some defines for array sizes ? > > > > Also, why initialize those arrays ? You are overwriting them below. > > You could just use a static size array instead. > > > > I assume it is guaranteed that there is only exactly one instance > > of this device in the system. Have you tried what happens if you > > declare two instances anyway ? The result must be interesting, > > with all those static variables. > > yes, that static arrays are not very forward-thinking and yes my > arrays are not consistent. I'll convert to dynamically allocating the > channels for v2 > > > > >> +}; > >> +static const struct hwmon_channel_info gsc_temp = { > >> + .type = hwmon_temp, > >> + .config = gsc_temp_config, > >> +}; > >> + > >> +static u32 gsc_fan_config[] = { > >> + HWMON_F_INPUT, > >> + HWMON_F_INPUT, > >> + HWMON_F_INPUT, > >> + HWMON_F_INPUT, > >> + HWMON_F_INPUT, > >> + HWMON_F_INPUT, > >> + 0, > >> +}; > > > > > > The matching gsc_fan_ch has only 5 entries. > > right - certainly an issue > > > > > > >> +static const struct hwmon_channel_info gsc_fan = { > >> + .type = hwmon_fan, > >> + .config = gsc_fan_config, > >> +}; > >> + > >> +static const struct hwmon_channel_info *gsc_info[] = { > >> + &gsc_temp, > >> + &gsc_in, > >> + &gsc_fan, > >> + NULL > >> +}; > >> + > >> +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 const struct hwmon_chip_info gsc_chip_info = { > >> + .ops = &gsc_hwmon_ops, > >> + .info = gsc_info, > >> +}; > >> + > >> +static int gsc_hwmon_probe(struct platform_device *pdev) > >> +{ > >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); > >> + struct device_node *np; > >> + struct device *hwmon; > >> + int temp_count = 0; > >> + int in_count = 0; > >> + int fan_count = 0; > >> + int ret; > >> + > >> + dev_dbg(&pdev->dev, "%s\n", __func__); > > > > > > You declare local 'dev' variables all over the place, except here, > > where it would actually be used multiple times. > > > > Please either declare one here as well, or drop all the others. > > will do > > > > >> + np = of_get_next_child(pdev->dev.of_node, NULL); > >> + while (np) { > >> + u32 reg, type; > >> + const char *label; > >> + > >> + of_property_read_u32(np, "reg", ®); > >> + of_property_read_u32(np, "type", &type); > >> + label = of_get_property(np, "label", NULL); > > > > > > It must be interesting to see what happens if no 'label' property > > is provided. Have you tried ? Also, no validation of 'reg' and 'type' ? > > Are you sure ? > > will add validation > > > > >> + switch(type) { > >> + case 0: /* temperature sensor */ > >> + gsc_temp_config[temp_count] = HWMON_T_INPUT | > >> + HWMON_T_LABEL; > >> + gsc_temp_ch[temp_count].reg = reg; > >> + strncpy(gsc_temp_ch[temp_count].name, label, 32); > > > > > > This leaves the string unterminated if it is too long. Have you tested > > what happens in this situation ? Consider using strlcpy instead. > > > > Also please use sizeof() instead of '32'. > > ok > > > > >> + if (temp_count < ARRAY_SIZE(gsc_temp_config)) > >> + temp_count++; > > > > > > I would suggest to abort with EINVAL if this happens. Otherwise the last > > entry > > is overwritten, which doesn't make much sense. Also, this accepts up to > > ARRAY_SIZE() > > entries, leaving no termination. > > > >> + break; > >> + case 1: /* voltage sensor */ > >> + gsc_in_config[in_count] = HWMON_I_INPUT | > >> + HWMON_I_LABEL; > >> + gsc_in_ch[in_count].reg = reg; > > > > > > So a reg value of 0xXXyy is auto-converted to 0xYY ? > > Do you mean stuffing a u32 into a u8? > That is what you do, isn't it ? So reg=0xffff and reg=0xfeff will both map to 0xff. Or, in other word, the code happily accepts invalid values and converts them into something else. > > > >> + strncpy(gsc_in_ch[in_count].name, label, 32); > >> + if (in_count < ARRAY_SIZE(gsc_in_config)) > >> + in_count++; > >> + break; > >> + case 2: /* fan controller setpoint */ > >> + gsc_fan_config[fan_count] = HWMON_F_INPUT | > >> + HWMON_F_LABEL; > >> + gsc_fan_ch[fan_count].reg = reg; > > > > > > It is going to be interesting to see what happens if there are more than > > 5 such entries. > > will fix > > > > >> + strncpy(gsc_fan_ch[fan_count].name, label, 32); > >> + if (fan_count < ARRAY_SIZE(gsc_fan_config)) > >> + fan_count++; > >> + break; > > > > > > All other types are silently ignored ? > > will fix > > > > >> + } > >> + np = of_get_next_child(pdev->dev.of_node, np); > >> + } > >> + /* terminate list */ > >> + gsc_in_config[in_count] = 0; > >> + gsc_temp_config[temp_count] = 0; > >> + gsc_fan_config[fan_count] = 0; > >> + > > > > I would suggest to move above code into a separate function. > > will do > > > > >> + hwmon = devm_hwmon_device_register_with_info(&pdev->dev, > >> + KBUILD_MODNAME, gsc, > >> + &gsc_chip_info, > >> NULL); > >> + if (IS_ERR(hwmon)) { > >> + ret = PTR_ERR(hwmon); > >> + dev_err(&pdev->dev, "Unable to register hwmon device: > >> %d\n", > >> + ret); > >> + return ret; > >> + } > > > > > > The error would be ENOMEM. Is it necessary to report that again ? > > could also return -EINVAL but not with the args I'm passing in so I'll > change it to: > return PTR_ERR_OR_ZERO(hwmon); > Much preferred. Thanks, Guenter -- 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..9cdc3cb 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -475,6 +475,12 @@ 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_GSC + help + Support for the Gateworks System Controller A/D converters. + 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..3e14bea --- /dev/null +++ b/drivers/hwmon/gsc-hwmon.c @@ -0,0 +1,299 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Gateworks Corporation + */ +#define DEBUG + +#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> + +/* map channel to channel info */ +struct gsc_hwmon_ch { + u8 reg; + char name[32]; +}; +static struct gsc_hwmon_ch gsc_temp_ch[16]; +static struct gsc_hwmon_ch gsc_in_ch[16]; +static struct gsc_hwmon_ch gsc_fan_ch[5]; + +static int +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, + int ch, long *val) +{ + struct gsc_dev *gsc = dev_get_drvdata(dev); + int sz, ret; + u8 reg; + u8 buf[3]; + + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); + switch (type) { + case hwmon_in: + sz = 3; + reg = gsc_in_ch[ch].reg; + break; + case hwmon_temp: + sz = 2; + reg = gsc_temp_ch[ch].reg; + break; + default: + return -EOPNOTSUPP; + } + + ret = regmap_bulk_read(gsc->regmap_hwmon, reg, &buf, sz); + if (!ret) { + *val = 0; + while (sz-- > 0) + *val |= (buf[sz] << (8*sz)); + if ((type == hwmon_temp) && *val > 0x8000) + *val -= 0xffff; + } + + return ret; +} + +static int +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int ch, const char **buf) +{ + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); + switch (type) { + case hwmon_in: + case hwmon_temp: + case hwmon_fan: + switch (attr) { + case hwmon_in_label: + *buf = gsc_in_ch[ch].name; + return 0; + break; + case hwmon_temp_label: + *buf = gsc_temp_ch[ch].name; + return 0; + break; + case hwmon_fan_label: + *buf = gsc_fan_ch[ch].name; + return 0; + break; + default: + break; + } + break; + default: + break; + } + + return -ENOTSUPP; +} + +static int +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, + int ch, long val) +{ + struct gsc_dev *gsc = dev_get_drvdata(dev); + int ret; + u8 reg; + u8 buf[3]; + + dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch); + switch (type) { + case hwmon_fan: + buf[0] = val & 0xff; + buf[1] = (val >> 8) & 0xff; + reg = gsc_fan_ch[ch].reg; + ret = regmap_bulk_write(gsc->regmap_hwmon, reg, &buf, 2); + break; + default: + ret = -EOPNOTSUPP; + break; + } + + return ret; +} + +static umode_t +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr, + int ch) +{ + const struct gsc_dev *gsc = _data; + struct device *dev = gsc->dev; + umode_t mode = 0; + + switch (type) { + case hwmon_fan: + if (attr == hwmon_fan_input) + mode = (S_IRUGO | S_IWUSR); + break; + case hwmon_temp: + case hwmon_in: + mode = S_IRUGO; + break; + default: + break; + } + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type, + attr, ch, mode); + + return mode; +} + +static u32 gsc_in_config[] = { + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + HWMON_I_INPUT, + 0 +}; +static const struct hwmon_channel_info gsc_in = { + .type = hwmon_in, + .config = gsc_in_config, +}; + +static u32 gsc_temp_config[] = { + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + HWMON_T_INPUT, + 0 +}; +static const struct hwmon_channel_info gsc_temp = { + .type = hwmon_temp, + .config = gsc_temp_config, +}; + +static u32 gsc_fan_config[] = { + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT, + HWMON_F_INPUT, + 0, +}; +static const struct hwmon_channel_info gsc_fan = { + .type = hwmon_fan, + .config = gsc_fan_config, +}; + +static const struct hwmon_channel_info *gsc_info[] = { + &gsc_temp, + &gsc_in, + &gsc_fan, + NULL +}; + +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 const struct hwmon_chip_info gsc_chip_info = { + .ops = &gsc_hwmon_ops, + .info = gsc_info, +}; + +static int gsc_hwmon_probe(struct platform_device *pdev) +{ + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent); + struct device_node *np; + struct device *hwmon; + int temp_count = 0; + int in_count = 0; + int fan_count = 0; + int ret; + + dev_dbg(&pdev->dev, "%s\n", __func__); + np = of_get_next_child(pdev->dev.of_node, NULL); + while (np) { + u32 reg, type; + const char *label; + + of_property_read_u32(np, "reg", ®); + of_property_read_u32(np, "type", &type); + label = of_get_property(np, "label", NULL); + switch(type) { + case 0: /* temperature sensor */ + gsc_temp_config[temp_count] = HWMON_T_INPUT | + HWMON_T_LABEL; + gsc_temp_ch[temp_count].reg = reg; + strncpy(gsc_temp_ch[temp_count].name, label, 32); + if (temp_count < ARRAY_SIZE(gsc_temp_config)) + temp_count++; + break; + case 1: /* voltage sensor */ + gsc_in_config[in_count] = HWMON_I_INPUT | + HWMON_I_LABEL; + gsc_in_ch[in_count].reg = reg; + strncpy(gsc_in_ch[in_count].name, label, 32); + if (in_count < ARRAY_SIZE(gsc_in_config)) + in_count++; + break; + case 2: /* fan controller setpoint */ + gsc_fan_config[fan_count] = HWMON_F_INPUT | + HWMON_F_LABEL; + gsc_fan_ch[fan_count].reg = reg; + strncpy(gsc_fan_ch[fan_count].name, label, 32); + if (fan_count < ARRAY_SIZE(gsc_fan_config)) + fan_count++; + break; + } + np = of_get_next_child(pdev->dev.of_node, np); + } + /* terminate list */ + gsc_in_config[in_count] = 0; + gsc_temp_config[temp_count] = 0; + gsc_fan_config[fan_count] = 0; + + hwmon = devm_hwmon_device_register_with_info(&pdev->dev, + KBUILD_MODNAME, gsc, + &gsc_chip_info, NULL); + if (IS_ERR(hwmon)) { + ret = PTR_ERR(hwmon); + dev_err(&pdev->dev, "Unable to register hwmon device: %d\n", + ret); + return ret; + } + + return 0; +} + +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");
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. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/hwmon/Kconfig | 6 + drivers/hwmon/Makefile | 1 + drivers/hwmon/gsc-hwmon.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 drivers/hwmon/gsc-hwmon.c