RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Geert,

As always, thank you for your review!


On Monday, November 12, 2018, Geert Uytterhoeven wrote:
> > +config PINCTRL_RZA2
> > +       bool "Renesas RZ/A2 gpio and pinctrl driver"
> > +       depends on OF
> > +       depends on ARCH_R7S9210 || COMPILE_TEST
> > +       select GPIOLIB
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> > +       select GENERIC_PINCONF
> 
> Given the PFS_ISEL bit, I think you can select GPIOLIB_IRQCHIP, and
> implement interrupt support on GPIOs.
> Of course that can be added later...

Yes, I was thinking that too.
I have had user ask about that before, so maybe that will be a 2019 
task.


> > +       help
> > +         This selects pinctrl driver for Renesas RZ/A2 platforms.
> 
> GPIO and pinctrl?

True.


> > +enum pfc_pin_port_name {PORT0, PORT1, PORT2, PORT3, PORT4, PORT5, PORT6,
> PORT7,
> > +                       PORT8, PORT9, PORTA, PORTB, PORTC, PORTD, PORTE,
> PORTF,
> > +                       PORTG, PORTH, PORTJ, PORTK, PORTL, PORTM,
> RZA2_NPORTS};
> 
> The only reason for this enum is to fix the value of RZA2_NPORTS,  right?

At the moment, yes.
Originally I thought I would need these. But in the end, I did not.
So I can just define RZA2_NPORTS for now. Maybe for future SoCs,
That value will change depending on the SoC.


