[PATCH 0/2] pinctrl: ocelot: Add fixes for ocelot driver

[PATCH 0/2] pinctrl: ocelot: Add fixes for ocelot driver

[Horatiu Vultur]

The patch series fixes 2 issues with pincfg.
- first issue is that on lan966x uses different offsets than sparx5
  so it can't use directly the ocelot_confops
- second issue is pincfg stop working when regmap support was addded.

Horatiu Vultur (2):
  pinctrl: ocelot: Fix pincfg for lan966x
  pinctrl: ocelot: Fix pincfg

 drivers/pinctrl/pinctrl-ocelot.c | 222 +++++++++++++++++++------------
 1 file changed, 138 insertions(+), 84 deletions(-)

-- 
2.33.0

[PATCH 1/2] pinctrl: ocelot: Fix pincfg for lan966x

[Horatiu Vultur]

The blamed commit introduce support for lan966x which use the same
pinconf_ops as sparx5. The problem is that pinconf_ops is specific to
sparx5. More precisely the offset of the bits in the pincfg register are
different and also lan966x doesn't have support for
PIN_CONFIG_INPUT_SCHMITT_ENABLE.

Fix this by making pinconf_ops more generic such that it can be also
used by lan966x. This is done by introducing 'ocelot_pincfg_data' which
contains the offset and what is supported for each SOC.

Fixes: 531d6ab36571 ("pinctrl: ocelot: Extend support for lan966x")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/pinctrl/pinctrl-ocelot.c | 194 ++++++++++++++++++++-----------
 1 file changed, 123 insertions(+), 71 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 5f4a8c5c6650..6212abe2b66f 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -29,19 +29,12 @@
 #define ocelot_clrsetbits(addr, clear, set) \
 	writel((readl(addr) & ~(clear)) | (set), (addr))
 
-/* PINCONFIG bits (sparx5 only) */
 enum {
 	PINCONF_BIAS,
 	PINCONF_SCHMITT,
 	PINCONF_DRIVE_STRENGTH,
 };
 
-#define BIAS_PD_BIT BIT(4)
-#define BIAS_PU_BIT BIT(3)
-#define BIAS_BITS   (BIAS_PD_BIT|BIAS_PU_BIT)
-#define SCHMITT_BIT BIT(2)
-#define DRIVE_BITS  GENMASK(1, 0)
-
 /* GPIO standard registers */
 #define OCELOT_GPIO_OUT_SET	0x0
 #define OCELOT_GPIO_OUT_CLR	0x4
@@ -321,6 +314,14 @@ struct ocelot_pin_caps {
 	unsigned char a_functions[OCELOT_FUNC_PER_PIN];	/* Additional functions */
 };
 
+struct ocelot_pincfg_data {
+	bool has_schmitt;
+	u8 schmitt_bit;
+	u8 pd_bit;
+	u8 pu_bit;
+	u8 drive_bits;
+};
+
 struct ocelot_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
@@ -330,6 +331,12 @@ struct ocelot_pinctrl {
 	struct pinctrl_desc *desc;
 	struct ocelot_pmx_func func[FUNC_MAX];
 	u8 stride;
+	struct ocelot_pincfg_data *pincfg_data;
+};
+
+struct ocelot_match_data {
+	struct pinctrl_desc desc;
+	struct ocelot_pincfg_data pincfg_data;
 };
 
 #define LUTON_P(p, f0, f1)						\
