diff mbox series

[v2,2/9] pinctrl: renesas: rzg2l: Clean up and refactor OEN read/write functions

Message ID 20240611113204.3004-3-paul.barker.ct@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Configure GbEth for RGMII on RZ/G2L family | expand

Commit Message

Paul Barker June 11, 2024, 11:31 a.m. UTC 
The variable naming in the various OEN functions has been confusing. We
were passing the _pin & bit variables from rzg2l_pinctrl_pinconf_get()
and rzg2l_pinctrl_pinconf_set() as the offset & pin argument to the
read_oen() and write_oen() functions. This doesn't make sense, the first
of these isn't actually an offset and the second is not needed for
RZ/V2H but leads to confusion with the bit variable used within these
functions.

To tidy this up, instead pass the _pin variable directly to the
read_oen() and write_oen() functions with consistent naming. Then
rzg3s_read_oen() and rzg3s_write_oen() can use macros to get the port
and pin numbers it needs.

Also, merge rzg3s_oen_is_supported() into rzg3s_pin_to_oen_bit() to give
a single translation function which returns an error if the pin doesn't
support OEN. While we're here, remove an unnecessary branch and clarify
the variable naming.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
Changes v1->v2:
  * Merged patches 1 & 2 from the previous series, updated to be
    compatible with recent patches adding RZ/V2H support.

 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 74 +++++++++++--------------
 1 file changed, 32 insertions(+), 42 deletions(-)

Comments

Geert Uytterhoeven June 17, 2024, 12:02 p.m. UTC | #1 
Hi Paul,

On Tue, Jun 11, 2024 at 1:33 PM Paul Barker
<paul.barker.ct@bp.renesas.com> wrote:
> The variable naming in the various OEN functions has been confusing. We
> were passing the _pin & bit variables from rzg2l_pinctrl_pinconf_get()
> and rzg2l_pinctrl_pinconf_set() as the offset & pin argument to the
> read_oen() and write_oen() functions. This doesn't make sense, the first
> of these isn't actually an offset and the second is not needed for
> RZ/V2H but leads to confusion with the bit variable used within these
> functions.
>
> To tidy this up, instead pass the _pin variable directly to the
> read_oen() and write_oen() functions with consistent naming. Then
> rzg3s_read_oen() and rzg3s_write_oen() can use macros to get the port
> and pin numbers it needs.
>
> Also, merge rzg3s_oen_is_supported() into rzg3s_pin_to_oen_bit() to give
> a single translation function which returns an error if the pin doesn't
> support OEN. While we're here, remove an unnecessary branch and clarify
> the variable naming.
>
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
> Changes v1->v2:
>   * Merged patches 1 & 2 from the previous series, updated to be
>     compatible with recent patches adding RZ/V2H support.

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

> @@ -994,53 +994,43 @@ static bool rzg2l_ds_is_supported(struct rzg2l_pinctrl *pctrl, u32 caps,
>         return false;
>  }
>
> -static bool rzg3s_oen_is_supported(u32 caps, u8 pin, u8 max_pin)
> +static int rzg3s_pin_to_oen_bit(const struct rzg2l_hwcfg *hwcfg, u32 caps, u32 port, u8 pin)
>  {
> -       if (!(caps & PIN_CFG_OEN))
> -               return false;
> +       u8 bit = pin * 2;
>
> -       if (pin > max_pin)
> -               return false;
> +       if (!(caps & PIN_CFG_OEN) || pin > hwcfg->oen_max_pin)
> +               return -EINVAL;
>
> -       return true;
> +       if (port == hwcfg->oen_max_port)
> +               bit += 1;
> +
> +       return bit;
>  }
>
> -static u8 rzg3s_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port)
> +static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin)
>  {
> -       if (pin)
> -               pin *= 2;
> +       u32 port = RZG2L_PIN_ID_TO_PORT(_pin);
> +       u8 pin = RZG2L_PIN_ID_TO_PIN(_pin);

It's OK to use RZG2L_PIN_ID_TO_PIN() unconditionally, as RZ/G3S does
not have any dedicated pins with the OEN capability, right?

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Paul Barker June 25, 2024, 7:56 p.m. UTC | #2 
On 17/06/2024 13:02, Geert Uytterhoeven wrote:
> Hi Paul,
> 
> On Tue, Jun 11, 2024 at 1:33 PM Paul Barker
> <paul.barker.ct@bp.renesas.com> wrote:
>> -static u8 rzg3s_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port)
>> +static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin)
>>  {
>> -       if (pin)
>> -               pin *= 2;
>> +       u32 port = RZG2L_PIN_ID_TO_PORT(_pin);
>> +       u8 pin = RZG2L_PIN_ID_TO_PIN(_pin);
> 
> It's OK to use RZG2L_PIN_ID_TO_PIN() unconditionally, as RZ/G3S does
> not have any dedicated pins with the OEN capability, right?

I thought about this a bit and came to the conclusion that no, it's not
ok.

For RZ/G2L, only mux'd pins have OEN capability (Ethernet TXC/TX_CLK
only).