> The above two sets of macros split information in two locations, and
> partially
> duplicates it. I think it becomes easier to read if you combine them, and
> factor
> out the addition of the base address. E.g.
> 
>         #define RZA2_PDR(port)        (0x0000 + (port) * 2) /* Port
> Direction, 16-bit */
> 
> Then you can write:
> 
>         reg16 = readw(pfc_base + RZA2_PDR(port));
>         mask16 = RZA2_PDR_MASK << (pin * 2);
>         reg16 &= ~mask16;
>         writew(reg16, pfc_base + RZA2_PDR(port));
> 
> > +static void rza2_set_pin_function(void __iomem *pfc_base, u8 port, u8
> pin,

OK. I'm all for 'easier to read'!


> > +       mask16 = 0x03 << (pin * 2);
> 
> mask16 = RZA2_PDR_MASK << (pin * 2);

OK.

> > +       /* PFS Register Write Protect : OFF */
> > +       writeb(0x00, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=0 */
> > +       writeb(0x40, RZA2_PWPR(pfc_base));      /* B0WI=0, PFSWE=1 */
> 
> #define PWPR_B0WI        BIT(7)        /* Bit Write Disable */
> #define PWPR_PFSWE       BIT(6)        /* PFS Register Write Enable */

OK.

> > +       /* Set Pin function (interrupt disabled, ISEL=0) */
> > +       writeb(func, RZA2_PFS(pfc_base, port, pin));
> 
> #define PFS_ISEL        BIT(6)        /* Interrupt Select */

OK.

> > +static int rza2_chip_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> 
> GPIO offset numbers are identical to PIN numbers, right?
> So you can use RZA2_PIN_ID_TO_PORT(), RZA2_PIN_ID_TO_PIN()
> for consistency.

Good point!


> > +static int rza2_chip_direction_input(struct gpio_chip *chip,
> > +                                    unsigned int offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> > +
> > +       rza2_pin_to_gpio(priv->base, port, pin, GPIOF_DIR_IN);
> 
> Perhaps pass offset to rza2_pin_to_gpio() directly?

OK. I can do that. Saves a couple of lines ;)


> > +static int rza2_chip_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct rza2_pinctrl_priv *priv = gpiochip_get_data(chip);
> > +       u8 port = offset / 8;
> > +       u8 pin = offset % 8;
> > +
> > +       return (readb(RZA2_PIDR(priv->base, port)) >> pin) & 1;
> 
> Might be easier to read if you maintain symmetry with below:
> 
>     return !!(readb(RZA2_PIDR(priv->base, port) & BIT(pin));

Cool.

> > +       if (value)
> > +               new_value |= (1 << pin);
> 
> new_value |= BIT(pin);
> 
> > +       else
> > +               new_value &= ~(1 << pin);
> 
> new_value &= ~BIT(pin);

I wondered about using BIT more often in the code.
Thanks.

> > +static const char * const rza2_gpio_names[] = {
> > +       "P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
> > +       "P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
> > +       "P2_0", "P2_1", "P2_2", "P2_3", "P2_4", "P2_5", "P2_6", "P2_7",
> > +       "P3_0", "P3_1", "P3_2", "P3_3", "P3_4", "P3_5", "P3_6", "P3_7",
> > +       "P4_0", "P4_1", "P4_2", "P4_3", "P4_4", "P4_5", "P4_6", "P4_7",
> > +       "P5_0", "P5_1", "P5_2", "P5_3", "P5_4", "P5_5", "P5_6", "P5_7",
> > +       "P6_0", "P6_1", "P6_2", "P6_3", "P6_4", "P6_5", "P6_6", "P6_7",
> > +       "P7_0", "P7_1", "P7_2", "P7_3", "P7_4", "P7_5", "P7_6", "P7_7",
> > +       "P8_0", "P8_1", "P8_2", "P8_3", "P8_4", "P8_5", "P8_6", "P8_7",
> > +       "P9_0", "P9_1", "P9_2", "P9_3", "P9_4", "P9_5", "P9_6", "P9_7",
> > +       "PA_0", "PA_1", "PA_2", "PA_3", "PA_4", "PA_5", "PA_6", "PA_7",
> > +       "PB_0", "PB_1", "PB_2", "PB_3", "PB_4", "PB_5", "PB_6", "PB_7",
> > +       "PC_0", "PC_1", "PC_2", "PC_3", "PC_4", "PC_5", "PC_6", "PC_7",
> > +       "PD_0", "PD_1", "PD_2", "PD_3", "PD_4", "PD_5", "PD_6", "PD_7",
> > +       "PE_0", "PE_1", "PE_2", "PE_3", "PE_4", "PE_5", "PE_6", "PE_7",
> > +       "PF_0", "PF_1", "PF_2", "PF_3", "P0_4", "PF_5", "PF_6", "PF_7",
> > +       "PG_0", "PG_1", "PG_2", "P0_3", "PG_4", "PG_5", "PG_6", "PG_7",
> > +       "PH_0", "PH_1", "PH_2", "PH_3", "PH_4", "PH_5", "PH_6", "PH_7",
> > +       /* port I does not exist */
> 
> Hence shouldn't you have 8 NULL entries here?

But there is no such port named "I". And even in the dt-bindings and other
documentation, the global pin ID is based off and a world where "I" is not
in the alphabet. So if I put 8 NULLs here, wouldn't that screw up all the
indexing??



> > +static struct gpio_chip chip = {
> > +       .names = rza2_gpio_names,
> 
> BTW, is their much value in filling gpio_chip.names[]?
> I had never seen that before.

It makes the files show up under /sys look nice.

For example, P5_6 is button SW4:

 $ echo 912 > /sys/class/gpio/export

Then you end up with "/sys/class/gpio/P5_6/"

 $ echo in > /sys/class/gpio/P5_6/direction
 $ cat /sys/class/gpio/P5_6/direction
 $ cat /sys/class/gpio/P5_6/value


> > +       .get = rza2_chip_get,
> > +       .set = rza2_chip_set,
> 
> You may want to implement .[gs]et_multiple(), too.

OK, I will have a look.


> > +struct pinctrl_gpio_range gpio_range;
> 
> Please make this static.
> Or even better, store it in rza2_pinctrl_priv?

OK.


> > +       for (i = 0; i < RZA2_NPINS; i++) {
> > +               unsigned int pin = RZA2_PIN_ID_TO_PIN(i);
> > +               unsigned int port = RZA2_PIN_ID_TO_PORT(i);
> > +
> > +               pins[i].number = i;
> > +               pins[i].name = devm_kasprintf(priv->dev, GFP_KERNEL,
> "P%c_%u",
> > +                                             port_names[port], pin);
> 
> These are identical to rza2_gpio_names[], so can't you use those?

Good point! I'll try that.


> > +{
> > +       struct rza2_pinctrl_priv *priv =
> pinctrl_dev_get_drvdata(pctldev);
> > +       struct property *of_pins;
> > +       int i;
> > +       unsigned int *pins;
> > +       unsigned int *psel_val;
> > +       const char **pin_fn;
> > +       int ret, npins;
> > +       int gsel, fsel;
> 
> Some people prefer "reverse Xmas tree ordering" i.e. sorted by decreasing
> declaration length.

https://lwn.net/Articles/758552/

"only a few maintainers insist on that, while most really do not care or
think that it's actively silly."

So are you one of those maintainers?????   :)


> > +       dev_dbg(priv->dev, "Parsed %s with %d pins\n", np->name, npins);
> 
> %pOF ... np

OK.


> > +remove_group:
> > +       pinctrl_generic_remove_group(pctldev, gsel);
> > +
> > +       dev_info(priv->dev, "Unable to parse DT node %s\n", np->name);
> 
> dev_err?
> 
> %pOF ... np

Thanks.



Chris