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
signature.asc
Description: PGP signature
