Hi Andy,
Thanks for your comments
On Sun, 10 Jul 2022 at 22:36, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> 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).
I will try to reduce lines as possible
>
> ...
>
> > +#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?
taken by mistake, 'x' will 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;
What do you mean? like if (arg>1) return -EINVAL? It just seems more readable.
>
> here and everywhere in the similar cases?
can you point me to which more cases you mean?
>
> ...
>
> > + val = ioread32(bank->base + NPCM8XX_GP_N_ODSC)
> > + & pinmask;
>
> What was happened to indentation? Check entire file for indentation to be okay.
Sorry, I didn't understand, could you explain the comment again?
>
> ...
>
> > + 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.
O.K.
>
> ...
>
> > + 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.
O.K.
>
> > + 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.
There it can be described in a formula. Will be done with switch-case
>
> ...
>
> > + struct device_node *np = to_of_node(child);
>
> Why?!
Will remove and replaced with fwnode functions.
>
> > + 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(...)
Why it is better to use dev_err_probe?
I am not sure that the error will be EPROBE_DEFER, all the failure
cases the driver returned the
error in the code.
>
> 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?
Yes,
>
> ...
>
> > +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?
using, devm_gpiochip_add_data, will remove gpiochip_remove.
>
> ...
>
> > + dev_set_drvdata(&pdev->dev, pctrl);
>
> platform_set_drv_data() ?
O.K.
>
> ...
>
> > +static const struct of_device_id npcm8xx_pinctrl_match[] = {
> > + { .compatible = "nuvoton,npcm845-pinctrl" },
> > + { },
>
> No comma for terminator entry.
O.K.
>
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Tomer
