diff mbox series

[v8,19/22] counter: Implement extension*_name sysfs attributes

Message ID c9b55d1cff6acac692a7853b0a25777ecf017b12.1613131238.git.vilhelm.gray@gmail.com (mailing list archive)
State New
Headers show
Series Introduce the Counter character device interface | expand

Commit Message

William Breathitt Gray Feb. 12, 2021, 12:13 p.m. UTC
The Generic Counter chrdev interface expects users to supply extension
IDs in order to select extensions for requests. In order for users to
know what extension ID belongs to which extension this information must
be exposed. The extension*_name attribute provides a way for users to
discover what extension ID belongs to which extension by reading the
respective extension name for an extension ID.

Cc: David Lechner <david@lechnology.com>
Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
 drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
 2 files changed, 50 insertions(+), 10 deletions(-)

Comments

Jonathan Cameron Feb. 14, 2021, 6:09 p.m. UTC | #1
On Fri, 12 Feb 2021 21:13:43 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> The Generic Counter chrdev interface expects users to supply extension
> IDs in order to select extensions for requests. In order for users to
> know what extension ID belongs to which extension this information must
> be exposed. The extension*_name attribute provides a way for users to
> discover what extension ID belongs to which extension by reading the
> respective extension name for an extension ID.
> 
> Cc: David Lechner <david@lechnology.com>
> Cc: Gwendal Grignou <gwendal@chromium.org>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
>  drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
>  2 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 6353f0a2f8f8..847e96f19d19 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -100,6 +100,15 @@ Description:
>  		Read-only attribute that indicates whether excessive noise is
>  		present at the channel Y counter inputs.
>  
> +What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
> +What:		/sys/bus/counter/devices/counterX/extensionZ_name
> +What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
> +KernelVersion:	5.13
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read-only attribute that indicates the component name of
> +		Extension Z.

Good to say what form this takes.

> +
William Breathitt Gray Feb. 19, 2021, 8:51 a.m. UTC | #2
On Sun, Feb 14, 2021 at 06:09:13PM +0000, Jonathan Cameron wrote:
> On Fri, 12 Feb 2021 21:13:43 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > The Generic Counter chrdev interface expects users to supply extension
> > IDs in order to select extensions for requests. In order for users to
> > know what extension ID belongs to which extension this information must
> > be exposed. The extension*_name attribute provides a way for users to
> > discover what extension ID belongs to which extension by reading the
> > respective extension name for an extension ID.
> > 
> > Cc: David Lechner <david@lechnology.com>
> > Cc: Gwendal Grignou <gwendal@chromium.org>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
> >  drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
> >  2 files changed, 50 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > index 6353f0a2f8f8..847e96f19d19 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > @@ -100,6 +100,15 @@ Description:
> >  		Read-only attribute that indicates whether excessive noise is
> >  		present at the channel Y counter inputs.
> >  
> > +What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
> > +What:		/sys/bus/counter/devices/counterX/extensionZ_name
> > +What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
> > +KernelVersion:	5.13
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Read-only attribute that indicates the component name of
> > +		Extension Z.
> 
> Good to say what form this takes.

Do you mean a description like this: "Read-only string attribute that
indicates the component name of Extension Z"?