For RZ/G3S, OEN capability also exists on XSPI/OCTA pins which are
dedicated pins. We don't currently support OEN for these pins in the
driver, but we should put a check in place now to be safe. I've done
this in v3 which I'm about to send...

Thanks,
Claudiu Beznea June 26, 2024, 6:19 a.m. UTC | #3 
On 25.06.2024 22:56, Paul Barker wrote:
> On 17/06/2024 13:02, Geert Uytterhoeven wrote:
>> Hi Paul,
>>
>> On Tue, Jun 11, 2024 at 1:33 PM Paul Barker
>> <paul.barker.ct@bp.renesas.com> wrote:
>>> -static u8 rzg3s_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port)
>>> +static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin)
>>>  {
>>> -       if (pin)
>>> -               pin *= 2;
>>> +       u32 port = RZG2L_PIN_ID_TO_PORT(_pin);
>>> +       u8 pin = RZG2L_PIN_ID_TO_PIN(_pin);
>>
>> It's OK to use RZG2L_PIN_ID_TO_PIN() unconditionally, as RZ/G3S does
>> not have any dedicated pins with the OEN capability, right?
> 
> I thought about this a bit and came to the conclusion that no, it's not
> ok.
> 
> For RZ/G2L, only mux'd pins have OEN capability (Ethernet TXC/TX_CLK
> only).
> 
> For RZ/G3S, OEN capability also exists on XSPI/OCTA pins which are
> dedicated pins. We don't currently support OEN for these pins in the
> driver, but we should put a check in place now to be safe.

Just my preference: I would avoid adding code that is not currently used or
cannot be properly tested.

Thank you,
Claudiu Beznea

> I've done
> this in v3 which I'm about to send...
> 
> Thanks,
>
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 901175f6d05c..b28be5be668d 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -289,8 +289,8 @@  struct rzg2l_pinctrl_data {
 #endif
 	void (*pwpr_pfc_lock_unlock)(struct rzg2l_pinctrl *pctrl, bool lock);
 	void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, u16 offset);
-	u32 (*oen_read)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
-	int (*oen_write)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
+	u32 (*oen_read)(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin);
+	int (*oen_write)(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin, u8 oen);
 	int (*hw_to_bias_param)(unsigned int val);
 	int (*bias_param_to_hw)(enum pin_config_param param);
 };
@@ -994,53 +994,43 @@  static bool rzg2l_ds_is_supported(struct rzg2l_pinctrl *pctrl, u32 caps,
 	return false;
 }
 
-static bool rzg3s_oen_is_supported(u32 caps, u8 pin, u8 max_pin)
+static int rzg3s_pin_to_oen_bit(const struct rzg2l_hwcfg *hwcfg, u32 caps, u32 port, u8 pin)
 {
-	if (!(caps & PIN_CFG_OEN))
-		return false;
+	u8 bit = pin * 2;
 
-	if (pin > max_pin)
-		return false;
+	if (!(caps & PIN_CFG_OEN) || pin > hwcfg->oen_max_pin)
+		return -EINVAL;
 
-	return true;
+	if (port == hwcfg->oen_max_port)
+		bit += 1;
+
+	return bit;
 }
 
