Re: [PATCH v2 03/10] pinctrl: axp209: use drv_data of pinctrl_pin_desc to store pin reg

[Maxime Ripard]

On Tue, Sep 26, 2017 at 01:17:05PM +0000, Quentin Schulz wrote:
> Hi Maxime,
> 
> On 26/09/2017 15:01, Maxime Ripard wrote:
> > On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote:
> >> Instead of using a function to retrieve each pin's correct control
> >> register, use drv_data within pinctrl_pin_desc to store the ctrl reg.
> >>
> >> Remove axp20x_gpio_get_reg and replace every occurrence by a get from
> >> drv_data.
> > 
> > Why do you need to do that? This should be explained.
> > 
> 
> Agreed that it misses an explanation.
> 
> Today, to get a register addr of one of the GPIOs in the PMIC, we
> basically get the GPIO number and returns the register via this info.
> 
> There are 3 GPIOs in AXP209, 2 in AXP813. I didn't want to have a switch
> case for the GPIO number and then an if/else inside one of the case to
> check if the device is AXP209 or AXP813 in which case we return -EINVAL
> instead of the GPIO2 reg. With support for new PMIC, we would have a
> bunch of if conditions and complexify the process for something really
> simple.

I'm not sure how that relates to your code actually. The only thing
that patch is doing is to move the register offset from a function to
the structure associated to the pin.

However, even in the AXP813 case, you're using exactly the same
values, so that's not really needed.

Now, you also mentionned the pin number. While this patch doesn't
really address it, it's also no really needed. The number of pins is
already known and registered in the GPIO framework. If the framework
doesn't already do it (which would be surprising), you can just check
that the pin number passed is not going to be higher than the one you
registered.

> IMHO, this also allows easier integration of future PMICs which might
> have different regs for the GPIOs.

Let's worry about future PMICs in the future.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature