Re: basic-mmio-gpio: add DT support

[Alexandre Courbot <gnurou@xxxxxxxxx>]

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?

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

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