* [RFC 1/4] counter: Simplify the count_read and count_write callbacks
2019-09-15 5:57 [RFC 0/4] counter: Simplify count_read/count_write/signal_read William Breathitt Gray
@ 2019-09-15 5:57 ` William Breathitt Gray
2019-09-15 13:39 ` Jonathan Cameron
2019-09-15 5:57 ` [RFC 2/4] counter: Simplify the signal_read callback William Breathitt Gray
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: William Breathitt Gray @ 2019-09-15 5:57 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, William Breathitt Gray
The count_read and count_write callbacks are simplified to pass val as
unsigned long rather than as an opaque data structure. The opaque
counter_count_read_value and counter_count_write_value structures,
counter_count_value_type enum, and relevant counter_count_read_value_set
and counter_count_write_value_get functions, are removed as they are no
longer used.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
drivers/counter/counter.c | 66 +++++----------------------------------
include/linux/counter.h | 43 +++----------------------
2 files changed, 12 insertions(+), 97 deletions(-)
diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 106bc7180cd8..1d08f1437b1b 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -246,60 +246,6 @@ void counter_signal_read_value_set(struct counter_signal_read_value *const val,
}
EXPORT_SYMBOL_GPL(counter_signal_read_value_set);
-/**
- * counter_count_read_value_set - set counter_count_read_value data
- * @val: counter_count_read_value structure to set
- * @type: property Count data represents
- * @data: Count data
- *
- * This function sets an opaque counter_count_read_value structure with the
- * provided Count data.
- */
-void counter_count_read_value_set(struct counter_count_read_value *const val,
- const enum counter_count_value_type type,
- void *const data)
-{
- switch (type) {
- case COUNTER_COUNT_POSITION:
- val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
- break;
- default:
- val->len = 0;
- }
-}
-EXPORT_SYMBOL_GPL(counter_count_read_value_set);
-
-/**
- * counter_count_write_value_get - get counter_count_write_value data
- * @data: Count data
- * @type: property Count data represents
- * @val: counter_count_write_value structure containing data
- *
- * This function extracts Count data from the provided opaque
- * counter_count_write_value structure and stores it at the address provided by
- * @data.
- *
- * RETURNS:
- * 0 on success, negative error number on failure.
- */
-int counter_count_write_value_get(void *const data,
- const enum counter_count_value_type type,
- const struct counter_count_write_value *const val)
-{
- int err;
-
- switch (type) {
- case COUNTER_COUNT_POSITION:
- err = kstrtoul(val->buf, 0, data);
- if (err)
- return err;
- break;
- }
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(counter_count_write_value_get);
-
struct counter_attr_parm {
struct counter_device_attr_group *group;
const char *prefix;
@@ -788,13 +734,13 @@ static ssize_t counter_count_show(struct device *dev,
const struct counter_count_unit *const component = devattr->component;
struct counter_count *const count = component->count;
int err;
- struct counter_count_read_value val = { .buf = buf };
+ unsigned long val;
err = counter->ops->count_read(counter, count, &val);
if (err)
return err;
- return val.len;
+ return sprintf(buf, "%lu\n", val);
}
static ssize_t counter_count_store(struct device *dev,
@@ -806,9 +752,13 @@ static ssize_t counter_count_store(struct device *dev,
const struct counter_count_unit *const component = devattr->component;
struct counter_count *const count = component->count;
int err;
- struct counter_count_write_value val = { .buf = buf };
+ unsigned long val;
+
+ err = kstrtoul(buf, 0, &val);
+ if (err)
+ return err;
- err = counter->ops->count_write(counter, count, &val);
+ err = counter->ops->count_write(counter, count, val);
if (err)
return err;
diff --git a/include/linux/counter.h b/include/linux/counter.h
index a061cdcdef7c..7e40796598a6 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -300,24 +300,6 @@ struct counter_signal_read_value {
size_t len;
};
-/**
- * struct counter_count_read_value - Opaque Count read value
- * @buf: string representation of Count read value
- * @len: length of string in @buf
- */
-struct counter_count_read_value {
- char *buf;
- size_t len;
-};
-
-/**
- * struct counter_count_write_value - Opaque Count write value
- * @buf: string representation of Count write value
- */
-struct counter_count_write_value {
- const char *buf;
-};
-
/**
* struct counter_ops - Callbacks from driver
* @signal_read: optional read callback for Signal attribute. The read
@@ -328,15 +310,10 @@ struct counter_count_write_value {
* signal_read callback.
* @count_read: optional read callback for Count attribute. The read
* value of the respective Count should be passed back via
- * the val parameter. val points to an opaque type which
- * should be set only by calling the
- * counter_count_read_value_set function from within the
- * count_read callback.
+ * the val parameter.
* @count_write: optional write callback for Count attribute. The write
* value for the respective Count is passed in via the val
- * parameter. val points to an opaque type which should be
- * accessed only by calling the
- * counter_count_write_value_get function.
+ * parameter.
* @function_get: function to get the current count function mode. Returns
* 0 on success and negative error code on error. The index
* of the respective Count's returned function mode should
@@ -357,11 +334,9 @@ struct counter_ops {
struct counter_signal *signal,
struct counter_signal_read_value *val);
int (*count_read)(struct counter_device *counter,
- struct counter_count *count,
- struct counter_count_read_value *val);
+ struct counter_count *count, unsigned long *val);
int (*count_write)(struct counter_device *counter,
- struct counter_count *count,
- struct counter_count_write_value *val);
+ struct counter_count *count, unsigned long val);
int (*function_get)(struct counter_device *counter,
struct counter_count *count, size_t *function);
int (*function_set)(struct counter_device *counter,
@@ -486,19 +461,9 @@ enum counter_signal_value_type {
COUNTER_SIGNAL_LEVEL = 0
};
-enum counter_count_value_type {
- COUNTER_COUNT_POSITION = 0,
-};
-
void counter_signal_read_value_set(struct counter_signal_read_value *const val,
const enum counter_signal_value_type type,
void *const data);
-void counter_count_read_value_set(struct counter_count_read_value *const val,
- const enum counter_count_value_type type,
- void *const data);
-int counter_count_write_value_get(void *const data,
- const enum counter_count_value_type type,
- const struct counter_count_write_value *const val);
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC 1/4] counter: Simplify the count_read and count_write callbacks
2019-09-15 5:57 ` [RFC 1/4] counter: Simplify the count_read and count_write callbacks William Breathitt Gray
@ 2019-09-15 13:39 ` Jonathan Cameron
2019-09-15 13:47 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2019-09-15 13:39 UTC (permalink / raw)
To: William Breathitt Gray; +Cc: linux-kernel, linux-iio
On Sun, 15 Sep 2019 14:57:56 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> The count_read and count_write callbacks are simplified to pass val as
> unsigned long rather than as an opaque data structure. The opaque
> counter_count_read_value and counter_count_write_value structures,
> counter_count_value_type enum, and relevant counter_count_read_value_set
> and counter_count_write_value_get functions, are removed as they are no
> longer used.
>
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
Seems like a sensible bit of excessive abstraction removal to me. I'm not
totally sure why these got so complex in the first place though.
Can you recall the reason as it might help to judge why we no longer
think the same?
Thanks,
Jonathan
> ---
> drivers/counter/counter.c | 66 +++++----------------------------------
> include/linux/counter.h | 43 +++----------------------
> 2 files changed, 12 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..1d08f1437b1b 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -246,60 +246,6 @@ void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> }
> EXPORT_SYMBOL_GPL(counter_signal_read_value_set);
>
> -/**
> - * counter_count_read_value_set - set counter_count_read_value data
> - * @val: counter_count_read_value structure to set
> - * @type: property Count data represents
> - * @data: Count data
> - *
> - * This function sets an opaque counter_count_read_value structure with the
> - * provided Count data.
> - */
> -void counter_count_read_value_set(struct counter_count_read_value *const val,
> - const enum counter_count_value_type type,
> - void *const data)
> -{
> - switch (type) {
> - case COUNTER_COUNT_POSITION:
> - val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
> - break;
> - default:
> - val->len = 0;
> - }
> -}
> -EXPORT_SYMBOL_GPL(counter_count_read_value_set);
> -
> -/**
> - * counter_count_write_value_get - get counter_count_write_value data
> - * @data: Count data
> - * @type: property Count data represents
> - * @val: counter_count_write_value structure containing data
> - *
> - * This function extracts Count data from the provided opaque
> - * counter_count_write_value structure and stores it at the address provided by
> - * @data.
> - *
> - * RETURNS:
> - * 0 on success, negative error number on failure.
> - */
> -int counter_count_write_value_get(void *const data,
> - const enum counter_count_value_type type,
> - const struct counter_count_write_value *const val)
> -{
> - int err;
> -
> - switch (type) {
> - case COUNTER_COUNT_POSITION:
> - err = kstrtoul(val->buf, 0, data);
> - if (err)
> - return err;
> - break;
> - }
> -
> - return 0;
> -}
> -EXPORT_SYMBOL_GPL(counter_count_write_value_get);
> -
> struct counter_attr_parm {
> struct counter_device_attr_group *group;
> const char *prefix;
> @@ -788,13 +734,13 @@ static ssize_t counter_count_show(struct device *dev,
> const struct counter_count_unit *const component = devattr->component;
> struct counter_count *const count = component->count;
> int err;
> - struct counter_count_read_value val = { .buf = buf };
> + unsigned long val;
>
> err = counter->ops->count_read(counter, count, &val);
> if (err)
> return err;
>
> - return val.len;
> + return sprintf(buf, "%lu\n", val);
> }
>
> static ssize_t counter_count_store(struct device *dev,
> @@ -806,9 +752,13 @@ static ssize_t counter_count_store(struct device *dev,
> const struct counter_count_unit *const component = devattr->component;
> struct counter_count *const count = component->count;
> int err;
> - struct counter_count_write_value val = { .buf = buf };
> + unsigned long val;
> +
> + err = kstrtoul(buf, 0, &val);
> + if (err)
> + return err;
>
> - err = counter->ops->count_write(counter, count, &val);
> + err = counter->ops->count_write(counter, count, val);
> if (err)
> return err;
>
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..7e40796598a6 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -300,24 +300,6 @@ struct counter_signal_read_value {
> size_t len;
> };
>
> -/**
> - * struct counter_count_read_value - Opaque Count read value
> - * @buf: string representation of Count read value
> - * @len: length of string in @buf
> - */
> -struct counter_count_read_value {
> - char *buf;
> - size_t len;
> -};
> -
> -/**
> - * struct counter_count_write_value - Opaque Count write value
> - * @buf: string representation of Count write value
> - */
> -struct counter_count_write_value {
> - const char *buf;
> -};
> -
> /**
> * struct counter_ops - Callbacks from driver
> * @signal_read: optional read callback for Signal attribute. The read
> @@ -328,15 +310,10 @@ struct counter_count_write_value {
> * signal_read callback.
> * @count_read: optional read callback for Count attribute. The read
> * value of the respective Count should be passed back via
> - * the val parameter. val points to an opaque type which
> - * should be set only by calling the
> - * counter_count_read_value_set function from within the
> - * count_read callback.
> + * the val parameter.
> * @count_write: optional write callback for Count attribute. The write
> * value for the respective Count is passed in via the val
> - * parameter. val points to an opaque type which should be
> - * accessed only by calling the
> - * counter_count_write_value_get function.
> + * parameter.
> * @function_get: function to get the current count function mode. Returns
> * 0 on success and negative error code on error. The index
> * of the respective Count's returned function mode should
> @@ -357,11 +334,9 @@ struct counter_ops {
> struct counter_signal *signal,
> struct counter_signal_read_value *val);
> int (*count_read)(struct counter_device *counter,
> - struct counter_count *count,
> - struct counter_count_read_value *val);
> + struct counter_count *count, unsigned long *val);
> int (*count_write)(struct counter_device *counter,
> - struct counter_count *count,
> - struct counter_count_write_value *val);
> + struct counter_count *count, unsigned long val);
> int (*function_get)(struct counter_device *counter,
> struct counter_count *count, size_t *function);
> int (*function_set)(struct counter_device *counter,
> @@ -486,19 +461,9 @@ enum counter_signal_value_type {
> COUNTER_SIGNAL_LEVEL = 0
> };
>
> -enum counter_count_value_type {
> - COUNTER_COUNT_POSITION = 0,
> -};
> -
> void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> const enum counter_signal_value_type type,
> void *const data);
> -void counter_count_read_value_set(struct counter_count_read_value *const val,
> - const enum counter_count_value_type type,
> - void *const data);
> -int counter_count_write_value_get(void *const data,
> - const enum counter_count_value_type type,
> - const struct counter_count_write_value *const val);
>
> int counter_register(struct counter_device *const counter);
> void counter_unregister(struct counter_device *const counter);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/4] counter: Simplify the count_read and count_write callbacks
2019-09-15 13:39 ` Jonathan Cameron
@ 2019-09-15 13:47 ` Jonathan Cameron
2019-09-15 13:58 ` William Breathitt Gray
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2019-09-15 13:47 UTC (permalink / raw)
To: William Breathitt Gray; +Cc: linux-kernel, linux-iio
On Sun, 15 Sep 2019 14:39:17 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> On Sun, 15 Sep 2019 14:57:56 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
>
> > The count_read and count_write callbacks are simplified to pass val as
> > unsigned long rather than as an opaque data structure. The opaque
> > counter_count_read_value and counter_count_write_value structures,
> > counter_count_value_type enum, and relevant counter_count_read_value_set
> > and counter_count_write_value_get functions, are removed as they are no
> > longer used.
> >
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
>
> Seems like a sensible bit of excessive abstraction removal to me. I'm not
> totally sure why these got so complex in the first place though.
Ah. I should have read the cover letter rather than just diving in the code :)
All explained there I see.
>
> Can you recall the reason as it might help to judge why we no longer
> think the same?
>
> Thanks,
>
> Jonathan
> > ---
> > drivers/counter/counter.c | 66 +++++----------------------------------
> > include/linux/counter.h | 43 +++----------------------
> > 2 files changed, 12 insertions(+), 97 deletions(-)
> >
> > diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> > index 106bc7180cd8..1d08f1437b1b 100644
> > --- a/drivers/counter/counter.c
> > +++ b/drivers/counter/counter.c
> > @@ -246,60 +246,6 @@ void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> > }
> > EXPORT_SYMBOL_GPL(counter_signal_read_value_set);
> >
> > -/**
> > - * counter_count_read_value_set - set counter_count_read_value data
> > - * @val: counter_count_read_value structure to set
> > - * @type: property Count data represents
> > - * @data: Count data
> > - *
> > - * This function sets an opaque counter_count_read_value structure with the
> > - * provided Count data.
> > - */
> > -void counter_count_read_value_set(struct counter_count_read_value *const val,
> > - const enum counter_count_value_type type,
> > - void *const data)
> > -{
> > - switch (type) {
> > - case COUNTER_COUNT_POSITION:
> > - val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
> > - break;
> > - default:
> > - val->len = 0;
> > - }
> > -}
> > -EXPORT_SYMBOL_GPL(counter_count_read_value_set);
> > -
> > -/**
> > - * counter_count_write_value_get - get counter_count_write_value data
> > - * @data: Count data
> > - * @type: property Count data represents
> > - * @val: counter_count_write_value structure containing data
> > - *
> > - * This function extracts Count data from the provided opaque
> > - * counter_count_write_value structure and stores it at the address provided by
> > - * @data.
> > - *
> > - * RETURNS:
> > - * 0 on success, negative error number on failure.
> > - */
> > -int counter_count_write_value_get(void *const data,
> > - const enum counter_count_value_type type,
> > - const struct counter_count_write_value *const val)
> > -{
> > - int err;
> > -
> > - switch (type) {
> > - case COUNTER_COUNT_POSITION:
> > - err = kstrtoul(val->buf, 0, data);
> > - if (err)
> > - return err;
> > - break;
> > - }
> > -
> > - return 0;
> > -}
> > -EXPORT_SYMBOL_GPL(counter_count_write_value_get);
> > -
> > struct counter_attr_parm {
> > struct counter_device_attr_group *group;
> > const char *prefix;
> > @@ -788,13 +734,13 @@ static ssize_t counter_count_show(struct device *dev,
> > const struct counter_count_unit *const component = devattr->component;
> > struct counter_count *const count = component->count;
> > int err;
> > - struct counter_count_read_value val = { .buf = buf };
> > + unsigned long val;
> >
> > err = counter->ops->count_read(counter, count, &val);
> > if (err)
> > return err;
> >
> > - return val.len;
> > + return sprintf(buf, "%lu\n", val);
> > }
> >
> > static ssize_t counter_count_store(struct device *dev,
> > @@ -806,9 +752,13 @@ static ssize_t counter_count_store(struct device *dev,
> > const struct counter_count_unit *const component = devattr->component;
> > struct counter_count *const count = component->count;
> > int err;
> > - struct counter_count_write_value val = { .buf = buf };
> > + unsigned long val;
> > +
> > + err = kstrtoul(buf, 0, &val);
> > + if (err)
> > + return err;
> >
> > - err = counter->ops->count_write(counter, count, &val);
> > + err = counter->ops->count_write(counter, count, val);
> > if (err)
> > return err;
> >
> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index a061cdcdef7c..7e40796598a6 100644
> > --- a/include/linux/counter.h
> > +++ b/include/linux/counter.h
> > @@ -300,24 +300,6 @@ struct counter_signal_read_value {
> > size_t len;
> > };
> >
> > -/**
> > - * struct counter_count_read_value - Opaque Count read value
> > - * @buf: string representation of Count read value
> > - * @len: length of string in @buf
> > - */
> > -struct counter_count_read_value {
> > - char *buf;
> > - size_t len;
> > -};
> > -
> > -/**
> > - * struct counter_count_write_value - Opaque Count write value
> > - * @buf: string representation of Count write value
> > - */
> > -struct counter_count_write_value {
> > - const char *buf;
> > -};
> > -
> > /**
> > * struct counter_ops - Callbacks from driver
> > * @signal_read: optional read callback for Signal attribute. The read
> > @@ -328,15 +310,10 @@ struct counter_count_write_value {
> > * signal_read callback.
> > * @count_read: optional read callback for Count attribute. The read
> > * value of the respective Count should be passed back via
> > - * the val parameter. val points to an opaque type which
> > - * should be set only by calling the
> > - * counter_count_read_value_set function from within the
> > - * count_read callback.
> > + * the val parameter.
> > * @count_write: optional write callback for Count attribute. The write
> > * value for the respective Count is passed in via the val
> > - * parameter. val points to an opaque type which should be
> > - * accessed only by calling the
> > - * counter_count_write_value_get function.
> > + * parameter.
> > * @function_get: function to get the current count function mode. Returns
> > * 0 on success and negative error code on error. The index
> > * of the respective Count's returned function mode should
> > @@ -357,11 +334,9 @@ struct counter_ops {
> > struct counter_signal *signal,
> > struct counter_signal_read_value *val);
> > int (*count_read)(struct counter_device *counter,
> > - struct counter_count *count,
> > - struct counter_count_read_value *val);
> > + struct counter_count *count, unsigned long *val);
> > int (*count_write)(struct counter_device *counter,
> > - struct counter_count *count,
> > - struct counter_count_write_value *val);
> > + struct counter_count *count, unsigned long val);
> > int (*function_get)(struct counter_device *counter,
> > struct counter_count *count, size_t *function);
> > int (*function_set)(struct counter_device *counter,
> > @@ -486,19 +461,9 @@ enum counter_signal_value_type {
> > COUNTER_SIGNAL_LEVEL = 0
> > };
> >
> > -enum counter_count_value_type {
> > - COUNTER_COUNT_POSITION = 0,
> > -};
> > -
> > void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> > const enum counter_signal_value_type type,
> > void *const data);
> > -void counter_count_read_value_set(struct counter_count_read_value *const val,
> > - const enum counter_count_value_type type,
> > - void *const data);
> > -int counter_count_write_value_get(void *const data,
> > - const enum counter_count_value_type type,
> > - const struct counter_count_write_value *const val);
> >
> > int counter_register(struct counter_device *const counter);
> > void counter_unregister(struct counter_device *const counter);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC 1/4] counter: Simplify the count_read and count_write callbacks
2019-09-15 13:47 ` Jonathan Cameron
@ 2019-09-15 13:58 ` William Breathitt Gray
0 siblings, 0 replies; 8+ messages in thread
From: William Breathitt Gray @ 2019-09-15 13:58 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-kernel, linux-iio
On Sun, Sep 15, 2019 at 02:47:00PM +0100, Jonathan Cameron wrote:
> On Sun, 15 Sep 2019 14:39:17 +0100
> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>
> > On Sun, 15 Sep 2019 14:57:56 +0900
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >
> > > The count_read and count_write callbacks are simplified to pass val as
> > > unsigned long rather than as an opaque data structure. The opaque
> > > counter_count_read_value and counter_count_write_value structures,
> > > counter_count_value_type enum, and relevant counter_count_read_value_set
> > > and counter_count_write_value_get functions, are removed as they are no
> > > longer used.
> > >
> > > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> >
> > Seems like a sensible bit of excessive abstraction removal to me. I'm not
> > totally sure why these got so complex in the first place though.
> Ah. I should have read the cover letter rather than just diving in the code :)
> All explained there I see.
>
> >
> > Can you recall the reason as it might help to judge why we no longer
> > think the same?
> >
> > Thanks,
> >
> > Jonathan
The cover letter probably explains it well enough, but it may be good
anyway for posterity to add on a bit about the origins of the opaque
structures.
If I recall correctly, it was from early on when I had a dedicated set
of functions for "quadrature counters" as opposed to "simple counters"
-- eventually we abandoned that design and decided instead to keep the
interface simple since devices could be represented robustly enough with
just the core Count, Signal, and Synapse components.
I decided to keep the opaque structures anyway in the hope that they
could be used in the future to restrict the attributes exposed to a
certain set of "counter types" defined for the Counter subsystems; i.e.
"position" counters would only expose spatial coordinates, "tally"
counters would expose tally units, etc.
However, I've come to realized that this type of organization is best
left to the userspace application and that the kernelspace code should
instead be focused on maintaining a simple and robust interface for
representing the core concept of a "counter" device.
William Breathitt Gray
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 2/4] counter: Simplify the signal_read callback
2019-09-15 5:57 [RFC 0/4] counter: Simplify count_read/count_write/signal_read William Breathitt Gray
2019-09-15 5:57 ` [RFC 1/4] counter: Simplify the count_read and count_write callbacks William Breathitt Gray
@ 2019-09-15 5:57 ` William Breathitt Gray
2019-09-15 5:57 ` [RFC 3/4] docs: driver-api: generic-counter: Update Count and Signal data types William Breathitt Gray
2019-09-15 5:57 ` [RFC 4/4] counter: 104-quad-8: Update count_read/count_write/signal_read callbacks William Breathitt Gray
3 siblings, 0 replies; 8+ messages in thread
From: William Breathitt Gray @ 2019-09-15 5:57 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, William Breathitt Gray
The signal_read callback is simplified to pass val as a
counter_signal_val enum rather than as an opaque data structure. The
opaque counter_signal_read_value structure and relevant
counter_signal_read_value_set function are removed as they are no longer
used. In addition, the counter_signal_level enum is replaced by the
similar counter_signal_value enum.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
drivers/counter/counter.c | 35 +++++++----------------------------
include/linux/counter.h | 31 +++++--------------------------
2 files changed, 12 insertions(+), 54 deletions(-)
diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 1d08f1437b1b..6a683d086008 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -220,32 +220,6 @@ ssize_t counter_device_enum_available_read(struct counter_device *counter,
}
EXPORT_SYMBOL_GPL(counter_device_enum_available_read);
-static const char *const counter_signal_level_str[] = {
- [COUNTER_SIGNAL_LEVEL_LOW] = "low",
- [COUNTER_SIGNAL_LEVEL_HIGH] = "high"
-};
-
-/**
- * counter_signal_read_value_set - set counter_signal_read_value data
- * @val: counter_signal_read_value structure to set
- * @type: property Signal data represents
- * @data: Signal data
- *
- * This function sets an opaque counter_signal_read_value structure with the
- * provided Signal data.
- */
-void counter_signal_read_value_set(struct counter_signal_read_value *const val,
- const enum counter_signal_value_type type,
- void *const data)
-{
- if (type == COUNTER_SIGNAL_LEVEL)
- val->len = sprintf(val->buf, "%s\n",
- counter_signal_level_str[*(enum counter_signal_level *)data]);
- else
- val->len = 0;
-}
-EXPORT_SYMBOL_GPL(counter_signal_read_value_set);
-
struct counter_attr_parm {
struct counter_device_attr_group *group;
const char *prefix;
@@ -315,6 +289,11 @@ struct counter_signal_unit {
struct counter_signal *signal;
};
+static const char *const counter_signal_value_str[] = {
+ [COUNTER_SIGNAL_LOW] = "low",
+ [COUNTER_SIGNAL_HIGH] = "high"
+};
+
static ssize_t counter_signal_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -323,13 +302,13 @@ static ssize_t counter_signal_show(struct device *dev,
const struct counter_signal_unit *const component = devattr->component;
struct counter_signal *const signal = component->signal;
int err;
- struct counter_signal_read_value val = { .buf = buf };
+ enum counter_signal_value val;
err = counter->ops->signal_read(counter, signal, &val);
if (err)
return err;
- return val.len;
+ return sprintf(buf, "%s\n", counter_signal_value_str[val]);
}
struct counter_name_unit {
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 7e40796598a6..32fb4d8cc3fd 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -290,24 +290,16 @@ struct counter_device_state {
const struct attribute_group **groups;
};
-/**
- * struct counter_signal_read_value - Opaque Signal read value
- * @buf: string representation of Signal read value
- * @len: length of string in @buf
- */
-struct counter_signal_read_value {
- char *buf;
- size_t len;
+enum counter_signal_value {
+ COUNTER_SIGNAL_LOW = 0,
+ COUNTER_SIGNAL_HIGH
};
/**
* struct counter_ops - Callbacks from driver
* @signal_read: optional read callback for Signal attribute. The read
* value of the respective Signal should be passed back via
- * the val parameter. val points to an opaque type which
- * should be set only by calling the
- * counter_signal_read_value_set function from within the
- * signal_read callback.
+ * the val parameter.
* @count_read: optional read callback for Count attribute. The read
* value of the respective Count should be passed back via
* the val parameter.
@@ -332,7 +324,7 @@ struct counter_signal_read_value {
struct counter_ops {
int (*signal_read)(struct counter_device *counter,
struct counter_signal *signal,
- struct counter_signal_read_value *val);
+ enum counter_signal_value *val);
int (*count_read)(struct counter_device *counter,
struct counter_count *count, unsigned long *val);
int (*count_write)(struct counter_device *counter,
@@ -452,19 +444,6 @@ struct counter_device {
void *priv;
};
-enum counter_signal_level {
- COUNTER_SIGNAL_LEVEL_LOW = 0,
- COUNTER_SIGNAL_LEVEL_HIGH
-};
-
-enum counter_signal_value_type {
- COUNTER_SIGNAL_LEVEL = 0
-};
-
-void counter_signal_read_value_set(struct counter_signal_read_value *const val,
- const enum counter_signal_value_type type,
- void *const data);
-
int counter_register(struct counter_device *const counter);
void counter_unregister(struct counter_device *const counter);
int devm_counter_register(struct device *dev,
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC 3/4] docs: driver-api: generic-counter: Update Count and Signal data types
2019-09-15 5:57 [RFC 0/4] counter: Simplify count_read/count_write/signal_read William Breathitt Gray
2019-09-15 5:57 ` [RFC 1/4] counter: Simplify the count_read and count_write callbacks William Breathitt Gray
2019-09-15 5:57 ` [RFC 2/4] counter: Simplify the signal_read callback William Breathitt Gray
@ 2019-09-15 5:57 ` William Breathitt Gray
2019-09-15 5:57 ` [RFC 4/4] counter: 104-quad-8: Update count_read/count_write/signal_read callbacks William Breathitt Gray
3 siblings, 0 replies; 8+ messages in thread
From: William Breathitt Gray @ 2019-09-15 5:57 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, William Breathitt Gray
Count data is now always represented as an unsigned integer, while
Signal data is either SIGNAL_LOW or SIGNAL_HIGH.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Documentation/driver-api/generic-counter.rst | 22 +++++++-------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
index 8382f01a53e3..161652fc1025 100644
--- a/Documentation/driver-api/generic-counter.rst
+++ b/Documentation/driver-api/generic-counter.rst
@@ -39,10 +39,7 @@ There are three core components to a counter:
COUNT
-----
A Count represents the count data for a set of Signals. The Generic
-Counter interface provides the following available count data types:
-
-* COUNT_POSITION:
- Unsigned integer value representing position.
+Counter interface represents the count data as an unsigned integer.
A Count has a count function mode which represents the update behavior
for the count data. The Generic Counter interface provides the following
@@ -93,19 +90,16 @@ SIGNAL
A Signal represents a counter input data; this is the input data that is
evaluated by the counter to determine the count data; e.g. a quadrature
signal output line of a rotary encoder. Not all counter devices provide
-user access to the Signal data.
-
-The Generic Counter interface provides the following available signal
-data types for when the Signal data is available for user access:
+user access to the Signal data, so exposure is optional for drivers.
-* SIGNAL_LEVEL:
- Signal line state level. The following states are possible:
+When the Signal data is available for user access, the Generic Counter
+interface provides the following available signal values:
- - SIGNAL_LEVEL_LOW:
- Signal line is in a low state.
+* SIGNAL_LOW:
+ Signal line is in a low state.
- - SIGNAL_LEVEL_HIGH:
- Signal line is in a high state.
+* SIGNAL_HIGH:
+ Signal line is in a high state.
A Signal may be associated with one or more Counts.
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC 4/4] counter: 104-quad-8: Update count_read/count_write/signal_read callbacks
2019-09-15 5:57 [RFC 0/4] counter: Simplify count_read/count_write/signal_read William Breathitt Gray
` (2 preceding siblings ...)
2019-09-15 5:57 ` [RFC 3/4] docs: driver-api: generic-counter: Update Count and Signal data types William Breathitt Gray
@ 2019-09-15 5:57 ` William Breathitt Gray
3 siblings, 0 replies; 8+ messages in thread
From: William Breathitt Gray @ 2019-09-15 5:57 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, William Breathitt Gray
The count_read and count_write callbacks pass unsigned long now, while
the signal_read callback passes an enum counter_signal_value.
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
drivers/counter/104-quad-8.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 00b113f4b958..17e67a84777d 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -562,11 +562,10 @@ static const struct iio_chan_spec quad8_channels[] = {
};
static int quad8_signal_read(struct counter_device *counter,
- struct counter_signal *signal, struct counter_signal_read_value *val)
+ struct counter_signal *signal, enum counter_signal_value *val)
{
const struct quad8_iio *const priv = counter->priv;
unsigned int state;
- enum counter_signal_level level;
/* Only Index signal levels can be read */
if (signal->id < 16)
@@ -575,22 +574,19 @@ static int quad8_signal_read(struct counter_device *counter,
state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS)
& BIT(signal->id - 16);
- level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW;
-
- counter_signal_read_value_set(val, COUNTER_SIGNAL_LEVEL, &level);
+ *val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
return 0;
}
static int quad8_count_read(struct counter_device *counter,
- struct counter_count *count, struct counter_count_read_value *val)
+ struct counter_count *count, unsigned long *val)
{
const struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
unsigned int flags;
unsigned int borrow;
unsigned int carry;
- unsigned long position;
int i;
flags = inb(base_offset + 1);
@@ -598,36 +594,27 @@ static int quad8_count_read(struct counter_device *counter,
carry = !!(flags & QUAD8_FLAG_CT);
/* Borrow XOR Carry effectively doubles count range */
- position = (unsigned long)(borrow ^ carry) << 24;
+ *val = (unsigned long)(borrow ^ carry) << 24;
/* Reset Byte Pointer; transfer Counter to Output Latch */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_CNTR_OUT,
base_offset + 1);
for (i = 0; i < 3; i++)
- position |= (unsigned long)inb(base_offset) << (8 * i);
-
- counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &position);
+ *val |= (unsigned long)inb(base_offset) << (8 * i);
return 0;
}
static int quad8_count_write(struct counter_device *counter,
- struct counter_count *count, struct counter_count_write_value *val)
+ struct counter_count *count, unsigned long val)
{
const struct quad8_iio *const priv = counter->priv;
const int base_offset = priv->base + 2 * count->id;
- int err;
- unsigned long position;
int i;
- err = counter_count_write_value_get(&position, COUNTER_COUNT_POSITION,
- val);
- if (err)
- return err;
-
/* Only 24-bit values are supported */
- if (position > 0xFFFFFF)
+ if (val > 0xFFFFFF)
return -EINVAL;
/* Reset Byte Pointer */
@@ -635,7 +622,7 @@ static int quad8_count_write(struct counter_device *counter,
/* Counter can only be set via Preset Register */
for (i = 0; i < 3; i++)
- outb(position >> (8 * i), base_offset);
+ outb(val >> (8 * i), base_offset);
/* Transfer Preset Register to Counter */
outb(QUAD8_CTR_RLD | QUAD8_RLD_PRESET_CNTR, base_offset + 1);
@@ -644,9 +631,9 @@ static int quad8_count_write(struct counter_device *counter,
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP, base_offset + 1);
/* Set Preset Register back to original value */
- position = priv->preset[count->id];
+ val = priv->preset[count->id];
for (i = 0; i < 3; i++)
- outb(position >> (8 * i), base_offset);
+ outb(val >> (8 * i), base_offset);
/* Reset Borrow, Carry, Compare, and Sign flags */
outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_FLAGS, base_offset + 1);
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread