Message ID | 20210304071506.18434-1-noltari@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2] gpio: regmap: set gpio_chip of_node | expand |
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;
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.
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?
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.
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
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 --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;