gpio: regmap: move struct gpio_regmap definition
diff mbox series

Message ID 20210302180601.12082-1-noltari@gmail.com
State New, archived
Headers show
Series
  • gpio: regmap: move struct gpio_regmap definition
Related show

Commit Message

Álvaro Fernández Rojas March 2, 2021, 6:06 p.m. UTC
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(-)

Comments

Michael Walle March 2, 2021, 6:10 p.m. UTC | #1
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
Álvaro Fernández Rojas March 2, 2021, 6:14 p.m. UTC | #2
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.
Michael Walle March 2, 2021, 7:24 p.m. UTC | #3
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
Álvaro Fernández Rojas March 2, 2021, 7:52 p.m. UTC | #4
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.
Linus Walleij March 2, 2021, 10:39 p.m. UTC | #5
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
Álvaro Fernández Rojas March 3, 2021, 7:05 a.m. UTC | #6
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.

Patch
diff mbox series

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