Re: [PATCH] HID: hig-lg, hif-lg4ff: Allow for custom device properties to be stored as private driver data

On Mon, 12 Mar 2012, Michal Malý wrote:

this is sort of a response to Simon Wood's recent G27 LED's patch but 
since the patch touches a completely different part of the Logitech 
driver I submitted it separately.

Michal,

thanks for the patch.

The main goal of this patch is to allow for some custom device specific 
properties like the rotation range to be stored as private driver data. 
Up until now the hid-lg treated the pointer to driver_data as unsigned 
long as stored some device quirks there. Because of the scope of the 
changes, Simon's implementation of the G27 LEDs code is unfortunately 
incompatible with this patch because it uses the linked list I'm trying 
to get rid of.

I hope Simon will be refreshing his patch to use LEDS_CLASS anyway.

The patch also introduces some spinlocking to hid-lg and hid-lg4ff to 
(hopefully) prevent any nasty race conditions you were concerned about.

From a cursory look the locking looks good (why do you need to disable 
IRQs while taking the lock? It doesn't seem to be taken in IRQ context 
anywhere, right?).
But it's somewhat difficult to review, as it's inter-mixed with other 
changes. Could you please split your patch in two independent patches, one 
adding proper locking, and second one implementing the drv_data change? 
Those changes don't have anything in common in principle, so it makes 
sense to have them separated.

Thanks.

-- 
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html