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
[off-list ref] wrote:

quoted

Add pinctrl support to Toshiba Visconti SoCs.

Signed-off-by: Nobuhiro Iwamatsu <redacted>

quoted

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

Thanks for using generic stuff!

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+/* pinmnux */

Spelling

Thanks, I will this.

quoted

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

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel