[PATCH 1/2] pinctrl: ocelot: correct pincfg regmap max_register

[PATCH 1/2] pinctrl: ocelot: correct pincfg regmap max_register

[Robert Marko]

Commit "pinctrl: ocelot: convert pinctrl to regmap" set the pincfg regmap
max_register to 32 which is incorrect as there are 64 pins in total.

Actual register space length is 0x100, which matches the DTS and can
actually fit one register per pin with stride of 4.

So, correct the max_register to 0xfc.

Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/pinctrl/pinctrl-ocelot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 349e063a04fa..84bfbe649b67 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1899,7 +1899,7 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 		.reg_bits = 32,
 		.val_bits = 32,
 		.reg_stride = 4,
-		.max_register = 32,
+		.max_register = 0xfc,
 		.name = "pincfg",
 	};
 
-- 
2.36.1

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

[Robert Marko]

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.

Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 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

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

[Robert Marko]

On Thu, Jul 14, 2022 at 1:01 AM Colin Foster
<colin.foster@in-advantage.com> 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/20220713193750.4079621-3-horatiu.vultur@microchip.com/
>
> 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 <robert.marko@sartura.hr>
> > ---
> >  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: robert.marko@sartura.hr
Web: www.sartura.hr

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

[Colin Foster]

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/20220713193750.4079621-3-horatiu.vultur@microchip.com/

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 <robert.marko@sartura.hr>
> ---
>  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
> 

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

[Colin Foster]

On Thu, Jul 14, 2022 at 01:07:24AM +0200, Robert Marko wrote:
> On Thu, Jul 14, 2022 at 1:01 AM Colin Foster <colin.foster@in-advantage.com>
> 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/20220713193750.4079621-3-horatiu.vultur@microchip.com/
> >
> > 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.

It doesn't bother me. The only difference was your 0xfc for the max
register, while Horatiu's (per my suggestion) was info->desc->npins. 
Just pointing that out in case anyone wants to raise that as a sticking
point.

> 
> Regards,
> Robert
> 

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

[Linus Walleij]

On Thu, Jul 14, 2022 at 1:07 AM Robert Marko <robert.marko@sartura.hr> wrote:
> On Thu, Jul 14, 2022 at 1:01 AM Colin Foster <colin.foster@in-advantage.com> 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/20220713193750.4079621-3-horatiu.vultur@microchip.com/
>>
>> 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.

Better too many fixes than too few!

I have merged Horatiu's fixes now.

Yours,
Linus Walleij