Re: [PATCH v12 07/17] counter: Update counter.h comments to reflect sysfs internalization

[William Breathitt Gray]

On Fri, Jul 09, 2021 at 12:49:20PM -0500, David Lechner wrote:
> On 7/5/21 3:18 AM, William Breathitt Gray wrote:
> > The Counter subsystem architecture and driver implementations have
> > changed in order to handle Counter sysfs interactions in a more
> > consistent way. This patch updates the Generic Counter interface
> > header file comments to reflect the changes.
> > 
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@xxxxxxxxx>
> > ---
> >   drivers/counter/counter-core.c |  3 +++
> >   include/linux/counter.h        | 43 ++++++++++++++++------------------
> >   2 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > index 15f735ef296e..9442e3b91468 100644
> > --- a/drivers/counter/counter-core.c
> > +++ b/drivers/counter/counter-core.c
> > @@ -41,6 +41,9 @@ static struct bus_type counter_bus_type = {
> >    * This function registers a Counter to the system. A sysfs "counter" directory
> >    * will be created and populated with sysfs attributes correlating with the
> >    * Counter Signals, Synapses, and Counts respectively.
> > + *
> > + * RETURNS:
> > + * 0 on success, negative error number on failure.
> >    */
> >   int counter_register(struct counter_device *const counter)
> >   {
> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index b69277f5c4c5..e7fd6d81a929 100644
> > --- a/include/linux/counter.h
> > +++ b/include/linux/counter.h
> > @@ -188,11 +188,10 @@ struct counter_comp {
> >   
> >   /**
> >    * struct counter_signal - Counter Signal node
> > - * @id:		unique ID used to identify signal
> > - * @name:	device-specific Signal name; ideally, this should match the name
> > - *		as it appears in the datasheet documentation
> > - * @ext:	optional array of Counter Signal extensions
> > - * @num_ext:	number of Counter Signal extensions specified in @ext
> > + * @id:		unique ID used to identify the Signal
> > + * @name:	device-specific Signal name
> > + * @ext:	optional array of Signal extensions
> > + * @num_ext:	number of Signal extensions specified in @ext
> >    */
> >   struct counter_signal {
> >   	int id;
> > @@ -206,7 +205,7 @@ struct counter_signal {
> >    * struct counter_synapse - Counter Synapse node
> >    * @actions_list:	array of available action modes
> >    * @num_actions:	number of action modes specified in @actions_list
> > - * @signal:		pointer to associated signal
> > + * @signal:		pointer to the associated Signal
> >    */
> >   struct counter_synapse {
> >   	const enum counter_synapse_action *actions_list;
> > @@ -217,15 +216,14 @@ struct counter_synapse {
> >   
> >   /**
> >    * struct counter_count - Counter Count node
> > - * @id:			unique ID used to identify Count
> > - * @name:		device-specific Count name; ideally, this should match
> > - *			the name as it appears in the datasheet documentation
> > - * @functions_list:	array available function modes
> > + * @id:			unique ID used to identify the Count
> > + * @name:		device-specific Count name
> > + * @functions_list:	array of available function modes
> >    * @num_functions:	number of function modes specified in @functions_list
> > - * @synapses:		array of synapses for initialization
> > - * @num_synapses:	number of synapses specified in @synapses
> > - * @ext:		optional array of Counter Count extensions
> > - * @num_ext:		number of Counter Count extensions specified in @ext
> > + * @synapses:		array of Synapses for initialization
> > + * @num_synapses:	number of Synapses specified in @synapses
> > + * @ext:		optional array of Count extensions
> > + * @num_ext:		number of Count extensions specified in @ext
> >    */
> >   struct counter_count {
> >   	int id;
> > @@ -243,15 +241,14 @@ struct counter_count {
> >   
> >   /**
> >    * struct counter_ops - Callbacks from driver
> > - * @signal_read:	optional read callback for Signal attribute. The read
> > - *			level of the respective Signal should be passed back via
> > - *			the level parameter.
> > - * @count_read:		optional read callback for Count attribute. The read
> > - *			value of the respective Count should be passed back via
> > - *			the val parameter.
> 
> Are these no longer optional? If they really are optional, it would be nice to
> keep that information in the description.

I'd expect drivers to have at least the count_read() and function_read()
callbacks defined and set; otherwise such a counter driver would be
rather useless to a user. I'll update the documentation here to make it
clear the other callbacks are optional.

> > - * @count_write:	optional write callback for Count attribute. The write
> > - *			value for the respective Count is passed in via the val
> > + * @signal_read:	read callback for Signals. The read level of the
> > + *			respective Signal should be passed back via the level
> > + *			parameter.
> > + * @count_read:		read callback for Counts. The read value of the
> > + *			respective Count should be passed back via the value
> >    *			parameter.
> > + * @count_write:	write callback for Counts. The write value for the
> > + *			respective Count is passed in via the value parameter.
> >    * @function_read:	read callback the Count function modes. The read
> >    *			function mode of the respective Count should be passed
> >    *			back via the function parameter.
> > @@ -291,7 +288,7 @@ struct counter_ops {
> >   
> >   /**
> >    * struct counter_device - Counter data structure
> > - * @name:		name of the device as it appears in the datasheet
> > + * @name:		name of the device
> 
> Is there a recommended naming convention if using the datasheet is no longer
> recommended?

I decided to remove the "as it appears in the datasheet" phrase because
there may be cases where the datasheet name isn't the best name to
provide for the counter device. For example, with the 104-quad-8 driver
there is ambiguity about whether it's more useful for the name to be
"104-QUAD-8" to match the PC104 card, or "LS7266R1" to match the counter
chip integrated on the 104-QUAD-8 card. Another example would be the
recent interrupt-cnt driver which isn't for a particular device (it
counts any interrupt source) so thus does not have a corresponding
datasheet. I figure it's best to leave it up to the driver maintainer to
choose an apt name that will make sense for the users.

William Breathitt Gray

Attachment: signature.asc
Description: PGP signature