[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-sound
Subject:    Re: [PATCH 2/4] hid-lenovo: Add support for X1 Tablet cover LED control
From:       Dennis Wassenberg <dennis.wassenberg () secunet ! com>
Date:       2016-12-19 10:43:53
Message-ID: 80c99979-0caa-bbd2-d750-96d546618e58 () secunet ! com
[Download RAW message or body]

Hi Jiri,

Thanks for your comments. I added comments in-lined.

Best regards,

Dennis

On 19.12.2016 10:54, Jiri Kosina wrote:
> Hi Dennis,
> 
> thanks a lot for the patch.
> 
> On Fri, 9 Dec 2016, Dennis Wassenberg wrote:
> 
> > +int hid_lenovo_led_set(enum hid_lenovo_led_type led, bool on)
> > +{
> > +	struct led_classdev *dev = NULL;
> > +	struct lenovo_led_list_entry *entry;
> > +
> > +	if (led >= HID_LENOVO_LED_MAX)
> > +		return -EINVAL;
> > +
> > +	hid_lenovo_initial_leds[led] = on ? LED_FULL : LED_OFF;
> > +
> > +	list_for_each_entry(entry, &hid_lenovo_leds, list) {
> > +		if (entry->type == led) {
> > +			dev = entry->dev;
> > +			break;
> > +		}
> > +	}
> 
> How exactly is this synchronized against lenovo_remove_tpx1cover()?
> 
In case of that the tpx1cover is disconnected it will be removed from hid_lenovo_leds \
list. That means not included at the hid_lenovo_leds list. In this case dev is still \
NULL and the function will return -ENODEV. The static array hid_lenovo_initial_leds \
is still set to store the current state of a LED type. This is used to set the LED \
appropriately if the tpx1cover is replugged.

Maybe I should add a mutex to protect the hid_lenovo_leds list operations in \
hid-lenovo.c to fix the case if a unplug occurred concurrently to setting an led?!

> > +
> > +	if (!dev)
> > +		return -ENODEV;
> > +
> > +	if (!dev->brightness_set)
> > +		return -ENODEV;
> > +
> > +	dev->brightness_set(dev, on ? LED_FULL : LED_OFF);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(hid_lenovo_led_set);
> 
> Does this really need to be exported to the whole universe? (I guess this 
> will be further discussed in 4/4).

No, it is sufficient if thinkpad-helper can access it.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-sound" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]