On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski [off-list ref] wrote:

Add a pincontrol driver for BCM6328. BCM628 supports muxing 32 pins as
GPIOs, as LEDs for the integrated LED controller, or various other
functions. Its pincontrol mux registers also control other aspects, like
switching the second USB port between host and device mode.

Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>

This looks good. Just thinking following the other patches
that maybe this should be a syscon child driver.

+#define BCM6328_NGPIO          32

I wonder why the pin control driver cares about that?

+struct bcm6328_pinctrl {
+       struct pinctrl_dev *pctldev;
+       struct pinctrl_desc desc;
+
+       void __iomem *mode;
+       void __iomem *mux[3];
+
+       /* register access lock */
+       spinlock_t lock;
+
+       struct gpio_chip gpio;

Usually this should be the other way around: the gpio_chip
knows about the pin controller, the pin controller does not
know about the gpio_chip.

I don't see it used in the code either? Artifact?

+static void bcm6328_rmw_mux(struct bcm6328_pinctrl *pctl, unsigned pin,
+                           u32 mode, u32 mux)
+{
+       unsigned long flags;
+       u32 reg;
+
+       spin_lock_irqsave(&pctl->lock, flags);
+       if (pin < 32) {
+               reg = __raw_readl(pctl->mode);
+               reg &= ~BIT(pin);
+               if (mode)
+                       reg |= BIT(pin);
+               __raw_writel(reg, pctl->mode);
+       }
+
+       reg = __raw_readl(pctl->mux[pin / 16]);
+       reg &= ~(3UL << (pin % 16));
+       reg |= mux << (pin % 16);
+       __raw_writel(reg, pctl->mux[pin / 16]);
+
+       spin_unlock_irqrestore(&pctl->lock, flags);
+}

Why all this __raw_* accessors? What is wrong with readl_relaxed()
and writel_relaxed()? Or MIPS doesn't have them?

+#ifdef CONFIG_OF
+       .dt_node_to_map         = pinconf_generic_dt_node_to_map_pin,
+       .dt_free_map            = pinctrl_utils_free_map,
+#endif

Nice, but why not just add
depends on OF
to the Kconfig and get rid of the #ifdef?

+static int bcm6328_pinctrl_probe(struct platform_device *pdev)
+{
+       struct bcm6328_pinctrl *pctl;
+       struct resource *res;
+       void __iomem *mode, *mux;
+
+       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mode");
+       mode = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(mode))
+               return PTR_ERR(mode);
+
+       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mux");
+       mux = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(mux))
+               return PTR_ERR(mux);

This mishmash of remap windows makes me prefer the syscon
design pattern.

Yours,
Linus Walleij