Re: [PATCH v1 1/6] rockchip: gpio: rk_gpio: use ROCKCHIP_GPIOS_PER_BANK as divider

[Kever Yang]

Hi Johan,

On 2023/3/19 19:34, Johan Jonker wrote:

On 3/18/23 21:20, Simon Glass wrote:
Hi Johan,

On Thu, 16 Mar 2023 at 10:46, Johan Jonker <jbx6...@gmail.com> wrote:
The current divider to calculate the bank ID can change.
Use a constant ROCKCHIP_GPIOS_PER_BANK as fixed divider.
What is the motivation for this patch?
The gpio-ranges property format:

gpio-ranges = <[pin controller phandle], [GPIO controller offset],
                 [pin controller offset], [number of pins]>;

1: Given the Rockchip TRM not all gpio-banks have 32 pins per bank.

Could you share which TRM did you find gpio-banks is not 32 pins?

The design should be 32 pins per bank for all the SoCs, although some 
banks may not have full 32 pins,

but all the pin ID should follow the rules with 32pin per bank.


Thanks,

- Kever

2: The "gpio-ranges" syntax allows multiple items with variable number of pins.

===

Theoretical example:

gpio-ranges = <&pinctrl 0 32 32>; // 32/32 => bank 1

vs.

gpio-ranges = <&pinctrl 16 48 16>, // 48/16 => bank 3 vs. 48/32 => bank 1
               <&pinctrl 0 32 16>; // 32/16  => bank 2 vs. 32/32 => bank 1

Both descriptions are valid.
The number of pins in the second example is reduced to 16 per item.
Using that as divider will give a wrong bank number.
Use a constant instead.
For the Rockchip situation simple parsing the first item is enough the know 
it's bank number for now.
To do it correct one could parse a list of gpio-range items, but that makes it 
more complicated.

>From gpio.txt:
Each offset runs from 0 to N. It is perfectly fine to pile any number of
ranges with just one pin-to-GPIO line mapping if the ranges are concocted, but
in practice these ranges are often lumped in discrete sets.

Signed-off-by: Johan Jonker <jbx6...@gmail.com>
---
  drivers/gpio/rk_gpio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass <s...@chromium.org>

diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
index f7ad4d68..0a2acf18 100644
--- a/drivers/gpio/rk_gpio.c
+++ b/drivers/gpio/rk_gpio.c
@@ -160,7 +160,7 @@ static int rockchip_gpio_probe(struct udevice *dev)
                                              0, &args);
         if (!ret || ret != -ENOENT) {
                 uc_priv->gpio_count = args.args[2];
-               priv->bank = args.args[1] / args.args[2];
+               priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;
         } else {
                 uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;
                 end = strrchr(dev->name, '@');
--
2.20.1

Regards,
SImon