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: AQHWefGasimkgzJai0eJZKBxcJKrA6lQjFiAgAfTFFA=
Date:   Fri, 4 Sep 2020 11:14:04 +0000
Message-ID: <BN6PR03MB2514AE2330F47235028B4455E72D0@BN6PR03MB2514.namprd03.prod.outlook.com>
References: <20200824083646.84886-1-cristian.pop@analog.com>
 <20200830122425.3e00332b@archlinux>
In-Reply-To: <20200830122425.3e00332b@archlinux>
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: 805e8d9e-0ae1-4e80-3ed3-08d850c3a246
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Sep 2020 11:14:04.2783
 (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: /CZ3utqU0jFPO2p4phEBJedLLFSH7HSC/CqetEEm8NvvkijUS5K49mL2tZGjQy5lnDlRaopjPR/iRQE+EEzVyw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR03MB4611
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.687
 definitions=2020-09-04_06:2020-09-04,2020-09-04 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 impostorscore=0
 adultscore=0 priorityscore=1501 mlxlogscore=999 clxscore=1015
 lowpriorityscore=0 malwarescore=0 spamscore=0 suspectscore=0 phishscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2006250000 definitions=main-2009040102
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Best ragards,
Cristian Pop

> -----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 d=
evice
> channel
>=20
> [External]
>=20
> On Mon, 24 Aug 2020 11:36:46 +0300
> Cristian Pop <cristian.pop@analog.com> wrote:
>=20
> > 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 devic=
e
> *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)
>=20
> I'm not keen on this. We shouldn't be calling read_raw at all in this pat=
h.
> There is no way it can return a valid label.
>=20
> > +			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_d=
ev
> *indio_dev)
> >  			attrcount_orig++;
> >  	}
> >  	attrcount =3D attrcount_orig;
> > +
>=20
> Please avoid unrelated white space changes.
>=20
> >  	/*
> >  	 * New channel registration method - relies on the fact a group does
> >  	 * 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;
>=20
> This can't be part of chan_spec as that is constant in many drivers.
> We need a separate way of doing this.  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.
>=20
If "label_name" can't be part of the "struct iio_chan_spec " why would I pa=
ss
it to the callback function? Just identify the channel?
I would implement a callback :
int (*read_string)(struct iio_dev *indio_dev,
	const struct iio_chan_spec *chan, const char *buf, long info);
and use this callback for other purposes as well, not only for "label" attr=
ibute.

> I would add a new callback to iio_info that is passed the iio_chan_spec a=
nd
> returns a const char * for the label name.
>=20
> 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 (tha=
t
> can be driver specific)
>=20
> 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 f=
ashion
> to that done for info mask elements.
>=20
> It will be more complex than your approach, but make it more 'separable' =
as a
> feature in drivers.
>=20
> Jonathan
>=20
>=20
> >  	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,