Re: [PATCH V3] leds: trigger: Introduce an USB port trigger

[Matthias Brugger]

On 24/08/16 13:02, RafaÅ MiÅecki wrote:
> On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@xxxxxxxx> wrote:
> > On 24/08/16 00:03, RafaÅ MiÅecki wrote:

[...]

> > > +static int usbport_trig_notify(struct notifier_block *nb, unsigned long
> > > action,
> > > +                              void *data)
> > > +{
> > > +       struct usbport_trig_data *usbport_data =
> > > +               container_of(nb, struct usbport_trig_data, nb);
> > > +       struct led_classdev *led_cdev = usbport_data->led_cdev;
> > > +
> > > +       switch (action) {
> > > +       case USB_DEVICE_ADD:
> > > +               if (usbport_trig_usb_dev_observed(usbport_data, data)) {
> >
> >
> > Maybe we should switch this and fist see if the usbport is observed before
> > evaluating the action. Also cast data to "struct usb_device *" to make that
> > clear.
>
> I'm aware there is one duplicated line of code, I did to first
> evaluate very quick test (checking unsigned long value), then iterate
> over ports & keep only 1 switch block. I could move
> usbport_trig_usb_dev_observed call up, but it would be executed for
> other actions as well (currently just USB_BUS_ADD and USB_BUS_REMOVE).

Ok. I'm a USB noop but from my understanding the notifier is only called 
when a device or a hub gets added/removed. So this shouldn't happen that 
much. Therefor it has no impact if we check if the usb device is in the 
observer list for all actions.

> > > +                       if (usbport_data->count++ == 0)
> >
> >
> > I'm a bit puzzled. I think:
> > if (++usbport_data->count > 0)
> > makes this more consistent with the remove case.
>
> Your condition would be always true (as we don't use negative
> numbers). The point is to enable LED only if it was disabled before.
> So I need to increase counter unconditionally but enable LED only if
> initial value (before increasing it) was 0.

Got it. My personal opinion is, that adding one line for 
incrementing/decrementing the counter would help to make this 
crystal-clear to everyone (at least to me :)

Cheers,
Matthias

> > > +module_init(usbport_trig_init);
> > > +module_exit(usbport_trig_exit);
> > > +
> > > +MODULE_AUTHOR("RafaÅ MiÅecki <rafal@milecki@pl>");
> >
> >
> > Nit: rafal@xxxxxxxxxx
>
> Oops, thanks!