-static u8 rzg3s_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port)
+static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin)
 {
-	if (pin)
-		pin *= 2;
+	u32 port = RZG2L_PIN_ID_TO_PORT(_pin);
+	u8 pin = RZG2L_PIN_ID_TO_PIN(_pin);
+	int bit;
 
-	if (offset / RZG2L_PINS_PER_PORT == max_port)
-		pin += 1;
-
-	return pin;
-}
-
-static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin)
-{
-	u8 max_port = pctrl->data->hwcfg->oen_max_port;
-	u8 max_pin = pctrl->data->hwcfg->oen_max_pin;
-	u8 bit;
-
-	if (!rzg3s_oen_is_supported(caps, pin, max_pin))
+	bit = rzg3s_pin_to_oen_bit(pctrl->data->hwcfg, caps, port, pin);
+	if (bit < 0)
 		return 0;
 
-	bit = rzg3s_pin_to_oen_bit(offset, pin, max_port);
-
 	return !(readb(pctrl->base + ETH_MODE) & BIT(bit));
 }
 
-static int rzg3s_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen)
+static int rzg3s_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin, u8 oen)
 {
-	u8 max_port = pctrl->data->hwcfg->oen_max_port;
-	u8 max_pin = pctrl->data->hwcfg->oen_max_pin;
+	u32 port = RZG2L_PIN_ID_TO_PORT(_pin);
+	u8 pin = RZG2L_PIN_ID_TO_PIN(_pin);
 	unsigned long flags;
-	u8 val, bit;
+	int bit;
+	u8 val;
 
-	if (!rzg3s_oen_is_supported(caps, pin, max_pin))
-		return -EINVAL;
-
-	bit = rzg3s_pin_to_oen_bit(offset, pin, max_port);
+	bit = rzg3s_pin_to_oen_bit(pctrl->data->hwcfg, caps, port, pin);
+	if (bit < 0)
+		return bit;
 
 	spin_lock_irqsave(&pctrl->lock, flags);
 	val = readb(pctrl->base + ETH_MODE);
@@ -1119,12 +1109,12 @@  static int rzv2h_bias_param_to_hw(enum pin_config_param param)
 	return -EINVAL;
 }
 
-static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, u32 offset)
+static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, unsigned int _pin)
 {
 	static const char * const pin_names[] = { "ET0_TXC_TXCLK", "ET1_TXC_TXCLK",
 						  "XSPI0_RESET0N", "XSPI0_CS0N",
 						  "XSPI0_CKN", "XSPI0_CKP" };
-	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
+	const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[_pin];
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(pin_names); i++) {
@@ -1136,19 +1126,19 @@  static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, u32 offset)
 	return 0;
 }
 
-static u32 rzv2h_oen_read(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin)
+static u32 rzv2h_oen_read(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin)
 {
 	u8 bit;
 
 	if (!(caps & PIN_CFG_OEN))
 		return 0;
 
-	bit = rzv2h_pin_to_oen_bit(pctrl, offset);
+	bit = rzv2h_pin_to_oen_bit(pctrl, _pin);
 
 	return !(readb(pctrl->base + PFC_OEN) & BIT(bit));
 }
 
-static int rzv2h_oen_write(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen)
+static int rzv2h_oen_write(struct rzg2l_pinctrl *pctrl, u32 caps, unsigned int _pin, u8 oen)
 {
 	const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
 	const struct rzg2l_register_offsets *regs = &hwcfg->regs;
@@ -1159,7 +1149,7 @@  static int rzv2h_oen_write(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8
 	if (!(caps & PIN_CFG_OEN))
 		return -EINVAL;
 
-	bit = rzv2h_pin_to_oen_bit(pctrl, offset);
+	bit = rzv2h_pin_to_oen_bit(pctrl, _pin);
 	spin_lock_irqsave(&pctrl->lock, flags);
 	val = readb(pctrl->base + PFC_OEN);
 	if (oen)
@@ -1217,7 +1207,7 @@  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
 	case PIN_CONFIG_OUTPUT_ENABLE:
 		if (!pctrl->data->oen_read)
 			return -EOPNOTSUPP;
-		arg = pctrl->data->oen_read(pctrl, cfg, _pin, bit);
+		arg = pctrl->data->oen_read(pctrl, cfg, _pin);
 		if (!arg)
 			return -EINVAL;
 		break;
@@ -1358,7 +1348,7 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			arg = pinconf_to_config_argument(_configs[i]);
 			if (!pctrl->data->oen_write)
 				return -EOPNOTSUPP;
-			ret = pctrl->data->oen_write(pctrl, cfg, _pin, bit, !!arg);
+			ret = pctrl->data->oen_write(pctrl, cfg, _pin, !!arg);
 			if (ret)
 				return ret;
 			break;