| Message ID | 20240611113204.3004-2-paul.barker.ct@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Configure GbEth for RGMII on RZ/G2L family | expand |
Hi Paul, On Tue, Jun 11, 2024 at 1:32 PM Paul Barker <paul.barker.ct@bp.renesas.com> wrote: > We currently support OEN read/write for the RZ/G3S SoC but not the > RZ/G2L SoC family (consisting of RZ/G2L, RZ/G2LC, RZ/G2UL, RZ/V2L & > RZ/Five). The appropriate functions are renamed to clarify this. > > We should also only set the oen_read and oen_write function pointers for > the devices which support these operations. This requires us to check > that these function pointers are valid before calling them. > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > --- > Changes v1->v2: > * New patch to clarify function names. Thanks for your patch! > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > @@ -1016,31 +1016,31 @@ static u8 rzg2l_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port) > return pin; > } > > -static u32 rzg2l_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin) > +static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin) > -static int rzg2l_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, u32 offset, u8 pin, u8 oen) As commit 7d566a4d270c52ff ("pinctrl: renesas: rzg2l: Add function pointers for OEN register access") did not rename rzg2l_{read,write}_oen() to rzg2l_oen_{read,write}(), to match the .oen_{read,write}() callback names, this is a good opportunity to fix that oversight. The v2h variants already match the callback names. > @@ -1215,6 +1215,8 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, > break; > > case PIN_CONFIG_OUTPUT_ENABLE: > + if (!pctrl->data->oen_read) > + return -EOPNOTSUPP; Perhaps the check for PIN_CFG_OEN in each of the .oen_read() callbacks should be moved here? > arg = pctrl->data->oen_read(pctrl, cfg, _pin, bit); > if (!arg) > return -EINVAL; > @@ -1354,6 +1356,8 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, > > case PIN_CONFIG_OUTPUT_ENABLE: > arg = pinconf_to_config_argument(_configs[i]); > + if (!pctrl->data->oen_write) > + return -EOPNOTSUPP; Likewise. > ret = pctrl->data->oen_write(pctrl, cfg, _pin, bit, !!arg); > if (ret) > return ret; Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On 17/06/2024 12:52, Geert Uytterhoeven wrote: > Hi Paul, > > On Tue, Jun 11, 2024 at 1:32 PM Paul Barker > <paul.barker.ct@bp.renesas.com> wrote: >> We currently support OEN read/write for the RZ/G3S SoC but not the >> RZ/G2L SoC family (consisting of RZ/G2L, RZ/G2LC, RZ/G2UL, RZ/V2L & >> RZ/Five). The appropriate functions are renamed to clarify this. >> >> We should also only set the oen_read and oen_write function pointers for >> the devices which support these operations. This requires us to check >> that these function pointers are valid before calling them. >> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> >> --- >> Changes v1->v2: >> * New patch to clarify function names. > > Thanks for your patch! > >> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c >> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c > >> @@ -1016,31 +1016,31 @@ static u8 rzg2l_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port) >> return pin; >> } >> >> -static u32 rzg2l_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin) >> +static u32 rzg3s_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin) > >> -static int rzg2l_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, u32 offset, u8 pin, u8 oen) > > As commit 7d566a4d270c52ff ("pinctrl: renesas: rzg2l: Add function > pointers for OEN register access") did not rename > rzg2l_{read,write}_oen() to rzg2l_oen_{read,write}(), to match the > .oen_{read,write}() callback names, this is a good opportunity to fix > that oversight. Ack. > > The v2h variants already match the callback names. > >> @@ -1215,6 +1215,8 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, >> break; >> >> case PIN_CONFIG_OUTPUT_ENABLE: >> + if (!pctrl->data->oen_read) >> + return -EOPNOTSUPP; > > Perhaps the check for PIN_CFG_OEN in each of the .oen_read() > callbacks should be moved here? Ack. > >> arg = pctrl->data->oen_read(pctrl, cfg, _pin, bit); >> if (!arg) >> return -EINVAL; >> @@ -1354,6 +1356,8 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, >> >> case PIN_CONFIG_OUTPUT_ENABLE: >> arg = pinconf_to_config_argument(_configs[i]); >> + if (!pctrl->data->oen_write) >> + return -EOPNOTSUPP; > > Likewise. Ack. I'll fix these in v3. Thanks,
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index 32945d4c8dc0..901175f6d05c 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c @@ -994,7 +994,7 @@ static bool rzg2l_ds_is_supported(struct rzg2l_pinctrl *pctrl, u32 caps, return false; } -static bool rzg2l_oen_is_supported(u32 caps, u8 pin, u8 max_pin) +static bool rzg3s_oen_is_supported(u32 caps, u8 pin, u8 max_pin) { if (!(caps & PIN_CFG_OEN)) return false; @@ -1005,7 +1005,7 @@ static bool rzg2l_oen_is_supported(u32 caps, u8 pin, u8 max_pin) return true; } -static u8 rzg2l_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port) +static u8 rzg3s_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port) { if (pin) pin *= 2; @@ -1016,31 +1016,31 @@ static u8 rzg2l_pin_to_oen_bit(u32 offset, u8 pin, u8 max_port) return pin; } -static u32 rzg2l_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 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 (!rzg2l_oen_is_supported(caps, pin, max_pin)) + if (!rzg3s_oen_is_supported(caps, pin, max_pin)) return 0; - bit = rzg2l_pin_to_oen_bit(offset, pin, max_port); + bit = rzg3s_pin_to_oen_bit(offset, pin, max_port); return !(readb(pctrl->base + ETH_MODE) & BIT(bit)); } -static int rzg2l_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, u32 offset, u8 pin, u8 oen) { u8 max_port = pctrl->data->hwcfg->oen_max_port; u8 max_pin = pctrl->data->hwcfg->oen_max_pin; unsigned long flags; u8 val, bit; - if (!rzg2l_oen_is_supported(caps, pin, max_pin)) + if (!rzg3s_oen_is_supported(caps, pin, max_pin)) return -EINVAL; - bit = rzg2l_pin_to_oen_bit(offset, pin, max_port); + bit = rzg3s_pin_to_oen_bit(offset, pin, max_port); spin_lock_irqsave(&pctrl->lock, flags); val = readb(pctrl->base + ETH_MODE); @@ -1215,6 +1215,8 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev, break; case PIN_CONFIG_OUTPUT_ENABLE: + if (!pctrl->data->oen_read) + return -EOPNOTSUPP; arg = pctrl->data->oen_read(pctrl, cfg, _pin, bit); if (!arg) return -EINVAL; @@ -1354,6 +1356,8 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev, case PIN_CONFIG_OUTPUT_ENABLE: arg = pinconf_to_config_argument(_configs[i]); + if (!pctrl->data->oen_write) + return -EOPNOTSUPP; ret = pctrl->data->oen_write(pctrl, cfg, _pin, bit, !!arg); if (ret) return ret; @@ -3065,8 +3069,6 @@ static struct rzg2l_pinctrl_data r9a07g043_data = { #endif .pwpr_pfc_lock_unlock = &rzg2l_pwpr_pfc_lock_unlock, .pmc_writeb = &rzg2l_pmc_writeb, - .oen_read = &rzg2l_read_oen, - .oen_write = &rzg2l_write_oen, .hw_to_bias_param = &rzg2l_hw_to_bias_param, .bias_param_to_hw = &rzg2l_bias_param_to_hw, }; @@ -3082,8 +3084,6 @@ static struct rzg2l_pinctrl_data r9a07g044_data = { .hwcfg = &rzg2l_hwcfg, .pwpr_pfc_lock_unlock = &rzg2l_pwpr_pfc_lock_unlock, .pmc_writeb = &rzg2l_pmc_writeb, - .oen_read = &rzg2l_read_oen, - .oen_write = &rzg2l_write_oen, .hw_to_bias_param = &rzg2l_hw_to_bias_param, .bias_param_to_hw = &rzg2l_bias_param_to_hw, }; @@ -3098,8 +3098,8 @@ static struct rzg2l_pinctrl_data r9a08g045_data = { .hwcfg = &rzg3s_hwcfg, .pwpr_pfc_lock_unlock = &rzg2l_pwpr_pfc_lock_unlock, .pmc_writeb = &rzg2l_pmc_writeb, - .oen_read = &rzg2l_read_oen, - .oen_write = &rzg2l_write_oen, + .oen_read = &rzg3s_read_oen, + .oen_write = &rzg3s_write_oen, .hw_to_bias_param = &rzg2l_hw_to_bias_param, .bias_param_to_hw = &rzg2l_bias_param_to_hw, };
We currently support OEN read/write for the RZ/G3S SoC but not the RZ/G2L SoC family (consisting of RZ/G2L, RZ/G2LC, RZ/G2UL, RZ/V2L & RZ/Five). The appropriate functions are renamed to clarify this. We should also only set the oen_read and oen_write function pointers for the devices which support these operations. This requires us to check that these function pointers are valid before calling them. Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> --- Changes v1->v2: * New patch to clarify function names. drivers/pinctrl/renesas/pinctrl-rzg2l.c | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-)