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

[Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>]

Hi,

Thanks for your review.

On Fri, Aug 28, 2020 at 02:41:05PM +0200, Linus Walleij wrote:
> 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.
> 

I understood this.
Since this is a bit control for variables, clear_bit() and other are not
used. As you write in the comment below, I fix not using unnecessary macros.

> > +/* 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.
> 

OK, I will add a commnent.

> > +                       /* 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.
> 

OK, I will fix not using this macro.

> > +               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.
> 

OK, I will fix using DIV_ROUND_CLOSEST and comment.

> > +                       /* 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

Thanks, I will this.

> 
> > +       /* 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.

OK.

> 
> > +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?
> 

Certainly this feature is not called and is not needed, because there
is no GPIO driver for this SoC now. I will remove these.


> Other than that it looks all right, also the plugin for SoC is nicely designed.
> 
> Thanks!
> Linus Walleij
>

Best regards,
  Nobuhiro