Messages in this thread | | | Date | Fri, 25 Dec 2020 12:30:04 -0500 | From | William Breathitt Gray <> | Subject | Re: [PATCH v6 3/5] counter: Add character device interface |
| |
Hi David,
I agree with your suggested changes -- just a couple select comments following below.
On Sun, Dec 13, 2020 at 05:58:26PM -0600, David Lechner wrote: > > +static int counter_add_watch(struct counter_device *const counter, > > + const unsigned long arg) > > +{
[...]
> > + > > +dummy_component: > > + comp_node.component = watch.component; > > > In my experiments, I added a events_validate driver callback here to > validate each event as it is added. This way the user can know exactly > which event caused the problem rather than waiting for the event_config > callback.
Yes, this is a good idea and I have use for this in the 104-quad-8 driver as well. I'm going to name this "watch_validate" however, because I need to validate the requested channel as well as the requested event here (both part of the struct counter_watch).
> > diff --git a/include/linux/counter.h b/include/linux/counter.h > > index 3f3f8ba6c1b4..98cd7c035968 100644 > > --- a/include/linux/counter.h > > > > > > +/** > > + * struct counter_event_node - Counter Event node > > + * @l: list of current watching Counter events > > + * @event: event that triggers > > + * @channel: event channel > > + * @comp_list: list of components to watch when event triggers > > + */ > > +struct counter_event_node { > > + struct list_head l; > > + u8 event; > > + u8 channel; > > + struct list_head comp_list; > > +}; > > + > > > Unless this is needed outside of the drivers/counter/ directory, I > would suggest putting it in drivers/counter/counter-chrdev.h instead > of include/linux/counter.h.
The "events_list" member of the struct counter_device is a list of struct counter_event_node. The events_configure() callback should parse through this list to determine the current events configuration request. As such, driver authors will need this structure available via include/linux/counter.h so they can parse "events_list".
William Breathitt Gray [unhandled content-type:application/pgp-signature] | |