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

[Matthias Brugger <mbrugger@suse.com>]

On 24/08/16 13:02, Rafał Miłecki wrote:
> On 24 August 2016 at 12:49, Matthias Brugger <mbrugger@suse.com> 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@milecki.pl
>
> Oops, thanks!
>