Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Pop, Cristian" <Cristian.Pop@analog.com>
To:     Jonathan Cameron <jic23@kernel.org>
CC:     "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a device
 channel
Thread-Topic: [RFC PATCH v2] iio: core: Add optional symbolic label to a
 device channel
Thread-Index: AQHWefGasimkgzJai0eJZKBxcJKrA6lQjFiAgAfTFFCAACrC4A==
Date:   Fri, 4 Sep 2020 13:35:12 +0000
Message-ID: <BN6PR03MB251405FC8654C3539D7BD9C1E72D0@BN6PR03MB2514.namprd03.prod.outlook.com>
References: <20200824083646.84886-1-cristian.pop@analog.com>
 <20200830122425.3e00332b@archlinux>
 <BN6PR03MB2514AE2330F47235028B4455E72D0@BN6PR03MB2514.namprd03.prod.outlook.com>
In-Reply-To: <BN6PR03MB2514AE2330F47235028B4455E72D0@BN6PR03MB2514.namprd03.prod.outlook.com>
Accept-Language: ro-RO, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 57ff7d45-ce0a-4e93-7678-08d850d75991
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Sep 2020 13:35:12.3011
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 6hDvonBjte0AmBdrUiUwmpv1CqBrZR1skcrfSV/Eo954RKaWWcCFpia4zVX7Hj5BeT6wxqIhcanpChBmNqcIuw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR03MB2513
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-09-04_07:2020-09-04,2020-09-04 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 adultscore=0
 bulkscore=0 phishscore=0 impostorscore=0 suspectscore=0 priorityscore=1501
 lowpriorityscore=0 mlxscore=0 clxscore=1015 mlxlogscore=999 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000
 definitions=main-2009040123
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Pop, Cristian
> Sent: Friday, September 4, 2020 2:14 PM
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [RFC PATCH v2] iio: core: Add optional symbolic label to a d=
evice
> channel
>=20
> Best ragards,
> Cristian Pop
>=20
> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, August 30, 2020 2:24 PM
> > To: Pop, Cristian <Cristian.Pop@analog.com>
> > Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [RFC PATCH v2] iio: core: Add optional symbolic label to
> > a device channel
> >
> > [External]
> >
> > On Mon, 24 Aug 2020 11:36:46 +0300
> > Cristian Pop <cristian.pop@analog.com> wrote:
> >
> > > If a label is defined in the device tree for this channel add that
> > > to the channel specific attributes. This is useful for userspace to
> > > be able to identify an individual channel.
> > >
> > > Signed-off-by: Cristian Pop <cristian.pop@analog.com>
> > > ---
> > >  Changes in v2:
> > > 	- Move label check before "read_raw" callback.
> > > 	- Move the responsability to of parsing channel labels, to the
> > > 	  driver.
> > >
> > >  drivers/iio/industrialio-core.c | 10 ++++++++--
> > >  include/linux/iio/iio.h         |  2 ++
> > >  include/linux/iio/types.h       |  1 +
> > >  3 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-core.c
> > > b/drivers/iio/industrialio-core.c index 1527f01a44f1..32277e94f02d
> > > 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -135,6 +135,7 @@ static const char * const iio_modifier_names[] =
=3D
> > > {
> > >  /* relies on pairs of these shared then separate */  static const
> > > char * const iio_chan_info_postfix[] =3D {
> > >  	[IIO_CHAN_INFO_RAW] =3D "raw",
> > > +	[IIO_CHAN_INFO_LABEL] =3D "label",
> > >  	[IIO_CHAN_INFO_PROCESSED] =3D "input",
> > >  	[IIO_CHAN_INFO_SCALE] =3D "scale",
> > >  	[IIO_CHAN_INFO_OFFSET] =3D "offset", @@ -653,14 +654,18 @@ static
> > > ssize_t iio_read_channel_info(struct device
> > *dev,
> > >  	int ret;
> > >  	int val_len =3D 2;
> > >
> > > -	if (indio_dev->info->read_raw_multi)
> > > +	if (indio_dev->info->read_raw_multi) {
> > >  		ret =3D indio_dev->info->read_raw_multi(indio_dev, this_attr- c,
> > >
> > 	INDIO_MAX_RAW_ELEMENTS,
> > >  							vals, &val_len,
> > >  							this_attr->address);
> > > -	else
> > > +	} else {
> > >  		ret =3D indio_dev->info->read_raw(indio_dev, this_attr->c,
> > >  				    &vals[0], &vals[1], this_attr->address);
> > > +		if (ret < 0 && this_attr->address =3D=3D IIO_CHAN_INFO_LABEL
> > &&
> > > +			this_attr->c->label_name)
> >
> > I'm not keen on this. We shouldn't be calling read_raw at all in this p=
ath.
> > There is no way it can return a valid label.
> >
> > > +			return sprintf(buf, "%s\n", this_attr->c->label_name);
> > > +	}
> > >
> > >  	if (ret < 0)
> > >  		return ret;
> > > @@ -1399,6 +1404,7 @@ static int iio_device_register_sysfs(struct
> > > iio_dev
> > *indio_dev)
> > >  			attrcount_orig++;
> > >  	}
> > >  	attrcount =3D attrcount_orig;
> > > +
> >
> > Please avoid unrelated white space changes.
> >
> > >  	/*
> > >  	 * New channel registration method - relies on the fact a group doe=
s
> > >  	 * not need to be initialized if its name is NULL.
> > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index
> > > a1be82e74c93..39209f3b62be 100644
> > > --- a/include/linux/iio/iio.h
> > > +++ b/include/linux/iio/iio.h
> > > @@ -223,6 +223,7 @@ struct iio_event_spec {
> > >   *			correspond to the first name that the channel is
> > referred
> > >   *			to by in the datasheet (e.g. IND), or the nearest
> > >   *			possible compound name (e.g. IND-INC).
> > > + * @label_name:		Unique name to identify which channel this is.
> > >   * @modified:		Does a modifier apply to this channel. What
> > these are
> > >   *			depends on the channel type.  Modifier is set in
> > >   *			channel2. Examples are IIO_MOD_X for axial sensors
> > about
> > > @@ -260,6 +261,7 @@ struct iio_chan_spec {
> > >  	const struct iio_chan_spec_ext_info *ext_info;
> > >  	const char		*extend_name;
> > >  	const char		*datasheet_name;
> > > +	const char		*label_name;
> >
> > This can't be part of chan_spec as that is constant in many drivers.
> > We need a separate way of doing this. =20
What do you mean by "chan_spec is constant in many drivers"? Instances of t=
he "struct iio_chan_spec" are created in the driver. Also it makes sense fo=
r me to add "const char              *label_name;" in this structure since =
it is an attribute of the channel, and it doesn't change at runtime, only w=
hen parsing the device tree and assigning the value to it, when an instance=
 of "iio_chan_spec" is created.
>> Don't use info_mask, but
> > register it separately for each channel in a similar way to we do the
> > name and label attributes for the whole device.
Don't understand this part. "name" and "label" of the device are elements o=
f "struct iio_dev", as "const char  *label_name;" is part of "struct iio_ch=
an_spec", the equivalent structure for holding channel attributes.
Who will hold the label values otherwise, if not the " iio_chan_spec " stru=
cture?
> > I would add a new callback to iio_info that is passed the
> > iio_chan_spec and returns a const char * for the label name.
I do agree with the callback, it can be a more generic callback, to return =
strings, for other purposes also.
Something like this:
    int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec con=
st *chan,  char *string,  long mask);
or
    int (*read_string)(struct iio_dev *indio_dev,  struct iio_chan_spec con=
st *chan, const char **string, long mask);
The callback will be called in "iio_read_channel_info", so I think that a s=
ystem file will be created for it.
> >
> > The driver would be responsible for doing a lookup based on what it
> > has cached from the dt parse, probably indexed off address or
> > scan_index (that can be driver specific)
> >
> > To create the attribute you need to add this to
> > iio_device_register_sysfs and use the various core functions to build
> > the attribute name in a similar fashion to that done for info mask elem=
ents.
> >
> > It will be more complex than your approach, but make it more
> > 'separable' as a feature in drivers.
> >
> > Jonathan
> >
> >
> > >  	unsigned		modified:1;
> > >  	unsigned		indexed:1;
> > >  	unsigned		output:1;
> > > diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> > > index e6fd3645963c..c8f65f476eb2 100644
> > > --- a/include/linux/iio/types.h
> > > +++ b/include/linux/iio/types.h
> > > @@ -34,6 +34,7 @@ enum iio_available_type {
> > >
> > >  enum iio_chan_info_enum {
> > >  	IIO_CHAN_INFO_RAW =3D 0,
> > > +	IIO_CHAN_INFO_LABEL,
> > >  	IIO_CHAN_INFO_PROCESSED,
> > >  	IIO_CHAN_INFO_SCALE,
> > >  	IIO_CHAN_INFO_OFFSET,