William Breathitt Gray
Jonathan Cameron Feb. 21, 2021, 2:05 p.m. UTC | #3
On Fri, 19 Feb 2021 17:51:37 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Sun, Feb 14, 2021 at 06:09:13PM +0000, Jonathan Cameron wrote:
> > On Fri, 12 Feb 2021 21:13:43 +0900
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >   
> > > The Generic Counter chrdev interface expects users to supply extension
> > > IDs in order to select extensions for requests. In order for users to
> > > know what extension ID belongs to which extension this information must
> > > be exposed. The extension*_name attribute provides a way for users to
> > > discover what extension ID belongs to which extension by reading the
> > > respective extension name for an extension ID.
> > > 
> > > Cc: David Lechner <david@lechnology.com>
> > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
> > >  drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
> > >  2 files changed, 50 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > index 6353f0a2f8f8..847e96f19d19 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > @@ -100,6 +100,15 @@ Description:
> > >  		Read-only attribute that indicates whether excessive noise is
> > >  		present at the channel Y counter inputs.
> > >  
> > > +What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
> > > +What:		/sys/bus/counter/devices/counterX/extensionZ_name
> > > +What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
> > > +KernelVersion:	5.13
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Read-only attribute that indicates the component name of
> > > +		Extension Z.  
> > 
> > Good to say what form this takes.  
> 
> Do you mean a description like this: "Read-only string attribute that
> indicates the component name of Extension Z"?

My expectation would be that the possible strings are tightly constrained
(perhaps via review). So I'd like to see what they are and a brief description
of what each one means.

Jonathan

> 
> William Breathitt Gray
William Breathitt Gray Feb. 25, 2021, 11:32 p.m. UTC | #4
On Sun, Feb 21, 2021 at 02:05:07PM +0000, Jonathan Cameron wrote:
> On Fri, 19 Feb 2021 17:51:37 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Sun, Feb 14, 2021 at 06:09:13PM +0000, Jonathan Cameron wrote:
> > > On Fri, 12 Feb 2021 21:13:43 +0900
> > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > >   
> > > > The Generic Counter chrdev interface expects users to supply extension
> > > > IDs in order to select extensions for requests. In order for users to
> > > > know what extension ID belongs to which extension this information must
> > > > be exposed. The extension*_name attribute provides a way for users to
> > > > discover what extension ID belongs to which extension by reading the
> > > > respective extension name for an extension ID.
> > > > 
> > > > Cc: David Lechner <david@lechnology.com>
> > > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
> > > >  drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
> > > >  2 files changed, 50 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > > index 6353f0a2f8f8..847e96f19d19 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > > @@ -100,6 +100,15 @@ Description:
> > > >  		Read-only attribute that indicates whether excessive noise is
> > > >  		present at the channel Y counter inputs.
> > > >  
> > > > +What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
> > > > +What:		/sys/bus/counter/devices/counterX/extensionZ_name
> > > > +What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
> > > > +KernelVersion:	5.13
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Read-only attribute that indicates the component name of
> > > > +		Extension Z.  
> > > 
> > > Good to say what form this takes.  
> > 
> > Do you mean a description like this: "Read-only string attribute that
> > indicates the component name of Extension Z"?
> 
> My expectation would be that the possible strings are tightly constrained
> (perhaps via review). So I'd like to see what they are and a brief description
> of what each one means.
> 
> Jonathan

Okay I see what you mean now. These names will match the sysfs attribute
filenames. So for example, if Extension 9 of Count 2 of Counter device
is /sys/bus/counter/devices/counter4/count2/ceiling, then the attribute
/sys/bus/counter/devices/counter4/count2/extension9_name will hold a
value of "ceiling".

The idea is that the user walks down through each extension*_name to
find sysfs attribute name for the Extension that they want. When they
find the desired Extension name in say sysfs attribute extension9_name,
then they know 9 is the ID number for that Extension.

There is an alternative design I was considering: instead of
extension*_name attributes, we could have each Extension sysfs attribute
have a matching *_extension_id attribute which provides the respective
Extension ID. So for example, using the same Extension as before:
/sys/bus/counter/devices/counter4/count2/ceiling_extension_id will hold
a value of 9.

Do you think this alternative design would be more intuitive to users?

