On 04/05/2022 12:35, Marc Zyngier wrote:
> Hi Jon,
>
> On Wed, 04 May 2022 12:19:36 +0100,
> Jon Hunter <[email protected]> wrote:
>>
>> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
>> immutable") added a warning to indicate if the gpiolib is altering the
>> internals of irqchips. Following this change the following warning is
>> now observed for the max77620 gpio driver ...
>>
>> WARNING KERN gpio gpiochip0: (max77620-gpio): not an immutable chip,
>> please consider fixing it!
>>
>> Fix the above warning by making the max77620 gpio driver immutable.
>
> Thanks for looking into this. Comments below.
>
>>
>> Signed-off-by: Jon Hunter <[email protected]>
>> ---
>> This warning is observed with v5.18-rc5 and so it would be great if
>> we can fix for v5.18. This is triggering a kernel warning test failure
>> on one of our platforms.
>
> I'm surprised. This is definitely *not* 5.18 material, and I can't see
> the patches in Linus' tree. Are you sure you're not running -next
> instead?
Ah, you are correct, it was -next and not v5.18-rc5!
>>
>> drivers/gpio/gpio-max77620.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-max77620.c b/drivers/gpio/gpio-max77620.c
>> index ebf9dea6546b..aa92658780d6 100644
>> --- a/drivers/gpio/gpio-max77620.c
>> +++ b/drivers/gpio/gpio-max77620.c
>> @@ -119,14 +119,23 @@ static void max77620_gpio_bus_sync_unlock(struct irq_data *data)
>> mutex_unlock(&gpio->buslock);
>> }
>>
>> -static struct irq_chip max77620_gpio_irqchip = {
>> +static void max77620_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
>> +{
>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>> +
>> + seq_printf(p, dev_name(gc->parent));
>> +}
>> +
>
> I'd rather you don't do this. This was added as a band-aid for drivers
> that display a device-specific string in /proc/interrupts, and that we
> cannot fix because the string is in effect an ABI.
>
> Here, you're going in the opposite direction (making the name
> device-specific while it was constant so far). This has the same
> ABI-breaking effect, and we shouldn't do that. It is also rather
> pointless, as we already have all the required debugging information
> in debugfs (and I assume that this is the reason this is added).
OK, I will drop this.
>> +static const struct irq_chip max77620_gpio_irqchip = {
>> .name = "max77620-gpio",
>> .irq_mask = max77620_gpio_irq_mask,
>> .irq_unmask = max77620_gpio_irq_unmask,
>
> You seem to be missing the updates for these two functions. Please see
> the updated documentation in commit 5644b66a9c63 ("Documentation:
> Update the recommended pattern for GPIO irqchips").
Yes I see. OK, I will add that.
Thanks! Jon
--
nvpublic