mbox series

[RFC,0/6] leds: Fix pca955x GPIO pin mappings

Message ID 20210723075858.376378-1-andrew@aj.id.au
Headers show
Series leds: Fix pca955x GPIO pin mappings | expand

Message

Andrew Jeffery July 23, 2021, 7:58 a.m. UTC
Hello,

This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the
pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What
needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to
the pin numberspace of the PCA955x devices. The series solves that by
implementing pinctrl and pinmux in the leds-pca955x driver.

Things I'm unsure about:

1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what
   others thoughts are on that (Linus?).

2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map
   parsing rather than supplying a subnode-specific callback. This was necessary
   to handle the PCA955x devicetree binding in a backwards compatible way.

3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the
   properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the
   problem we have. But it's quite a bit of code...

4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()
   callback for pinctrl, and it seems odd to me that it isn't required.

Please review!

Andrew

Andrew Jeffery (6):
  pinctrl: Add pinctrl_gpio_as_pin()
  pinctrl: Add hook for device-specific map parsing
  leds: pca955x: Relocate chipdef-related descriptors
  leds: pca955x: Use pinctrl to map GPIOs to pins
  ARM: dts: rainier: Add presence-detect and fault indictor GPIO
    expander
  pinctrl: Check get_group_pins callback on init

 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts |  76 +++
 drivers/leds/leds-pca955x.c                  | 554 +++++++++++++++----
 drivers/pinctrl/core.c                       |  28 +-
 include/linux/pinctrl/pinctrl.h              |   4 +
 4 files changed, 566 insertions(+), 96 deletions(-)

Comments

Andrew Jeffery July 28, 2021, 5:43 a.m. UTC | #1
On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:
> 

> 

> On Friday, July 23, 2021, Andrew Jeffery <andrew@aj.id.au> wrote:

> > Hello,

> > 

> > This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the

> > pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What

> > needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to

> > the pin numberspace of the PCA955x devices. The series solves that by

> > implementing pinctrl and pinmux in the leds-pca955x driver.

> > 

> > Things I'm unsure about:

> > 

> > 1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what

> >    others thoughts are on that (Linus?).

> > 

> > 2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map

> >    parsing rather than supplying a subnode-specific callback. This was necessary

> >    to handle the PCA955x devicetree binding in a backwards compatible way.

> > 

> > 3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the

> >    properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the

> >    problem we have. But it's quite a bit of code...

> > 

> > 4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()

> >    callback for pinctrl, and it seems odd to me that it isn't required.

> > 

> > Please review!

> 

> 

> Sounds like a hack.


Yes, possibly. Feedback like this is why I sent the series as an RFC.

> I was briefly looking into patches 1-4 and suddenly 

> realized that the fix can be similar as in PCA9685 (PWM), I.e. we 

> always have chips for the entire pin space and one may map them 

> accordingly, requested in one realm (LED) in the other (GPIO) 

> automatically is BUSY. Or I missed the point?


No, you haven't missed the point. I will look at the PCA9685 driver.

That said, my goal was to implement the behaviour intended by the 
existing binding (i.e. fix a bug). However, userspace would never have 
got the results it expected with the existing driver implementation, so 
I guess you could argue that no such (useful) userspace exists. Given 
that, we could adopt the strategy of always defining a gpiochip 
covering the whole pin space, and parts of the devicetree binding just 
become redundant.

Andrew
Andy Shevchenko July 28, 2021, 9:13 a.m. UTC | #2
On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:

> > On Friday, July 23, 2021, Andrew Jeffery <andrew@aj.id.au> wrote:


> > > This series does a bunch of crimes, so it's an RFC. I'm cross-posting to the

> > > pinctrl/GPIO and LEDs lists because the PCA955x devices impact all of them. What

> > > needs fixing is the leds-pca955x driver's failure to map the GPIO numberspace to

> > > the pin numberspace of the PCA955x devices. The series solves that by

> > > implementing pinctrl and pinmux in the leds-pca955x driver.

> > >

> > > Things I'm unsure about:

> > >

> > > 1. Patch 1: The pinctrl_gpio_as_pin() API feels a bit dirty, not sure what

> > >    others thoughts are on that (Linus?).

> > >

> > > 2. Patch 2: I've added a new callback to hook the entirety of the pinctrl map

> > >    parsing rather than supplying a subnode-specific callback. This was necessary