William Breathitt Gray
Jonathan Cameron Feb. 27, 2021, 3:14 p.m. UTC | #5
On Fri, 26 Feb 2021 08:32:59 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Sun, Feb 21, 2021 at 02:05:07PM +0000, Jonathan Cameron wrote:
> > On Fri, 19 Feb 2021 17:51:37 +0900
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >   
> > > On Sun, Feb 14, 2021 at 06:09:13PM +0000, Jonathan Cameron wrote:  
> > > > On Fri, 12 Feb 2021 21:13:43 +0900
> > > > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> > > >     
> > > > > The Generic Counter chrdev interface expects users to supply extension
> > > > > IDs in order to select extensions for requests. In order for users to
> > > > > know what extension ID belongs to which extension this information must
> > > > > be exposed. The extension*_name attribute provides a way for users to
> > > > > discover what extension ID belongs to which extension by reading the
> > > > > respective extension name for an extension ID.
> > > > > 
> > > > > Cc: David Lechner <david@lechnology.com>
> > > > > Cc: Gwendal Grignou <gwendal@chromium.org>
> > > > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > > > > ---
> > > > >  Documentation/ABI/testing/sysfs-bus-counter |  9 ++++
> > > > >  drivers/counter/counter-sysfs.c             | 51 +++++++++++++++++----
> > > > >  2 files changed, 50 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > index 6353f0a2f8f8..847e96f19d19 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-bus-counter
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-counter
> > > > > @@ -100,6 +100,15 @@ Description:
> > > > >  		Read-only attribute that indicates whether excessive noise is
> > > > >  		present at the channel Y counter inputs.
> > > > >  
> > > > > +What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
> > > > > +What:		/sys/bus/counter/devices/counterX/extensionZ_name
> > > > > +What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
> > > > > +KernelVersion:	5.13
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Read-only attribute that indicates the component name of
> > > > > +		Extension Z.    
> > > > 
> > > > Good to say what form this takes.    
> > > 
> > > Do you mean a description like this: "Read-only string attribute that
> > > indicates the component name of Extension Z"?  
> > 
> > My expectation would be that the possible strings are tightly constrained
> > (perhaps via review). So I'd like to see what they are and a brief description
> > of what each one means.
> > 
> > Jonathan  
> 
> Okay I see what you mean now. These names will match the sysfs attribute
> filenames. So for example, if Extension 9 of Count 2 of Counter device
> is /sys/bus/counter/devices/counter4/count2/ceiling, then the attribute
> /sys/bus/counter/devices/counter4/count2/extension9_name will hold a
> value of "ceiling".
> 
> The idea is that the user walks down through each extension*_name to
> find sysfs attribute name for the Extension that they want. When they
> find the desired Extension name in say sysfs attribute extension9_name,
> then they know 9 is the ID number for that Extension.
> 
> There is an alternative design I was considering: instead of
> extension*_name attributes, we could have each Extension sysfs attribute
> have a matching *_extension_id attribute which provides the respective
> Extension ID. So for example, using the same Extension as before:
> /sys/bus/counter/devices/counter4/count2/ceiling_extension_id will hold
> a value of 9.
> 
> Do you think this alternative design would be more intuitive to users?
It feels like the user is going to start from what they want to enable
then get the ID from that.   With the current way around they'll have
to search the extensionX_name files to find it, rather than a direct
look up.  So immediate thought is this second way would be easier to
use, but perhaps others think differently.

Jonathan

> 
> William Breathitt Gray
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 6353f0a2f8f8..847e96f19d19 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -100,6 +100,15 @@  Description:
 		Read-only attribute that indicates whether excessive noise is
 		present at the channel Y counter inputs.
 
