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

[PATCH v2 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.

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 | 207 ++++++++++++++++++++-----------
 1 file changed, 132 insertions(+), 75 deletions(-)

-- 
2.33.0

[PATCH v2 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..10787056c5c7 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 = {
+	.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 = {
+		.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 = {
+	.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 = {
+		.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

[PATCH v2 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 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 | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 10787056c5c7..d88d6af71e46 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;
 }
@@ -1944,7 +1950,6 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
 		.reg_bits = 32,
 		.val_bits = 32,
 		.reg_stride = 4,
-		.max_register = 32,
 		.name = "pincfg",
 	};
 
-- 
2.33.0

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

[Colin Foster]

On Fri, Jul 08, 2022 at 09:55:10PM +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.
> Another issue with the same commit is that it 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 | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index 10787056c5c7..d88d6af71e46 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;
>  }
> @@ -1944,7 +1950,6 @@ static struct regmap *ocelot_pinctrl_create_pincfg(struct platform_device *pdev)
>  		.reg_bits = 32,
>  		.val_bits = 32,
>  		.reg_stride = 4,
> -		.max_register = 32,

What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
there's no max register?


Should it be this?

struct regmap_config regmap_config = {
    ...
};
regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;

>  		.name = "pincfg",
>  	};
>  
> -- 
> 2.33.0
> 

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

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 10:10 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.

...

> +struct ocelot_pincfg_data {
> +       bool has_schmitt;
> +       u8 schmitt_bit;
> +       u8 pd_bit;
> +       u8 pu_bit;
> +       u8 drive_bits;

I would go with mandatory fields first and leave optional (that is
with boolean flag) at last.

> +};

...

>  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;

It might waste too many bytes in some cases. I would recommend moving
it somewhere above, definitely before the u8 member.

> +};

Yes, I understand that for a certain architecture it might be the same
result in sizeof(), the rationale is to make code better in case
somebody copies'n'pastes pieces or ideas from it.

...

>                 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;

> +               val = (val & info->pincfg_data->schmitt_bit ? true : false);


!!(val & ...) will be a much shorter equivalent to ternary.

>                 break;

...

> +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,
> +       }

Please, keep a comma here. It's definitely not a terminating entry, so
it might help in the future.

Ditto for all cases like this.

>  };

...

> +       struct ocelot_match_data *data;

Any specific reason why this is not const?

...

> +       data = (struct ocelot_match_data *)device_get_match_data(dev);

And here you drop the qualifier...

I would recommend making it const and dropping the cast completely.

> +       if (!data)
> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 10:17 PM Colin Foster
<colin.foster@in-advantage.com> wrote:
> On Fri, Jul 08, 2022 at 09:55:10PM +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.
> > Another issue with the same commit is that it a limit of 32 registers

it has a limit

> > which is incorrect. The sparx5 has 64 while lan966x has 77.

...

> > -             .max_register = 32,
>
> What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
> there's no max register?

Good question!

> Should it be this?
>
> struct regmap_config regmap_config = {
>     ...
> };
> regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;
>
> >               .name = "pincfg",
> >       };

If regmap configuration may be const, I would prefer to have a
hardcoded value and different configuration based on the chip, but if
it's not feasible, then this could suffice.

-- 
With Best Regards,
Andy Shevchenko

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

[Horatiu Vultur]

The 07/08/2022 23:58, Andy Shevchenko wrote:

Hi Andy,

> 
> On Fri, Jul 8, 2022 at 10:10 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.
> 
> ...
> 
> > +struct ocelot_pincfg_data {
> > +       bool has_schmitt;
> > +       u8 schmitt_bit;
> > +       u8 pd_bit;
> > +       u8 pu_bit;
> > +       u8 drive_bits;
> 
> I would go with mandatory fields first and leave optional (that is
> with boolean flag) at last.
> 
> > +};
> 
> ...
> 
> >  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;
> 
> It might waste too many bytes in some cases. I would recommend moving
> it somewhere above, definitely before the u8 member.
> 
> > +};
> 
> Yes, I understand that for a certain architecture it might be the same
> result in sizeof(), the rationale is to make code better in case
> somebody copies'n'pastes pieces or ideas from it.
> 
> ...
> 
> >                 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;
> 
> > +               val = (val & info->pincfg_data->schmitt_bit ? true : false);
> 
> 
> !!(val & ...) will be a much shorter equivalent to ternary.
> 
> >                 break;
> 
> ...
> 
> > +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,
> > +       }
> 
> Please, keep a comma here. It's definitely not a terminating entry, so
> it might help in the future.
> 
> Ditto for all cases like this.
> 
> >  };
> 
> ...
> 
> > +       struct ocelot_match_data *data;
> 
> Any specific reason why this is not const?
> 
> ...
> 
> > +       data = (struct ocelot_match_data *)device_get_match_data(dev);
> 
> And here you drop the qualifier...
> 
> I would recommend making it const and dropping the cast completely.

If I make this const, but then few lines after I will get the following
warnings:

drivers/pinctrl/pinctrl-ocelot.c:1983:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1983 |  info->desc = &data->desc;
     |             ^
drivers/pinctrl/pinctrl-ocelot.c:1984:20: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1984 |  info->pincfg_data = &data->pincfg_data;
     |                    ^

Of course I can make also info->desc and info->pincfg_data const but
then I will get the following warning:

