Re: [PATCH 01/13] pinctrl: add bcm63xx base code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi,

On 22 August 2016 at 14:46, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
>
>> Setup directory and add a helper for bcm63xx pinctrl support.
>>
>> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx>
>
>>  drivers/pinctrl/bcm63xx/Kconfig           |   3 +
>>  drivers/pinctrl/bcm63xx/Makefile          |   1 +
>
> Why not just put it in the existing pinctrl/bcm directory?

Because at the time I started writing these drivers it was still
exclusive for ARCH_BCM, will move them there.

> The drivers in there share nothing but being Broadcom
> anyways. And when you're at it, do take a look at the
> other Broadcom drivers to check if they would
> happen to share something with your hardware, I find
> it dazzling that hardware engineers repeatedly reinvents
> pin controllers but what can I do.
>
>> +config PINCTRL_BCM63XX
>> +       bool
>> +       select GPIO_GENERIC
>
> depends on ARCH_****?

This isn't a user selectable symbol so I don't see the need for that.

>
> depends on OF_GPIO?
>
> Will it really be used on non-DT systems?

I plan to use it on both mips/bcm63xx (no dts support) and bmips (dts
support), so yes.

>
>> +#define BANK_SIZE      sizeof(u32)
>> +#define PINS_PER_BANK  (BANK_SIZE * BITS_PER_BYTE)
>> +
>> +#ifdef CONFIG_OF
>> +static int bcm63xx_gpio_of_xlate(struct gpio_chip *gc,
>> +                                const struct of_phandle_args *gpiospec,
>> +                                u32 *flags)
>> +{
>> +       struct gpio_chip *base = gpiochip_get_data(gc);
>> +       int pin = gpiospec->args[0];
>> +
>> +       if (gc != &base[pin / PINS_PER_BANK])
>> +               return -EINVAL;
>> +
>> +       pin = pin % PINS_PER_BANK;
>> +
>> +       if (pin >= gc->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[1];
>> +
>> +       return pin;
>> +}
>> +#endif
>
> - Do you really have to support the !OF_GPIO case? (depend in Kconfig,
>   get rid of #ifdef)

See above.

>
> - The only reason for not using of_gpio_simple_xlate() seems to be that
>   you have several GPIO banks. So why not model every bank as a
>   separate device? Or did you consider this already?

I did consider it, but it makes the !OF case more complicated, and the
current of_gpio base code requires changing for it.

That's because some of the pinctrl chips need to set the
gpio-direction for muxes, and for that need to have a reference to the
gpio-controller(s). Since any muxes directly tied to the controller
node will get applied as soon as the controller is registered, it
needs to aquire the gpio-controller references first.

On the gpio-controller side, to flag these a requiring pinctrl those
would then have a gpio-range property, which will cause the probe
being deferred until the reference to the controller can be resolved.
Which waits for the gpio-controller to be registered, so it can
resolve its references to it. A true catch 22 ;-)

This could probably resolved by deferring resolving node-to-pincontrol
devices to gpio request time. Not sure if this then would conflict
which gpio-hogs on such a gpio-controller node?

I haven't really thought how much work it would be for the !OF case,
but would probably mean I need to acquire references based on their
pdev names.

>> +               gc[i].request = gpiochip_generic_request;
>> +               gc[i].free = gpiochip_generic_free;
>> +
>> +#ifdef CONFIG_OF
>> +               gc[i].of_gpio_n_cells = 2;
>> +               gc[i].of_xlate = bcm63xx_gpio_of_xlate;
>> +#endif
>> +
>> +               gc[i].label = label;
>> +               gc[i].ngpio = pins;
>> +
>> +               devm_gpiochip_add_data(dev, &gc[i], gc);
>
> Because this also gets pretty hairy... since you don't have one device
> per gpiochip.
>
>> +static void bcm63xx_setup_pinranges(struct gpio_chip *gc, const char *name,
>> +                                   int ngpio)
>> +{
>> +       int i, chips = DIV_ROUND_UP(ngpio, PINS_PER_BANK);
>> +
>> +       for (i = 0; i < chips; i++) {
>> +               int offset, pins;
>> +
>> +               offset = i * PINS_PER_BANK;
>> +               pins = min_t(int, ngpio - offset, PINS_PER_BANK);
>> +
>> +               gpiochip_add_pin_range(&gc[i], name, 0, offset, pins);
>> +       }
>> +}
>
> And this, same thing. Could be so much easier if it was just the same
> driver instantiated twice for two banks.
>
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirout");
>> +       dirout = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(dirout))
>> +               return ERR_CAST(dirout);
>> +
>> +       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>> +       data = devm_ioremap_resource(&pdev->dev, res);
>> +       if (IS_ERR(data))
>> +               return ERR_CAST(data);
>> +
>> +       sz = resource_size(res);
>> +
>> +       ret = bcm63xx_setup_gpio(&pdev->dev, gc, dirout, data, sz, ngpio);
>> +       if (ret)
>> +               return ERR_PTR(ret);
>> +
>> +       pctldev = devm_pinctrl_register(&pdev->dev, desc, priv);
>> +       if (IS_ERR(pctldev))
>> +               return pctldev;
>> +
>> +       bcm63xx_setup_pinranges(gc, pinctrl_dev_get_devname(pctldev), ngpio);
>> +
>> +       dev_info(&pdev->dev, "registered at mmio %p\n", dirout);
>> +
>> +       return pctldev;
>
> A current trend in simple MMIO devices is to simply add themselves as a
> compatible string in drivers/gpio/gpio-mmio.c. Example:
>
> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/drivers/gpio/gpio-mmio.c?h=devel&id=05cc995f4d44c2b14a1f15f3271cc9715cb81099
>
> This could be a viable approach if you find a way to flag that such a GPIO
> has a pin control backend.

The most obvious would be having a gpio-ranges property, but this
leads to issues mentioned above. And only works for OF.


Regards
Jonas
--
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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux