| Message ID | 7993a30fbc2e50a2d228fa0c8fad643c4034b101.1506428208.git-series.quentin.schulz@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: > Instead of using a function to retrieve each pin's correct control > register, use drv_data within pinctrl_pin_desc to store the ctrl reg. > > Remove axp20x_gpio_get_reg and replace every occurrence by a get from > drv_data. Why do you need to do that? This should be explained. Maxime
Hi Maxime, On 26/09/2017 15:01, Maxime Ripard wrote: > On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: >> Instead of using a function to retrieve each pin's correct control >> register, use drv_data within pinctrl_pin_desc to store the ctrl reg. >> >> Remove axp20x_gpio_get_reg and replace every occurrence by a get from >> drv_data. > > Why do you need to do that? This should be explained. > Agreed that it misses an explanation. Today, to get a register addr of one of the GPIOs in the PMIC, we basically get the GPIO number and returns the register via this info. There are 3 GPIOs in AXP209, 2 in AXP813. I didn't want to have a switch case for the GPIO number and then an if/else inside one of the case to check if the device is AXP209 or AXP813 in which case we return -EINVAL instead of the GPIO2 reg. With support for new PMIC, we would have a bunch of if conditions and complexify the process for something really simple. IMHO, this also allows easier integration of future PMICs which might have different regs for the GPIOs. I don't *need* it but I find this solution nicer. Thanks, Quentin
On Tue, Sep 26, 2017 at 01:17:05PM +0000, Quentin Schulz wrote: > Hi Maxime, > > On 26/09/2017 15:01, Maxime Ripard wrote: > > On Tue, Sep 26, 2017 at 12:17:13PM +0000, Quentin Schulz wrote: > >> Instead of using a function to retrieve each pin's correct control > >> register, use drv_data within pinctrl_pin_desc to store the ctrl reg. > >> > >> Remove axp20x_gpio_get_reg and replace every occurrence by a get from > >> drv_data. > > > > Why do you need to do that? This should be explained. > > > > Agreed that it misses an explanation. > > Today, to get a register addr of one of the GPIOs in the PMIC, we > basically get the GPIO number and returns the register via this info. > > There are 3 GPIOs in AXP209, 2 in AXP813. I didn't want to have a switch > case for the GPIO number and then an if/else inside one of the case to > check if the device is AXP209 or AXP813 in which case we return -EINVAL > instead of the GPIO2 reg. With support for new PMIC, we would have a > bunch of if conditions and complexify the process for something really > simple. I'm not sure how that relates to your code actually. The only thing that patch is doing is to move the register offset from a function to the structure associated to the pin. However, even in the AXP813 case, you're using exactly the same values, so that's not really needed. Now, you also mentionned the pin number. While this patch doesn't really address it, it's also no really needed. The number of pins is already known and registered in the GPIO framework. If the framework doesn't already do it (which would be surprising), you can just check that the pin number passed is not going to be higher than the one you registered. > IMHO, this also allows easier integration of future PMICs which might > have different regs for the GPIOs. Let's worry about future PMICs in the future. Maxime
Hi Quentin,
[auto build test WARNING on ]
url: https://github.com/0day-ci/linux/commits/Quentin-Schulz/add-pinmuxing-support-for-pins-in-AXP209-and-AXP813-PMICs/20170929-162846
base:
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All warnings (new ones prefixed by >>):
drivers//pinctrl/pinctrl-axp209.c: In function 'axp20x_gpio_get_direction':
>> drivers//pinctrl/pinctrl-axp209.c:136:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
int reg = (int)gpio->desc->pins[offset].pin.drv_data;
^
drivers//pinctrl/pinctrl-axp209.c: In function 'axp20x_gpio_set':
drivers//pinctrl/pinctrl-axp209.c:171:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
int reg = (int)gpio->desc->pins[offset].pin.drv_data;
^
drivers//pinctrl/pinctrl-axp209.c: In function 'axp20x_pmx_set':
drivers//pinctrl/pinctrl-axp209.c:183:12: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
int reg = (int)gpio->desc->pins[offset].pin.drv_data;
^
vim +136 drivers//pinctrl/pinctrl-axp209.c
132
133 static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
134 {
135 struct axp20x_gpio *gpio = gpiochip_get_data(chip);
> 136 int reg = (int)gpio->desc->pins[offset].pin.drv_data;
137 unsigned int val;
138 int ret;
139
140 ret = regmap_read(gpio->regmap, reg, &val);
141 if (ret)
142 return ret;
143
144 /*
145 * This shouldn't really happen if the pin is in use already,
146 * or if it's not in use yet, it doesn't matter since we're
147 * going to change the value soon anyway. Default to output.
148 */
149 if ((val & AXP20X_GPIO_FUNCTIONS) > 2)
150 return 0;
151
152 /*
153 * The GPIO directions are the three lowest values.
154 * 2 is input, 0 and 1 are output
155 */
156 return val & 2;
157 }
158
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/pinctrl/pinctrl-axp209.c b/drivers/pinctrl/pinctrl-axp209.c index b35e8dd..4bbcba2 100644 --- a/drivers/pinctrl/pinctrl-axp209.c +++ b/drivers/pinctrl/pinctrl-axp209.c @@ -32,10 +32,11 @@ #define AXP20X_GPIO_FUNCTION_OUT_HIGH 1 #define AXP20X_GPIO_FUNCTION_INPUT 2 -#define AXP20X_PINCTRL_PIN(_pin_num, _pin) \ +#define AXP20X_PINCTRL_PIN(_pin_num, _pin, _regs) \ { \ .number = _pin_num, \ .name = _pin, \ + .drv_data = _regs, \ } #define AXP20X_PIN(_pin, ...) \ @@ -91,17 +92,17 @@ struct axp20x_gpio { }; static const struct axp20x_desc_pin axp209_pins[] = { - AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0"), + AXP20X_PIN(AXP20X_PINCTRL_PIN(0, "GPIO0", (void *)AXP20X_GPIO0_CTRL), AXP20X_FUNCTION(0x0, "gpio_out"), AXP20X_FUNCTION(0x2, "gpio_in"), AXP20X_FUNCTION(0x3, "ldo"), AXP20X_FUNCTION(0x4, "adc")), - AXP20X_PIN(AXP20X_PINCTRL_PIN(1, "GPIO1"), + AXP20X_PIN(AXP20X_PINCTRL_PIN(1, "GPIO1", (void *)AXP20X_GPIO1_CTRL), AXP20X_FUNCTION(0x0, "gpio_out"), AXP20X_FUNCTION(0x2, "gpio_in"), AXP20X_FUNCTION(0x3, "ldo"), AXP20X_FUNCTION(0x4, "adc")), - AXP20X_PIN(AXP20X_PINCTRL_PIN(2, "GPIO2"), + AXP20X_PIN(AXP20X_PINCTRL_PIN(2, "GPIO2", (void *)AXP20X_GPIO2_CTRL), AXP20X_FUNCTION(0x0, "gpio_out"), AXP20X_FUNCTION(0x2, "gpio_in")), }; @@ -111,20 +112,6 @@ static const struct axp20x_pinctrl_desc axp20x_pinctrl_data = { .npins = ARRAY_SIZE(axp209_pins), }; -static int axp20x_gpio_get_reg(unsigned offset) -{ - switch (offset) { - case 0: - return AXP20X_GPIO0_CTRL; - case 1: - return AXP20X_GPIO1_CTRL; - case 2: - return AXP20X_GPIO2_CTRL; - } - - return -EINVAL; -} - static int axp20x_gpio_input(struct gpio_chip *chip, unsigned offset) { return pinctrl_gpio_direction_input(chip->base + offset); @@ -146,12 +133,9 @@ static int axp20x_gpio_get(struct gpio_chip *chip, unsigned offset) static int axp20x_gpio_get_direction(struct gpio_chip *chip, unsigned offset) { struct axp20x_gpio *gpio = gpiochip_get_data(chip); + int reg = (int)gpio->desc->pins[offset].pin.drv_data; unsigned int val; - int reg, ret; - - reg = axp20x_gpio_get_reg(offset); - if (reg < 0) - return reg; + int ret; ret = regmap_read(gpio->regmap, reg, &val); if (ret) @@ -184,11 +168,7 @@ static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset, int value) { struct axp20x_gpio *gpio = gpiochip_get_data(chip); - int reg; - - reg = axp20x_gpio_get_reg(offset); - if (reg < 0) - return; + int reg = (int)gpio->desc->pins[offset].pin.drv_data; regmap_update_bits(gpio->regmap, reg, AXP20X_GPIO_FUNCTIONS, @@ -200,11 +180,7 @@ static int axp20x_pmx_set(struct pinctrl_dev *pctldev, unsigned int offset, u8 config) { struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev); - int reg; - - reg = axp20x_gpio_get_reg(offset); - if (reg < 0) - return reg; + int reg = (int)gpio->desc->pins[offset].pin.drv_data; return regmap_update_bits(gpio->regmap, reg, AXP20X_GPIO_FUNCTIONS, config);
Instead of using a function to retrieve each pin's correct control register, use drv_data within pinctrl_pin_desc to store the ctrl reg. Remove axp20x_gpio_get_reg and replace every occurrence by a get from drv_data. Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> --- drivers/pinctrl/pinctrl-axp209.c | 42 +++++++-------------------------- 1 file changed, 9 insertions(+), 33 deletions(-)