All of lore.kernel.org
 help / color / mirror / Atom feed
From: Quentin Schulz <quentin.schulz@free-electrons.com>
To: Jonathan Cameron <jic23@kernel.org>,
	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: Tue, 19 Jul 2016 08:55:17 +0200	[thread overview]
Message-ID: <578DCED5.6080408@free-electrons.com> (raw)
In-Reply-To: <aa889446-d68c-a05f-83c1-4d54fe814000@kernel.org>

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.

[...]

WARNING: multiple messages have this Message-ID (diff)
From: quentin.schulz@free-electrons.com (Quentin Schulz)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon
Date: Tue, 19 Jul 2016 08:55:17 +0200	[thread overview]
Message-ID: <578DCED5.6080408@free-electrons.com> (raw)
In-Reply-To: <aa889446-d68c-a05f-83c1-4d54fe814000@kernel.org>

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.

[...]

  reply	other threads:[~2016-07-19  6:55 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
2016-07-18 12:24     ` Jonathan Cameron
2016-07-19  6:55     ` Quentin Schulz [this message]
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=578DCED5.6080408@free-electrons.com \
    --to=quentin.schulz@free-electrons.com \
    --cc=antoine.tenart@free-electrons.com \
    --cc=jdelvare@suse.com \
    --cc=jic23@kernel.org \
    --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=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: link
Be 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.