[2/5] dt-bindings: gpio: gpio-thunderx: Describe pin-cfg option

Add support for pin-cfg to configure GPIO Pins

Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
---
 Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
 1 file changed, 4 insertions(+)

Hi Linus,

Thanks for reviewing.

On Mon, May 02, 2022 at 12:15:34AM +0200, Linus Walleij wrote:
> On Wed, Apr 27, 2022 at 4:47 PM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
> > Add support for pin-cfg to configure GPIO Pins
> >
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/gpio/gpio-thunderx.txt | 4 ++++
> 
> Would be nice to rewrite this binding in YAML
> 

Sure, will take care in V2.

> >    - First cell is the GPIO pin number relative to the controller.
> >    - Second cell is triggering flags as defined in interrupts.txt.
> > +- pin-cfg: Configuration of pin's function, filters, XOR and output mode.
> > +  - First cell is the GPIO pin number
> > +  - Second cell is a value written to GPIO_BIT_CFG register at driver probe.
> 
> Just poking magic hex values into some random register as
> part of a binding is not a good idea.
> 
> This looks like trying to reinvent the pin config subsystem.
> 
> GPIO is using the standard pin configurations in the second cell of
> the handle, use them in this driver as well and add new ones if we
> need.
> 
> You find the existing flags here:
> include/dt-bindings/gpio/gpio.h
> 
> If you need something more sophisticated than a simple flag, I think
> you need to implement proper pin config.
> 
> Yours,
> Linus Walleij

The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
pin function, selecting which signal is reported to GPIO output or which signal GPIO
input need to connect, filters, XOR and output mode.
We will define new entry specific to thunderx GPIO usage, instead of pin-cfg.

Thanks,
Piyush

On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> pin function, selecting which signal is reported to GPIO output or which signal GPIO
> input need to connect, filters, XOR and output mode.

Then implement pin control for this driver instead of inventing a custom hack?
https://docs.kernel.org/driver-api/pin-control.html

Several drivers implement pin control with a GPIO front-end, for example:
drivers/gpio/gpio-pl061.c is used as a front end with
drivers/pinctrl/pinctrl-single.c

There are also composite drivers in drivers/pinctrl that implement both
pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c

Yours,
Linus Walleij

On Fri, Jun 03, 2022 at 12:35:57PM +0200, Linus Walleij wrote:
> On Fri, Jun 3, 2022 at 11:06 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
> > The purpose of this pin-cfg entry is different than the standard GPIO pin config usage.
> > It is to write a value to GPIO_BIT_CFG register which is used to configure fields like
> > pin function, selecting which signal is reported to GPIO output or which signal GPIO
> > input need to connect, filters, XOR and output mode.
> 
> Then implement pin control for this driver instead of inventing a custom hack?
> https://docs.kernel.org/driver-api/pin-control.html
> 
> Several drivers implement pin control with a GPIO front-end, for example:
> drivers/gpio/gpio-pl061.c is used as a front end with
> drivers/pinctrl/pinctrl-single.c
> 
> There are also composite drivers in drivers/pinctrl that implement both
> pincontrol (incl muxing) and GPIO, such as drivers/pinctrl/pinctrl-sx150x.c
> 
> Yours,
> Linus Walleij

Hi Linus,

Thanks for the reply.
But as in this case, we expect a 32 bit reg value via DTS for this driver
only from user with internal understanding of marvell soc and this reg bit
value can have many different combinations as the register fields can vary
for different marvell SoCs.
This patch just reads the reg value from DTS and writes it to the register.

Thanks,
Piyush

On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:

> Thanks for the reply.
> But as in this case, we expect a 32 bit reg value via DTS for this driver
> only from user with internal understanding of marvell soc and this reg bit
> value can have many different combinations as the register fields can vary
> for different marvell SoCs.
> This patch just reads the reg value from DTS and writes it to the register.

I understand that this is convenient but it does not use the right kernel
abstractions and it does not use device tree bindings the right way
either.

Rewrite the patches using definitions and fine control and move away
from magic numbers to be poked into registers.

Yours,
Linus Walleij

On 26/06/2022 00:59, Linus Walleij wrote:
> On Mon, Jun 13, 2022 at 10:04 AM Piyush Malgujar <pmalgujar@marvell.com> wrote:
> 
>> Thanks for the reply.
>> But as in this case, we expect a 32 bit reg value via DTS for this driver
>> only from user with internal understanding of marvell soc and this reg bit
>> value can have many different combinations as the register fields can vary
>> for different marvell SoCs.
>> This patch just reads the reg value from DTS and writes it to the register.
> 
> I understand that this is convenient but it does not use the right kernel
> abstractions and it does not use device tree bindings the right way
> either.
> 
> Rewrite the patches using definitions and fine control and move away
> from magic numbers to be poked into registers.

+1

Let's don't repeat the same pattern Samsung pinctrl has.

Best regards,
Krzysztof