[PATCH 0/4] gpio: sunxi: Handle pin configuration flags
This series updates the sunxi GPIO driver to handle pin pull-up/down, so consumer drivers do not need to call the non-DM sunxi_gpio_set_* functions. As an example, the last patch updates the MMC driver to use this functionality. The helpers added here will also be used for the upcoming DM_PINCTRL driver.
Samuel Holland (4): sunxi: gpio: Return void from setter functions sunxi: gpio: Add per-bank drive and pull setters gpio: sunxi: Implement .set_flags mmc: sunxi: Use DM_GPIO flags to set pull-up
arch/arm/include/asm/arch-sunxi/gpio.h | 6 ++- arch/arm/mach-sunxi/pinmux.c | 28 +++++++----- drivers/gpio/sunxi_gpio.c | 62 +++++++++++--------------- drivers/mmc/sunxi_mmc.c | 8 +--- 4 files changed, 51 insertions(+), 53 deletions(-)
The return values of these functions are always zero, and they are never checked. Since they are not needed, remove them.
Signed-off-by: Samuel Holland samuel@sholland.org ---
arch/arm/include/asm/arch-sunxi/gpio.h | 4 ++-- arch/arm/mach-sunxi/pinmux.c | 8 ++------ 2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h index f3ab1aea0e..2b72b2263b 100644 --- a/arch/arm/include/asm/arch-sunxi/gpio.h +++ b/arch/arm/include/asm/arch-sunxi/gpio.h @@ -226,8 +226,8 @@ void sunxi_gpio_set_cfgbank(struct sunxi_gpio *pio, int bank_offset, u32 val); void sunxi_gpio_set_cfgpin(u32 pin, u32 val); int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset); int sunxi_gpio_get_cfgpin(u32 pin); -int sunxi_gpio_set_drv(u32 pin, u32 val); -int sunxi_gpio_set_pull(u32 pin, u32 val); +void sunxi_gpio_set_drv(u32 pin, u32 val); +void sunxi_gpio_set_pull(u32 pin, u32 val); int sunxi_name_to_gpio(const char *name);
#if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c index 642483f06c..cf9d9daf7c 100644 --- a/arch/arm/mach-sunxi/pinmux.c +++ b/arch/arm/mach-sunxi/pinmux.c @@ -45,7 +45,7 @@ int sunxi_gpio_get_cfgpin(u32 pin) return sunxi_gpio_get_cfgbank(pio, pin); }
-int sunxi_gpio_set_drv(u32 pin, u32 val) +void sunxi_gpio_set_drv(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin); u32 index = GPIO_DRV_INDEX(pin); @@ -53,11 +53,9 @@ int sunxi_gpio_set_drv(u32 pin, u32 val) struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset); - - return 0; }
-int sunxi_gpio_set_pull(u32 pin, u32 val) +void sunxi_gpio_set_pull(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin); u32 index = GPIO_PULL_INDEX(pin); @@ -65,6 +63,4 @@ int sunxi_gpio_set_pull(u32 pin, u32 val) struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset); - - return 0; }
On 10/21/21 06:52, Samuel Holland wrote:
The return values of these functions are always zero, and they are never checked. Since they are not needed, remove them.
Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
The GPIO and pinctrl drivers need these setters for pin configuration. Since they are DM drivers, they should not be using hardcoded base addresses. Factor out variants of the setter functions which take a pointer to the GPIO bank's MMIO registers.
Signed-off-by: Samuel Holland samuel@sholland.org ---
arch/arm/include/asm/arch-sunxi/gpio.h | 2 ++ arch/arm/mach-sunxi/pinmux.c | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h index 2b72b2263b..106605adf5 100644 --- a/arch/arm/include/asm/arch-sunxi/gpio.h +++ b/arch/arm/include/asm/arch-sunxi/gpio.h @@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val); int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset); int sunxi_gpio_get_cfgpin(u32 pin); void sunxi_gpio_set_drv(u32 pin, u32 val); +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val); void sunxi_gpio_set_pull(u32 pin, u32 val); +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val); int sunxi_name_to_gpio(const char *name);
#if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c index cf9d9daf7c..b2093b623a 100644 --- a/arch/arm/mach-sunxi/pinmux.c +++ b/arch/arm/mach-sunxi/pinmux.c @@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin) void sunxi_gpio_set_drv(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin); - u32 index = GPIO_DRV_INDEX(pin); - u32 offset = GPIO_DRV_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
+ sunxi_gpio_set_drv_bank(pio, pin, val); +} + +void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val) +{ + u32 index = GPIO_DRV_INDEX(bank_offset); + u32 offset = GPIO_DRV_OFFSET(bank_offset); + clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset); }
void sunxi_gpio_set_pull(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin); - u32 index = GPIO_PULL_INDEX(pin); - u32 offset = GPIO_PULL_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
+ sunxi_gpio_set_pull_bank(pio, pin, val); +} + +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val) +{ + u32 index = GPIO_PULL_INDEX(bank_offset); + u32 offset = GPIO_PULL_OFFSET(bank_offset); + clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset); }
On 10/21/21 06:52, Samuel Holland wrote:
The GPIO and pinctrl drivers need these setters for pin configuration. Since they are DM drivers, they should not be using hardcoded base addresses. Factor out variants of the setter functions which take a pointer to the GPIO bank's MMIO registers.
Signed-off-by: Samuel Holland samuel@sholland.org
arch/arm/include/asm/arch-sunxi/gpio.h | 2 ++ arch/arm/mach-sunxi/pinmux.c | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h index 2b72b2263b..106605adf5 100644 --- a/arch/arm/include/asm/arch-sunxi/gpio.h +++ b/arch/arm/include/asm/arch-sunxi/gpio.h @@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val); int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset); int sunxi_gpio_get_cfgpin(u32 pin); void sunxi_gpio_set_drv(u32 pin, u32 val);
Please, add Sphinx style documentation for the new functions, preferably in the header file. Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val); void sunxi_gpio_set_pull(u32 pin, u32 val); +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val); int sunxi_name_to_gpio(const char *name);
#if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c index cf9d9daf7c..b2093b623a 100644 --- a/arch/arm/mach-sunxi/pinmux.c +++ b/arch/arm/mach-sunxi/pinmux.c @@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin) void sunxi_gpio_set_drv(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin);
- u32 index = GPIO_DRV_INDEX(pin);
- u32 offset = GPIO_DRV_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
- sunxi_gpio_set_drv_bank(pio, pin, val);
+}
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val) +{
u32 index = GPIO_DRV_INDEX(bank_offset);
u32 offset = GPIO_DRV_OFFSET(bank_offset);
clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset); }
void sunxi_gpio_set_pull(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin);
- u32 index = GPIO_PULL_INDEX(pin);
- u32 offset = GPIO_PULL_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
- sunxi_gpio_set_pull_bank(pio, pin, val);
+}
+void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val) +{
- u32 index = GPIO_PULL_INDEX(bank_offset);
- u32 offset = GPIO_PULL_OFFSET(bank_offset);
- clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);
Please, simplify this: %s/&pio->pull[0] + index/&pio->pull[index]/
Otherwise the change looks correct to me.
Best regards
Heinrich
}
On Fri, 5 Nov 2021 15:12:16 +0100 Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
On 10/21/21 06:52, Samuel Holland wrote:
The GPIO and pinctrl drivers need these setters for pin configuration. Since they are DM drivers, they should not be using hardcoded base addresses. Factor out variants of the setter functions which take a pointer to the GPIO bank's MMIO registers.
Signed-off-by: Samuel Holland samuel@sholland.org
arch/arm/include/asm/arch-sunxi/gpio.h | 2 ++ arch/arm/mach-sunxi/pinmux.c | 20 ++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h index 2b72b2263b..106605adf5 100644 --- a/arch/arm/include/asm/arch-sunxi/gpio.h +++ b/arch/arm/include/asm/arch-sunxi/gpio.h @@ -227,7 +227,9 @@ void sunxi_gpio_set_cfgpin(u32 pin, u32 val); int sunxi_gpio_get_cfgbank(struct sunxi_gpio *pio, int bank_offset); int sunxi_gpio_get_cfgpin(u32 pin); void sunxi_gpio_set_drv(u32 pin, u32 val);
Please, add Sphinx style documentation for the new functions, preferably in the header file. Cf. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...
I don't think this is necessary or useful, really. Those function are not really an advertised interface, more something we use internally, mostly for our SPL.
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val); void sunxi_gpio_set_pull(u32 pin, u32 val); +void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val); int sunxi_name_to_gpio(const char *name);
#if !defined CONFIG_SPL_BUILD && defined CONFIG_AXP_GPIO diff --git a/arch/arm/mach-sunxi/pinmux.c b/arch/arm/mach-sunxi/pinmux.c index cf9d9daf7c..b2093b623a 100644 --- a/arch/arm/mach-sunxi/pinmux.c +++ b/arch/arm/mach-sunxi/pinmux.c @@ -48,19 +48,31 @@ int sunxi_gpio_get_cfgpin(u32 pin) void sunxi_gpio_set_drv(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin);
- u32 index = GPIO_DRV_INDEX(pin);
- u32 offset = GPIO_DRV_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
- sunxi_gpio_set_drv_bank(pio, pin, val);
+}
+void sunxi_gpio_set_drv_bank(struct sunxi_gpio *pio, u32 bank_offset, u32 val) +{
u32 index = GPIO_DRV_INDEX(bank_offset);
u32 offset = GPIO_DRV_OFFSET(bank_offset);
clrsetbits_le32(&pio->drv[0] + index, 0x3 << offset, val << offset); }
void sunxi_gpio_set_pull(u32 pin, u32 val) { u32 bank = GPIO_BANK(pin);
- u32 index = GPIO_PULL_INDEX(pin);
- u32 offset = GPIO_PULL_OFFSET(pin); struct sunxi_gpio *pio = BANK_TO_GPIO(bank);
- sunxi_gpio_set_pull_bank(pio, pin, val);
+}
+void sunxi_gpio_set_pull_bank(struct sunxi_gpio *pio, int bank_offset, u32 val) +{
- u32 index = GPIO_PULL_INDEX(bank_offset);
- u32 offset = GPIO_PULL_OFFSET(bank_offset);
- clrsetbits_le32(&pio->pull[0] + index, 0x3 << offset, val << offset);
Please, simplify this: %s/&pio->pull[0] + index/&pio->pull[index]/
Fixing this up locally.
Cheers, Andre
Otherwise the change looks correct to me.
Best regards
Heinrich
}
This, along with gpio_flags_xlate(), allows the GPIO driver to handle pull-up/down flags provided by consumer drivers or in the device tree.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c index cdbc40d48f..92fee0118d 100644 --- a/drivers/gpio/sunxi_gpio.c +++ b/drivers/gpio/sunxi_gpio.c @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name) return ret ? ret : gpio; }
-static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - - sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT); - - return 0; -} - -static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset); - - sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); - clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0); - - return 0; -} - static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) return dat & 0x1; }
-static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset); - - clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0); - return 0; -} - static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, if (ret) return ret; desc->offset = args->args[1]; - desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; + desc->flags = gpio_flags_xlate(args->args[2]); + + return 0; +} + +static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset, + ulong flags) +{ + struct sunxi_gpio_plat *plat = dev_get_plat(dev); + + if (flags & GPIOD_IS_OUT) { + u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE); + u32 num = GPIO_NUM(offset); + + clrsetbits_le32(&plat->regs->dat, 1 << num, value << num); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); + } else if (flags & GPIOD_IS_IN) { + u32 pull = 0; + + if (flags & GPIOD_PULL_UP) + pull = 1; + else if (flags & GPIOD_PULL_DOWN) + pull = 2; + sunxi_gpio_set_pull_bank(plat->regs, offset, pull); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT); + }
return 0; }
static const struct dm_gpio_ops gpio_sunxi_ops = { - .direction_input = sunxi_gpio_direction_input, - .direction_output = sunxi_gpio_direction_output, .get_value = sunxi_gpio_get_value, - .set_value = sunxi_gpio_set_value, .get_function = sunxi_gpio_get_function, .xlate = sunxi_gpio_xlate, + .set_flags = sunxi_gpio_set_flags, };
/**
On Wed, 20 Oct 2021 at 22:53, Samuel Holland samuel@sholland.org wrote:
This, along with gpio_flags_xlate(), allows the GPIO driver to handle pull-up/down flags provided by consumer drivers or in the device tree.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On 10/21/21 06:52, Samuel Holland wrote:
This, along with gpio_flags_xlate(), allows the GPIO driver to handle pull-up/down flags provided by consumer drivers or in the device tree.
Signed-off-by: Samuel Holland samuel@sholland.org Reviewed-by: Simon Glass sjg@chromium.org
drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c index cdbc40d48f..92fee0118d 100644 --- a/drivers/gpio/sunxi_gpio.c +++ b/drivers/gpio/sunxi_gpio.c @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name) return ret ? ret : gpio; }
-static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset) -{
- struct sunxi_gpio_plat *plat = dev_get_plat(dev);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
- return 0;
-}
-static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset,
int value)-{
- struct sunxi_gpio_plat *plat = dev_get_plat(dev);
- u32 num = GPIO_NUM(offset);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);
- clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
- return 0;
-}
- static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev);
@@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) return dat & 0x1; }
-static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset,
int value)-{
- struct sunxi_gpio_plat *plat = dev_get_plat(dev);
- u32 num = GPIO_NUM(offset);
- clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
- return 0;
-}
- static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev);
@@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, if (ret) return ret; desc->offset = args->args[1];
- desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0;
- desc->flags = gpio_flags_xlate(args->args[2]);
- return 0;
+}
+static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset,
ulong flags)+{
struct sunxi_gpio_plat *plat = dev_get_plat(dev);
if (flags & GPIOD_IS_OUT) {
u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE);u32 num = GPIO_NUM(offset);clrsetbits_le32(&plat->regs->dat, 1 << num, value << num);sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT);} else if (flags & GPIOD_IS_IN) {
u32 pull = 0;if (flags & GPIOD_PULL_UP)pull = 1;else if (flags & GPIOD_PULL_DOWN)pull = 2;sunxi_gpio_set_pull_bank(plat->regs, offset, pull);sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);}
return 0; }
static const struct dm_gpio_ops gpio_sunxi_ops = {
- .direction_input = sunxi_gpio_direction_input,
- .direction_output = sunxi_gpio_direction_output,
Removing these operations is not related to your commit message.
dm_set_gpio_value() seems to be using them.
.get_value = sunxi_gpio_get_value,
- .set_value = sunxi_gpio_set_value,
Same here.
Best regards
Heinrich
.get_function = sunxi_gpio_get_function, .xlate = sunxi_gpio_xlate,
.set_flags = sunxi_gpio_set_flags, };
/**
On 11/5/21 9:43 AM, Heinrich Schuchardt wrote:
On 10/21/21 06:52, Samuel Holland wrote:
This, along with gpio_flags_xlate(), allows the GPIO driver to handle pull-up/down flags provided by consumer drivers or in the device tree.
Signed-off-by: Samuel Holland samuel@sholland.org Reviewed-by: Simon Glass sjg@chromium.org
drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c index cdbc40d48f..92fee0118d 100644 --- a/drivers/gpio/sunxi_gpio.c +++ b/drivers/gpio/sunxi_gpio.c @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name) return ret ? ret : gpio; } -static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
- return 0; -}
-static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); - clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
- return 0; -}
static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) return dat & 0x1; } -static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset);
- clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0); - return 0; -}
static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, if (ret) return ret; desc->offset = args->args[1]; - desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; + desc->flags = gpio_flags_xlate(args->args[2]);
+ return 0; +}
+static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset, + ulong flags) +{ + struct sunxi_gpio_plat *plat = dev_get_plat(dev);
+ if (flags & GPIOD_IS_OUT) { + u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE); + u32 num = GPIO_NUM(offset);
+ clrsetbits_le32(&plat->regs->dat, 1 << num, value << num); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); + } else if (flags & GPIOD_IS_IN) { + u32 pull = 0;
+ if (flags & GPIOD_PULL_UP) + pull = 1; + else if (flags & GPIOD_PULL_DOWN) + pull = 2; + sunxi_gpio_set_pull_bank(plat->regs, offset, pull); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT); + } return 0; } static const struct dm_gpio_ops gpio_sunxi_ops = { - .direction_input = sunxi_gpio_direction_input, - .direction_output = sunxi_gpio_direction_output,
Removing these operations is not related to your commit message.
dm_gpio_set_value() seems to be using them.
It does not use any of these operations when set_flags is provided. The same applies to _dm_gpio_set_flags().
asm-generic/gpio.h says about set_flags:
This method is required and should be implemented by new drivers. At some point, it will supersede direction_input() and direction_output(), which wil be removed.
So I believe it is intended to replace the other three operations.
Regards, Samuel
.get_value = sunxi_gpio_get_value, - .set_value = sunxi_gpio_set_value,
Same here.
Best regards
Heinrich
.get_function = sunxi_gpio_get_function, .xlate = sunxi_gpio_xlate, + .set_flags = sunxi_gpio_set_flags, }; /**
On Fri, 5 Nov 2021 16:46:07 -0500 Samuel Holland samuel@sholland.org wrote:
On 11/5/21 9:43 AM, Heinrich Schuchardt wrote:
On 10/21/21 06:52, Samuel Holland wrote:
This, along with gpio_flags_xlate(), allows the GPIO driver to handle pull-up/down flags provided by consumer drivers or in the device tree.
Signed-off-by: Samuel Holland samuel@sholland.org Reviewed-by: Simon Glass sjg@chromium.org
drivers/gpio/sunxi_gpio.c | 62 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/sunxi_gpio.c b/drivers/gpio/sunxi_gpio.c index cdbc40d48f..92fee0118d 100644 --- a/drivers/gpio/sunxi_gpio.c +++ b/drivers/gpio/sunxi_gpio.c @@ -139,27 +139,6 @@ int sunxi_name_to_gpio(const char *name) return ret ? ret : gpio; } -static int sunxi_gpio_direction_input(struct udevice *dev, unsigned offset) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT);
- return 0; -}
-static int sunxi_gpio_direction_output(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset);
- sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); - clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0);
- return 0; -}
static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -172,16 +151,6 @@ static int sunxi_gpio_get_value(struct udevice *dev, unsigned offset) return dat & 0x1; } -static int sunxi_gpio_set_value(struct udevice *dev, unsigned offset, - int value) -{ - struct sunxi_gpio_plat *plat = dev_get_plat(dev); - u32 num = GPIO_NUM(offset);
- clrsetbits_le32(&plat->regs->dat, 1 << num, value ? (1 << num) : 0); - return 0; -}
static int sunxi_gpio_get_function(struct udevice *dev, unsigned offset) { struct sunxi_gpio_plat *plat = dev_get_plat(dev); @@ -205,18 +174,41 @@ static int sunxi_gpio_xlate(struct udevice *dev, struct gpio_desc *desc, if (ret) return ret; desc->offset = args->args[1]; - desc->flags = args->args[2] & GPIO_ACTIVE_LOW ? GPIOD_ACTIVE_LOW : 0; + desc->flags = gpio_flags_xlate(args->args[2]);
+ return 0; +}
+static int sunxi_gpio_set_flags(struct udevice *dev, unsigned int offset, + ulong flags) +{ + struct sunxi_gpio_plat *plat = dev_get_plat(dev);
+ if (flags & GPIOD_IS_OUT) { + u32 value = !!(flags & GPIOD_IS_OUT_ACTIVE); + u32 num = GPIO_NUM(offset);
+ clrsetbits_le32(&plat->regs->dat, 1 << num, value << num); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_OUTPUT); + } else if (flags & GPIOD_IS_IN) { + u32 pull = 0;
+ if (flags & GPIOD_PULL_UP) + pull = 1; + else if (flags & GPIOD_PULL_DOWN) + pull = 2; + sunxi_gpio_set_pull_bank(plat->regs, offset, pull); + sunxi_gpio_set_cfgbank(plat->regs, offset, SUNXI_GPIO_INPUT); + } return 0; } static const struct dm_gpio_ops gpio_sunxi_ops = { - .direction_input = sunxi_gpio_direction_input, - .direction_output = sunxi_gpio_direction_output,
Removing these operations is not related to your commit message.
dm_gpio_set_value() seems to be using them.
It does not use any of these operations when set_flags is provided. The same applies to _dm_gpio_set_flags().
asm-generic/gpio.h says about set_flags:
This method is required and should be implemented by new drivers. At some point, it will supersede direction_input() and direction_output(), which wil be removed.
So I believe it is intended to replace the other three operations.
That's what I get from the code and this header file comment as well.
Reviewed-by: Andre Przywara andre.przywara@arm.com
Cheers, Andre
Regards, Samuel
.get_value = sunxi_gpio_get_value, - .set_value = sunxi_gpio_set_value,
Same here.
Best regards
Heinrich
.get_function = sunxi_gpio_get_function, .xlate = sunxi_gpio_xlate, + .set_flags = sunxi_gpio_set_flags, }; /**
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org ---
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */ - if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, - GPIOD_IS_IN)) { - int cd_pin = gpio_get_number(&priv->cd_gpio); - - sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP); - } + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, + GPIOD_IS_IN | GPIOD_PULL_UP);
upriv->mmc = &plat->mmc;
Hi,
On 10/21/21 1:52 PM, Samuel Holland wrote:
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */
- if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN)) {int cd_pin = gpio_get_number(&priv->cd_gpio);sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);- }
- gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN | GPIOD_PULL_UP);
Is it right to set the pull-up?
Best Regards, Jaehoon Chung
upriv->mmc = &plat->mmc;
On Fri, 22 Oct 2021 06:58:48 +0900 Jaehoon Chung jh80.chung@samsung.com wrote:
Hi Jaehoon,
thanks for having a look!
Hi,
On 10/21/21 1:52 PM, Samuel Holland wrote:
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */
- if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN)) {int cd_pin = gpio_get_number(&priv->cd_gpio);sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);- }
- gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN | GPIOD_PULL_UP);Is it right to set the pull-up?
You mean, unconditionally? I mean it's just copying the current code, which does that (see the "minus" lines just above).
But I think you have a point: I don't see any pull up specified in any DT, and I think most (if not all) boards have a discrete pull up resistor on that line.
But I don't dare to touch that code - at least for this series, as it works (TM) right now. After the full DM_PINCTRL series this might be another story, though.
Cheers, Andre
Best Regards, Jaehoon Chung
upriv->mmc = &plat->mmc;
Hi Andre,
On 10/22/21 6:00 PM, Andre Przywara wrote:
On Fri, 22 Oct 2021 06:58:48 +0900 Jaehoon Chung jh80.chung@samsung.com wrote:
Hi Jaehoon,
thanks for having a look!
Hi,
On 10/21/21 1:52 PM, Samuel Holland wrote:
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */
- if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN)) {int cd_pin = gpio_get_number(&priv->cd_gpio);sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);- }
- gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN | GPIOD_PULL_UP);Is it right to set the pull-up?
You mean, unconditionally? I mean it's just copying the current code, which does that (see the "minus" lines just above).
Yes, it looks like something strange. AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W. But it's not different with original behavior, as you mentioned.
But I think you have a point: I don't see any pull up specified in any DT, and I think most (if not all) boards have a discrete pull up resistor on that line.
But I don't dare to touch that code - at least for this series, as it works (TM) right now. After the full DM_PINCTRL series this might be another story, though.
Right. I understood exactly. Thanks for explanation.
P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)?
Best Regards, Jaehoon Chung
Cheers, Andre
Best Regards, Jaehoon Chung
upriv->mmc = &plat->mmc;
On Fri, 22 Oct 2021 19:10:20 +0900 Jaehoon Chung jh80.chung@samsung.com wrote:
Hi Andre,
On 10/22/21 6:00 PM, Andre Przywara wrote:
On Fri, 22 Oct 2021 06:58:48 +0900 Jaehoon Chung jh80.chung@samsung.com wrote:
Hi Jaehoon,
thanks for having a look!
Hi,
On 10/21/21 1:52 PM, Samuel Holland wrote:
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */
- if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN)) {int cd_pin = gpio_get_number(&priv->cd_gpio);sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);- }
- gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN | GPIOD_PULL_UP);Is it right to set the pull-up?
You mean, unconditionally? I mean it's just copying the current code, which does that (see the "minus" lines just above).
Yes, it looks like something strange. AFAIK, It can be changed that cd-gpios has dependent how to consist of H/W. But it's not different with original behavior, as you mentioned.
But I think you have a point: I don't see any pull up specified in any DT, and I think most (if not all) boards have a discrete pull up resistor on that line.
But I don't dare to touch that code - at least for this series, as it works (TM) right now. After the full DM_PINCTRL series this might be another story, though.
Right. I understood exactly. Thanks for explanation.
P.S: Can I test sunxi patch with NeoPlus2 board(Allwinner H5)?
Yes, please, any testing on any kind of Allwinner board is warmly welcomed. There are 160 supported Allwinner boards in the tree, and even though I have probably more of them than I want, it's only a fraction of them. And experience show that some boards are subtly broken. Also feel free to report any other broken things you see when testing!
Thanks, Andre
Best Regards, Jaehoon Chung
Cheers, Andre
Best Regards, Jaehoon Chung
upriv->mmc = &plat->mmc;
On 10/21/21 06:52, Samuel Holland wrote:
Now that the sunxi_gpio driver handles pull-up/down via the driver model, pin configuration does not need a platform-specific function.
Signed-off-by: Samuel Holland samuel@sholland.org
I tested on an OrangePi PC (orangepi_pc_defconfig). The 'mmc rescan' command detects correctly if an SD-card is present with this series applied.
Tested-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
drivers/mmc/sunxi_mmc.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c index c170c16d5a..955b29826f 100644 --- a/drivers/mmc/sunxi_mmc.c +++ b/drivers/mmc/sunxi_mmc.c @@ -700,12 +700,8 @@ static int sunxi_mmc_probe(struct udevice *dev) return ret;
/* This GPIO is optional */
- if (!gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN)) {int cd_pin = gpio_get_number(&priv->cd_gpio);sunxi_gpio_set_pull(cd_pin, SUNXI_GPIO_PULL_UP);- }
gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio,
GPIOD_IS_IN | GPIOD_PULL_UP);upriv->mmc = &plat->mmc;
participants (5)
-
Andre Przywara -
Heinrich Schuchardt -
Jaehoon Chung -
Samuel Holland -
Simon Glass