| Message ID | 535f785bf6116c0fb6f46afbb77e6d4bd3ef5f60.1462543458.git.chunkeey@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Christian, On 8 May 2016 at 15:08, Christian Lamparter <chunkeey@googlemail.com> wrote: > From: Álvaro Fernández Rojas <noltari@gmail.com> > > This patch adds support for defining memory-mapped GPIOs which > are compatible with the existing gpio-mmio interface. The generic > library provides support for many memory-mapped GPIO controllers > that are found in various on-board FPGA and ASIC solutions that > are used to control board's switches, LEDs, chip-selects, > Ethernet/USB PHY power, etc. > > For setting GPIO's there are three configurations: > 1. single input/output register resource (named "dat"), > 2. set/clear pair (named "set" and "clr"), > 3. single output register resource and single input resource > ("set" and dat"). > > The configuration is detected by which resources are present. > For the single output register, this drives a 1 by setting a bit > and a zero by clearing a bit. For the set clr pair, this drives > a 1 by setting a bit in the set register and clears it by setting > a bit in the clear register. The configuration is detected by > which resources are present. > > For setting the GPIO direction, there are three configurations: > a. simple bidirectional GPIOs that requires no configuration. > b. an output direction register (named "dirout") > where a 1 bit indicates the GPIO is an output. > c. an input direction register (named "dirin") > where a 1 bit indicates the GPIO is an input. > > The first user for this binding is "wd,mbl-gpio". > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > --- > +#define ADD(_name, _func) { .compatible = _name, .data = _func } I don't see the point in having a macro for such a simple data structure, but since this v8 and Linus hasn't complained I guess it's fine. Using a macro here makes it impossible to grep for 'compatible'. Doing 'git grep compatible drivers/gpio/' is sometimes very useful to see which hardware the driver actually supports. > +static const struct of_device_id bgpio_of_match[] = { > + ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt), > + { } > +}; > +#undef ADD > +MODULE_DEVICE_TABLE(of, bgpio_of_match); > + > +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev, > + unsigned long *flags) > +{ > + const int (*parse_dt)(struct platform_device *, > + struct bgpio_pdata *, unsigned long *); > + const struct device_node *node = pdev->dev.of_node; > + const struct of_device_id *of_id; > + struct bgpio_pdata *pdata; > + int err = -ENODEV; > + > + of_id = of_match_node(bgpio_of_match, node); > + if (!of_id) > + return NULL; > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata), > + GFP_KERNEL); > + if (!pdata) > + return ERR_PTR(-ENOMEM); > + > + parse_dt = (const void *)of_id->data; You can retrieve OF match data using of_device_get_match_data(). Saves you a couple of lines and better explains what your doing. > + if (parse_dt) > + err = parse_dt(pdev, pdata, flags); > + if (err) > + return ERR_PTR(err); > + > + return pdata; > +} regards, Joachim Eastwood
On Sunday, May 08, 2016 07:17:13 PM Joachim Eastwood wrote: > > +#define ADD(_name, _func) { .compatible = _name, .data = _func } > > I don't see the point in having a macro for such a simple data > structure, but since this v8 and Linus hasn't complained I guess it's > fine. > > Using a macro here makes it impossible to grep for 'compatible'. Doing > 'git grep compatible drivers/gpio/' is sometimes very useful to see > which hardware the driver actually supports. Ok. I'll definitely picking it up. I'll wait until Tuesday/Wednesday for more comments and then release a new series. > > +static const struct of_device_id bgpio_of_match[] = { > > + ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt), > > + { } > > +}; > > +#undef ADD > > +MODULE_DEVICE_TABLE(of, bgpio_of_match); > > + > > +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev, > > + unsigned long *flags) > > +{ [...] > > + of_id = of_match_node(bgpio_of_match, node); > > + if (!of_id) > > + return NULL; [...] > You can retrieve OF match data using of_device_get_match_data(). > Saves you a couple of lines and better explains what your doing. Yes thanks that's useful too since I don't need the of_id variable anymore. Both improvements save a total of 8 lines. so it's 328 insertions(+) vs 380 deletions(-). Regards, Christian
On Sun, May 8, 2016 at 3:08 PM, Christian Lamparter <chunkeey@googlemail.com> wrote: > From: Álvaro Fernández Rojas <noltari@gmail.com> > > This patch adds support for defining memory-mapped GPIOs which > are compatible with the existing gpio-mmio interface. Overall very nice, just waiting for the next version. > The first user for this binding is "wd,mbl-gpio". And that binding defines that we have a register named "dat". > + if (of_property_read_bool(dev->of_node, "no-output")) And then this too. Do we want these generic MMIO bindings (dat, no-output) in a special document like Documentation/devicetree/bindings/gpio/gpio-mmio.txt? Going forward? This patch set mainly deals with refactorings, but in the long run we want to slim things down a bit and use standard bindings I think. Yours, Linus Walleij
On Tuesday, May 10, 2016 02:08:45 PM Linus Walleij wrote: > On Sun, May 8, 2016 at 3:08 PM, Christian Lamparter > <chunkeey@googlemail.com> wrote: > > > From: Álvaro Fernández Rojas <noltari@gmail.com> > > > > This patch adds support for defining memory-mapped GPIOs which > > are compatible with the existing gpio-mmio interface. > > Overall very nice, just waiting for the next version. K, will deliver. I noticed that you sent a mail in which you stated that you applied the dt binding already. Can you update your devel branch on git.kernel.org's linux-gpio? Then, I'll simply rebase my series and sent the remaining two patches. (unless you tell me otherwise). > > The first user for this binding is "wd,mbl-gpio". > > And that binding defines that we have a register named "dat". Yeah, I had to remove all non wd related bits. But since this series was posted (over and over :D) on a public mailing-list the original "generic" linux-mmio binding is available for everybody to perusal[0] and study. I think what we can make would be something like a devicetree template out of it. This way people can remove unused flags and regnames for their compatible device tree binding. (But first: need to finish that ppc-gpio.txt). > > + if (of_property_read_bool(dev->of_node, "no-output")) > > And then this too. > > Do we want these generic MMIO bindings (dat, no-output) > in a special document like > Documentation/devicetree/bindings/gpio/gpio-mmio.txt? > > Going forward? Ah, I was thinking about Documentation/gpio... Since there's no way it would go in the devicetree/bindings without having a compatible? (And there's technically none). As far as I know the problem here is not that it would be impossible to do that (updating a .dts file is "easy"...), but updating .dtb to a tiny flash-rom on the device might not be. So we have to make every effort to preserve compatibility for those devices (and old/incomplete/broken dtbs) as long as the device is supported. About adding new device: This will work in the following way: 1. new drives will need to supply their hardware-specific devicetree binding file to the dt maintainers (This "vendor,device.txt" file will be like the wd,mbl-gpio.txt - but modified for the hardware (this is where the template would be handy) 2. Make a one-liner patch which adds a compatible string to gpio-mmio.c's bgpio_of_match table: + { .compatible = "vendor,device", .data = bgpio_basic_mmio_parse_dt }, (Of course, not having parses for the "ngpio" property and the flags like big-endian, reg-output-reg,set, unreadable-reg-dir, ... properties from the get-go is sad, these can add back once a driver/binding needs it). I think brcm63xx will be following shortly. So we can test the procedure. > This patch set mainly deals with refactorings, but in the > long run we want to slim things down a bit and use standard > bindings I think. Well, to do that, I think we need to collect enough devices to make it a real "class" of devices first. Regards, Christian [0] <https://lkml.org/lkml/2016/4/28/921>
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index 6c1cb3b..4b7b74c 100644 --- a/drivers/gpio/gpio-mmio.c +++ b/drivers/gpio/gpio-mmio.c @@ -61,6 +61,8 @@ o ` ~~~~\___/~~~~ ` controller in FPGA is ,.` #include <linux/bitops.h> #include <linux/platform_device.h> #include <linux/mod_devicetable.h> +#include <linux/of.h> +#include <linux/of_device.h> static void bgpio_write8(void __iomem *reg, unsigned long data) { @@ -569,6 +571,66 @@ static void __iomem *bgpio_map(struct platform_device *pdev, return devm_ioremap_resource(&pdev->dev, r); } +#ifdef CONFIG_OF +static int bgpio_basic_mmio_parse_dt(struct platform_device *pdev, + struct bgpio_pdata *pdata, + unsigned long *flags) +{ + struct device *dev = &pdev->dev; + int err; + + pdata->base = -1; + + if (of_property_read_bool(dev->of_node, "no-output")) + *flags |= BGPIOF_NO_OUTPUT; + + return 0; +} + +#define ADD(_name, _func) { .compatible = _name, .data = _func } + +static const struct of_device_id bgpio_of_match[] = { + ADD("wd,mbl-gpio", bgpio_basic_mmio_parse_dt), + { } +}; +#undef ADD +MODULE_DEVICE_TABLE(of, bgpio_of_match); + +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev, + unsigned long *flags) +{ + const int (*parse_dt)(struct platform_device *, + struct bgpio_pdata *, unsigned long *); + const struct device_node *node = pdev->dev.of_node; + const struct of_device_id *of_id; + struct bgpio_pdata *pdata; + int err = -ENODEV; + + of_id = of_match_node(bgpio_of_match, node); + if (!of_id) + return NULL; + + pdata = devm_kzalloc(&pdev->dev, sizeof(struct bgpio_pdata), + GFP_KERNEL); + if (!pdata) + return ERR_PTR(-ENOMEM); + + parse_dt = (const void *)of_id->data; + if (parse_dt) + err = parse_dt(pdev, pdata, flags); + if (err) + return ERR_PTR(err); + + return pdata; +} +#else +static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev, + unsigned long *flags) +{ + return NULL; +} +#endif /* CONFIG_OF */ + static int bgpio_pdev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -579,10 +641,19 @@ static int bgpio_pdev_probe(struct platform_device *pdev) void __iomem *dirout; void __iomem *dirin; unsigned long sz; - unsigned long flags = pdev->id_entry->driver_data; + unsigned long flags = 0; int err; struct gpio_chip *gc; - struct bgpio_pdata *pdata = dev_get_platdata(dev); + struct bgpio_pdata *pdata; + + pdata = bgpio_parse_dt(pdev, &flags); + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + + if (!pdata) { + pdata = dev_get_platdata(dev); + flags = pdev->id_entry->driver_data; + } r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat"); if (!r) @@ -646,6 +717,7 @@ MODULE_DEVICE_TABLE(platform, bgpio_id_table); static struct platform_driver bgpio_driver = { .driver = { .name = "basic-mmio-gpio", + .of_match_table = of_match_ptr(bgpio_of_match), }, .id_table = bgpio_id_table, .probe = bgpio_pdev_probe,