> > >    to handle the PCA955x devicetree binding in a backwards compatible way.

> > >

> > > 3. Patch 4: The PCA955x devices don't actually have any pinmux hardware, but the

> > >    properties of the pinctrl/pinmux subsystems in the kernel map nicely onto the

> > >    problem we have. But it's quite a bit of code...

> > >

> > > 4. Patch 6: I also lost a bunch of time to overlooking the get_group_pins()

> > >    callback for pinctrl, and it seems odd to me that it isn't required.

> > >

> > > Please review!

> >

> >

> > Sounds like a hack.

>

> Yes, possibly. Feedback like this is why I sent the series as an RFC.

>

> > I was briefly looking into patches 1-4 and suddenly

> > realized that the fix can be similar as in PCA9685 (PWM), I.e. we

> > always have chips for the entire pin space and one may map them

> > accordingly, requested in one realm (LED) in the other (GPIO)

> > automatically is BUSY. Or I missed the point?

>

> No, you haven't missed the point. I will look at the PCA9685 driver.

>

> That said, my goal was to implement the behaviour intended by the

> existing binding (i.e. fix a bug).


Okay, so it implies that this used to work at some point. What has
changed from that point? Why can't we simply fix the culprit commit?

> However, userspace would never have

> got the results it expected with the existing driver implementation, so

> I guess you could argue that no such (useful) userspace exists. Given

> that, we could adopt the strategy of always defining a gpiochip

> covering the whole pin space, and parts of the devicetree binding just

> become redundant.


I'm lost now. GPIO has its own userspace ABI, how does it work right
now in application to this chip?

-- 
With Best Regards,
Andy Shevchenko
Andrew Jeffery July 29, 2021, 12:38 a.m. UTC | #3
On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:

> >

> > > I was briefly looking into patches 1-4 and suddenly

> > > realized that the fix can be similar as in PCA9685 (PWM), I.e. we

> > > always have chips for the entire pin space and one may map them

> > > accordingly, requested in one realm (LED) in the other (GPIO)

> > > automatically is BUSY. Or I missed the point?

> >

> > No, you haven't missed the point. I will look at the PCA9685 driver.

> >

> > That said, my goal was to implement the behaviour intended by the

> > existing binding (i.e. fix a bug).

> 

> Okay, so it implies that this used to work at some point. 


I don't think this is true. It only "works" if the lines specified as 
GPIO in the devicetree are contiguous from line 0. That way the pin and 
GPIO number spaces align. I suspect that's all that's been tested up 
until this point.

We now have a board with a PCA9552 where the first 8 pins are LED and 
the last 8 pins are GPIO, and if you specify this in the devicetree 
according to the binding you hit the failure to map between the two 
number spaces.

> What has

> changed from that point? Why can't we simply fix the culprit commit?


As such nothing has changed, I think it's always been broken, just we 
haven't had hardware configurations that demonstrated the failure.

> 

> > However, userspace would never have

> > got the results it expected with the existing driver implementation, so

> > I guess you could argue that no such (useful) userspace exists. Given

> > that, we could adopt the strategy of always defining a gpiochip

> > covering the whole pin space, and parts of the devicetree binding just

> > become redundant.

> 

> I'm lost now. GPIO has its own userspace ABI, how does it work right

> now in application to this chip?


As above, it "works" if the GPIOs specified in the devicetree are 
contiguous from line 0. It's broken if they're not.

Andrew
Andy Shevchenko July 29, 2021, 7:40 a.m. UTC | #4
On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:

> > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > > On Fri, 23 Jul 2021, at 17:45, Andy Shevchenko wrote:

> > >

> > > > I was briefly looking into patches 1-4 and suddenly

> > > > realized that the fix can be similar as in PCA9685 (PWM), I.e. we

> > > > always have chips for the entire pin space and one may map them

> > > > accordingly, requested in one realm (LED) in the other (GPIO)

> > > > automatically is BUSY. Or I missed the point?

> > >

> > > No, you haven't missed the point. I will look at the PCA9685 driver.

> > >

> > > That said, my goal was to implement the behaviour intended by the

> > > existing binding (i.e. fix a bug).

> >

> > Okay, so it implies that this used to work at some point.

>

> I don't think this is true. It only "works" if the lines specified as

> GPIO in the devicetree are contiguous from line 0. That way the pin and

