diff mbox series

[v2] gpio: regmap: set gpio_chip of_node

Message ID 20210304071506.18434-1-noltari@gmail.com
State Superseded
Headers show
Series [v2] gpio: regmap: set gpio_chip of_node | expand

Commit Message

Álvaro Fernández Rojas March 4, 2021, 7:15 a.m. UTC 
This is needed for properly registering gpio regmap as a child of a regmap
pin controller.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Reviewed-by: Michael Walle <michael@walle.cc>
---
 v2: split this patch from the bcm63xx-pinctrl series

 drivers/gpio/gpio-regmap.c  | 1 +
 include/linux/gpio/regmap.h | 3 +++
 2 files changed, 4 insertions(+)

Comments

Michael Walle March 4, 2021, 8:18 a.m. UTC | #1 
Am 2021-03-04 08:15, schrieb Álvaro Fernández Rojas:
> This is needed for properly registering gpio regmap as a child of a 
> regmap
> pin controller.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: Michael Walle <michael@walle.cc>
> ---
>  v2: split this patch from the bcm63xx-pinctrl series
> 
>  drivers/gpio/gpio-regmap.c  | 1 +
>  include/linux/gpio/regmap.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index fed1e269c42a..8898ab3e1d59 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -249,6 +249,7 @@ struct gpio_regmap *gpio_regmap_register(const
> struct gpio_regmap_config *config
> 
>  	chip = &gpio->gpio_chip;
>  	chip->parent = config->parent;
> +	chip->of_node = config->of_node ?: dev_of_node(config->parent);
>  	chip->base = -1;
>  	chip->ngpio = config->ngpio;
>  	chip->names = config->names;
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index ad76f3d0a6ba..566d76d0dbae 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -4,6 +4,7 @@
>  #define _LINUX_GPIO_REGMAP_H
> 
>  struct device;
> +struct device_node;
>  struct gpio_regmap;
>  struct irq_domain;
>  struct regmap;
> @@ -15,6 +16,7 @@ struct regmap;
>   * struct gpio_regmap_config - Description of a generic regmap 
> gpio_chip.
>   * @parent:		The parent device
>   * @regmap:		The regmap used to access the registers
> + * @of_node:		(Optional) The device node
>   *			given, the name of the device is used

Something is messed up here ;) This line should be together with
the one containing @regmap. While at it please add the
"If not given, the of_node of the parent device is used."

-michael

