Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

On Tue, Nov 07, 2017 at 12:13:44PM +0100, Thierry Reding wrote:
> On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote:
> > On 11/06/2017 05:18 AM, Thierry Reding wrote:
> > > On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote:
[...]
> > >> @@ -312,8 +321,29 @@ struct gpio_chip {
> > >>   extern const char *gpiochip_is_requested(struct gpio_chip *chip,
> > >>   			unsigned offset);
> > >>   
> > >> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data,
> > >> +				 struct  *irq_lock_key);
> > >> +#ifdef CONFIG_LOCKDEP
> > >> +/*
> > >> + * Lockdep requires that each irqchip instance be created with a
> > >> + * unique key so as to avoid unnecessary warnings. This upfront
> > >> + * boilerplate static inlines provides such a key for each
> > >> + * unique instance which is created now from inside gpiochip_add_data_key().
> > >> + */
> > >> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data)
> > >> +{
> > >> +	static struct lock_class_key key;
> > >> +
> > >> +	return gpiochip_add_data_key(chip, data, key);
> > >> +}
> > > 
> > > This looks like a neat improvement, but I think it can be done in a
> > > follow-up to remove the boilerplate in drivers.
> > 
> > Can't agree here - it better to be considered now. 
> > Now only two GPIO drivers define lock_class_key:
> > ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class;
> > ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class;
> > 
> > and these drivers do not use gpioirq framework (your tegra driver will be the third).
> > 
> > So, if proposed changes will be applied all drivers switched to use it will need to define
> > its own lock_class_key again and it will be step back.
> 
> I think this would be a minor, mostly mechanical refactoring to do as
> follow-up. But since you feel very strongly about it, I'll add that into
> the series.

After implementing this, I'm having second thoughts. We've got a bunch
of drivers calling gpiochip_add_data() that never register an IRQ chip
but which will each add a struct lock_class_key after this change, and
it will never be used. Now, struct lock_class_key is only 8 bytes big,
so maybe this isn't a big deal, but it still seems like a waste.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]