> GPIO number spaces align. I suspect that's all that's been tested up

> until this point.

>

> We now have a board with a PCA9552 where the first 8 pins are LED and

> the last 8 pins are GPIO, and if you specify this in the devicetree

> according to the binding you hit the failure to map between the two

> number spaces.

>

> > What has

> > changed from that point? Why can't we simply fix the culprit commit?

>

> As such nothing has changed, I think it's always been broken, just we

> haven't had hardware configurations that demonstrated the failure.

>

> >

> > > However, userspace would never have

> > > got the results it expected with the existing driver implementation, so

> > > I guess you could argue that no such (useful) userspace exists. Given

> > > that, we could adopt the strategy of always defining a gpiochip

> > > covering the whole pin space, and parts of the devicetree binding just

> > > become redundant.

> >

> > I'm lost now. GPIO has its own userspace ABI, how does it work right

> > now in application to this chip?

>

> As above, it "works" if the GPIOs specified in the devicetree are

> contiguous from line 0. It's broken if they're not.


So, "it never works" means there is no bug. Now, what we need is to
keep the same enumeration scheme, but if you wish to be used half/half
(or any other ratio), the driver should do like the above mentioned
PWM, i.e. register entire space and depending on the requestor either
proceed with a line or mark it as BUSY.

Ideally, looking into what the chip can do, this should be indeed
converted to some like pin control + PWM + LED + GPIO drivers. Then
the function in pin mux configuration can show what exactly is enabled
on the certain line(s).


-- 
With Best Regards,
Andy Shevchenko
Andrew Jeffery Aug. 3, 2021, 4:07 a.m. UTC | #5
On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:
> On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:

> > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > > > However, userspace would never have

> > > > got the results it expected with the existing driver implementation, so

> > > > I guess you could argue that no such (useful) userspace exists. Given

> > > > that, we could adopt the strategy of always defining a gpiochip

> > > > covering the whole pin space, and parts of the devicetree binding just

> > > > become redundant.

> > >

> > > I'm lost now. GPIO has its own userspace ABI, how does it work right

> > > now in application to this chip?

> >

> > As above, it "works" if the GPIOs specified in the devicetree are

> > contiguous from line 0. It's broken if they're not.

> 

> So, "it never works" means there is no bug. Now, what we need is to

> keep the same enumeration scheme, but if you wish to be used half/half

> (or any other ratio), the driver should do like the above mentioned

> PWM, i.e. register entire space and depending on the requestor either

> proceed with a line or mark it as BUSY.

> 

> Ideally, looking into what the chip can do, this should be indeed

> converted to some like pin control + PWM + LED + GPIO drivers. Then

> the function in pin mux configuration can show what exactly is enabled

> on the certain line(s).


So just to clarify, you want both solutions here?

1. A gpiochip that covers the entire pin space
2. A pinmux implementation that manages pin allocation to the different drivers

In that case we can largely leave this series as is? We only need to 
adjust how we configure the gpiochip by dropping the pin-mapping 
implementation?

I don't have a need to implement a PWM driver for it right now but that 
would make sense to do at some point.

Andrew
Andy Shevchenko Aug. 3, 2021, 10:33 a.m. UTC | #6
On Tue, Aug 3, 2021 at 7:07 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:
> > On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:
> > > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > However, userspace would never have
> > > > > got the results it expected with the existing driver implementation, so
> > > > > I guess you could argue that no such (useful) userspace exists. Given
> > > > > that, we could adopt the strategy of always defining a gpiochip
> > > > > covering the whole pin space, and parts of the devicetree binding just
> > > > > become redundant.
> > > >
> > > > I'm lost now. GPIO has its own userspace ABI, how does it work right
> > > > now in application to this chip?
> > >
> > > As above, it "works" if the GPIOs specified in the devicetree are
> > > contiguous from line 0. It's broken if they're not.
> >
> > So, "it never works" means there is no bug. Now, what we need is to
> > keep the same enumeration scheme, but if you wish to be used half/half
> > (or any other ratio), the driver should do like the above mentioned
> > PWM, i.e. register entire space and depending on the requestor either
> > proceed with a line or mark it as BUSY.
> >
> > Ideally, looking into what the chip can do, this should be indeed
> > converted to some like pin control + PWM + LED + GPIO drivers. Then
> > the function in pin mux configuration can show what exactly is enabled
> > on the certain line(s).
>
> So just to clarify, you want both solutions here?
>
> 1. A gpiochip that covers the entire pin space
> 2. A pinmux implementation that manages pin allocation to the different drivers
>
> In that case we can largely leave this series as is? We only need to
> adjust how we configure the gpiochip by dropping the pin-mapping
> implementation?

