| Message ID | 20190915055759.408690-2-vilhelm.gray@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | counter: Simplify count_read/count_write/signal_read | expand |
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);
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); >
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
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);
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(-)