| Message ID | 20210208222203.22335-8-info@metux.net |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,01/12] of: base: improve error message in of_phandle_iterator_next() | expand |
On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > > Add support for probing via device tree. > --- > drivers/gpio/gpio-amd-fch.c | 58 +++++++++++++++++++++++++ > include/dt-bindings/gpio/amd-fch-gpio.h | 36 +++++++++++++++ > include/linux/platform_data/gpio/gpio-amd-fch.h | 24 ++-------- > 3 files changed, 98 insertions(+), 20 deletions(-) > create mode 100644 include/dt-bindings/gpio/amd-fch-gpio.h > > diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c > index 2a21354ed6a0..32024f99dae5 100644 > --- a/drivers/gpio/gpio-amd-fch.c > +++ b/drivers/gpio/gpio-amd-fch.c > @@ -136,12 +136,61 @@ static int amd_fch_gpio_request(struct gpio_chip *chip, > return 0; > } > > +static struct amd_fch_gpio_pdata *load_pdata(struct device *dev) Please stick to the amd_fch_ prefix to all symbols for consistency. > +{ > + struct amd_fch_gpio_pdata *pdata; > + int ret; > + > + pdata = devm_kzalloc(dev, sizeof(struct amd_fch_gpio_pdata), > + GFP_KERNEL); > + if (!pdata) > + goto nomem; > + > + pdata->gpio_num = of_property_count_elems_of_size(dev->of_node, > + "gpio-regs", > + sizeof(u32)); > + pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num, > + GFP_KERNEL); > + if (!pdata->gpio_reg) > + goto nomem; > + > + pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num, > + GFP_KERNEL); > + if (!pdata->gpio_names) > + goto nomem; > + > + ret = of_property_read_variable_u32_array(dev->of_node, "gpio-regs", > + pdata->gpio_reg, > + pdata->gpio_num, > + pdata->gpio_num); > + if (ret != pdata->gpio_num) { > + dev_err(dev, "failed reading gpio-regs from DT: %d\n", ret); > + return NULL; > + } > + > + ret = of_property_read_string_array(dev->of_node, "gpio-line-names", > + pdata->gpio_names, pdata->gpio_num); > + if (ret != pdata->gpio_num) { > + dev_err(dev, "failed reading gpio-names from DT: %d\n", ret); > + return NULL; > + } Since you're not iterating over DT nodes nor use any interfaces specific to OF - I suggest you use the generic device properties to load the fill the platform data. > + > + return pdata; > + > +nomem: > + dev_err(dev, "load_pdata: failed allocating memory\n"); > + return NULL; > +} > + > static int amd_fch_gpio_probe(struct platform_device *pdev) > { > struct amd_fch_gpio_priv *priv; > struct amd_fch_gpio_pdata *pdata; > > pdata = dev_get_platdata(&pdev->dev); > + if (!pdata) > + pdata = load_pdata(&pdev->dev); > + > if (!pdata) { > dev_err(&pdev->dev, "no platform_data\n"); > return -ENOENT; > @@ -165,6 +214,9 @@ static int amd_fch_gpio_probe(struct platform_device *pdev) > priv->gc.get_direction = amd_fch_gpio_get_direction; > priv->gc.get = amd_fch_gpio_get; > priv->gc.set = amd_fch_gpio_set; > +#ifdef CONFIG_OF_GPIO > + priv->gc.of_node = pdev->dev.of_node; > +#endif > > spin_lock_init(&priv->lock); > > @@ -177,9 +229,15 @@ static int amd_fch_gpio_probe(struct platform_device *pdev) > return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); > } > > +static const struct of_device_id amd_fch_gpio_of_match[] = { > + { .compatible = "amd,fch-gpio" }, > + {} > +}; Don't you need the module table? > + > static struct platform_driver amd_fch_gpio_driver = { > .driver = { > .name = AMD_FCH_GPIO_DRIVER_NAME, > + .of_match_table = amd_fch_gpio_of_match, > }, > .probe = amd_fch_gpio_probe, > }; > diff --git a/include/dt-bindings/gpio/amd-fch-gpio.h b/include/dt-bindings/gpio/amd-fch-gpio.h > new file mode 100644 > index 000000000000..7a47e6debcdb > --- /dev/null > +++ b/include/dt-bindings/gpio/amd-fch-gpio.h > @@ -0,0 +1,36 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > + > +/* > + * AMD FCH gpio platform data definitions > + * > + * Copyright (C) 2020 metux IT consult > + * Author: Enrico Weigelt <info@metux.net> > + * > + */ > + > +#ifndef __DT_BINDINGS_GPIO_AMD_FCH_REGS_H > +#define __DT_BINDINGS_GPIO_AMD_FCH_REGS_H > + > +/* > + * gpio registers addresses > + * > + * these regs need to be assigned by board setup, since they're wired > + * in very board specifici was, rarely documented, this should not be > + * available to users. > + */ > +#define AMD_FCH_GPIO_REG_GPIO49 0x40 > +#define AMD_FCH_GPIO_REG_GPIO50 0x41 > +#define AMD_FCH_GPIO_REG_GPIO51 0x42 > +#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 0x43 > +#define AMD_FCH_GPIO_REG_GPIO57 0x44 > +#define AMD_FCH_GPIO_REG_GPIO58 0x45 > +#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 0x46 > +#define AMD_FCH_GPIO_REG_GPIO64 0x47 > +#define AMD_FCH_GPIO_REG_GPIO68 0x48 > +#define AMD_FCH_GPIO_REG_GPIO66_SPKR 0x5B > +#define AMD_FCH_GPIO_REG_GPIO71 0x4D > +#define AMD_FCH_GPIO_REG_GPIO32_GE1 0x59 > +#define AMD_FCH_GPIO_REG_GPIO33_GE2 0x5A > +#define AMT_FCH_GPIO_REG_GEVT22 0x09 > + > +#endif /* __DT_BINDINGS_GPIO_AMD_FCH_REGS_H */ > diff --git a/include/linux/platform_data/gpio/gpio-amd-fch.h b/include/linux/platform_data/gpio/gpio-amd-fch.h > index 255d51c9d36d..336f7387e82c 100644 > --- a/include/linux/platform_data/gpio/gpio-amd-fch.h > +++ b/include/linux/platform_data/gpio/gpio-amd-fch.h > @@ -11,25 +11,9 @@ > #ifndef __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H > #define __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H > > -#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" > +#include <dt-bindings/gpio/amd-fch-gpio.h> > > -/* > - * gpio register index definitions > - */ > -#define AMD_FCH_GPIO_REG_GPIO49 0x40 > -#define AMD_FCH_GPIO_REG_GPIO50 0x41 > -#define AMD_FCH_GPIO_REG_GPIO51 0x42 > -#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 0x43 > -#define AMD_FCH_GPIO_REG_GPIO57 0x44 > -#define AMD_FCH_GPIO_REG_GPIO58 0x45 > -#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 0x46 > -#define AMD_FCH_GPIO_REG_GPIO64 0x47 > -#define AMD_FCH_GPIO_REG_GPIO68 0x48 > -#define AMD_FCH_GPIO_REG_GPIO66_SPKR 0x5B > -#define AMD_FCH_GPIO_REG_GPIO71 0x4D > -#define AMD_FCH_GPIO_REG_GPIO32_GE1 0x59 > -#define AMD_FCH_GPIO_REG_GPIO33_GE2 0x5A > -#define AMT_FCH_GPIO_REG_GEVT22 0x09 > +#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" > > /* > * struct amd_fch_gpio_pdata - GPIO chip platform data > @@ -39,8 +23,8 @@ > */ > struct amd_fch_gpio_pdata { > int gpio_num; > - int *gpio_reg; > - const char * const *gpio_names; > + u32 *gpio_reg; > + const char * *gpio_names; > }; > > #endif /* __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H */ > -- > 2.11.0 > Bartosz
On Mon, Feb 8, 2021 at 11:24 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > Add support for probing via device tree. (...) > + pdata->gpio_num = of_property_count_elems_of_size(dev->of_node, > + "gpio-regs", > + sizeof(u32)); > + pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num, > + GFP_KERNEL); > + if (!pdata->gpio_reg) > + goto nomem; I don't know what the idea is with this but register are not normally defined in the DTS files. The registers are determined from the compatible value. > + pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num, > + GFP_KERNEL); > + if (!pdata->gpio_names) > + goto nomem; (...) > + ret = of_property_read_string_array(dev->of_node, "gpio-line-names", > + pdata->gpio_names, pdata->gpio_num); And this is already handled by the core. Yours, Linus Walleij
On 01.03.21 15:51, Linus Walleij wrote: Hi, > I don't know what the idea is with this but register are not normally defined > in the DTS files. The registers are determined from the compatible value. The idea is basically replacing the pdata struct by oftree node. (subsequent patches in this queue use this by doing the board setup via compiled-in dtb, instead of the currently hardcoded tables). On these SoCs, the gpio setup is a little bit more complex than just having a fixed range of registers (one per pin): the actual meaning depends und Soc model and board type - some regs aren't even gpios. (I'm still in progress of RE'ing the bios blob, to find out more, eg. pinmux setups, etc). Writing to the wrong regs can cause weird effects (actually not even sure whether it could lead to damage) In essence: only a specific subset of the register range can be used for GPIOs - the others shouldn't ever be touched. And this specific subset is soc/board specific. --mtx
On Thu, Mar 11, 2021 at 12:20 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > On 01.03.21 15:51, Linus Walleij wrote: > > I don't know what the idea is with this but register are not normally defined > > in the DTS files. The registers are determined from the compatible value. > > The idea is basically replacing the pdata struct by oftree node. > (subsequent patches in this queue use this by doing the board setup via > compiled-in dtb, instead of the currently hardcoded tables). You are a bit late. We have built-in device properties (and corresponding API, which recently becomes swnode) which aims exactly this.
On 11.03.21 11:42, Andy Shevchenko wrote: Hi, > You are a bit late. We have built-in device properties (and > corresponding API, which recently becomes swnode) which aims exactly > this. Is there some compact notation for swnode's that's as small and simple as some piece of DTS ? My reasons for choosing built-in dtb have been: * it's a very small and compact notation for describing devices * no more open-coded registrations, etc * no more need for board drivers (except for the little piece of DT) --mtx
On Thu, Mar 18, 2021 at 9:00 AM Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > Is there some compact notation for swnode's that's as small and simple > as some piece of DTS ? Yes it's really neat. It's all in <linux/property.h> and examples in e.g. the testsuite: drivers/base/test/property-entry-test.c You can just grep for PROPERTY_ENTRY and you find some examples of how we use it. Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-amd-fch.c b/drivers/gpio/gpio-amd-fch.c index 2a21354ed6a0..32024f99dae5 100644 --- a/drivers/gpio/gpio-amd-fch.c +++ b/drivers/gpio/gpio-amd-fch.c @@ -136,12 +136,61 @@ static int amd_fch_gpio_request(struct gpio_chip *chip, return 0; } +static struct amd_fch_gpio_pdata *load_pdata(struct device *dev) +{ + struct amd_fch_gpio_pdata *pdata; + int ret; + + pdata = devm_kzalloc(dev, sizeof(struct amd_fch_gpio_pdata), + GFP_KERNEL); + if (!pdata) + goto nomem; + + pdata->gpio_num = of_property_count_elems_of_size(dev->of_node, + "gpio-regs", + sizeof(u32)); + pdata->gpio_reg = devm_kzalloc(dev, sizeof(int)*pdata->gpio_num, + GFP_KERNEL); + if (!pdata->gpio_reg) + goto nomem; + + pdata->gpio_names = devm_kzalloc(dev, sizeof(char*)*pdata->gpio_num, + GFP_KERNEL); + if (!pdata->gpio_names) + goto nomem; + + ret = of_property_read_variable_u32_array(dev->of_node, "gpio-regs", + pdata->gpio_reg, + pdata->gpio_num, + pdata->gpio_num); + if (ret != pdata->gpio_num) { + dev_err(dev, "failed reading gpio-regs from DT: %d\n", ret); + return NULL; + } + + ret = of_property_read_string_array(dev->of_node, "gpio-line-names", + pdata->gpio_names, pdata->gpio_num); + if (ret != pdata->gpio_num) { + dev_err(dev, "failed reading gpio-names from DT: %d\n", ret); + return NULL; + } + + return pdata; + +nomem: + dev_err(dev, "load_pdata: failed allocating memory\n"); + return NULL; +} + static int amd_fch_gpio_probe(struct platform_device *pdev) { struct amd_fch_gpio_priv *priv; struct amd_fch_gpio_pdata *pdata; pdata = dev_get_platdata(&pdev->dev); + if (!pdata) + pdata = load_pdata(&pdev->dev); + if (!pdata) { dev_err(&pdev->dev, "no platform_data\n"); return -ENOENT; @@ -165,6 +214,9 @@ static int amd_fch_gpio_probe(struct platform_device *pdev) priv->gc.get_direction = amd_fch_gpio_get_direction; priv->gc.get = amd_fch_gpio_get; priv->gc.set = amd_fch_gpio_set; +#ifdef CONFIG_OF_GPIO + priv->gc.of_node = pdev->dev.of_node; +#endif spin_lock_init(&priv->lock); @@ -177,9 +229,15 @@ static int amd_fch_gpio_probe(struct platform_device *pdev) return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv); } +static const struct of_device_id amd_fch_gpio_of_match[] = { + { .compatible = "amd,fch-gpio" }, + {} +}; + static struct platform_driver amd_fch_gpio_driver = { .driver = { .name = AMD_FCH_GPIO_DRIVER_NAME, + .of_match_table = amd_fch_gpio_of_match, }, .probe = amd_fch_gpio_probe, }; diff --git a/include/dt-bindings/gpio/amd-fch-gpio.h b/include/dt-bindings/gpio/amd-fch-gpio.h new file mode 100644 index 000000000000..7a47e6debcdb --- /dev/null +++ b/include/dt-bindings/gpio/amd-fch-gpio.h @@ -0,0 +1,36 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ + +/* + * AMD FCH gpio platform data definitions + * + * Copyright (C) 2020 metux IT consult + * Author: Enrico Weigelt <info@metux.net> + * + */ + +#ifndef __DT_BINDINGS_GPIO_AMD_FCH_REGS_H +#define __DT_BINDINGS_GPIO_AMD_FCH_REGS_H + +/* + * gpio registers addresses + * + * these regs need to be assigned by board setup, since they're wired + * in very board specifici was, rarely documented, this should not be + * available to users. + */ +#define AMD_FCH_GPIO_REG_GPIO49 0x40 +#define AMD_FCH_GPIO_REG_GPIO50 0x41 +#define AMD_FCH_GPIO_REG_GPIO51 0x42 +#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 0x43 +#define AMD_FCH_GPIO_REG_GPIO57 0x44 +#define AMD_FCH_GPIO_REG_GPIO58 0x45 +#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 0x46 +#define AMD_FCH_GPIO_REG_GPIO64 0x47 +#define AMD_FCH_GPIO_REG_GPIO68 0x48 +#define AMD_FCH_GPIO_REG_GPIO66_SPKR 0x5B +#define AMD_FCH_GPIO_REG_GPIO71 0x4D +#define AMD_FCH_GPIO_REG_GPIO32_GE1 0x59 +#define AMD_FCH_GPIO_REG_GPIO33_GE2 0x5A +#define AMT_FCH_GPIO_REG_GEVT22 0x09 + +#endif /* __DT_BINDINGS_GPIO_AMD_FCH_REGS_H */ diff --git a/include/linux/platform_data/gpio/gpio-amd-fch.h b/include/linux/platform_data/gpio/gpio-amd-fch.h index 255d51c9d36d..336f7387e82c 100644 --- a/include/linux/platform_data/gpio/gpio-amd-fch.h +++ b/include/linux/platform_data/gpio/gpio-amd-fch.h @@ -11,25 +11,9 @@ #ifndef __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H #define __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H -#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" +#include <dt-bindings/gpio/amd-fch-gpio.h> -/* - * gpio register index definitions - */ -#define AMD_FCH_GPIO_REG_GPIO49 0x40 -#define AMD_FCH_GPIO_REG_GPIO50 0x41 -#define AMD_FCH_GPIO_REG_GPIO51 0x42 -#define AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 0x43 -#define AMD_FCH_GPIO_REG_GPIO57 0x44 -#define AMD_FCH_GPIO_REG_GPIO58 0x45 -#define AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 0x46 -#define AMD_FCH_GPIO_REG_GPIO64 0x47 -#define AMD_FCH_GPIO_REG_GPIO68 0x48 -#define AMD_FCH_GPIO_REG_GPIO66_SPKR 0x5B -#define AMD_FCH_GPIO_REG_GPIO71 0x4D -#define AMD_FCH_GPIO_REG_GPIO32_GE1 0x59 -#define AMD_FCH_GPIO_REG_GPIO33_GE2 0x5A -#define AMT_FCH_GPIO_REG_GEVT22 0x09 +#define AMD_FCH_GPIO_DRIVER_NAME "gpio_amd_fch" /* * struct amd_fch_gpio_pdata - GPIO chip platform data @@ -39,8 +23,8 @@ */ struct amd_fch_gpio_pdata { int gpio_num; - int *gpio_reg; - const char * const *gpio_names; + u32 *gpio_reg; + const char * *gpio_names; }; #endif /* __LINUX_PLATFORM_DATA_GPIO_AMD_FCH_H */