Nope. It's far from what I think of. Re-reading again your cover
letter it points out that pin mux per se does not exist in the
hardware. In this case things become a bit too complicated, but we
still may manage to handle them. Before I was thinking about this
hierarchy

1. pinmux driver (which is actually the main driver here)
2. LED driver (using regmap API)
3. GPIO driver (via gpio-regmap)
4. PWM driver.

Now what we need here is some kind of "virtual" pinmux. Do I
understand correctly?

To be clear: I do not like putting everything into one driver when the
logical parts may be separated.
Andrew Jeffery Aug. 4, 2021, 4:55 a.m. UTC | #7
On Tue, 3 Aug 2021, at 20:03, Andy Shevchenko wrote:
> On Tue, Aug 3, 2021 at 7:07 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > On Thu, 29 Jul 2021, at 17:10, Andy Shevchenko wrote:

> > > On Thu, Jul 29, 2021 at 3:39 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > > > On Wed, 28 Jul 2021, at 18:43, Andy Shevchenko wrote:

> > > > > On Wed, Jul 28, 2021 at 8:43 AM Andrew Jeffery <andrew@aj.id.au> wrote:

> > > > > > However, userspace would never have

> > > > > > got the results it expected with the existing driver implementation, so

> > > > > > I guess you could argue that no such (useful) userspace exists. Given

> > > > > > that, we could adopt the strategy of always defining a gpiochip

> > > > > > covering the whole pin space, and parts of the devicetree binding just

> > > > > > become redundant.

> > > > >

> > > > > I'm lost now. GPIO has its own userspace ABI, how does it work right

> > > > > now in application to this chip?

> > > >

> > > > As above, it "works" if the GPIOs specified in the devicetree are

> > > > contiguous from line 0. It's broken if they're not.

> > >

> > > So, "it never works" means there is no bug. Now, what we need is to

> > > keep the same enumeration scheme, but if you wish to be used half/half

> > > (or any other ratio), the driver should do like the above mentioned

> > > PWM, i.e. register entire space and depending on the requestor either

> > > proceed with a line or mark it as BUSY.

> > >

> > > Ideally, looking into what the chip can do, this should be indeed

> > > converted to some like pin control + PWM + LED + GPIO drivers. Then

> > > the function in pin mux configuration can show what exactly is enabled

> > > on the certain line(s).

> >

> > So just to clarify, you want both solutions here?

> >

> > 1. A gpiochip that covers the entire pin space

> > 2. A pinmux implementation that manages pin allocation to the different drivers

> >

> > In that case we can largely leave this series as is? We only need to

> > adjust how we configure the gpiochip by dropping the pin-mapping

> > implementation?

> 

> Nope. It's far from what I think of. Re-reading again your cover

> letter it points out that pin mux per se does not exist in the

> hardware. In this case things become a bit too complicated, but we

> still may manage to handle them. Before I was thinking about this

> hierarchy

> 

> 1. pinmux driver (which is actually the main driver here)

> 2. LED driver (using regmap API)

> 3. GPIO driver (via gpio-regmap)

> 4. PWM driver.


Okay - I need to look at gpio-regmap, this wasn't something I was aware 
of.

> 

> Now what we need here is some kind of "virtual" pinmux. Do I

> understand correctly?


Possibly. My thoughts went to pinctrl as part of its job is mutual 
exclusion *and* pin mapping, plus we get the nice debugfs interface 
with the pin allocation details. The need for pin mapping came from 
trying to stay true to the intent of the existing devicetree binding. 
If we throw that out and have the gpiochip cover the pin space for the 
chip then using pinctrl only gives us mutual exclusion and the debugfs 
interface. pinctrl seems pretty heavy-weight to use *just* for mutual 
exclusion - with no requirement for pin mapping I feel whether or not 
we go this way hinges on the utility of debugfs.

As outlined earlier, there's no mux hardware, the only thing that 
changes is software's intent.

> 

> To be clear: I do not like putting everything into one driver when the

> logical parts may be separated.


Right, its already a bit unwieldy.

Andrew