@@ -1334,15 +1341,17 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
 		ret = 0;
 		switch (reg) {
 		case PINCONF_BIAS:
-			*val = regcfg & BIAS_BITS;
+			*val = regcfg &
+				(info->pincfg_data->pd_bit |
+				 info->pincfg_data->pu_bit);
 			break;
 
 		case PINCONF_SCHMITT:
-			*val = regcfg & SCHMITT_BIT;
+			*val = regcfg & info->pincfg_data->schmitt_bit;
 			break;
 
 		case PINCONF_DRIVE_STRENGTH:
-			*val = regcfg & DRIVE_BITS;
+			*val = regcfg & info->pincfg_data->drive_bits;
 			break;
 
 		default:
@@ -1383,19 +1392,23 @@ static int ocelot_hw_set_value(struct ocelot_pinctrl *info,
 		ret = 0;
 		switch (reg) {
 		case PINCONF_BIAS:
-			ret = ocelot_pincfg_clrsetbits(info, pin, BIAS_BITS,
+			ret = ocelot_pincfg_clrsetbits(info, pin,
+						       info->pincfg_data->pd_bit |
+						       info->pincfg_data->pu_bit,
 						       val);
 			break;
 
 		case PINCONF_SCHMITT:
-			ret = ocelot_pincfg_clrsetbits(info, pin, SCHMITT_BIT,
+			ret = ocelot_pincfg_clrsetbits(info, pin,
+						       info->pincfg_data->schmitt_bit,
 						       val);
 			break;
 
 		case PINCONF_DRIVE_STRENGTH:
 			if (val <= 3)
 				ret = ocelot_pincfg_clrsetbits(info, pin,
-							       DRIVE_BITS, val);
+							       info->pincfg_data->drive_bits,
+							       val);
 			else
 				ret = -EINVAL;
 			break;
@@ -1425,17 +1438,20 @@ static int ocelot_pinconf_get(struct pinctrl_dev *pctldev,
 		if (param == PIN_CONFIG_BIAS_DISABLE)
 			val = (val == 0);
 		else if (param == PIN_CONFIG_BIAS_PULL_DOWN)
-			val = (val & BIAS_PD_BIT ? true : false);
+			val = (val & info->pincfg_data->pd_bit ? true : false);
 		else    /* PIN_CONFIG_BIAS_PULL_UP */
-			val = (val & BIAS_PU_BIT ? true : false);
+			val = (val & info->pincfg_data->pu_bit ? true : false);
 		break;
 
 	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+		if (!info->pincfg_data->has_schmitt)
+			return -EOPNOTSUPP;
+
 		err = ocelot_hw_get_value(info, pin, PINCONF_SCHMITT, &val);
 		if (err)
 			return err;
 
-		val = (val & SCHMITT_BIT ? true : false);
+		val = (val & info->pincfg_data->schmitt_bit ? true : false);
 		break;
 
 	case PIN_CONFIG_DRIVE_STRENGTH:
@@ -1491,8 +1507,8 @@ static int ocelot_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 		case PIN_CONFIG_BIAS_PULL_UP:
 		case PIN_CONFIG_BIAS_PULL_DOWN:
 			arg = (param == PIN_CONFIG_BIAS_DISABLE) ? 0 :
-			(param == PIN_CONFIG_BIAS_PULL_UP) ? BIAS_PU_BIT :
-			BIAS_PD_BIT;
+			(param == PIN_CONFIG_BIAS_PULL_UP) ? info->pincfg_data->pu_bit :
+			info->pincfg_data->pd_bit;
 
 			err = ocelot_hw_set_value(info, pin, PINCONF_BIAS, arg);
 			if (err)
@@ -1501,7 +1517,10 @@ static int ocelot_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			break;
 
 		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
-			arg = arg ? SCHMITT_BIT : 0;
+			if (!info->pincfg_data->has_schmitt)
+				return -EOPNOTSUPP;
+
+			arg = arg ? info->pincfg_data->schmitt_bit : 0;
 			err = ocelot_hw_set_value(info, pin, PINCONF_SCHMITT,
 						  arg);
 			if (err)
@@ -1562,69 +1581,96 @@ static const struct pinctrl_ops ocelot_pctl_ops = {
 	.dt_free_map = pinconf_generic_dt_free_map,
 };
 
-static struct pinctrl_desc luton_desc = {
-	.name = "luton-pinctrl",
-	.pins = luton_pins,
-	.npins = ARRAY_SIZE(luton_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data luton_desc = {
+	{
+		.name = "luton-pinctrl",
+		.pins = luton_pins,
+		.npins = ARRAY_SIZE(luton_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.owner = THIS_MODULE,
+	}
 };
 
-static struct pinctrl_desc serval_desc = {
-	.name = "serval-pinctrl",
-	.pins = serval_pins,
-	.npins = ARRAY_SIZE(serval_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data serval_desc = {
+	{
+		.name = "serval-pinctrl",
+		.pins = serval_pins,
+		.npins = ARRAY_SIZE(serval_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.owner = THIS_MODULE,
+	}
 };
 
-static struct pinctrl_desc ocelot_desc = {
-	.name = "ocelot-pinctrl",
-	.pins = ocelot_pins,
-	.npins = ARRAY_SIZE(ocelot_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data ocelot_desc = {
+	{
+		.name = "ocelot-pinctrl",
+		.pins = ocelot_pins,
+		.npins = ARRAY_SIZE(ocelot_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.owner = THIS_MODULE,
+	}
 };
 
-static struct pinctrl_desc jaguar2_desc = {
-	.name = "jaguar2-pinctrl",
-	.pins = jaguar2_pins,
-	.npins = ARRAY_SIZE(jaguar2_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data jaguar2_desc = {
+	{
+		.name = "jaguar2-pinctrl",
+		.pins = jaguar2_pins,
+		.npins = ARRAY_SIZE(jaguar2_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.owner = THIS_MODULE,
+	}
 };
 
-static struct pinctrl_desc servalt_desc = {
-	.name = "servalt-pinctrl",
-	.pins = servalt_pins,
-	.npins = ARRAY_SIZE(servalt_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data servalt_desc = {
+	{
+		.name = "servalt-pinctrl",
+		.pins = servalt_pins,
+		.npins = ARRAY_SIZE(servalt_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.owner = THIS_MODULE,
+	}
 };
 
-static struct pinctrl_desc sparx5_desc = {
-	.name = "sparx5-pinctrl",
-	.pins = sparx5_pins,
-	.npins = ARRAY_SIZE(sparx5_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &ocelot_pmx_ops,
-	.confops = &ocelot_confops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data sparx5_desc = {
+	{
+		.name = "sparx5-pinctrl",
+		.pins = sparx5_pins,
+		.npins = ARRAY_SIZE(sparx5_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &ocelot_pmx_ops,
+		.confops = &ocelot_confops,
+		.owner = THIS_MODULE,
+	},
+	{
+		.has_schmitt = true,
+		.pd_bit = BIT(4),
+		.pu_bit = BIT(3),
+		.schmitt_bit = BIT(2),
+		.drive_bits = GENMASK(1, 0),
+	}
 };
 
-static struct pinctrl_desc lan966x_desc = {
-	.name = "lan966x-pinctrl",
-	.pins = lan966x_pins,
-	.npins = ARRAY_SIZE(lan966x_pins),
-	.pctlops = &ocelot_pctl_ops,
-	.pmxops = &lan966x_pmx_ops,
-	.confops = &ocelot_confops,
-	.owner = THIS_MODULE,
+static struct ocelot_match_data lan966x_desc = {
+	{
+		.name = "lan966x-pinctrl",
+		.pins = lan966x_pins,
+		.npins = ARRAY_SIZE(lan966x_pins),
+		.pctlops = &ocelot_pctl_ops,
+		.pmxops = &lan966x_pmx_ops,
+		.confops = &ocelot_confops,
+		.owner = THIS_MODULE,
+	},
+	{
+		.has_schmitt = false,
+		.pd_bit = BIT(3),
+		.pu_bit = BIT(2),
+		.drive_bits = GENMASK(1, 0),
+	}
 };
 
 static int ocelot_create_group_func_map(struct device *dev,
@@ -1914,6 +1960,7 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 static int ocelot_pinctrl_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct ocelot_match_data *data;
 	struct ocelot_pinctrl *info;
 	struct reset_control *reset;
 	struct regmap *pincfg;
@@ -1929,7 +1976,12 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 	if (!info)
 		return -ENOMEM;
 
-	info->desc = (struct pinctrl_desc *)device_get_match_data(dev);
+	data = (struct ocelot_match_data *)device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
+	info->desc = &data->desc;
+	info->pincfg_data = &data->pincfg_data;
 
 	reset = devm_reset_control_get_optional_shared(dev, "switch");
 	if (IS_ERR(reset))
-- 
2.33.0

Re: [PATCH 1/2] pinctrl: ocelot: Fix pincfg for lan966x

[Michael Walle]

> -static struct pinctrl_desc luton_desc = {
> -	.name = "luton-pinctrl",
> -	.pins = luton_pins,
> -	.npins = ARRAY_SIZE(luton_pins),
> -	.pctlops = &ocelot_pctl_ops,
> -	.pmxops = &ocelot_pmx_ops,
> -	.owner = THIS_MODULE,
> +static struct ocelot_match_data luton_desc = {
> +	{
.desc = {

Same for all the structs. Also it looks like it is way too big
for a stable backport:
 "It cannot be bigger than 100 lines, with context."

> +		.name = "luton-pinctrl",
> +		.pins = luton_pins,
> +		.npins = ARRAY_SIZE(luton_pins),
> +		.pctlops = &ocelot_pctl_ops,
> +		.pmxops = &ocelot_pmx_ops,
> +		.owner = THIS_MODULE,
> +	}
>  };

-michael

Re: [PATCH 1/2] pinctrl: ocelot: Fix pincfg for lan966x

[Horatiu Vultur]

The 07/08/2022 10:27, Michael Walle wrote:

Hi Walle,

> 
> > -static struct pinctrl_desc luton_desc = {
> > -     .name = "luton-pinctrl",
> > -     .pins = luton_pins,
> > -     .npins = ARRAY_SIZE(luton_pins),
> > -     .pctlops = &ocelot_pctl_ops,
> > -     .pmxops = &ocelot_pmx_ops,
> > -     .owner = THIS_MODULE,
> > +static struct ocelot_match_data luton_desc = {
> > +     {
> .desc = {
> 
> Same for all the structs. 

Yes, I will do that.


> Also it looks like it is way too big
> for a stable backport:
>  "It cannot be bigger than 100 lines, with context."

Don't you think it is a little bit silly to apply that rule to this
patch. In the way that 70% of the patch is just indentation.

> 
> > +             .name = "luton-pinctrl",
> > +             .pins = luton_pins,
> > +             .npins = ARRAY_SIZE(luton_pins),
> > +             .pctlops = &ocelot_pctl_ops,
> > +             .pmxops = &ocelot_pmx_ops,
> > +             .owner = THIS_MODULE,
> > +     }
> >  };
> 
> -michael

-- 
/Horatiu

[PATCH 2/2] pinctrl: ocelot: Fix pincfg

[Horatiu Vultur]

The blamed commit changed to use regmaps instead of __iomem. But it
didn't update the register offsets to be at word offset, so it uses byte
offset.

Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/pinctrl/pinctrl-ocelot.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 6212abe2b66f..e84f2f82901f 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -303,6 +303,13 @@ static const char *const ocelot_function_names[] = {
 	[FUNC_RCVRD_CLK]	= "rcvrd_clk",
 };
 
+const struct regmap_config regmap_pincfg = {
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+		.name = "pincfg",
+};
+
 struct ocelot_pmx_func {
 	const char **groups;
 	unsigned int ngroups;
@@ -1334,7 +1341,8 @@ 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_pincfg.reg_stride,
+				  &regcfg);
 		if (ret)
 			return ret;
 
@@ -1368,14 +1376,16 @@ 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_pincfg.reg_stride,
+			  &val);
 	if (ret)
 		return ret;
 
 	val &= ~clrbits;
 	val |= setbits;
 
-	ret = regmap_write(info->pincfg, regaddr, val);
+	ret = regmap_write(info->pincfg, regaddr * regmap_pincfg.reg_stride,
+			   val);
 
 	return ret;
 }
@@ -1940,21 +1950,13 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 {
 	void __iomem *base;
 
-	const struct regmap_config regmap_config = {
-		.reg_bits = 32,
-		.val_bits = 32,
-		.reg_stride = 4,
-		.max_register = 32,
-		.name = "pincfg",
-	};
-
-	base = devm_platform_ioremap_resource(pdev, 1);
+		base = devm_platform_ioremap_resource(pdev, 1);
 	if (IS_ERR(base)) {
 		dev_dbg(&pdev->dev, "Failed to ioremap config registers (no extended pinconf)\n");
 		return NULL;
 	}
 
-	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
+	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_pincfg);
 }
 
 static int ocelot_pinctrl_probe(struct platform_device *pdev)
-- 
2.33.0

Re: [PATCH 2/2] pinctrl: ocelot: Fix pincfg

[Colin Foster]

Hi Horatiu,

On Thu, Jul 07, 2022 at 08:53:42PM +0200, Horatiu Vultur wrote:
> The blamed commit changed to use regmaps instead of __iomem. But it
> didn't update the register offsets to be at word offset, so it uses byte
> offset.
> 
> Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>

Sorry about this one. It sounded familiar though:
https://patchwork.ozlabs.org/project/linux-gpio/patch/20220125161245.418882-1-horatiu.vultur@microchip.com/

The only takeaway from that was the use of regmap_get_reg_stride, which
was done in
commit baf927a833ca ("microchip-sgpio: Fix support for regmap")

And I see it is only for pincfg - which I don't have any hardware to
test that. Apologies again!

> ---
>  drivers/pinctrl/pinctrl-ocelot.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index 6212abe2b66f..e84f2f82901f 100644
> --- a/drivers/pinctrl/pinctrl-ocelot.c
> +++ b/drivers/pinctrl/pinctrl-ocelot.c
> @@ -303,6 +303,13 @@ static const char *const ocelot_function_names[] = {
>  	[FUNC_RCVRD_CLK]	= "rcvrd_clk",
>  };
>  
> +const struct regmap_config regmap_pincfg = {
> +		.reg_bits = 32,
> +		.val_bits = 32,
> +		.reg_stride = 4,
> +		.name = "pincfg",
> +};
> +
>  struct ocelot_pmx_func {
>  	const char **groups;
>  	unsigned int ngroups;
> @@ -1334,7 +1341,8 @@ 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_pincfg.reg_stride,
> +				  &regcfg);
>  		if (ret)
>  			return ret;
>  
> @@ -1368,14 +1376,16 @@ 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_pincfg.reg_stride,
> +			  &val);
>  	if (ret)
>  		return ret;
>  
>  	val &= ~clrbits;
>  	val |= setbits;
>  
> -	ret = regmap_write(info->pincfg, regaddr, val);
> +	ret = regmap_write(info->pincfg, regaddr * regmap_pincfg.reg_stride,
> +			   val);
>  
>  	return ret;
>  }
> @@ -1940,21 +1950,13 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
>  {
>  	void __iomem *base;
>  
> -	const struct regmap_config regmap_config = {
> -		.reg_bits = 32,
> -		.val_bits = 32,
> -		.reg_stride = 4,
> -		.max_register = 32,
> -		.name = "pincfg",
> -	};
> -
> -	base = devm_platform_ioremap_resource(pdev, 1);
> +		base = devm_platform_ioremap_resource(pdev, 1);
>  	if (IS_ERR(base)) {
>  		dev_dbg(&pdev->dev, "Failed to ioremap config registers (no extended pinconf)\n");
>  		return NULL;
>  	}
>  
> -	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
> +	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_pincfg);
>  }
>  
>  static int ocelot_pinctrl_probe(struct platform_device *pdev)
> -- 
> 2.33.0
> 

Re: [PATCH 2/2] pinctrl: ocelot: Fix pincfg

[Horatiu Vultur]

The 07/07/2022 17:31, Colin Foster wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Horatiu,

Hi Colin,

> 
> On Thu, Jul 07, 2022 at 08:53:42PM +0200, Horatiu Vultur wrote:
> > The blamed commit changed to use regmaps instead of __iomem. But it
> > didn't update the register offsets to be at word offset, so it uses byte
> > offset.
> >
> > Fixes: 076d9e71bcf8 ("pinctrl: ocelot: convert pinctrl to regmap")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> Sorry about this one. It sounded familiar though:
> https://patchwork.ozlabs.org/project/linux-gpio/patch/20220125161245.418882-1-horatiu.vultur@microchip.com/
> 
> The only takeaway from that was the use of regmap_get_reg_stride, which
> was done in
> commit baf927a833ca ("microchip-sgpio: Fix support for regmap")

That is correct. I will update this to use regmap_get_reg_stride.

> 
> And I see it is only for pincfg - which I don't have any hardware to
> test that. Apologies again!

No worries.

> 
> > ---
> >  drivers/pinctrl/pinctrl-ocelot.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> > index 6212abe2b66f..e84f2f82901f 100644
> > --- a/drivers/pinctrl/pinctrl-ocelot.c
> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
> > @@ -303,6 +303,13 @@ static const char *const ocelot_function_names[] = {
> >       [FUNC_RCVRD_CLK]        = "rcvrd_clk",
> >  };
> >
> > +const struct regmap_config regmap_pincfg = {
> > +             .reg_bits = 32,
> > +             .val_bits = 32,
> > +             .reg_stride = 4,
> > +             .name = "pincfg",
> > +};
> > +
> >  struct ocelot_pmx_func {
> >       const char **groups;
> >       unsigned int ngroups;
> > @@ -1334,7 +1341,8 @@ 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_pincfg.reg_stride,
> > +                               &regcfg);
> >               if (ret)
> >                       return ret;
> >
> > @@ -1368,14 +1376,16 @@ 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_pincfg.reg_stride,
> > +                       &val);
> >       if (ret)
> >               return ret;
> >
> >       val &= ~clrbits;
> >       val |= setbits;
> >
> > -     ret = regmap_write(info->pincfg, regaddr, val);
> > +     ret = regmap_write(info->pincfg, regaddr * regmap_pincfg.reg_stride,
> > +                        val);
> >
> >       return ret;
> >  }
> > @@ -1940,21 +1950,13 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> >  {
> >       void __iomem *base;
> >
> > -     const struct regmap_config regmap_config = {
> > -             .reg_bits = 32,
> > -             .val_bits = 32,
> > -             .reg_stride = 4,
> > -             .max_register = 32,
> > -             .name = "pincfg",
> > -     };
> > -
> > -     base = devm_platform_ioremap_resource(pdev, 1);
> > +             base = devm_platform_ioremap_resource(pdev, 1);
> >       if (IS_ERR(base)) {
> >               dev_dbg(&pdev->dev, "Failed to ioremap config registers (no extended pinconf)\n");
> >               return NULL;
> >       }
> >
> > -     return devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
> > +     return devm_regmap_init_mmio(&pdev->dev, base, &regmap_pincfg);
> >  }
> >
> >  static int ocelot_pinctrl_probe(struct platform_device *pdev)
> > --
> > 2.33.0
> >

-- 
/Horatiu

Re: [PATCH 2/2] pinctrl: ocelot: Fix pincfg

[kernel test robot]

Hi Horatiu,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Horatiu-Vultur/pinctrl-ocelot-Add-fixes-for-ocelot-driver/20220708-025029
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220708/202207080905.doKxbD08-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/162d70439a9da8a7142091d2fe2690f92756e34b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Horatiu-Vultur/pinctrl-ocelot-Add-fixes-for-ocelot-driver/20220708-025029
        git checkout 162d70439a9da8a7142091d2fe2690f92756e34b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/pinctrl/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/pinctrl/pinctrl-ocelot.c:306:28: sparse: sparse: symbol 'regmap_pincfg' was not declared. Should it be static?

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/2] pinctrl: ocelot: Fix pincfg

[kernel test robot]

Hi Horatiu,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linus/master v5.19-rc5 next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Horatiu-Vultur/pinctrl-ocelot-Add-fixes-for-ocelot-driver/20220708-025029
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220708/202207081432.wshyILwP-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/pinctrl/pinctrl-ocelot.c:1954 ocelot_pinctrl_create_pincfg() warn: inconsistent indenting

vim +1954 drivers/pinctrl/pinctrl-ocelot.c

ce8dc0943357a5 Alexandre Belloni 2018-01-06  1949  
076d9e71bcf8a8 Colin Foster      2021-11-19  1950  static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
076d9e71bcf8a8 Colin Foster      2021-11-19  1951  {
076d9e71bcf8a8 Colin Foster      2021-11-19  1952  	void __iomem *base;
076d9e71bcf8a8 Colin Foster      2021-11-19  1953  
94ef32970d4076 Michael Walle     2022-02-16 @1954  		base = devm_platform_ioremap_resource(pdev, 1);
076d9e71bcf8a8 Colin Foster      2021-11-19  1955  	if (IS_ERR(base)) {
076d9e71bcf8a8 Colin Foster      2021-11-19  1956  		dev_dbg(&pdev->dev, "Failed to ioremap config registers (no extended pinconf)\n");
076d9e71bcf8a8 Colin Foster      2021-11-19  1957  		return NULL;
076d9e71bcf8a8 Colin Foster      2021-11-19  1958  	}
076d9e71bcf8a8 Colin Foster      2021-11-19  1959  
162d70439a9da8 Horatiu Vultur    2022-07-07  1960  	return devm_regmap_init_mmio(&pdev->dev, base, &regmap_pincfg);
076d9e71bcf8a8 Colin Foster      2021-11-19  1961  }
076d9e71bcf8a8 Colin Foster      2021-11-19  1962  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp