Hi Prabhakar,

On Wed, Jun 16, 2021 at 3:27 PM Lad Prabhakar
[off-list ref] wrote:

As this is selected by PINCTRL_PFC_R9A07G044, it should probably be
invisible, i.e. add "if COMPILE_TEST"?

+       depends on OF
+       depends on ARCH_R9A07G044 || COMPILE_TEST

Hence no need for this dependency.

Please drop these, and use GPIO_LINE_DIRECTION_{IN,OUT} instead.

+
+#define RZG2L_PIN_ID_TO_PORT(id)       ((id) / RZG2L_MAX_PINS_PER_PORT)
+#define RZG2L_PIN_ID_TO_PIN(id)                ((id) % RZG2L_MAX_PINS_PER_PORT)
+
+struct rzg2l_pinctrl {
+       struct pinctrl_dev              *pctrl_dev;
+       struct pinctrl_desc             pctrl_desc;
+
+       void __iomem                    *base;
+       struct device                   *dev;
+       struct clk                      *clk;
+
+       struct gpio_chip                gpio_chip;
+       struct pinctrl_gpio_range       gpio_range;
+
+       const struct rzg2l_pin_soc      *psoc;
+
+       spinlock_t                      lock;
+       unsigned int                    nports;
+};
+
+static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
+                                      int pins, unsigned long pfc_mode)

unsigned int pfc_mode?

+{
+       u32 port = RZG2L_PIN_ID_TO_PORT(pins);
+       u8 bit = RZG2L_PIN_ID_TO_PIN(pins);
+       unsigned long flags;
+       u32 reg32, mask32;
+       u16 reg16, mask16;
+       u8 reg8;
+
+       spin_lock_irqsave(&pctrl->lock, flags);
+
+       /* Set pin to 'Non-use (Hi-Z input protection)'  */
+       reg16 = readw(pctrl->base + PM(port));
+       mask16 = PM_MASK << (bit * 2);
+       reg16 = reg16 & ~mask16;

reg16 &= ...

Perhaps drop the mask16:

    reg16 &= ~(PM_MASK << (bit * 2));

+       writew(reg16, pctrl->base + PM(port));
+
+       /* Temporarily switch to GPIO mode with PMC register */
+       reg8 = readb(pctrl->base + PMC(port));
+       writeb(reg8 & ~BIT(bit), pctrl->base + PMC(port));
+
+       /* Set the PWPR register to allow PFC register to write */
+       writel(0x00, pctrl->base + PWPR);        /* B0WI=0, PFCWE=0 */
+       writel(PWPR_PFCWE, pctrl->base + PWPR);  /* B0WI=0, PFCWE=1 */
+
+       /* Select Pin function mode with PFC register */
+       reg32 = readl(pctrl->base + PFC(port));
+       mask32 = PFC_MASK << (bit * 4);
+       reg32 = reg32 & ~mask32;

reg32 &= ...

Perhaps drop the mask32, too?

+       pfc_mode = pfc_mode << (bit * 4);

pfc_mode <<= ...

+       writel(reg32 | pfc_mode, pctrl->base + PFC(port));

Or just drop the line before, and do:

    writel(reg32 | (pfc_mode << (bit * 4)), pctrl->base + PFC(port));

+
+       /* Set the PWPR register to be write-protected */
+       writel(0x00, pctrl->base + PWPR);        /* B0WI=0, PFCWE=0 */
+       writel(PWPR_B0WI, pctrl->base + PWPR);  /* B0WI=1, PFCWE=0 */
+
+       /* Switch to Peripheral pin function with PMC register */
+       reg8 = readb(pctrl->base + PMC(port));
+       writeb(reg8 | BIT(bit), pctrl->base + PMC(port));
+
+       spin_unlock_irqrestore(&pctrl->lock, flags);
+};
+
+static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
+                                unsigned int func_selector,
+                                unsigned int group_selector)
+{
+       struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+       struct function_desc *func;
+       struct group_desc *group;
+       unsigned long data;
+       int *pins;
+       int i;

unsigned int i

+
+       func = pinmux_generic_get_function(pctldev, func_selector);
+       if (!func)
+               return -EINVAL;
+       group = pinctrl_generic_get_group(pctldev, group_selector);
+       if (!group)
+               return -EINVAL;
+
+       pins = group->pins;
+       data = (unsigned long)group->data;
+
+       dev_dbg(pctldev->dev, "enable function %s group %s\n",
+               func->name, group->name);
+
+       for (i = 0; i < group->num_pins; i++)
+               rzg2l_pinctrl_set_pfc_mode(pctrl, *(pins + i), data);

pins[i]

+
+       return 0;
+};
+
+static const struct pinctrl_ops rzg2l_pinctrl_pctlops = {
+       .get_groups_count = pinctrl_generic_get_group_count,
+       .get_group_name = pinctrl_generic_get_group_name,
+       .get_group_pins = pinctrl_generic_get_group_pins,
+       .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
+       .dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static const struct pinmux_ops rzg2l_pinctrl_pmxops = {
+       .get_functions_count = pinmux_generic_get_function_count,
+       .get_function_name = pinmux_generic_get_function_name,
+       .get_function_groups = pinmux_generic_get_function_groups,
+       .set_mux = rzg2l_pinctrl_set_mux,
+       .strict = true,
+};
+
+static int rzg2l_pinctrl_add_groups(struct rzg2l_pinctrl *pctrl)
+{
+       int ret, i;

unsigned int i

+
+       for (i = 0; i < pctrl->psoc->ngroups; i++) {
+               const struct group_desc *group = pctrl->psoc->groups + i;
+
+               ret = pinctrl_generic_add_group(pctrl->pctrl_dev, group->name,
+                                               group->pins, group->num_pins,
+                                               group->data);
+               if (ret < 0) {
+                       dev_err(pctrl->dev, "Failed to register group %s\n",
+                               group->name);

This can really only fail in case of out-of-memory, or if group->name
is NULL, so I don't think there's a need to print an error message.

+                       return ret;
+               }
+       }
+
+       return 0;
+}
+
+static int rzg2l_pinctrl_add_functions(struct rzg2l_pinctrl *pctrl)
+{
+       int ret, i;

unsigned int i

+
+       for (i = 0; i < pctrl->psoc->nfuncs; i++) {
+               const struct function_desc *func = pctrl->psoc->funcs + i;
+
+               ret = pinmux_generic_add_function(pctrl->pctrl_dev, func->name,
+                                                 func->group_names,
+                                                 func->num_group_names,
+                                                 func->data);
+               if (ret < 0) {
+                       dev_err(pctrl->dev, "Failed to register function %s\n",
+                               func->name);

This can really only fail in case of out-of-memory, or if func->name
is NULL, so I don't think there's a need to print an error message.

+                       return ret;
+               }
+       }
+
+       return 0;
+}

+static void rzg2l_gpio_set_direction(struct rzg2l_pinctrl *pctrl, u32 port,
+                                    u8 bit, bool output)
+{
+       unsigned long flags;
+       u16 reg16;
+
+       spin_lock_irqsave(&pctrl->lock, flags);
+
+       reg16 = readw(pctrl->base + PM(port));
+       reg16 = reg16 & ~(PM_MASK << (bit * 2));

reg16 &= ...

or just combine with the line before?

+
+       if (output)
+               writew(reg16 | (PM_OUTPUT << (bit * 2)),
+                      pctrl->base + PM(port));
+       else
+               writew(reg16 | (PM_INPUT << (bit * 2)),
+                      pctrl->base + PM(port));

This can be simplified to

        if (output)
                reg16 |= PM_OUTPUT << (bit * 2);
        else
                reg16 |= PM_INPUT << (bit * 2);
        writew(reg16, pctrl->base + PM(port));

or perhaps even shortened to:

        reg16 |= (output ? PM_OUTPUT : PM_INPUT) << (bit * 2);
        writew(reg16, pctrl->base + PM(port));

+
+       spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+static int rzg2l_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+       struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
+       u32 port = RZG2L_PIN_ID_TO_PORT(offset);
+       u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
+
+       if (!(readb(pctrl->base + PMC(port)) & BIT(bit))) {
+               u16 reg16;
+
+               reg16 = readw(pctrl->base + PM(port));
+               reg16 = (reg16 >> (bit * 2)) & PM_MASK;
+               if (reg16 == PM_OUTPUT)
+                       return GPIOF_OUTPUT;
+               else if (reg16 == PM_INPUT)
+                       return GPIOF_INPUT;
+               else if (reg16 == PM_OUTPUT_INPUT)
+                       return GPIOF_BIDIRECTION;
+               else
+                       return GPIOF_HI_Z;

These should return either GPIO_LINE_DIRECTION_OUT or
GPIO_LINE_DIRECTION_IN. No other non-error values are defined for
the .get_direction() callback.

+       } else {
+               return -EINVAL;
+       }
+}

+static int rzg2l_gpio_direction_output(struct gpio_chip *chip,
+                                      unsigned int offset, int value)
+{
+       struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
+       u32 port = RZG2L_PIN_ID_TO_PORT(offset);
+       u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
+
+       rzg2l_gpio_set_direction(pctrl, port, bit, true);
+       rzg2l_gpio_set(chip, offset, value);

Probably the order of these two operations should be reversed, to
avoid glitches.

+
+       return 0;
+}
+
+static int rzg2l_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+       struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
+       u32 port = RZG2L_PIN_ID_TO_PORT(offset);
+       u8 bit = RZG2L_PIN_ID_TO_PIN(offset);
+       u16 reg16;
+
+       reg16 = readw(pctrl->base + PM(port));
+       reg16 = (reg16 >> (bit * 2)) & PM_MASK;
+
+       if (reg16 == PM_INPUT || reg16 == PM_OUTPUT_INPUT)

BTW, how do you configure a pin for PM_OUTPUT_INPUT?

+               return !!(readb(pctrl->base + PIN(port)) & BIT(bit));
+       else if (reg16 == PM_OUTPUT)
+               return !!(readb(pctrl->base + P(port)) & BIT(bit));
+       else
+               return -EINVAL;
+}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds