Re: [PATCH v2 2/8] pinctrl: visconti: Add Toshiba Visconti SoCs pinctrl support

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Nobuhiro!

Thanks for your patch. Some review comments below!

On Mon, Aug 24, 2020 at 2:30 PM Nobuhiro Iwamatsu
<nobuhiro1.iwamatsu@xxxxxxxxxxxxx> wrote:
>
> Add pinctrl support to Toshiba Visconti SoCs.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>

> +config PINCTRL_VISCONTI
> +       bool
> +       select PINMUX
> +       select GENERIC_PINCONF
> +       select GENERIC_PINCTRL_GROUPS
> +       select GENERIC_PINMUX_FUNCTIONS

Thanks for using generic stuff!

> +#define SET_BIT(data, idx)     ((data) |= BIT(idx))
> +#define CLR_BIT(data, idx)     ((data) &= ~BIT(idx))

Please do not reinvent things like this. Either open code
it, and if you want optimizations (probably not relevant in
this case) you would use:

#include <linux/bitmap.h>

set_bit() and clear_bit() if you want atomic bit ops
__set_bit() and __clear_bit() for nonatomic

The upside to using these standard calls is that they will
unfold into assembly optimizations for the architecture if
possible.

If you roll your own locking use the latter primitives.

> +/* private data */
> +struct visconti_pinctrl {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct pinctrl_dev *pctl;
> +       struct pinctrl_desc pctl_desc;
> +
> +       const struct visconti_pinctrl_devdata  *devdata;
> +
> +       spinlock_t lock;

At least add a comment to this lock to say what it is locking.

> +                       /* update pudsel setting */
> +                       val = readl(priv->base + pin->pudsel_offset);
> +                       CLR_BIT(val, pin->pud_shift);
> +                       val |= set_val << pin->pud_shift;

I would just inline the &= operation but it is up to you.

> +               case PIN_CONFIG_DRIVE_STRENGTH:
> +                       arg = pinconf_to_config_argument(configs[i]);
> +                       dev_dbg(priv->dev, "DRV_STR arg = %d\n", arg);
> +                       switch (arg) {
> +                       case 2:
> +                       case 4:
> +                       case 8:
> +                       case 16:
> +                       case 24:
> +                       case 32:
> +                               set_val = (arg / 2) - 1;

This looks like you want to use

set_val = DIV_ROUND_CLOSEST(arg, 2);

Also add a comment on WHY you do this.

> +                       /* update drive setting */
> +                       val = readl(priv->base + pin->dsel_offset);
> +                       val &= ~(GENMASK(3, 0) << pin->dsel_shift);

Could this GENMASK be a #define so we know what it is?

> +/* pinmnux */

Spelling

> +       /* update mux */
> +       val = readl(priv->base + mux->offset);
> +       val &= ~mux->mask;
> +       val |= mux->val;

So here you do things explicitly and in the other case with custom
macros. I think this is better because it is easy to read.

> +static int visconti_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                     struct pinctrl_gpio_range *range,
> +                                     unsigned int pin)

Since you implement this, what GPIO driver are you using this with?

Other than that it looks all right, also the plugin for SoC is nicely designed.

Thanks!
Linus Walleij