From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@roeck-us.net (Guenter Roeck) Date: Fri, 15 Jul 2016 07:03:25 -0700 Subject: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon In-Reply-To: <1468576754-3273-5-git-send-email-quentin.schulz@free-electrons.com> References: <1468576754-3273-1-git-send-email-quentin.schulz@free-electrons.com> <1468576754-3273-5-git-send-email-quentin.schulz@free-electrons.com> Message-ID: <5788ED2D.9050405@roeck-us.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/15/2016 02:59 AM, Quentin Schulz wrote: > Currently, iio_hwmon only exposes values of the IIO channels it can read > but no label by channel is exposed. > > This adds exposition of sysfs files containing label for IIO channels it > can read based on extended_name field of the iio_chan_spec of the channel. > If the extended_name field is empty, the sysfs file is not created by > iio_hwmon. > > Signed-off-by: Quentin Schulz > --- > > patch added in v2 > > drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 68 insertions(+), 9 deletions(-) > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c > index c0da4d9..28d15b2 100644 > --- a/drivers/hwmon/iio_hwmon.c > +++ b/drivers/hwmon/iio_hwmon.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > > /** > @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev, > return sprintf(buf, "%d\n", result); > } > > +static ssize_t iio_hwmon_read_label(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr); > + struct iio_hwmon_state *state = dev_get_drvdata(dev); > + const char *label = state->channels[sattr->index].channel->extend_name; > + > + if (label) > + return sprintf(buf, "%s\n", label); > + Can the name disappear on the fly, or be changed on the fly ? Then this is unusable. We should and can only provide labels if a name exists and is permanent. Otherwise all we do is to confuse user space. > + return 0; > +} > + > static int iio_hwmon_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct iio_hwmon_state *st; > - struct sensor_device_attribute *a; > - int ret, i; > + struct sensor_device_attribute *a, *b; > + int ret, i, j = 0; > int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1; > enum iio_chan_type type; > struct iio_channel *channels; > @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev) > st->num_channels++; > > st->attrs = devm_kzalloc(dev, > - sizeof(*st->attrs) * (st->num_channels + 1), > + sizeof(*st->attrs) * > + (2 * st->num_channels + 1), > GFP_KERNEL); > if (st->attrs == NULL) { > ret = -ENOMEM; > @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev) > } > > sysfs_attr_init(&a->dev_attr.attr); > + > + b = NULL; > + if (st->channels[i].channel->extend_name) { > + b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL); > + if (b == NULL) { > + ret = -ENOMEM; > + goto error_release_channels; > + } > + > + sysfs_attr_init(&b->dev_attr.attr); Why is this initialization here and not with the rest of the initialization of this attribute ? > + } > + > ret = iio_get_channel_type(&st->channels[i], &type); > if (ret < 0) > goto error_release_channels; > @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev) > case IIO_VOLTAGE: > a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > "in%d_input", > - in_i++); > + in_i); > + if (b) > + b->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "in%d_label", > + in_i); > + in_i++; > break; > case IIO_TEMP: > a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > "temp%d_input", > - temp_i++); > + temp_i); > + > + if (b) > + b->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "temp%d_label", > + temp_i); > + temp_i++; > break; > case IIO_CURRENT: > a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > "curr%d_input", > - curr_i++); > + curr_i); > + > + if (b) > + b->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "curr%d_label", > + curr_i); > + curr_i++; > break; > case IIO_HUMIDITYRELATIVE: > a->dev_attr.attr.name = kasprintf(GFP_KERNEL, > "humidity%d_input", > - humidity_i++); > + humidity_i); > + > + if (b) > + b->dev_attr.attr.name = kasprintf(GFP_KERNEL, > + "humidity%d_label", > + humidity_i); > + humidity_i++; > break; > default: > ret = -EINVAL; > goto error_release_channels; > } > - if (a->dev_attr.attr.name == NULL) { > + if (a->dev_attr.attr.name == NULL || > + (b && b->dev_attr.attr.name == NULL)) { > ret = -ENOMEM; > goto error_release_channels; > } Just realized that we have a memory leak here. The 'name' memory is never released. > a->dev_attr.show = iio_hwmon_read_val; > a->dev_attr.attr.mode = S_IRUGO; > a->index = i; > - st->attrs[i] = &a->dev_attr.attr; > + st->attrs[j++] = &a->dev_attr.attr; > + > + if (b) { > + b->dev_attr.show = iio_hwmon_read_label; > + b->dev_attr.attr.mode = S_IRUGO; > + b->index = i; > + st->attrs[j++] = &b->dev_attr.attr; > + } > } > > st->attr_group.attrs = st->attrs; >