Re: basic-mmio-gpio: add DT support

[Mark Rutland <mark.rutland@xxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
> >>> Add DT support while keeping legacy support.
> >>>
> >>> Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxx) 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html