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

From: Tim Harvey
Date: Wed Feb 28 2018 - 16:44:45 EST

On Tue, Feb 27, 2018 at 6:05 PM, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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", &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