Re: [PATCH v2 02/10] pinctrl: rockchip: Add a pin_to_bank() helper

[Quentin Schulz]

Hi Jonas,

On 8/3/24 12:56 AM, Jonas Karlman wrote:
Add a pin_to_bank() helper that can locate a pin bank based on the pin
offset, to be used in get_gpio_mux() and gpio_request_enable() ops.
Reset ctrl->nr_pins to 0 so that pin_to_bank() can locate a bank after
the second probe in U-Boot proper.

Signed-off-by: Jonas Karlman <jo...@kwiboo.se>
---
v2: New patch split from "pinctrl: rockchip: Add gpio_request_enable()
     ops" of "rockchip: Add gpio request() ops" series
---
  .../pinctrl/rockchip/pinctrl-rockchip-core.c   | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
index f6af22501c36..894ff74aee98 100644
--- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
+++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
@@ -176,6 +176,23 @@ static int rockchip_get_mux(struct rockchip_pin_bank 
*bank, int pin)
        return ((val >> bit) & mask);
  }
  
+static struct rockchip_pin_bank *rockchip_pin_to_bank(struct udevice *dev,
+                                                     unsigned int pin)
+{
+       struct rockchip_pinctrl_priv *priv = dev_get_priv(dev);
+       struct rockchip_pin_ctrl *ctrl = priv->ctrl;
+       struct rockchip_pin_bank *bank = ctrl->pin_banks;
+       int i;
+
+       for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
+               if (pin >= bank->pin_base &&
+                   pin < bank->pin_base + bank->nr_pins)
+                       return bank;
+       }
+
+       return NULL;
+}
+
  static int rockchip_pinctrl_get_gpio_mux(struct udevice *dev, int banknum,
                                         int index)
  {     struct rockchip_pinctrl_priv *priv = dev_get_priv(dev);
@@ -539,6 +556,7 @@ static struct rockchip_pin_ctrl 
*rockchip_pinctrl_get_soc_data(struct udevice *d
        drv_pmu_offs = ctrl->pmu_drv_offset;
        drv_grf_offs = ctrl->grf_drv_offset;
        bank = ctrl->pin_banks;
+       ctrl->nr_pins = 0;
  

Please in a separate commit, this seems to be an important bug to fix.

Now, I don't understand why we would probe twice in U-Boot proper. This 
seems very bad, shouldn't one probe be enough?

Further, I assume the issue is that we modify the static struct 
rockchip_pin_ctrl from within the pre-reloc probe and since the content 
won't have changed after relocation, whatever we modify in this struct 
will persist. This is.... bad. I would therefore suggest we migrate to 
const struct instead and have some kind of priv data that is allocated 
and initialized during probe and doesn't persist between probes (though 
I'm still perplexed by the double probe thingy).

Cheers,
Quentin