linux-kernel - Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D7QLITNTXRUJ.3NA44E6PQMAUX@bootlin.com>
Date: Wed, 12 Feb 2025 17:08:56 +0100
From: "Mathieu Dubois-Briand" <mathieu.dubois-briand@...tlin.com>
To: "Andy Shevchenko" <andriy.shevchenko@...el.com>
Cc: "Lee Jones" <lee@...nel.org>, "Rob Herring" <robh@...nel.org>,
 "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley"
 <conor+dt@...nel.org>, "Kamel Bouhara" <kamel.bouhara@...tlin.com>, "Linus
 Walleij" <linus.walleij@...aro.org>, "Bartosz Golaszewski" <brgl@...ev.pl>,
 "Dmitry Torokhov" <dmitry.torokhov@...il.com>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-gpio@...r.kernel.org>, <linux-input@...r.kernel.org>,
 <linux-pwm@...r.kernel.org>, Grégory Clement
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 4/7] gpio: max7360: Add MAX7360 gpio support

On Wed Feb 12, 2025 at 4:14 PM CET, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 01:57:34PM +0100, Mathieu Dubois-Briand wrote:
> > On Mon Jan 27, 2025 at 2:07 PM CET, Andy Shevchenko wrote:
> > > On Mon, Jan 13, 2025 at 01:42:28PM +0100, Mathieu Dubois-Briand wrote:
>
> ...
>
> > > > +	parent = to_platform_device(pdev->dev.parent);
> > >
> > > Why do you need this? Can't the fwnode be propagated to the children and then
> > > the respective APIs to be used?
> > 
> > I'm not sure to understand this correctly, what do you mean by
> > propagating the fwnode to the children?
> > 
> > Just a quick summary of the situation and what I try to do. The device
> > tree looks like this, only keeping the interesting properties:
> > 
> > io-expander@38 {
> >   ...
> >   interrupts = <23 IRQ_TYPE_LEVEL_LOW>,
> >                <24 IRQ_TYPE_LEVEL_LOW>;
> >   interrupt-names = "inti", "intk";
> > 
> >   max7360_gpio: gpio {
> >     ...
> >   };
> > 
> >   max7360_gpo: gpo {
> >     ...
> >   };
> > };
> > 
> > Our pdev fwnode points either to the "gpio" or "gpo" nodes, the one from
> > our parent device points to "io-expander@38". Here we need to get the
> > "inti" interrupt from the parent node. What would be the correct way to
> > do it?
>
> Ah, I see now. This is being used only for IRQs, but don't you want to call
> actually fwnode_irq_get_byname()? It will makes the intention clearer.
>

Sure! I can definitely call fwnode_irq_get_byname().

>
> > > > +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios)) {
> > > > +		dev_err(&pdev->dev, "Missing ngpios OF property\n");
> > > > +		return -ENODEV;
> > > > +	}
> > >
> > > This is not needed, it is already done in GPIOLIB core.
> > 
> > I believe this is still needed:
> > - For gpos, we need the gpio count to correctly set the partition
> >   between gpo and keypad columns in max7360_set_gpos_count().
>
> Shouldn't be that done somewhere in the GPIO valid mask initialisation?
>
> > - For gpios, we need the gpio count to setup the IRQs.
>
> Doesn't GPIOLIB parse the property before initializing the IRQ valid mask
> and other init callbacks?

No, I believe I have to register the IRQ before registering the GPIO, so
I can get the IRQ domain.

Right now I have something like:

irq_chip->num_irqs = ngpios;
devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev), max7360_gpio->regmap, irq, flags, 0, irq_chip, &irq_chip_data);
gpio_config.irq_domain = regmap_irq_get_domain(irq_chip_data);
devm_gpio_regmap_register(dev, &gpio_config);

Also, gpiolib will store ngpios in the gpio_chip structure, but while
using gpio-regmap, this structure is masked behind the opaque
gpio_regmap structure. So I believe there is no easy way to retrieve its
value.

This part of the code changed a lot, maybe it would be easier if I push
a new version of the series and we continue the discussion there?

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com