| Message ID | 20210302180601.12082-1-noltari@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: regmap: move struct gpio_regmap definition | expand |
Hi, Am 2021-03-02 19:06, schrieb Álvaro Fernández Rojas: > struct gpio_regmap should be declared in gpio/regmap.h. > This way other drivers can access the structure data when registering a > gpio > regmap controller. The intention was really to keep this private to the gpio-regmap driver. Why do you need to access to the properties? -michael > Fixes: ebe363197e52 ("gpio: add a reusable generic gpio_chip using > regmap") > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > drivers/gpio/gpio-regmap.c | 20 ------------------ > include/linux/gpio/regmap.h | 41 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 40 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c > index 5412cb3b0b2a..23b0a8572f53 100644 > --- a/drivers/gpio/gpio-regmap.c > +++ b/drivers/gpio/gpio-regmap.c > @@ -11,26 +11,6 @@ > #include <linux/module.h> > #include <linux/regmap.h> > > -struct gpio_regmap { > - struct device *parent; > - struct regmap *regmap; > - struct gpio_chip gpio_chip; > - > - int reg_stride; > - int ngpio_per_reg; > - unsigned int reg_dat_base; > - unsigned int reg_set_base; > - unsigned int reg_clr_base; > - unsigned int reg_dir_in_base; > - unsigned int reg_dir_out_base; > - > - int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, > - unsigned int offset, unsigned int *reg, > - unsigned int *mask); > - > - void *driver_data; > -}; > - > static unsigned int gpio_regmap_addr(unsigned int addr) > { > if (addr == GPIO_REGMAP_ADDR_ZERO) > diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h > index ad76f3d0a6ba..ce2fc6e9b62b 100644 > --- a/include/linux/gpio/regmap.h > +++ b/include/linux/gpio/regmap.h > @@ -4,13 +4,52 @@ > #define _LINUX_GPIO_REGMAP_H > > struct device; > -struct gpio_regmap; > struct irq_domain; > struct regmap; > > #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1)) > #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO) > > +/** > + * struct gpio_regmap - GPIO regmap controller > + * @parent: The parent device > + * @regmap: The regmap used to access the registers > + * given, the name of the device is used > + * @gpio_chip: GPIO chip controller > + * @ngpio_per_reg: Number of GPIOs per register > + * @reg_stride: (Optional) May be set if the registers (of the > + * same type, dat, set, etc) are not consecutive. > + * @reg_dat_base: (Optional) (in) register base address > + * @reg_set_base: (Optional) set register base address > + * @reg_clr_base: (Optional) clear register base address > + * @reg_dir_in_base: (Optional) in setting register base address > + * @reg_dir_out_base: (Optional) out setting register base address > + * @reg_mask_xlate: (Optional) Translates base address and GPIO > + * offset to a register/bitmask pair. If not > + * given the default gpio_regmap_simple_xlate() > + * is used. > + * @driver_data: (Optional) driver-private data > + */ > +struct gpio_regmap { > + struct device *parent; > + struct regmap *regmap; > + struct gpio_chip gpio_chip; > + > + int reg_stride; > + int ngpio_per_reg; > + unsigned int reg_dat_base; > + unsigned int reg_set_base; > + unsigned int reg_clr_base; > + unsigned int reg_dir_in_base; > + unsigned int reg_dir_out_base; > + > + int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, > + unsigned int offset, unsigned int *reg, > + unsigned int *mask); > + > + void *driver_data; > +}; > + > /** > * struct gpio_regmap_config - Description of a generic regmap > gpio_chip. > * @parent: The parent device
Hi Michael, El 02/03/2021 a las 19:10, Michael Walle escribió: > Hi, > > Am 2021-03-02 19:06, schrieb Álvaro Fernández Rojas: >> struct gpio_regmap should be declared in gpio/regmap.h. >> This way other drivers can access the structure data when registering >> a gpio >> regmap controller. > > The intention was really to keep this private to the gpio-regmap > driver. Why do you need to access to the properties? I'm trying to add support for bcm63xx pin controllers, and Linus suggested that I could use gpio regmap instead of adding duplicated code. However, I need to access gpio_chip inside gpio_regmap to call pinctrl_add_gpio_range() with gpio_chip.base. > > -michael > >> Fixes: ebe363197e52 ("gpio: add a reusable generic gpio_chip using >> regmap") >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> drivers/gpio/gpio-regmap.c | 20 ------------------ >> include/linux/gpio/regmap.h | 41 ++++++++++++++++++++++++++++++++++++- >> 2 files changed, 40 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c >> index 5412cb3b0b2a..23b0a8572f53 100644 >> --- a/drivers/gpio/gpio-regmap.c >> +++ b/drivers/gpio/gpio-regmap.c >> @@ -11,26 +11,6 @@ >> #include <linux/module.h> >> #include <linux/regmap.h> >> >> -struct gpio_regmap { >> - struct device *parent; >> - struct regmap *regmap; >> - struct gpio_chip gpio_chip; >> - >> - int reg_stride; >> - int ngpio_per_reg; >> - unsigned int reg_dat_base; >> - unsigned int reg_set_base; >> - unsigned int reg_clr_base; >> - unsigned int reg_dir_in_base; >> - unsigned int reg_dir_out_base; >> - >> - int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, >> - unsigned int offset, unsigned int *reg, >> - unsigned int *mask); >> - >> - void *driver_data; >> -}; >> - >> static unsigned int gpio_regmap_addr(unsigned int addr) >> { >> if (addr == GPIO_REGMAP_ADDR_ZERO) >> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h >> index ad76f3d0a6ba..ce2fc6e9b62b 100644 >> --- a/include/linux/gpio/regmap.h >> +++ b/include/linux/gpio/regmap.h >> @@ -4,13 +4,52 @@ >> #define _LINUX_GPIO_REGMAP_H >> >> struct device; >> -struct gpio_regmap; >> struct irq_domain; >> struct regmap; >> >> #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1)) >> #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO) >> >> +/** >> + * struct gpio_regmap - GPIO regmap controller >> + * @parent: The parent device >> + * @regmap: The regmap used to access the registers >> + * given, the name of the device is used >> + * @gpio_chip: GPIO chip controller >> + * @ngpio_per_reg: Number of GPIOs per register >> + * @reg_stride: (Optional) May be set if the registers (of the >> + * same type, dat, set, etc) are not consecutive. >> + * @reg_dat_base: (Optional) (in) register base address >> + * @reg_set_base: (Optional) set register base address >> + * @reg_clr_base: (Optional) clear register base address >> + * @reg_dir_in_base: (Optional) in setting register base address >> + * @reg_dir_out_base: (Optional) out setting register base address >> + * @reg_mask_xlate: (Optional) Translates base address and GPIO >> + * offset to a register/bitmask pair. If not >> + * given the default gpio_regmap_simple_xlate() >> + * is used. >> + * @driver_data: (Optional) driver-private data >> + */ >> +struct gpio_regmap { >> + struct device *parent; >> + struct regmap *regmap; >> + struct gpio_chip gpio_chip; >> + >> + int reg_stride; >> + int ngpio_per_reg; >> + unsigned int reg_dat_base; >> + unsigned int reg_set_base; >> + unsigned int reg_clr_base; >> + unsigned int reg_dir_in_base; >> + unsigned int reg_dir_out_base; >> + >> + int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, >> + unsigned int offset, unsigned int *reg, >> + unsigned int *mask); >> + >> + void *driver_data; >> +}; >> + >> /** >> * struct gpio_regmap_config - Description of a generic regmap >> gpio_chip. >> * @parent: The parent device Best regards, Álvaro.
Hi, Am 2021-03-02 19:14, schrieb Álvaro Fernández Rojas: > El 02/03/2021 a las 19:10, Michael Walle escribió: >> Am 2021-03-02 19:06, schrieb Álvaro Fernández Rojas: >>> struct gpio_regmap should be declared in gpio/regmap.h. >>> This way other drivers can access the structure data when registering >>> a gpio >>> regmap controller. >> >> The intention was really to keep this private to the gpio-regmap >> driver. Why do you need to access to the properties? > > I'm trying to add support for bcm63xx pin controllers, and Linus > suggested that I could use gpio regmap instead of adding duplicated > code. nice! > However, I need to access gpio_chip inside gpio_regmap to call > pinctrl_add_gpio_range() with gpio_chip.base. Can't we add something to gpio-regmap.c which will (1) either call pinctrl_add_gpio_range(), just (2) return the struct gpio_chip* or (3) even only gpio_chip.base? I don't know how many sense (1) make and how reusable that code would be for other drivers, though. Linus? -michael
Hi Michael, El 02/03/2021 a las 20:24, Michael Walle escribió: > Hi, > > Am 2021-03-02 19:14, schrieb Álvaro Fernández Rojas: >> El 02/03/2021 a las 19:10, Michael Walle escribió: >>> Am 2021-03-02 19:06, schrieb Álvaro Fernández Rojas: >>>> struct gpio_regmap should be declared in gpio/regmap.h. >>>> This way other drivers can access the structure data when >>>> registering a gpio >>>> regmap controller. >>> >>> The intention was really to keep this private to the gpio-regmap >>> driver. Why do you need to access to the properties? >> >> I'm trying to add support for bcm63xx pin controllers, and Linus >> suggested that I could use gpio regmap instead of adding duplicated >> code. > > nice! > >> However, I need to access gpio_chip inside gpio_regmap to call >> pinctrl_add_gpio_range() with gpio_chip.base. > > Can't we add something to gpio-regmap.c which will (1) either call > pinctrl_add_gpio_range(), just (2) return the struct gpio_chip* or > (3) even only gpio_chip.base? Sure, I'm OK with any way of doing it, so it's up to you and Linus :) > > I don't know how many sense (1) make and how reusable that code would > be for other drivers, though. Linus? > > -michael Best regards, Álvaro.
On Tue, Mar 2, 2021 at 7:14 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote: > I'm trying to add support for bcm63xx pin controllers, and Linus > suggested that I could use gpio regmap instead of adding duplicated code. > However, I need to access gpio_chip inside gpio_regmap to call > pinctrl_add_gpio_range() with gpio_chip.base. Can't you just put the ranges in the device tree using the standard property gpio-ranges? These will be added automatically after the chip is added. It is documented in Documentation/devicetree/bindings/gpio/gpio.txt a bit down the file. The code is in of_gpiochip_add_pin_range() in gpiolib-of.c called from of_gpiochip_add() which is always called when gpiochip_add_data_with_key(), the main gpiochip registering function is called. This would just do the work for you with no effort in the driver. It is a bit counterintuitive that this can be done in the device tree but the hierarchical IRQs cannot do the same clever manouver to map IRQs, sorry. Yours, Linus Walleij
Hi Linus, > El 2 mar 2021, a las 23:39, Linus Walleij <linus.walleij@linaro.org> escribió: > > On Tue, Mar 2, 2021 at 7:14 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote: > >> I'm trying to add support for bcm63xx pin controllers, and Linus >> suggested that I could use gpio regmap instead of adding duplicated code. >> However, I need to access gpio_chip inside gpio_regmap to call >> pinctrl_add_gpio_range() with gpio_chip.base. > > Can't you just put the ranges in the device tree using the standard > property gpio-ranges? Ok, I’ll use that on v3 :). So I guess that I should also call gpio_direction_input() and gpio_direction_output() directly to replace gpio_chip->direction_input() and gpio_chip->direction_output() for the two drivers that need it (BCM6358 and BCM6368). > > These will be added automatically after the chip is added. > > It is documented in > Documentation/devicetree/bindings/gpio/gpio.txt > a bit down the file. Thanks for the link :) > > The code is in of_gpiochip_add_pin_range() in gpiolib-of.c > called from of_gpiochip_add() which is always called > when gpiochip_add_data_with_key(), the main gpiochip > registering function is called. > > This would just do the work for you with no effort in the driver. > > It is a bit counterintuitive that this can be done in the device > tree but the hierarchical IRQs cannot do the same clever > manouver to map IRQs, sorry. > > Yours, > Linus Walleij Best regards, Álvaro.
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 5412cb3b0b2a..23b0a8572f53 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -11,26 +11,6 @@ #include <linux/module.h> #include <linux/regmap.h> -struct gpio_regmap { - struct device *parent; - struct regmap *regmap; - struct gpio_chip gpio_chip; - - int reg_stride; - int ngpio_per_reg; - unsigned int reg_dat_base; - unsigned int reg_set_base; - unsigned int reg_clr_base; - unsigned int reg_dir_in_base; - unsigned int reg_dir_out_base; - - int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, - unsigned int offset, unsigned int *reg, - unsigned int *mask); - - void *driver_data; -}; - static unsigned int gpio_regmap_addr(unsigned int addr) { if (addr == GPIO_REGMAP_ADDR_ZERO) diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h index ad76f3d0a6ba..ce2fc6e9b62b 100644 --- a/include/linux/gpio/regmap.h +++ b/include/linux/gpio/regmap.h @@ -4,13 +4,52 @@ #define _LINUX_GPIO_REGMAP_H struct device; -struct gpio_regmap; struct irq_domain; struct regmap; #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1)) #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO) +/** + * struct gpio_regmap - GPIO regmap controller + * @parent: The parent device + * @regmap: The regmap used to access the registers + * given, the name of the device is used + * @gpio_chip: GPIO chip controller + * @ngpio_per_reg: Number of GPIOs per register + * @reg_stride: (Optional) May be set if the registers (of the + * same type, dat, set, etc) are not consecutive. + * @reg_dat_base: (Optional) (in) register base address + * @reg_set_base: (Optional) set register base address + * @reg_clr_base: (Optional) clear register base address + * @reg_dir_in_base: (Optional) in setting register base address + * @reg_dir_out_base: (Optional) out setting register base address + * @reg_mask_xlate: (Optional) Translates base address and GPIO + * offset to a register/bitmask pair. If not + * given the default gpio_regmap_simple_xlate() + * is used. + * @driver_data: (Optional) driver-private data + */ +struct gpio_regmap { + struct device *parent; + struct regmap *regmap; + struct gpio_chip gpio_chip; + + int reg_stride; + int ngpio_per_reg; + unsigned int reg_dat_base; + unsigned int reg_set_base; + unsigned int reg_clr_base; + unsigned int reg_dir_in_base; + unsigned int reg_dir_out_base; + + int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, + unsigned int offset, unsigned int *reg, + unsigned int *mask); + + void *driver_data; +}; + /** * struct gpio_regmap_config - Description of a generic regmap gpio_chip. * @parent: The parent device
struct gpio_regmap should be declared in gpio/regmap.h. This way other drivers can access the structure data when registering a gpio regmap controller. Fixes: ebe363197e52 ("gpio: add a reusable generic gpio_chip using regmap") Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- drivers/gpio/gpio-regmap.c | 20 ------------------ include/linux/gpio/regmap.h | 41 ++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 21 deletions(-)