2022-07-13 23:04:13

by Colin Foster

Subject: Re: [PATCH 2/2] pinctrl: ocelot: fix pinconf

Hi Robert,

On Thu, Jul 14, 2022 at 12:51:51AM +0200, Robert Marko wrote:
> Commit "pinctrl: ocelot: convert pinctrl to regmap" moved to using
> regmap_read/write, however it neglected to also carry out alignment
> to register stride of 4.
>
> This would cause the following error:
> [ 1.720873] pinctrl-ocelot 6110101e0.pinctrl: pin_config_set op failed for pin 34
> [ 1.728110] sdhci-sparx5 600800000.mmc: Error applying setting, reverse things back
>
> So, regmap_read would return -EINVAL as it was being passed address
> of the pin without stride, so for example pin 34 would end up being
> 0x22 in hex.
>
> Fix this by accouting for the stride in register address.

Sorry for the bug. Horaitu found this as well and recently submitted
patches:

https://patchwork.ozlabs.org/project/linux-gpio/patch/[email protected]/

The second patch in his set fixes both of these issues (reg_stride and
max_register).

>
> Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> Signed-off-by: Robert Marko <[email protected]>
> ---
> drivers/pinctrl/pinctrl-ocelot.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index 84bfbe649b67..a71145367b15 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -1327,7 +1327,9 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
> if (info->pincfg) {
> u32 regcfg;
>
> - ret = regmap_read(info->pincfg, pin, &regcfg);
> + ret = regmap_read(info->pincfg,
> + pin * regmap_get_reg_stride(info->pincfg),
> + &regcfg);
> if (ret)
> return ret;
>
> @@ -1359,14 +1361,18 @@ static int ocelot_pincfg_clrsetbits(struct ocelot_pinctrl *info, u32 regaddr,
> u32 val;
> int ret;
>
> - ret = regmap_read(info->pincfg, regaddr, &val);
> + ret = regmap_read(info->pincfg,
> + regaddr * regmap_get_reg_stride(info->pincfg),
> + &val);
> if (ret)
> return ret;
>
> val &= ~clrbits;
> val |= setbits;
>
> - ret = regmap_write(info->pincfg, regaddr, val);
> + ret = regmap_write(info->pincfg,
> + regaddr * regmap_get_reg_stride(info->pincfg),
> + val);
>
> return ret;
> }
> --
> 2.36.1
>

2022-07-13 23:56:14

by Robert Marko

Subject: Re: [PATCH 2/2] pinctrl: ocelot: fix pinconf

On Thu, Jul 14, 2022 at 1:01 AM Colin Foster
<[email protected]> wrote:
>
> Hi Robert,
>
> On Thu, Jul 14, 2022 at 12:51:51AM +0200, Robert Marko wrote:
> > Commit "pinctrl: ocelot: convert pinctrl to regmap" moved to using
> > regmap_read/write, however it neglected to also carry out alignment
> > to register stride of 4.
> >
> > This would cause the following error:
> > [ 1.720873] pinctrl-ocelot 6110101e0.pinctrl: pin_config_set op failed for pin 34
> > [ 1.728110] sdhci-sparx5 600800000.mmc: Error applying setting, reverse things back
> >
> > So, regmap_read would return -EINVAL as it was being passed address
> > of the pin without stride, so for example pin 34 would end up being
> > 0x22 in hex.
> >
> > Fix this by accouting for the stride in register address.
>
> Sorry for the bug. Horaitu found this as well and recently submitted
> patches:
>
> https://patchwork.ozlabs.org/project/linux-gpio/patch/[email protected]/
>
> The second patch in his set fixes both of these issues (reg_stride and
> max_register).

Yeah, I noticed his patch only after sending this.
Sorry for the noise.

Regards,
Robert
>
> >
> > Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> > Signed-off-by: Robert Marko <[email protected]>
> > ---
> > drivers/pinctrl/pinctrl-ocelot.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> > index 84bfbe649b67..a71145367b15 100644
> > --- a/drivers/pinctrl/pinctrl-ocelot.c
> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
> > @@ -1327,7 +1327,9 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
> > if (info->pincfg) {
> > u32 regcfg;
> >
> > - ret = regmap_read(info->pincfg, pin, &regcfg);
> > + ret = regmap_read(info->pincfg,
> > + pin * regmap_get_reg_stride(info->pincfg),
> > + &regcfg);
> > if (ret)
> > return ret;
> >
> > @@ -1359,14 +1361,18 @@ static int ocelot_pincfg_clrsetbits(struct ocelot_pinctrl *info, u32 regaddr,
> > u32 val;
> > int ret;
> >
> > - ret = regmap_read(info->pincfg, regaddr, &val);
> > + ret = regmap_read(info->pincfg,
> > + regaddr * regmap_get_reg_stride(info->pincfg),
> > + &val);
> > if (ret)
> > return ret;
> >
> > val &= ~clrbits;
> > val |= setbits;
> >
> > - ret = regmap_write(info->pincfg, regaddr, val);
> > + ret = regmap_write(info->pincfg,
> > + regaddr * regmap_get_reg_stride(info->pincfg),
> > + val);
> >
> > return ret;
> > }
> > --
> > 2.36.1
> >



--
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: [email protected]
Web: http://www.sartura.hr