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

[PATCH v3 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 added.

v2->v3:
- reorder ocelot_pincfg_data fields, mandatory fields go first
- move the field pincfg_data inside ocelot_pinctrl
- add back max_register for regmap_config for pincfg.
- make struct ocelot_match_data const and drop the cast.

v1->v2:
- use regmap_get_reg_stride instead of making regmap_config global
- update how ocelot_match_data structs are initialized

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

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

-- 
2.33.0

[PATCH v3 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 | 196 ++++++++++++++++++++-----------
 1 file changed, 125 insertions(+), 71 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 5f4a8c5c6650..4edb36cfa161 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 {
+	u8 pd_bit;
+	u8 pu_bit;
+	u8 drive_bits;
+	u8 schmitt_bit;
+	bool has_schmitt;
+};
+
 struct ocelot_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
@@ -328,10 +329,16 @@ struct ocelot_pinctrl {
 	struct regmap *map;
 	struct regmap *pincfg;
 	struct pinctrl_desc *desc;
+	struct ocelot_pincfg_data *pincfg_data;
 	struct ocelot_pmx_func func[FUNC_MAX];
 	u8 stride;
 };
 
+struct ocelot_match_data {
+	struct pinctrl_desc desc;
+	struct ocelot_pincfg_data pincfg_data;
+};
+
 #define LUTON_P(p, f0, f1)						\
 static struct ocelot_pin_caps luton_pin_##p = {				\
 	.pin = p,							\
@@ -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);
 		else    /* PIN_CONFIG_BIAS_PULL_UP */
-			val = (val & BIAS_PU_BIT ? true : false);
+			val = !!(val & info->pincfg_data->pu_bit);
 		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);
 		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,95 @@ 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 = {
+	.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 = {
+	.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 = {
+	.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 = {
+	.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 = {
+	.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 = {
+	.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,
+	},
+	.pincfg_data = {
+		.pd_bit = BIT(4),
+		.pu_bit = BIT(3),
+		.drive_bits = GENMASK(1, 0),
+		.has_schmitt = true,
+		.schmitt_bit = BIT(2),
+	},
 };
 
-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 = {
+	.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,
+	},
+	.pincfg_data = {
+		.pd_bit = BIT(3),
+		.pu_bit = BIT(2),
+		.drive_bits = GENMASK(1, 0),
+	},
 };
 
 static int ocelot_create_group_func_map(struct device *dev,
@@ -1913,6 +1958,7 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 
 static int ocelot_pinctrl_probe(struct platform_device *pdev)
 {
+	const struct ocelot_match_data *data;
 	struct device *dev = &pdev->dev;
 	struct ocelot_pinctrl *info;
 	struct reset_control *reset;
@@ -1929,7 +1975,15 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 	if (!info)
 		return -ENOMEM;
 
-	info->desc = (struct pinctrl_desc *)device_get_match_data(dev);
+	data = device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
+	info->desc = devm_kmemdup(dev, &data->desc,
+				  sizeof(struct pinctrl_desc), GFP_KERNEL);
+	info->pincfg_data = devm_kmemdup(dev, &data->pincfg_data,
+					 sizeof(struct ocelot_match_data),
+					 GFP_KERNEL);
 
 	reset = devm_reset_control_get_optional_shared(dev, "switch");
 	if (IS_ERR(reset))
-- 
2.33.0

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
>
> 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.

Thanks for an update!
My comments below.

...

I believe introducing

  struct ocelot_pincfg_data *opd = info->pincfg_data;

may allow to reduce LoCs...

> +                       *val = regcfg &
> +                               (info->pincfg_data->pd_bit |
> +                                info->pincfg_data->pu_bit);

...like here:

 *val = regcfg & (opd->pd_bit | opd->pu_bit);

...

> +       info->desc = devm_kmemdup(dev, &data->desc,
> +                                 sizeof(struct pinctrl_desc), GFP_KERNEL);

sizeof(*info->desc)

and missed the NULL check.

...

> +       info->pincfg_data = devm_kmemdup(dev, &data->pincfg_data,
> +                                        sizeof(struct ocelot_match_data),

sizeof(*info->pincfg_data)
(isn't it a bug here?)

> +                                        GFP_KERNEL);

and missed the NULL check.

-- 
With Best Regards,
Andy Shevchenko

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

[Horatiu Vultur]

The 07/11/2022 21:47, Andy Shevchenko wrote:
> 
> On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
> <horatiu.vultur@microchip.com> wrote:
> >
> > 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.
> 
> 
> ...
> 
> > +       info->pincfg_data = devm_kmemdup(dev, &data->pincfg_data,
> > +                                        sizeof(struct ocelot_match_data),
> 
> sizeof(*info->pincfg_data)
> (isn't it a bug here?)

Yes it looks like it is. I think underneath it still allocates a page so
that could be the reason why I haven't see any crashes when I have tried
it.
I will fix this in the next version.

> 
> > +                                        GFP_KERNEL);
> 
> and missed the NULL check.
> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
/Horatiu

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

[Colin Foster]

On Mon, Jul 11, 2022 at 09:21:12PM +0200, Horatiu Vultur wrote:
> +
>  struct ocelot_pinctrl {
>  	struct device *dev;
>  	struct pinctrl_dev *pctl;
> @@ -328,10 +329,16 @@ struct ocelot_pinctrl {
>  	struct regmap *map;
>  	struct regmap *pincfg;
>  	struct pinctrl_desc *desc;
> +	struct ocelot_pincfg_data *pincfg_data;

I'm curious as to why desc and pincfg_data can't be const. [[ checks ]]
devm_pinctrl_register needs it - which is good enough for me.

But can pincfg_data be const? That way you could do:

const struct ocelot_pincfg_data *pincfg_data;

...

info->pincfg_data = &data->pincfg_data; 


No need for another devm_kmemdup here then:

> -	info->desc = (struct pinctrl_desc *)device_get_match_data(dev);
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return -EINVAL;
> +
> +	info->desc = devm_kmemdup(dev, &data->desc,
> +				  sizeof(struct pinctrl_desc), GFP_KERNEL);
> +	info->pincfg_data = devm_kmemdup(dev, &data->pincfg_data,
> +					 sizeof(struct ocelot_match_data),
> +					 GFP_KERNEL);
>  
>  	reset = devm_reset_control_get_optional_shared(dev, "switch");
>  	if (IS_ERR(reset))
> -- 
> 2.33.0
> 

[PATCH v3 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.
Another issue with the same commit is that it has a limit of 32 registers
which is incorrect. The sparx5 has 64 while lan966x has 77.

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

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 4edb36cfa161..b859e5caed09 100644
--- a/drivers/pinctrl/pinctrl-ocelot.c
+++ b/drivers/pinctrl/pinctrl-ocelot.c
@@ -1334,7 +1334,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;
 
@@ -1368,14 +1370,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;
 }
@@ -1935,7 +1941,8 @@ static const struct of_device_id ocelot_pinctrl_of_match[] = {
 	{},
 };
 
-static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
+static struct regmap *ocelot_pinctrl_create_pincfg(struct ocelot_pinctrl *info,
+						   struct platform_device *pdev)
 {
 	void __iomem *base;
 
@@ -1943,7 +1950,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 = info->desc->npins * 4,
 		.name = "pincfg",
 	};
 
@@ -2010,7 +2017,7 @@ static int ocelot_pinctrl_probe(struct platform_device *pdev)
 
 	/* Pinconf registers */
 	if (info->desc->confops) {
-		pincfg = ocelot_pinctrl_create_pincfg(pdev);
+		pincfg = ocelot_pinctrl_create_pincfg(info, pdev);
 		if (IS_ERR(pincfg))
 			dev_dbg(dev, "Failed to create pincfg regmap\n");
 		else
-- 
2.33.0

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
<horatiu.vultur@microchip.com> 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.
> Another issue with the same commit is that it has a limit of 32 registers
> which is incorrect. The sparx5 has 64 while lan966x has 77.

...

> -static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> +static struct regmap *ocelot_pinctrl_create_pincfg(struct ocelot_pinctrl *info,
> +                                                  struct platform_device *pdev)

const?

And I would leave pdev to be the first parameter, if there are no
other functions that have them like this.

-- 
With Best Regards,
Andy Shevchenko

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

[Horatiu Vultur]

The 07/11/2022 21:51, Andy Shevchenko wrote:
> 
> On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
> <horatiu.vultur@microchip.com> 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.
> > Another issue with the same commit is that it has a limit of 32 registers
> > which is incorrect. The sparx5 has 64 while lan966x has 77.
> 
> ...
> 
> > -static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> > +static struct regmap *ocelot_pinctrl_create_pincfg(struct ocelot_pinctrl *info,
> > +                                                  struct platform_device *pdev)
> 
> const?
> 
> And I would leave pdev to be the first parameter, if there are no
> other functions that have them like this.

I will do that in the next version.
Just for my understanding/knowledge why is this desire to have const or
to keep the const?

> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
/Horatiu

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 10:23 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
>
> The 07/11/2022 21:51, Andy Shevchenko wrote:
> >
> > On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
> > <horatiu.vultur@microchip.com> 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.
> > > Another issue with the same commit is that it has a limit of 32 registers
> > > which is incorrect. The sparx5 has 64 while lan966x has 77.
> >
> > ...
> >
> > > -static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> > > +static struct regmap *ocelot_pinctrl_create_pincfg(struct ocelot_pinctrl *info,
> > > +                                                  struct platform_device *pdev)
> >
> > const?
> >
> > And I would leave pdev to be the first parameter, if there are no
> > other functions that have them like this.
>
> I will do that in the next version.
> Just for my understanding/knowledge why is this desire to have const or
> to keep the const?

For non-POD types it's a good coding practice to reduce surface of
attack, if any (the data will be located in the pages with RO flag
set, and attempt to write will give you a page fault or other
exception, it depends on architecture).
Also a common sense, if you don't change data (which is actually
initial configuration or so), then why shouldn't you use const?
Note, in cases when it's not initial data, but runtime stuff (like
really run time), const is obviously either can't or not needed to be
used.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 10:29 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jul 11, 2022 at 10:23 PM Horatiu Vultur
> <horatiu.vultur@microchip.com> wrote:
> >
> > The 07/11/2022 21:51, Andy Shevchenko wrote:
> > >
> > > On Mon, Jul 11, 2022 at 9:17 PM Horatiu Vultur
> > > <horatiu.vultur@microchip.com> 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.
> > > > Another issue with the same commit is that it has a limit of 32 registers
> > > > which is incorrect. The sparx5 has 64 while lan966x has 77.
> > >
> > > ...
> > >
> > > > -static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
> > > > +static struct regmap *ocelot_pinctrl_create_pincfg(struct ocelot_pinctrl *info,
> > > > +                                                  struct platform_device *pdev)
> > >
> > > const?
> > >
> > > And I would leave pdev to be the first parameter, if there are no
> > > other functions that have them like this.
> >
> > I will do that in the next version.
> > Just for my understanding/knowledge why is this desire to have const or
> > to keep the const?
>
> For non-POD types it's a good coding practice to reduce surface of
> attack, if any (the data will be located in the pages with RO flag
> set, and attempt to write will give you a page fault or other
> exception, it depends on architecture).
> Also a common sense, if you don't change data (which is actually
> initial configuration or so), then why shouldn't you use const?
> Note, in cases when it's not initial data, but runtime stuff (like
> really run time), const is obviously either can't or not needed to be
> used.

One more specifically for drivers (related to the first item above),
it allows one to scope the point of failure in case of wrong
configuration comes in. The device might misbehave badly because of
some garbage somewhere. Also, the driver won't write data to that
area, which is just a good preventive programming practice (but this I
already implied by the second item above).


-- 
With Best Regards,
Andy Shevchenko