>   * @label:		(Optional) Descriptive name for GPIO controller.
>   *			If not given, the name of the device is used.
> @@ -57,6 +59,7 @@ struct regmap;
>  struct gpio_regmap_config {
>  	struct device *parent;
>  	struct regmap *regmap;
> +	struct device_node *of_node;
> 
>  	const char *label;
>  	int ngpio;
Álvaro Fernández Rojas March 4, 2021, 8:27 a.m. UTC | #2 
Hi Michael,

> El 4 mar 2021, a las 9:18, Michael Walle <michael@walle.cc> escribió:
> 
> Am 2021-03-04 08:15, schrieb Álvaro Fernández Rojas:
>> This is needed for properly registering gpio regmap as a child of a regmap
>> pin controller.
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> Reviewed-by: Michael Walle <michael@walle.cc>
>> ---
>> v2: split this patch from the bcm63xx-pinctrl series
>> drivers/gpio/gpio-regmap.c  | 1 +
>> include/linux/gpio/regmap.h | 3 +++
>> 2 files changed, 4 insertions(+)
>> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
>> index fed1e269c42a..8898ab3e1d59 100644
>> --- a/drivers/gpio/gpio-regmap.c
>> +++ b/drivers/gpio/gpio-regmap.c
>> @@ -249,6 +249,7 @@ struct gpio_regmap *gpio_regmap_register(const
>> struct gpio_regmap_config *config
>> 	chip = &gpio->gpio_chip;
>> 	chip->parent = config->parent;
>> +	chip->of_node = config->of_node ?: dev_of_node(config->parent);
>> 	chip->base = -1;
>> 	chip->ngpio = config->ngpio;
>> 	chip->names = config->names;
>> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
>> index ad76f3d0a6ba..566d76d0dbae 100644
>> --- a/include/linux/gpio/regmap.h
>> +++ b/include/linux/gpio/regmap.h
>> @@ -4,6 +4,7 @@
>> #define _LINUX_GPIO_REGMAP_H
>> struct device;
>> +struct device_node;
>> struct gpio_regmap;
>> struct irq_domain;
>> struct regmap;
>> @@ -15,6 +16,7 @@ struct regmap;
>>  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
>>  * @parent:		The parent device
>>  * @regmap:		The regmap used to access the registers
>> + * @of_node:		(Optional) The device node
>>  *			given, the name of the device is used
> 
> Something is messed up here ;) This line should be together with
> the one containing @regmap. While at it please add the
> "If not given, the of_node of the parent device is used."

I think I was still sleeping when I did this…
Excuse me for that :(

> 
> -michael
> 
>>  * @label:		(Optional) Descriptive name for GPIO controller.
>>  *			If not given, the name of the device is used.
>> @@ -57,6 +59,7 @@ struct regmap;
>> struct gpio_regmap_config {
>> 	struct device *parent;
>> 	struct regmap *regmap;
>> +	struct device_node *of_node;
>> 	const char *label;
>> 	int ngpio;

Best regards,
Álvaro.
Andy Shevchenko March 4, 2021, 3:22 p.m. UTC | #3 
On Thu, Mar 4, 2021 at 5:18 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>
> This is needed for properly registering gpio regmap as a child of a regmap
> pin controller.

> +       chip->of_node = config->of_node ?: dev_of_node(config->parent);

After a closer look I have no clue why you need this patch at all.
The second part, i.e. assigning parent's fwnode, is done already in
the GPIO library core.
The first part, keeping fwnode in the regmap configuration puzzles me. Why?
Álvaro Fernández Rojas March 4, 2021, 3:26 p.m. UTC | #4 
Hi Andy,

> El 4 mar 2021, a las 16:22, Andy Shevchenko <andy.shevchenko@gmail.com> escribió:
> 
> On Thu, Mar 4, 2021 at 5:18 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>> 
>> This is needed for properly registering gpio regmap as a child of a regmap
>> pin controller.
> 
>> +       chip->of_node = config->of_node ?: dev_of_node(config->parent);
> 
> After a closer look I have no clue why you need this patch at all.
> The second part, i.e. assigning parent's fwnode, is done already in
> the GPIO library core.
> The first part, keeping fwnode in the regmap configuration puzzles me. Why?

I’ve flagged this as superseded since Linus asked me to send it with bcm63xx patches and I’ve already answered this same question there.

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Best regards,
Álvaro.
Michael Walle March 4, 2021, 3:31 p.m. UTC | #5 
Am 2021-03-04 16:22, schrieb Andy Shevchenko:
> On Thu, Mar 4, 2021 at 5:18 PM Álvaro Fernández Rojas 
> <noltari@gmail.com> wrote:
>> 
>> This is needed for properly registering gpio regmap as a child of a 
>> regmap
>> pin controller.
> 
>> +       chip->of_node = config->of_node ?: 
>> dev_of_node(config->parent);
> 
> After a closer look I have no clue why you need this patch at all.
> The second part, i.e. assigning parent's fwnode, is done already in
> the GPIO library core.
> The first part, keeping fwnode in the regmap configuration puzzles me. 
> Why?

You're right if chip->of_node is not set it will eventually be set to
node of the parent device.

In case of the BCM driver the parent device is the pinctrl device and
the node is a children (if that is correct I cannot say). So, in this
case you cannot use the node of the parent, because that would be the
pinctrl one.

You could just use
   chip->of_node = dev_of_node(config->parent)

But then it is not obvious that it is optional. Hence my suggestion
to explicitly set to the parent of_node if none is supplied.

-michael
kernel test robot March 17, 2021, 4:06 p.m. UTC | #6 
Hi "Álvaro,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on gpio/for-next]
[also build test ERROR on v5.12-rc3 next-20210317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/lvaro-Fern-ndez-Rojas/gpio-regmap-set-gpio_chip-of_node/20210304-152016
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-a013-20210317 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8ef111222a3dd12a9175f69c3bff598c46e8bdf7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/832a71aa1e741f0ef3b28952bf53627a820a7d68
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review lvaro-Fern-ndez-Rojas/gpio-regmap-set-gpio_chip-of_node/20210304-152016
        git checkout 832a71aa1e741f0ef3b28952bf53627a820a7d68
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpio/gpio-regmap.c:252:8: error: no member named 'of_node' in 'struct gpio_chip'
           chip->of_node = config->of_node ?: dev_of_node(config->parent);
           ~~~~  ^
   1 error generated.


vim +252 drivers/gpio/gpio-regmap.c

   192	
   193	/**
   194	 * gpio_regmap_register() - Register a generic regmap GPIO controller
   195	 * @config: configuration for gpio_regmap
   196	 *
   197	 * Return: A pointer to the registered gpio_regmap or ERR_PTR error value.
   198	 */
   199	struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
   200	{
   201		struct gpio_regmap *gpio;
   202		struct gpio_chip *chip;
   203		int ret;
   204	
   205		if (!config->parent)
   206			return ERR_PTR(-EINVAL);
   207	
   208		if (!config->ngpio)
   209			return ERR_PTR(-EINVAL);
   210	
   211		/* we need at least one */
   212		if (!config->reg_dat_base && !config->reg_set_base)
   213			return ERR_PTR(-EINVAL);
   214	
   215		/* if we have a direction register we need both input and output */
   216		if ((config->reg_dir_out_base || config->reg_dir_in_base) &&
   217		    (!config->reg_dat_base || !config->reg_set_base))
   218			return ERR_PTR(-EINVAL);
   219	
   220		/* we don't support having both registers simultaneously for now */
   221		if (config->reg_dir_out_base && config->reg_dir_in_base)
   222			return ERR_PTR(-EINVAL);
   223	
   224		gpio = kzalloc(sizeof(*gpio), GFP_KERNEL);
   225		if (!gpio)
   226			return ERR_PTR(-ENOMEM);
   227	
   228		gpio->parent = config->parent;
   229		gpio->regmap = config->regmap;
   230		gpio->ngpio_per_reg = config->ngpio_per_reg;
   231		gpio->reg_stride = config->reg_stride;
   232		gpio->reg_mask_xlate = config->reg_mask_xlate;
   233		gpio->reg_dat_base = config->reg_dat_base;
   234		gpio->reg_set_base = config->reg_set_base;
   235		gpio->reg_clr_base = config->reg_clr_base;
   236		gpio->reg_dir_in_base = config->reg_dir_in_base;
   237		gpio->reg_dir_out_base = config->reg_dir_out_base;
   238	
   239		/* if not set, assume there is only one register */
   240		if (!gpio->ngpio_per_reg)
   241			gpio->ngpio_per_reg = config->ngpio;
   242	
   243		/* if not set, assume they are consecutive */
   244		if (!gpio->reg_stride)
   245			gpio->reg_stride = 1;
   246	
   247		if (!gpio->reg_mask_xlate)
   248			gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
   249	
   250		chip = &gpio->gpio_chip;
   251		chip->parent = config->parent;
 > 252		chip->of_node = config->of_node ?: dev_of_node(config->parent);
   253		chip->base = -1;
   254		chip->ngpio = config->ngpio;
   255		chip->names = config->names;
   256		chip->label = config->label ?: dev_name(config->parent);
   257	
   258		/*
   259		 * If our regmap is fast_io we should probably set can_sleep to false.
   260		 * Right now, the regmap doesn't save this property, nor is there any
   261		 * access function for it.
   262		 * The only regmap type which uses fast_io is regmap-mmio. For now,
   263		 * assume a safe default of true here.
   264		 */
   265		chip->can_sleep = true;
   266	
   267		chip->get = gpio_regmap_get;
   268		if (gpio->reg_set_base && gpio->reg_clr_base)
   269			chip->set = gpio_regmap_set_with_clear;
   270		else if (gpio->reg_set_base)
   271			chip->set = gpio_regmap_set;
   272	
   273		if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
   274			chip->get_direction = gpio_regmap_get_direction;
   275			chip->direction_input = gpio_regmap_direction_input;
   276			chip->direction_output = gpio_regmap_direction_output;
   277		}
   278	
   279		ret = gpiochip_add_data(chip, gpio);
   280		if (ret < 0)
   281			goto err_free_gpio;
   282	
   283		if (config->irq_domain) {
   284			ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
   285			if (ret)
   286				goto err_remove_gpiochip;
   287		}
   288	
   289		return gpio;
   290	
   291	err_remove_gpiochip:
   292		gpiochip_remove(chip);
   293	err_free_gpio:
   294		kfree(gpio);
   295		return ERR_PTR(ret);
   296	}
   297	EXPORT_SYMBOL_GPL(gpio_regmap_register);
   298	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index fed1e269c42a..8898ab3e1d59 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -249,6 +249,7 @@  struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 
 	chip = &gpio->gpio_chip;
 	chip->parent = config->parent;
+	chip->of_node = config->of_node ?: dev_of_node(config->parent);
 	chip->base = -1;
 	chip->ngpio = config->ngpio;
 	chip->names = config->names;
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index ad76f3d0a6ba..566d76d0dbae 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -4,6 +4,7 @@ 
 #define _LINUX_GPIO_REGMAP_H
 
 struct device;
+struct device_node;
 struct gpio_regmap;
 struct irq_domain;
 struct regmap;
@@ -15,6 +16,7 @@  struct regmap;
  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
  * @parent:		The parent device
  * @regmap:		The regmap used to access the registers
+ * @of_node:		(Optional) The device node
  *			given, the name of the device is used
  * @label:		(Optional) Descriptive name for GPIO controller.
  *			If not given, the name of the device is used.
@@ -57,6 +59,7 @@  struct regmap;
 struct gpio_regmap_config {
 	struct device *parent;
 	struct regmap *regmap;
+	struct device_node *of_node;
 
 	const char *label;
 	int ngpio;