On Wed, Jan 14, 2015 at 8:20 PM, Álvaro Fernández Rojas
<[email protected]> 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
>> <[email protected]> wrote:
>>> Add DT support while keeping legacy support.
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <[email protected]>
>>> ---
>>> 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 [email protected]) 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 devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
