Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Tue, 10 Oct 2023 16:59:17 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     David Lechner <dlechner@baylibre.com>
Cc:     linux-iio@vger.kernel.org, linux-staging@lists.linux.dev,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>,
        Axel Haslam <ahaslam@baylibre.com>,
        Philip Molloy <pmolloy@baylibre.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 12/17] iio: event: add optional event label support
Message-ID: <20231010165917.4c1cb0a9@jic23-huawei>
In-Reply-To: <20231005-ad2s1210-mainline-v4-12-ec00746840fc@baylibre.com>
References: <20231005-ad2s1210-mainline-v4-0-ec00746840fc@baylibre.com>
        <20231005-ad2s1210-mainline-v4-12-ec00746840fc@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu,  5 Oct 2023 19:50:29 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This adds a new optional field to struct iio_info to allow drivers to
> specify a label for the event. This is useful for cases where there are
> many events or the event attribute name is not descriptive enough or
> where an event doesn't have any other attributes.
> 
> The implementation is based on the existing label support for channels.
> So either all events of a device have a label attribute or none do.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>

Was going to moan about lack of docs, but I see they are in later patches.

Ok. General feature is fine. Applied.

Jonathan

> ---
> 
> v4 changes: New patch in v4.
> 
>  drivers/iio/industrialio-event.c | 55 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/iio.h          |  8 ++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 19f7a91157ee..910c1f14abd5 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -355,6 +355,21 @@ static ssize_t iio_ev_value_store(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t iio_ev_label_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> +	if (indio_dev->info->read_event_label)
> +		return indio_dev->info->read_event_label(indio_dev,
> +				 this_attr->c, iio_ev_attr_type(this_attr),
> +				 iio_ev_attr_dir(this_attr), buf);
> +
> +	return -EINVAL;
> +}
> +
>  static int iio_device_add_event(struct iio_dev *indio_dev,
>  	const struct iio_chan_spec *chan, unsigned int spec_index,
>  	enum iio_event_type type, enum iio_event_direction dir,
> @@ -411,6 +426,41 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
>  	return attrcount;
>  }
>  
> +static int iio_device_add_event_label(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int spec_index,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	char *postfix;
> +	int ret;
> +
> +	if (!indio_dev->info->read_event_label)
> +		return 0;
> +
> +	if (dir != IIO_EV_DIR_NONE)
> +		postfix = kasprintf(GFP_KERNEL, "%s_%s_label",
> +				iio_ev_type_text[type],
> +				iio_ev_dir_text[dir]);
> +	else
> +		postfix = kasprintf(GFP_KERNEL, "%s_label",
> +				iio_ev_type_text[type]);
> +	if (postfix == NULL)
> +		return -ENOMEM;
> +
> +	ret = __iio_add_chan_devattr(postfix, chan, &iio_ev_label_show, NULL,
> +				spec_index, IIO_SEPARATE, &indio_dev->dev, NULL,
> +				&iio_dev_opaque->event_interface->dev_attr_list);
> +
> +	kfree(postfix);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 1;
> +}
> +
>  static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
>  	struct iio_chan_spec const *chan)
>  {
> @@ -448,6 +498,11 @@ static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  		attrcount += ret;
> +
> +		ret = iio_device_add_event_label(indio_dev, chan, i, type, dir);
> +		if (ret < 0)
> +			return ret;
> +		attrcount += ret;
>  	}
>  	ret = attrcount;
>  	return ret;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 7bfa1b9bc8a2..d0ce3b71106a 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -427,6 +427,8 @@ struct iio_trigger; /* forward declaration */
>   * @write_event_config:	set if the event is enabled.
>   * @read_event_value:	read a configuration value associated with the event.
>   * @write_event_value:	write a configuration value for the event.
> + * @read_event_label:	function to request label name for a specified label,
> + *			for better event identification.
>   * @validate_trigger:	function to validate the trigger when the
>   *			current trigger gets changed.
>   * @update_scan_mode:	function to configure device and scan buffer when
> @@ -511,6 +513,12 @@ struct iio_info {
>  				 enum iio_event_direction dir,
>  				 enum iio_event_info info, int val, int val2);
>  
> +	int (*read_event_label)(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				char *label);
> +
>  	int (*validate_trigger)(struct iio_dev *indio_dev,
>  				struct iio_trigger *trig);
>  	int (*update_scan_mode)(struct iio_dev *indio_dev,
>