On Wed, Dec 30, 2020 at 02:37:19PM +0000, Jonathan Cameron wrote: > On Fri, 25 Dec 2020 19:15:34 -0500 > William Breathitt Gray <vilhelm.gray@xxxxxxxxx> wrote: > > > This is a reimplementation of the Generic Counter driver interface. > > There are no modifications to the Counter subsystem userspace interface, > > so existing userspace applications should continue to run seamlessly. > Hi William > > Been a while since I looked at this series. Its rather big and I'm lazy > (or busy depending on who I'm talking to :) > > Hmm. I'm a bit in two minds about how you should handle the huge amount of > description here. Some of it clearly belongs in the kernel docs (and some > is I think put there in a later patch). Other parts are specific to > this series, but I'm not 100% sure this much detail is really useful in the > git log. Note that we now have links to the threads for all patches applied > using b4 (which this will be) so it's fine to have really detailed stuff > in cover letters rather than the individual patches. I'll simplify the description here to something more succinct. > One thing that would be handy for review, might be if you put up a tree > somewhere with this applied and included a link. This is such a large set of changes that having a tree to checkout for review would be convenient. I typically push to my personal tree, so you can check out the changes there in the counter_chrdev_v* branches: https://gitlab.com/vilhelmgray/iio I'll include a link to it again in the cover letter for v8 when it's ready. > Mind you I don't feel that strongly about it if it you do want to maintain > it in the individual patch descriptions. > > I've been a bit lazy and not cropped this down as much as I ideally should > have done (to include only bits I'm commenting on). > > Anyhow, various minor things inline but this fundamentally looks fine to me. > > Jonathan > > > > > > Overview > > ======== > > > > The purpose of this patch is to internalize the sysfs interface code > > among the various counter drivers into a shared module. Counter drivers > > pass and take data natively (i.e. u8, u64, etc.) and the shared counter > > module handles the translation between the sysfs interface. > > Confusing statement. Between the sysfs interface and what? > Perhaps "handles the translation to/from the sysfs interface." Looks like I cut that line short by accident; it should read: "between the sysfs interface and the device drivers". I'll fix this up. > > This > > guarantees a standard userspace interface for all counter drivers, and > > helps generalize the Generic Counter driver ABI in order to support the > > Generic Counter chrdev interface (introduced in a subsequent patch) > > without significant changes to the existing counter drivers. > > > > A high-level view of how a count value is passed down from a counter > > driver is exemplified by the following. The driver callbacks are first > > registered to the Counter core component for use by the Counter > > userspace interface components: > > > > +----------------------------+ > > | Counter device driver | > > Looks like something snuck a tab in amongst your spaces. Ack. > > static int quad8_signal_read(struct counter_device *counter, > > - struct counter_signal *signal, enum counter_signal_value *val) > > + struct counter_signal *signal, > > + enum counter_signal_level *level) > > { > > const struct quad8_iio *const priv = counter->priv; > > unsigned int state; > > @@ -633,13 +634,13 @@ static int quad8_signal_read(struct counter_device *counter, > > state = inb(priv->base + QUAD8_REG_INDEX_INPUT_LEVELS) > > & BIT(signal->id - 16); > > > > - *val = (state) ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > > + *level = (state) ? COUNTER_SIGNAL_LEVEL_HIGH : COUNTER_SIGNAL_LEVEL_LOW; > > This bit of refactoring / renaming could have been split out as a precursor patch > I think. There may be other opportunities. Ack. I'll look around for additional changes I can pull out as precursor patches too. > > > > return 0; > > } > > > > static int quad8_count_read(struct counter_device *counter, > > - struct counter_count *count, unsigned long *val) > > + struct counter_count *count, u64 *val) > > Could the type change for val have been done as a precursor? I don't think we can pull this one out as a precursor unfortunately. Since unsigned long is passed in as pointer, we could get a type mismatch if we're on a 32-bit system. For this to work, it requires the u64 change in struct counter_ops and subsequent dependent code, so we'll have to keep it as part of this patch for now. > > @@ -785,18 +782,21 @@ static int quad8_function_set(struct counter_device *counter, > > *quadrature_mode = 1; > > > > switch (function) { > > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X1: > > + case COUNTER_FUNCTION_QUADRATURE_X1_A: > > *scale = 0; > > mode_cfg |= QUAD8_CMR_QUADRATURE_X1; > > break; > > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X2: > > + case COUNTER_FUNCTION_QUADRATURE_X2_A: > > *scale = 1; > > mode_cfg |= QUAD8_CMR_QUADRATURE_X2; > > break; > > - case QUAD8_COUNT_FUNCTION_QUADRATURE_X4: > > + case COUNTER_FUNCTION_QUADRATURE_X4: > > *scale = 2; > > mode_cfg |= QUAD8_CMR_QUADRATURE_X4; > > break; > > + default: > > + mutex_unlock(&priv->lock); > > + return -EINVAL; > > This looks like a sensible precaution / cleanup but could have been > done separately to the rest of the patch I think? Ack. > > @@ -1229,30 +1194,28 @@ static ssize_t quad8_count_ceiling_write(struct counter_device *counter, > > > > mutex_unlock(&priv->lock); > > > > - return len; > > + return -EINVAL; > > ? That looks like the good exit path to me. You're right, this should be a return 0. > > +/** > > + * counter_register - register Counter to the system > > + * @counter: pointer to Counter to register > > + * > > + * 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. > > Where easy to do it's worth documenting return values. Ack. > > +static void devm_counter_unregister(struct device *dev, void *res) > > +{ > > + counter_unregister(*(struct counter_device **)res); > > Rename this. It looks like it's a generic way of unwinding > devm_counter_register which it is definitely not... Ack. > > +/** > > + * struct counter_attribute - Counter sysfs attribute > > + * @dev_attr: device attribute for sysfs > > + * @l: node to add Counter attribute to attribute group list > > + * @comp: Counter component callbacks and data > > + * @scope: Counter scope of the attribute > > + * @parent: pointer to the parent component > > + */ > > +struct counter_attribute { > > + struct device_attribute dev_attr; > > + struct list_head l; > > + > > + struct counter_comp comp; > > + __u8 scope; > > Why not an enum? This should be enum; I missed it from the previous revision. > > + switch (a->comp.type) { > > + case COUNTER_COMP_FUNCTION: > > + return sprintf(buf, "%s\n", counter_function_str[data]); > > + case COUNTER_COMP_SIGNAL_LEVEL: > > + return sprintf(buf, "%s\n", counter_signal_value_str[data]); > > + case COUNTER_COMP_SYNAPSE_ACTION: > > + return sprintf(buf, "%s\n", counter_synapse_action_str[data]); > > + case COUNTER_COMP_ENUM: > > + return sprintf(buf, "%s\n", avail->strs[data]); > > + case COUNTER_COMP_COUNT_DIRECTION: > > + return sprintf(buf, "%s\n", counter_count_direction_str[data]); > > + case COUNTER_COMP_COUNT_MODE: > > + return sprintf(buf, "%s\n", counter_count_mode_str[data]); > > + default: > > Perhaps move the below return sprintf() up here? Ack. > > + break; > > + } > > + > > + return sprintf(buf, "%u\n", (unsigned int)data); > > +} > > + > > +static int find_in_string_array(u32 *const enum_item, const u32 *const enums, > > + const size_t num_enums, const char *const buf, > > + const char *const string_array[]) > > Please avoid defining such generically named functions. High chance of a clash > with something that turns up in generic headers sometime in the future. Ack. > > +static ssize_t enums_available_show(const u32 *const enums, > > + const size_t num_enums, > > + const char *const strs[], char *buf) > > +{ > > + size_t len = 0; > > + size_t index; > > + > > + for (index = 0; index < num_enums; index++) > > + len += sprintf(buf + len, "%s\n", strs[enums[index]]); > > Probably better to add protections on overrunning the buffer to this. > Sure it won't actually happen but that may not be obvious to someone reading > this code in future. > > Look at new sysfs_emit * family for this. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2efc459d06f1630001e3984854848a5647086232 Ack. > > +static ssize_t counter_comp_available_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + const struct counter_attribute *const a = to_counter_attribute(attr); > > + const struct counter_count *const count = a->parent; > > + const struct counter_synapse *const synapse = a->comp.priv; > > + const struct counter_available *const avail = a->comp.priv; > > + > > + switch (a->comp.type) { > > + case COUNTER_COMP_FUNCTION: > > + return enums_available_show(count->functions_list, > > + count->num_functions, > > + counter_function_str, buf); > > + case COUNTER_COMP_SYNAPSE_ACTION: > > + return enums_available_show(synapse->actions_list, > > + synapse->num_actions, > > + counter_synapse_action_str, buf); > > + case COUNTER_COMP_ENUM: > > + return strs_available_show(avail, buf); > > + case COUNTER_COMP_COUNT_MODE: > > + return enums_available_show(avail->enums, avail->num_items, > > + counter_count_mode_str, buf); > > + default: > > + break; > > Might as well return -EINVAL; here Ack. > > + /* Store list node */ > > + list_add(&counter_attr->l, &group->attr_list); > > + group->num_attr++; > > + > > + switch (comp->type) { > > + case COUNTER_COMP_FUNCTION: > > + case COUNTER_COMP_SYNAPSE_ACTION: > > + case COUNTER_COMP_ENUM: > > + case COUNTER_COMP_COUNT_MODE: > > + return counter_avail_attr_create(dev, group, comp, parent); > > + default: > > + break; > > return 0 in here. Also add a comment on why it isn't an error. Ack. > > +static int counter_sysfs_synapses_add(struct counter_device *const counter, > > + struct counter_attribute_group *const group, > > + struct counter_count *const count) > > +{ > > + const __u8 scope = COUNTER_SCOPE_COUNT; > > + struct device *const dev = &counter->dev; > > + size_t i; > > + struct counter_synapse *synapse; > > + size_t id; > > + struct counter_comp comp; > > + int err; > > + > > + /* Add each Synapse */ > > + for (i = 0; i < count->num_synapses; i++) { > Could reduce scope and make code a bit more readable by > pulling > > struct counter_synapse *synapse; > struct counter_comp comp; > size_t id; > > and maybe other things in here. Makes it clear their scope > is just within this loop. Ack. > > /** > > * struct counter_synapse - Counter Synapse node > > - * @action: index of current action mode > > * @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 > > Might have been nice to pull the cases that were purely capitalization out as > a separate patch immediately following this one. There aren't > a huge number, but from a review point of view it's a noop patch > so doesn't need the reviewer to be awake :) Ack. William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
