Re: basic-mmio-gpio: add DT support

Re: basic-mmio-gpio: add DT support

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
>> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
>> <noltari@gmail.com> wrote:
>>> Add DT support while keeping legacy support.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>>
>> There is no documentation for these new bindings?
>
> Actually, I was waiting for this patch (by kamlakant.patel@linaro.org) to be accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
> Anyway, I will add documentation on the next version of this patch.
>
>>
>>> index 16f6115..9792783 100644
>>> --- a/drivers/gpio/gpio-generic.c
>>> +++ b/drivers/gpio/gpio-generic.c
>>> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>>>  #include <linux/platform_device.h>
>>>  #include <linux/mod_devicetable.h>
>>>  #include <linux/basic_mmio_gpio.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_gpio.h>
>>>
>>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
>>>  {
>>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
>>>         return ret;
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id bgpio_dt_ids[] = {
>>> +       { .compatible = "basic-mmio-gpio" },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
>>> +
>>> +static int bgpio_probe_dt(struct platform_device *pdev)
>>> +{
>>> +       u32 tmp;
>>> +       struct bgpio_pdata *pdata;
>>> +       struct device_node *np;
>>> +
>>> +       np = pdev->dev.of_node;
>>> +       if (!np)
>>> +               return 0;
>>> +
>>> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> +       if (!pdata)
>>> +               return -ENOMEM;
>>> +
>>> +       pdata->label = dev_name(&pdev->dev);
>>> +       pdata->base = -1;
>>> +       if (of_find_property(np, "byte-be", NULL)) {
>>> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
>>> +       }
>>> +       if (of_find_property(np, "bit-be", NULL)) {
>>> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
>>> +       }
>>> +       if (of_find_property(np, "regset-nr", NULL)) {
>>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
>>> +       }
>>> +       if (of_find_property(np, "regdir-nr", NULL)) {
>>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
>>> +       }
>>> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
>>> +               pdata->ngpio = tmp;
>>> +       }
>>
>> I don't think this is acceptable. gpio-generic is designed to be used
>> as a framework for other drivers to build upon. These drivers should
>> have their own compatible strings, which should be enough to infer all
>> the properties you defined here.
>>
>
> gpio-generic is not only designed as a framework for other drivers, it can also be used directly by registering the driver through platform device (basic-mmio-gpio/basic-mmio-gpio-be).

This works for platform drivers which stay confined to the kernel, but
DT is a firmware definition where such generic bindings are much less
desirable.

> My intention is to make it DT compatible as a generic driver, which can be used for different hardware, by selecting different DT properties as configuration.
>
>> Device Tree identifies hardware precisely (vendor and model), and this
>> new binding is just not that. You *could* however have a very simple
>> driver that associates compatible strings to static tables containing
>> the values of the properties you wanted to see passed through the DT,
>> and have one single driver that covers many mmio-based GPIO devices.
>> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
>> hardware.
>>
>
> I don't think making a new driver mapping different compatible strings to static tables containing the values of the properties passed through DT is a sane way of doing it, since it will require multiple combinations to cover all the possibilites.
>
> My plan is to be able to do something like this (btw, I actually tested this patch on BCM63xx and works perfectly):
> gpio-controller@10000084 {
>         compatible = "basic-mmio-gpio", "brcm,brcm6368";

Here your compatible string should be

        compatible = "brcm,brcm6368", "basic-mmio-gpio";

I.e. from more precise to less precise. A dedicated BRCM6368 driver
should take precedence over your generic one.

>         reg = <0x10000084 0x4>, <0x1000008c 0x4>;
>         reg-names = "dirout", "dat";
>
>         num-gpios = <32>;
>         byte-be;
>
>         gpio-controller;
>         #gpio-cells = <2>;
> };
> And for other hardware you could do:
> gpio-controller@10000180 {
>         compatible = "basic-mmio-gpio", "foo,bar";
>         reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
>         reg-names = "dirin", "dirout", "dat";
>
>         num-gpios = <20>;
>         bit-be;
>         byte-be;
>         regdir-nr;
>
>         gpio-controller;
>         #gpio-cells = <2>;
> };
> This way you wouldn't need to add a wrapper for each specific hardware using basic-mmio-gpio, and you would save time by using the generic driver.

I understand the intent but IIUC the history of DT speaks  against
such generic bindings. On the other hand I can see some instances of
them like "simple-bus" for instance. Added the DT maintainers and
mailing list to get more informed opinions on the matter.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: basic-mmio-gpio: add DT support

[Mark Rutland]

On Mon, Jan 19, 2015 at 03:37:18AM +0000, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
> > El 14/01/2015 a las 6:32, Alexandre Courbot escribió:
> >> On Wed, Dec 17, 2014 at 7:41 AM, Álvaro Fernández Rojas
> >> <noltari@gmail.com> wrote:
> >>> Add DT support while keeping legacy support.
> >>>
> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >>> ---
> >>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> >>
> >> There is no documentation for these new bindings?
> >
> > Actually, I was waiting for this patch (by kamlakant.patel@linaro.org) to be accepted before: "[PATCH v1 5/5] gpio: document basic mmio gpio library".
> > Anyway, I will add documentation on the next version of this patch.
> >
> >>
> >>> index 16f6115..9792783 100644
> >>> --- a/drivers/gpio/gpio-generic.c
> >>> +++ b/drivers/gpio/gpio-generic.c
> >>> @@ -61,6 +61,9 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
> >>>  #include <linux/platform_device.h>
> >>>  #include <linux/mod_devicetable.h>
> >>>  #include <linux/basic_mmio_gpio.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/of_gpio.h>
> >>>
> >>>  static void bgpio_write8(void __iomem *reg, unsigned long data)
> >>>  {
> >>> @@ -488,8 +491,58 @@ static void __iomem *bgpio_map(struct platform_device *pdev,
> >>>         return ret;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_OF
> >>> +static const struct of_device_id bgpio_dt_ids[] = {
> >>> +       { .compatible = "basic-mmio-gpio" },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, bgpio_dt_ids);
> >>> +
> >>> +static int bgpio_probe_dt(struct platform_device *pdev)
> >>> +{
> >>> +       u32 tmp;
> >>> +       struct bgpio_pdata *pdata;
> >>> +       struct device_node *np;
> >>> +
> >>> +       np = pdev->dev.of_node;
> >>> +       if (!np)
> >>> +               return 0;
> >>> +
> >>> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >>> +       if (!pdata)
> >>> +               return -ENOMEM;
> >>> +
> >>> +       pdata->label = dev_name(&pdev->dev);
> >>> +       pdata->base = -1;
> >>> +       if (of_find_property(np, "byte-be", NULL)) {
> >>> +               pdata->flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
> >>> +       }
> >>> +       if (of_find_property(np, "bit-be", NULL)) {
> >>> +               pdata->flags |= BGPIOF_BIG_ENDIAN;
> >>> +       }
> >>> +       if (of_find_property(np, "regset-nr", NULL)) {
> >>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_SET;
> >>> +       }
> >>> +       if (of_find_property(np, "regdir-nr", NULL)) {
> >>> +               pdata->flags |= BGPIOF_UNREADABLE_REG_DIR;
> >>> +       }
> >>> +       if (!of_property_read_u32(np, "num-gpios", &tmp)) {
> >>> +               pdata->ngpio = tmp;
> >>> +       }
> >>
> >> I don't think this is acceptable. gpio-generic is designed to be used
> >> as a framework for other drivers to build upon. These drivers should
> >> have their own compatible strings, which should be enough to infer all
> >> the properties you defined here.
> >>
> >
> > gpio-generic is not only designed as a framework for other drivers, it can also be used directly by registering the driver through platform device (basic-mmio-gpio/basic-mmio-gpio-be).
> 
> This works for platform drivers which stay confined to the kernel, but
> DT is a firmware definition where such generic bindings are much less
> desirable.
> 
> > My intention is to make it DT compatible as a generic driver, which can be used for different hardware, by selecting different DT properties as configuration.
> >
> >> Device Tree identifies hardware precisely (vendor and model), and this
> >> new binding is just not that. You *could* however have a very simple
> >> driver that associates compatible strings to static tables containing
> >> the values of the properties you wanted to see passed through the DT,
> >> and have one single driver that covers many mmio-based GPIO devices.
> >> But I'm afraid "basic-mmio-gpio" is *way* to vague to describe
> >> hardware.
> >>
> >
> > I don't think making a new driver mapping different compatible strings to static tables containing the values of the properties passed through DT is a sane way of doing it, since it will require multiple combinations to cover all the possibilites.
> >
> > My plan is to be able to do something like this (btw, I actually tested this patch on BCM63xx and works perfectly):
> > gpio-controller@10000084 {
> >         compatible = "basic-mmio-gpio", "brcm,brcm6368";
> 
> Here your compatible string should be
> 
>         compatible = "brcm,brcm6368", "basic-mmio-gpio";
> 
> I.e. from more precise to less precise. A dedicated BRCM6368 driver
> should take precedence over your generic one.
> 
> >         reg = <0x10000084 0x4>, <0x1000008c 0x4>;
> >         reg-names = "dirout", "dat";
> >
> >         num-gpios = <32>;
> >         byte-be;
> >
> >         gpio-controller;
> >         #gpio-cells = <2>;
> > };
> > And for other hardware you could do:
> > gpio-controller@10000180 {
> >         compatible = "basic-mmio-gpio", "foo,bar";
> >         reg = <0x10000180 0x4>, <0x10000184 0x4>, <0x10000188 0x4>;
> >         reg-names = "dirin", "dirout", "dat";
> >
> >         num-gpios = <20>;
> >         bit-be;
> >         byte-be;
> >         regdir-nr;
> >
> >         gpio-controller;
> >         #gpio-cells = <2>;
> > };
> > This way you wouldn't need to add a wrapper for each specific hardware using basic-mmio-gpio, and you would save time by using the generic driver.
> 
> I understand the intent but IIUC the history of DT speaks  against
> such generic bindings. On the other hand I can see some instances of
> them like "simple-bus" for instance. Added the DT maintainers and
> mailing list to get more informed opinions on the matter.

While generic bindings can be ok, they either need to be incredibly
simple (e.g. simple-bus, which has no configuration whatsoever), or need
to be rigorously specified (e.g. the generic PCI host controller
binding, which follows an existing specification).

From the context above, this looks relatively complex. At the least, a
full binding document is required along with a rationale, so that can be
reviewed.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html