Re: [PATCH v1 2/2] pinctrl: nuvoton: add NPCM8XX pinctrl and GPIO driver

Andy Shevchenko <andy.shevchenko@xxxxxxxxx> · Sun, 10 Jul 2022 21:35:25 +0200

On Sun, Jul 10, 2022 at 12:44 PM Tomer Maimon <tmaimon77@xxxxxxxxx> wrote:
>
> Add pinctrl and GPIO controller driver support to Arbel BMC NPCM8XX SoC.
>
> Arbel BMC NPCM8XX pinctrl driver based on Poleg NPCM7XX, except the
> pin mux mapping difference the NPCM8XX GPIO supports adjust debounce
> period time.

Below my comments from a very brief review.

...

Looking at the length of it, and noticing some debug prints I believe
you may reduce the LoC amount by ~5-10% easily, starting from removing
debug prints (like in IRQ set type callback). Also some lines look
split by two where it's fine to have at one (with container_of() in
them, for example). Besides that read the latest examples on how to
provide constant IRQ chip description and what the proper IRQ data
accessors should be used.
Additionally pay attention to the headers the driver uses, for example
it misses bits.h (or bitops.h depending on the code, I haven't looked
deeply).

...

> +#define DRIVE_STRENGTH_MASK            0x0000FF00

Why not GENMASK()?

...

> +#define DSLO(x)                (((x) >> DRIVE_STRENGTH_LO_SHIFT) & 0xF)
> +#define DSHI(x)                (((x) >> DRIVE_STRENGTH_HI_SHIFT) & 0xF)

Ditto.

...

> +#define GPI            0x1 /* Not GPO */
> +#define GPO            0x2 /* Not GPI */
> +#define SLEW           0x4 /* Has Slew Control, NPCM8XX_GP_N_OSRC */
> +#define SLEWLPC                0x8 /* Has Slew Control, SRCNT.3 */

Why not BIT(x) for them?

...

> +                               regmap_update_bits(gcr_regmap, cfg->reg0,
> +                                                  BIT(cfg->bit0),
> +                                                  !!(cfg->fn0 == mode) ?
> +                                                  BIT(cfg->bit0) : 0);

What the purpose of '!!(x)' in all similar lines? Why wouldn't 'x' simply work?

...

> +       if (pincfg[pin].flag & SLEWLPC) {
> +               switch (arg) {
> +               case 0:
> +                       regmap_update_bits(gcr_regmap, NPCM8XX_GCR_SRCNT,
> +                                          SRCNT_ESPI, 0);
> +                       return 0;
> +               case 1:
> +                       regmap_update_bits(gcr_regmap, NPCM8XX_GCR_SRCNT,
> +                                          SRCNT_ESPI, SRCNT_ESPI);
> +                       return 0;
> +               default:
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return -EINVAL;

Why not to use usual pattern, i.e.

  if (error_condition)
    return -EINVAL;

here and everywhere in the similar cases?

...

> +               val = ioread32(bank->base + NPCM8XX_GP_N_ODSC)
> +               & pinmask;

What was happened to indentation? Check entire file for indentation to be okay.

...

> +       int v;
> +       struct npcm8xx_gpio *bank =
> +               &npcm->gpio_bank[pin / NPCM8XX_GPIO_PER_BANK];
> +       int gpio = BIT(pin % bank->gc.ngpio);

Keep it ordered in reversed xmas tree manner.

...

> +       if (DSLO(v) == nval) {
> +               dev_dbg(bank->gc.parent,
> +                       "setting pin %d to low strength [%d]\n", pin, nval);
> +               npcm_gpio_clr(&bank->gc, bank->base + NPCM8XX_GP_N_ODSC, gpio);
> +               return 0;
> +       } else if (DSHI(v) == nval) {

Redundant 'else'. Check entire file for the similar pattern and drop redundancy.

> +               dev_dbg(bank->gc.parent,
> +                       "setting pin %d to high strength [%d]\n", pin, nval);
> +               npcm_gpio_set(&bank->gc, bank->base + NPCM8XX_GP_N_ODSC, gpio);
> +               return 0;
> +       }

...

> +static void npcm8xx_pin_dbg_show(struct pinctrl_dev *pctldev,
> +                                struct seq_file *s, unsigned int offset)
> +{
> +       seq_printf(s, "pinctrl_ops.dbg: %d", offset);
> +}

Not sure it brings any additional information to what pin control core
issues on debug.

...

> +       dev_dbg(npcm->dev, "group size: %d\n", (int)ARRAY_SIZE(npcm8xx_groups));

Drop this noise. It's production. Or not? If not, why bother with
submitting not yet ready material?

...

> +                       } else if ((nanosecs > 1640) && (nanosecs <= 2280)) {
> +                               iowrite32(0x20, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> +                       } else if ((nanosecs > 2280) && (nanosecs <= 2700)) {
> +                               iowrite32(0x30, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> +                       } else if ((nanosecs > 2700) && (nanosecs <= 2856)) {
> +                               iowrite32(0x40, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> +                       } else if ((nanosecs > 2856) && (nanosecs <= 3496)) {
> +                               iowrite32(0x50, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> +                       } else if ((nanosecs > 3496) && (nanosecs <= 4136)) {
> +                               iowrite32(0x60, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));
> +                       } else if ((nanosecs > 4136) && (nanosecs <= 5025)) {
> +                               iowrite32(0x70, bank->base + NPCM8XX_GP_N_DBNCP0 + (i * 4));

With switch-case with ranges it will be much more visible what's going
on. Also think about it, maybe you can use some formula instead? Or
table (array of integers) approach where index will show the lowest
range value.

...

> +               struct device_node *np = to_of_node(child);

Why?!

> +               ret = of_address_to_resource(np, 0, &res);
> +               if (ret < 0) {
> +                       dev_err(dev, "Resource fail for GPIO bank %u\n", id);
> +                       return ret;
> +               }
> +
> +               pctrl->gpio_bank[id].base = ioremap(res.start, resource_size(&res));

fwnode_iomap()

...

> +               if (ret) {
> +                       dev_err(dev, "bgpio_init() failed\n");
> +                       return ret;
> +               }

Use

  return dev_err_probe(...);

In ->probe() and satellite functions.

...

> +               ret = irq_of_parse_and_map(np, 0);

fwnode_irq_get()

> +               if (!ret) {
> +                       dev_err(dev, "No IRQ for GPIO bank %u\n", id);
> +                       return -EINVAL;
> +               }

...

> +               girq->num_parents = 1;
> +               girq->parents = devm_kcalloc(pctrl->dev, 1,

Replace 1 with girq->num_parents

> +                                            sizeof(*girq->parents),
> +                                            GFP_KERNEL);
> +               if (!girq->parents) {
> +                       ret = -ENOMEM;
> +                       goto err_register;
> +               }

...

> +               ret = gpiochip_add_pin_range(&pctrl->gpio_bank[id].gc,
> +                                            dev_name(pctrl->dev),
> +                                            pctrl->gpio_bank[id].pinctrl_id,
> +                                            pctrl->gpio_bank[id].gc.base,
> +                                            pctrl->gpio_bank[id].gc.ngpio);
> +               if (ret < 0) {
> +                       dev_err(pctrl->dev, "Failed to add GPIO bank %u\n", id);
> +                       gpiochip_remove(&pctrl->gpio_bank[id].gc);
> +                       goto err_register;
> +               }
> +       }

We have a callback for ranges, can you use it?

...

> +err_register:
> +       for (; id > 0; id--)

while(id--)

> +               gpiochip_remove(&pctrl->gpio_bank[id - 1].gc);

But why not devm_gpiochip_add_... in the first place?

...

> +       dev_set_drvdata(&pdev->dev, pctrl);

platform_set_drv_data() ?

...

> +static const struct of_device_id npcm8xx_pinctrl_match[] = {
> +       { .compatible = "nuvoton,npcm845-pinctrl" },
> +       { },

No comma for terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko