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