drivers/pinctrl/pinctrl-ocelot.c: In function ‘ocelot_pinctrl_register’:
drivers/pinctrl/pinctrl-ocelot.c:1723:53: warning: passing argument 2 of ‘devm_pinctrl_register’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
1723 |  info->pctl = devm_pinctrl_register(&pdev->dev, info->desc, info);
     |                                                 ~~~~^~~~~~
In file included from include/linux/gpio/driver.h:10,
                 from drivers/pinctrl/pinctrl-ocelot.c:10:
                      include/linux/pinctrl/pinctrl.h:166:26: note: expected ‘struct pinctrl_desc *’ but argument is of type ‘const struct pinctrl_desc *’
166 |     struct pinctrl_desc *pctldesc,
    |     ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

> 
> > +       if (!data)
> > +               return -EINVAL;
> 
> --
> With Best Regards,
> Andy Shevchenko

-- 
/Horatiu

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

[Horatiu Vultur]

The 07/09/2022 00:02, Andy Shevchenko wrote:

Hi Colin, Andy,

> 
> On Fri, Jul 8, 2022 at 10:17 PM Colin Foster
> <colin.foster@in-advantage.com> wrote:
> > On Fri, Jul 08, 2022 at 09:55:10PM +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.
> > > Another issue with the same commit is that it a limit of 32 registers
> 
> it has a limit
> 
> > > which is incorrect. The sparx5 has 64 while lan966x has 77.
> 
> ...
> 
> > > -             .max_register = 32,
> >
> > What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
> > there's no max register?
> 
> Good question!

If .max_register is missing then I got the following:

# cd /sys/kernel/debug/regmap/e2004064.pinctrl-pincfg/
# cat range
0-0
# cat registers
0: 00000005

> 
> > Should it be this?
> >
> > struct regmap_config regmap_config = {
> >     ...
> > };
> > regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;
> >
> > >               .name = "pincfg",
> > >       };
> 
> If regmap configuration may be const, I would prefer to have a
> hardcoded value and different configuration based on the chip, but if
> it's not feasible, then this could suffice.

What about if we do something like:

const struct regmap_config regmap_config = {
    ...
    .max_register = info->desc->npins * 4,
    ...
};

This is based on what Colin suggested only that we keep the const.

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

-- 
/Horatiu

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 8:45 AM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
> The 07/08/2022 23:58, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 10:10 PM Horatiu Vultur
> > <horatiu.vultur@microchip.com> wrote:

Please, remove unneeded context when replying!

...

> > > +       struct ocelot_match_data *data;
> >
> > Any specific reason why this is not const?
> >
> > > +       data = (struct ocelot_match_data *)device_get_match_data(dev);
> >
> > And here you drop the qualifier...
> >
> > I would recommend making it const and dropping the cast completely.
>
> If I make this const, but then few lines after I will get the following
> warnings:
>
> drivers/pinctrl/pinctrl-ocelot.c:1983:13: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 1983 |  info->desc = &data->desc;
>      |             ^
> drivers/pinctrl/pinctrl-ocelot.c:1984:20: warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 1984 |  info->pincfg_data = &data->pincfg_data;
>      |                    ^
>
> Of course I can make also info->desc and info->pincfg_data const but
> then I will get the following warning:
>
> drivers/pinctrl/pinctrl-ocelot.c: In function ‘ocelot_pinctrl_register’:
> drivers/pinctrl/pinctrl-ocelot.c:1723:53: warning: passing argument 2 of ‘devm_pinctrl_register’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 1723 |  info->pctl = devm_pinctrl_register(&pdev->dev, info->desc, info);
>      |                                                 ~~~~^~~~~~
> In file included from include/linux/gpio/driver.h:10,
>                  from drivers/pinctrl/pinctrl-ocelot.c:10:
>                       include/linux/pinctrl/pinctrl.h:166:26: note: expected ‘struct pinctrl_desc *’ but argument is of type ‘const struct pinctrl_desc *’
> 166 |     struct pinctrl_desc *pctldesc,
>     |     ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
>
> > > +       if (!data)
> > > +               return -EINVAL;

This is not good. Any chances to make it const?

One way is to copy data in a newly allocated buffer (devm_kmemdup() for that).

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Jul 11, 2022 at 8:51 AM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
> The 07/09/2022 00:02, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 10:17 PM Colin Foster
> > <colin.foster@in-advantage.com> wrote:
> > > On Fri, Jul 08, 2022 at 09:55:10PM +0200, Horatiu Vultur wrote:

Please, remove unneeded context when replying!

...

> > > > -             .max_register = 32,
> > >
> > > What happens in /sys/kernel/debug/regmap/*-pincfg/{range,registers} when
> > > there's no max register?
> >
> > Good question!
>
> If .max_register is missing then I got the following:
>
> # cd /sys/kernel/debug/regmap/e2004064.pinctrl-pincfg/
> # cat range
> 0-0
> # cat registers
> 0: 00000005

This is effectively a regression.

> > > Should it be this?
> > >
> > > struct regmap_config regmap_config = {
> > >     ...
> > > };
> > > regmap_config.max_register = info->desc->npins * regmap_config.reg_stride;
> > >
> > > >               .name = "pincfg",
> > > >       };
> >
> > If regmap configuration may be const, I would prefer to have a
> > hardcoded value and different configuration based on the chip, but if
> > it's not feasible, then this could suffice.
>
> What about if we do something like:
>
> const struct regmap_config regmap_config = {
>     ...
>     .max_register = info->desc->npins * 4,
>     ...
> };
>
> This is based on what Colin suggested only that we keep the const.

As long as it's const, I like it.

-- 
With Best Regards,
Andy Shevchenko