+What:		/sys/bus/counter/devices/counterX/countY/extensionZ_name
+What:		/sys/bus/counter/devices/counterX/extensionZ_name
+What:		/sys/bus/counter/devices/counterX/signalY/extensionZ_name
+KernelVersion:	5.13
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read-only attribute that indicates the component name of
+		Extension Z.
+
 What:		/sys/bus/counter/devices/counterX/countY/function
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index 52513a213cc5..0cb3dba950bc 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
@@ -494,6 +494,7 @@  static ssize_t counter_comp_name_show(struct device *dev,
 
 static int counter_name_attr_create(struct device *const dev,
 				    struct counter_attribute_group *const group,
+				    const char *const attr_name,
 				    const char *const name)
 {
 	struct counter_attribute *counter_attr;
@@ -508,7 +509,7 @@  static int counter_name_attr_create(struct device *const dev,
 
 	/* Configure device attribute */
 	sysfs_attr_init(&counter_attr->dev_attr.attr);
-	counter_attr->dev_attr.attr.name = "name";
+	counter_attr->dev_attr.attr.name = attr_name;
 	counter_attr->dev_attr.attr.mode = 0444;
 	counter_attr->dev_attr.show = counter_comp_name_show;
 
@@ -519,6 +520,18 @@  static int counter_name_attr_create(struct device *const dev,
 	return 0;
 }
 
+static int counter_ext_name_attr_create(struct device *const dev,
+	struct counter_attribute_group *const group, const size_t i,
+	const char *const name)
+{
+	const char *attr_name;
+
+	attr_name = devm_kasprintf(dev, GFP_KERNEL, "extension%zu_name", i);
+	if (!attr_name)
+		return -ENOMEM;
+
+	return counter_name_attr_create(dev, group, attr_name, name);
+}
 
 static struct counter_comp counter_signal_comp = {
 	.type = COUNTER_COMP_SIGNAL_LEVEL,
@@ -534,6 +547,7 @@  static int counter_signal_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Signal attribute */
 	comp = counter_signal_comp;
@@ -543,14 +557,19 @@  static int counter_signal_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Signal name attribute */
-	err = counter_name_attr_create(dev, group, signal->name);
+	err = counter_name_attr_create(dev, group, "name", signal->name);
 	if (err < 0)
 		return err;
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < signal->num_ext; i++) {
-		err = counter_attr_create(dev, group, signal->ext + i, scope,
-					  signal);
+		ext = signal->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, signal);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -636,6 +655,7 @@  static int counter_count_attrs_create(struct counter_device *const counter,
 	int err;
 	struct counter_comp comp;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Create main Count attribute */
 	comp = counter_count_comp;
@@ -646,7 +666,7 @@  static int counter_count_attrs_create(struct counter_device *const counter,
 		return err;
 
 	/* Create Count name attribute */
-	err = counter_name_attr_create(dev, group, count->name);
+	err = counter_name_attr_create(dev, group, "name", count->name);
 	if (err < 0)
 		return err;
 
@@ -660,8 +680,13 @@  static int counter_count_attrs_create(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < count->num_ext; i++) {
-		err = counter_attr_create(dev, group, count->ext + i, scope,
-					  count);
+		ext = count->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, count);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}
@@ -725,6 +750,7 @@  static int counter_sysfs_attr_add(struct counter_device *const counter,
 	struct device *const dev = &counter->dev;
 	int err;
 	size_t i;
+	struct counter_comp *ext;
 
 	/* Add Signals sysfs attributes */
 	err = counter_sysfs_signals_add(counter, group);
@@ -739,7 +765,7 @@  static int counter_sysfs_attr_add(struct counter_device *const counter,
 	group += counter->num_counts;
 
 	/* Create name attribute */
-	err = counter_name_attr_create(dev, group, counter->name);
+	err = counter_name_attr_create(dev, group, "name", counter->name);
 	if (err < 0)
 		return err;
 
@@ -757,8 +783,13 @@  static int counter_sysfs_attr_add(struct counter_device *const counter,
 
 	/* Create an attribute for each extension */
 	for (i = 0; i < counter->num_ext; i++) {
-		err = counter_attr_create(dev, group, counter->ext + i, scope,
-					  NULL);
+		ext = counter->ext + i;
+
+		err = counter_attr_create(dev, group, ext, scope, NULL);
+		if (err < 0)
+			return err;
+
+		err = counter_ext_name_attr_create(dev, group, i, ext->name);
 		if (err < 0)
 			return err;
 	}