On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@xxxxxxxxx> 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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
