Re: [PATCH 03/13] pinctrl: add a pincontrol driver for BCM6328

[Jonas Gorski <jonas.gorski@xxxxxxxxx>]

On 22 August 2016 at 15:15, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> 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?

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

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

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

>
>> +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 linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html