From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756237AbdKGQuI (ORCPT ); Tue, 7 Nov 2017 11:50:08 -0500 Received: from lelnx193.ext.ti.com ([198.47.27.77]:34169 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755176AbdKGQuF (ORCPT ); Tue, 7 Nov 2017 11:50:05 -0500 Subject: Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration To: Thierry Reding CC: Linus Walleij , Jonathan Hunter , , , References: <20171102174941.3461-1-thierry.reding@gmail.com> <20171106111807.GA6756@ulmo> <20171107111344.GA7314@ulmo> <20171107115238.GB7314@ulmo> From: Grygorii Strashko Message-ID: <87fd58e7-8f46-5660-9c23-189e656529e0@ti.com> Date: Tue, 7 Nov 2017 10:49:58 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171107115238.GB7314@ulmo> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [128.247.59.147] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/07/2017 05:52 AM, Thierry Reding wrote: > 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. True. And this I've called my approach not ideal, but I do not see other way to do it :( - that's price to pay for gpioirq chip initialization integration in gpiochip_add_data() which limits APIs variation used by GPIO drivers. Any other opinions, thoughts? -- regards, -grygorii