From: Jonathan Cameron <jic23@kernel.org> To: Quentin Schulz <quentin.schulz@free-electrons.com>, jdelvare@suse.com, linux@roeck-us.net, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, maxime.ripard@free-electrons.com, wens@csie.org, lee.jones@linaro.org Cc: linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, thomas.petazzoni@free-electrons.com, antoine.tenart@free-electrons.com Subject: Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon Date: Mon, 18 Jul 2016 13:24:23 +0100 [thread overview] Message-ID: <aa889446-d68c-a05f-83c1-4d54fe814000@kernel.org> (raw) In-Reply-To: <1468576754-3273-5-git-send-email-quentin.schulz@free-electrons.com> On 15/07/16 10:59, 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. Hmm. This is not the intent of extended name at all. That exists to add a small amount of information to an constructed IIO channel name. Typically it's used to indicate physically wired stuff like: in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired to the vdd. In this particular case the use might actually work as the vdd makes it clear it's a voltage - in general that's not the case. Use of extended_name at all in IIO is only done after extensive review. It adds nasty custom ABI to a device, so the gain has to be considerable to use it. When I read your original suggestion of adding labels, I was expecting the use of datasheet_name. That has the advantage of being well defined by the datasheet (if not it should not be provided) + not being used in the construction of the IIO channel related attributes. However, that may still not correspond well to the expected labelling in hwmon. Thinking more on this, the label is going to often be a function of how the board is wired up... Perhaps it should be a characteristic of the channel_map (hence from DT or similar) rather than part of the IIO driver itself? At first glance hwmon labels appear to be pretty freeform... However we need to be very careful here as this is effectively defining a large chunk of new ABI. This isn't a thing that I have a particularly clear view on (as you might be able to tell ;). Other opinions sought! Jonathan > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > > 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 <linux/of.h> > #include <linux/hwmon-sysfs.h> > #include <linux/iio/consumer.h> > +#include <linux/iio/iio.h> > #include <linux/iio/types.h> > > /** > @@ -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); > + > + 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); > + } > + > 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; > } > 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; >
WARNING: multiple messages have this Message-ID (diff)
From: jic23@kernel.org (Jonathan Cameron) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon Date: Mon, 18 Jul 2016 13:24:23 +0100 [thread overview] Message-ID: <aa889446-d68c-a05f-83c1-4d54fe814000@kernel.org> (raw) In-Reply-To: <1468576754-3273-5-git-send-email-quentin.schulz@free-electrons.com> On 15/07/16 10:59, 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. Hmm. This is not the intent of extended name at all. That exists to add a small amount of information to an constructed IIO channel name. Typically it's used to indicate physically wired stuff like: in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired to the vdd. In this particular case the use might actually work as the vdd makes it clear it's a voltage - in general that's not the case. Use of extended_name at all in IIO is only done after extensive review. It adds nasty custom ABI to a device, so the gain has to be considerable to use it. When I read your original suggestion of adding labels, I was expecting the use of datasheet_name. That has the advantage of being well defined by the datasheet (if not it should not be provided) + not being used in the construction of the IIO channel related attributes. However, that may still not correspond well to the expected labelling in hwmon. Thinking more on this, the label is going to often be a function of how the board is wired up... Perhaps it should be a characteristic of the channel_map (hence from DT or similar) rather than part of the IIO driver itself? At first glance hwmon labels appear to be pretty freeform... However we need to be very careful here as this is effectively defining a large chunk of new ABI. This isn't a thing that I have a particularly clear view on (as you might be able to tell ;). Other opinions sought! Jonathan > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > > 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 <linux/of.h> > #include <linux/hwmon-sysfs.h> > #include <linux/iio/consumer.h> > +#include <linux/iio/iio.h> > #include <linux/iio/types.h> > > /** > @@ -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); > + > + 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); > + } > + > 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; > } > 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; >
next prev parent reply other threads:[~2016-07-18 12:24 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-15 9:59 [PATCH v2 0/4] add support for Allwinner SoCs ADC Quentin Schulz 2016-07-15 9:59 ` Quentin Schulz 2016-07-15 9:59 ` [PATCH v2 1/4] hwmon: iio_hwmon: defer probe when no channel is found Quentin Schulz 2016-07-15 9:59 ` Quentin Schulz 2016-07-16 17:00 ` [v2,1/4] " Guenter Roeck 2016-07-16 17:00 ` Guenter Roeck 2016-07-18 10:02 ` Maxime Ripard 2016-07-18 10:02 ` Maxime Ripard 2016-07-18 13:29 ` Guenter Roeck 2016-07-18 13:29 ` Guenter Roeck 2016-07-15 9:59 ` [PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC Quentin Schulz 2016-07-15 9:59 ` Quentin Schulz 2016-07-18 12:57 ` Maxime Ripard 2016-07-18 12:57 ` Maxime Ripard 2016-07-19 9:04 ` Quentin Schulz 2016-07-19 9:04 ` Quentin Schulz 2016-07-19 12:40 ` Maxime Ripard 2016-07-19 12:40 ` Maxime Ripard 2016-07-18 13:18 ` Jonathan Cameron 2016-07-18 13:18 ` Jonathan Cameron 2016-07-19 8:33 ` Quentin Schulz 2016-07-19 8:33 ` Quentin Schulz 2016-07-20 14:57 ` Jonathan Cameron 2016-07-20 14:57 ` Jonathan Cameron 2016-07-21 12:15 ` Quentin Schulz 2016-07-21 12:15 ` Quentin Schulz 2016-07-23 6:37 ` Jonathan Cameron 2016-07-23 6:37 ` Jonathan Cameron 2016-07-20 12:37 ` Quentin Schulz 2016-07-20 12:37 ` Quentin Schulz 2016-07-20 14:15 ` Crt Mori 2016-07-20 14:15 ` Crt Mori 2016-07-20 14:59 ` Jonathan Cameron 2016-07-20 14:59 ` Jonathan Cameron 2016-07-15 9:59 ` [PATCH v2 3/4] mfd: " Quentin Schulz 2016-07-15 9:59 ` Quentin Schulz 2016-07-18 13:02 ` Maxime Ripard 2016-07-18 13:02 ` Maxime Ripard 2016-07-19 12:04 ` Quentin Schulz 2016-07-19 12:04 ` Quentin Schulz 2016-07-18 13:25 ` Jonathan Cameron 2016-07-18 13:25 ` Jonathan Cameron 2016-07-19 7:31 ` Lee Jones 2016-07-19 7:31 ` Lee Jones 2016-07-20 15:01 ` Jonathan Cameron 2016-07-20 15:01 ` Jonathan Cameron 2016-07-21 12:12 ` Lee Jones 2016-07-21 12:12 ` Lee Jones 2016-07-21 20:08 ` Maxime Ripard 2016-07-21 20:08 ` Maxime Ripard 2016-07-22 13:55 ` Lee Jones 2016-07-22 13:55 ` Lee Jones 2016-07-23 6:42 ` Jonathan Cameron 2016-07-23 6:42 ` Jonathan Cameron 2016-07-25 9:55 ` Maxime Ripard 2016-07-25 9:55 ` Maxime Ripard 2016-07-19 8:35 ` Quentin Schulz 2016-07-19 8:35 ` Quentin Schulz 2016-07-15 9:59 ` [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon Quentin Schulz 2016-07-15 9:59 ` Quentin Schulz 2016-07-15 14:03 ` Guenter Roeck 2016-07-15 14:03 ` Guenter Roeck 2016-07-15 14:36 ` Quentin Schulz 2016-07-15 14:36 ` Quentin Schulz 2016-07-16 2:53 ` Guenter Roeck 2016-07-16 2:53 ` Guenter Roeck 2016-07-18 12:24 ` Jonathan Cameron [this message] 2016-07-18 12:24 ` Jonathan Cameron 2016-07-19 6:55 ` Quentin Schulz 2016-07-19 6:55 ` Quentin Schulz 2016-07-20 14:49 ` Jonathan Cameron 2016-07-20 14:49 ` Jonathan Cameron
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=aa889446-d68c-a05f-83c1-4d54fe814000@kernel.org \ --to=jic23@kernel.org \ --cc=antoine.tenart@free-electrons.com \ --cc=jdelvare@suse.com \ --cc=knaack.h@gmx.de \ --cc=lars@metafoo.de \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-hwmon@vger.kernel.org \ --cc=linux-iio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=maxime.ripard@free-electrons.com \ --cc=pmeerw@pmeerw.net \ --cc=quentin.schulz@free-electrons.com \ --cc=thomas.petazzoni@free-electrons.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.