On 22 August 2016 at 15:15, Linus Walleij [off-list ref] wrote:

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

quoted

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 <redacted>

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

quoted

+#define BCM6328_NGPIO          32

I wonder why the pin control driver cares about that?

quoted

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

That's because some of the drivers require it, and kept it here so to
keep the drivers as similar as possible.

quoted

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

BCM63XX is one of those weird targets that are usually big endian, and
there are no native endian / big endian *_relaxed() accessors.

quoted

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

As mentioned for the generic part, I want to use it on a !OF target as well.

quoted

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

At least for this one I would still need to have multiple syscons
because there already is one OF enabled driver that uses a non-syscon
access to drivers within this range.


Regards
Jonas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html