| Message ID | 1468576754-3273-5-git-send-email-quentin.schulz@free-electrons.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 <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); > + 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; > -- 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 15/07/2016 16:03, Guenter Roeck wrote: > On 07/15/2016 02:59 AM, Quentin Schulz wrote: [...] >> +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. It cannot, the extend_name field is const char* in the struct: http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247 [...] >> @@ -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 ? I don't get your question. I've followed the exact same pattern as for "a" variable's initialization. The initialization is before the switch case because the name of the exposed sysfs file depends on the type of the IIO channel. If I move the initialization in the switch case, I'll have duplicated code. >> + } >> + >> 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. > I don't know if we have to do something to revert the effects of sysfs_attr_init but you sure are right that the a and b's attribute's name is never freed. This case would be handled with devm_kasprintf, I guess? Thanks. >> 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; >> > Quentin -- 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 07/15/2016 07:36 AM, Quentin Schulz wrote: > On 15/07/2016 16:03, Guenter Roeck wrote: >> On 07/15/2016 02:59 AM, Quentin Schulz wrote: > [...] >>> +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. > > It cannot, the extend_name field is const char* in the struct: > http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247 > Then why "if(label)" ? > [...] >>> @@ -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 ? > > I don't get your question. I've followed the exact same pattern as for > "a" variable's initialization. > The initialization is before the switch case because the name of the > exposed sysfs file depends on the type of the IIO channel. If I move the > initialization in the switch case, I'll have duplicated code. > Yes, but that didn't require all those if statements. >>> + } >>> + >>> 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. >> > > I don't know if we have to do something to revert the effects of > sysfs_attr_init but you sure are right that the a and b's attribute's > name is never freed. This case would be handled with devm_kasprintf, I > guess? Thanks. > >>> 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) { sysfs_attr_init() might be better here to keep the initialization of '*b' as close together as possible. Guenetr >>> + 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; >>> >> > > Quentin > -- 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 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; > -- 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 18/07/2016 14:24, Jonathan Cameron wrote: > 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. Good to know for extend_name use cases. While doing further testing, I noticed the extend_name is also appended to the sysfs filename.. which is definitely not what we want. I've checked for datasheet_name and it is only used to be compared to adc_channel_label from iio_map structure. Same for adc_channel_label (which has to be the same as datasheet_name of the iio_chan_spec it is linked to). So I could use this instead of extend_name to put a label on a channel. However, I thought of it to be more a way to identify the hardware in the datasheet more than giving users a hint on what it is. That's what "git grep adc_channel_label" told me. It's definitely better to use datasheet_name over extend_name for channel labeling but I don't know if it's really the good variable to use for labeling? In my understanding of datasheet_name, in my case it would be more "temp_gpadc" than "SoC temperature", that's what I mean. > 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? Hmm.. I would not put a property in the DT only for labeling. [...] -- 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 19/07/16 07:55, Quentin Schulz wrote: > On 18/07/2016 14:24, Jonathan Cameron wrote: >> 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. > > Good to know for extend_name use cases. While doing further testing, I > noticed the extend_name is also appended to the sysfs filename.. which > is definitely not what we want. > > I've checked for datasheet_name and it is only used to be compared to > adc_channel_label from iio_map structure. Same for adc_channel_label > (which has to be the same as datasheet_name of the iio_chan_spec it is > linked to). So I could use this instead of extend_name to put a label on > a channel. > However, I thought of it to be more a way to identify the hardware in > the datasheet more than giving users a hint on what it is. That's what > "git grep adc_channel_label" told me. It's definitely better to use > datasheet_name over extend_name for channel labeling but I don't know if > it's really the good variable to use for labeling? In my understanding > of datasheet_name, in my case it would be more "temp_gpadc" than "SoC > temperature", that's what I mean. If we are going to do this I think we need a new field in iio_chan_spec for it. The problem as ever is going to be that we'll end up with 'fuzzy' ABI which we can't change - even if we end up with spelling mistakes sneaking through review - or entirely incorrect labels. > >> 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? > > Hmm.. I would not put a property in the DT only for labeling. It's an odd one because in many cases the map (which is effectively representing a wire) is the only relevant place to have such a label. Can see what you mean about not putting things in DT which are basically 'help'! > > [...] > -- 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/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;
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 <quentin.schulz@free-electrons.com> --- patch added in v2 drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 9 deletions(-)