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

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

v4->v5:
- add Reviewed-by, Acked-by tags for patch 2
- clean the indentation mess inside ocelot_pinconf_set

v3->v4:
- add missing check for NULL pointer when doing devm_kmemdup
- remove devm_kmemdup for pincfg_data as it can be const
- remove has_schmitt field, because we can use schmitt_bit for checking
  if it has support for PIN_CONFIG_INPUT_SCHMITT_ENABLE
- change the declaration of ocelot_pinctrl_create_pincfg

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 | 214 ++++++++++++++++++++-----------
 1 file changed, 137 insertions(+), 77 deletions(-)

-- 
2.33.0

[PATCH v5 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 | 195 ++++++++++++++++++++-----------
 1 file changed, 124 insertions(+), 71 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
index 5f4a8c5c6650..c6e0232770b7 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,13 @@ 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;
+};
+
 struct ocelot_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
@@ -328,10 +328,16 @@ struct ocelot_pinctrl {
 	struct regmap *map;
 	struct regmap *pincfg;
 	struct pinctrl_desc *desc;
+	const 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,							\
@@ -1325,6 +1331,7 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
 	int ret = -EOPNOTSUPP;
 
 	if (info->pincfg) {
+		const struct ocelot_pincfg_data *opd = info->pincfg_data;
 		u32 regcfg;
 
 		ret = regmap_read(info->pincfg, pin, &regcfg);
@@ -1334,15 +1341,15 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
 		ret = 0;
 		switch (reg) {
 		case PINCONF_BIAS:
-			*val = regcfg & BIAS_BITS;
+			*val = regcfg & (opd->pd_bit | opd->pu_bit);
 			break;
 
 		case PINCONF_SCHMITT:
-			*val = regcfg & SCHMITT_BIT;
+			*val = regcfg & opd->schmitt_bit;
 			break;
 
 		case PINCONF_DRIVE_STRENGTH:
-			*val = regcfg & DRIVE_BITS;
+			*val = regcfg & opd->drive_bits;
 			break;
 
 		default:
@@ -1379,23 +1386,27 @@ static int ocelot_hw_set_value(struct ocelot_pinctrl *info,
 	int ret = -EOPNOTSUPP;
 
 	if (info->pincfg) {
+		const struct ocelot_pincfg_data *opd = info->pincfg_data;
 
 		ret = 0;
 		switch (reg) {
 		case PINCONF_BIAS:
-			ret = ocelot_pincfg_clrsetbits(info, pin, BIAS_BITS,
+			ret = ocelot_pincfg_clrsetbits(info, pin,
+						       opd->pd_bit | opd->pu_bit,
 						       val);
 			break;
 
 		case PINCONF_SCHMITT:
-			ret = ocelot_pincfg_clrsetbits(info, pin, SCHMITT_BIT,
+			ret = ocelot_pincfg_clrsetbits(info, pin,
+						       opd->schmitt_bit,
 						       val);
 			break;
 
 		case PINCONF_DRIVE_STRENGTH:
 			if (val <= 3)
 				ret = ocelot_pincfg_clrsetbits(info, pin,
-							       DRIVE_BITS, val);
+							       opd->drive_bits,
+							       val);
 			else
 				ret = -EINVAL;
 			break;
@@ -1425,17 +1436,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->schmitt_bit)
+			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:
@@ -1479,6 +1493,7 @@ static int ocelot_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			      unsigned long *configs, unsigned int num_configs)
 {
 	struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
+	const struct ocelot_pincfg_data *opd = info->pincfg_data;
 	u32 param, arg, p;
 	int cfg, err = 0;
 
@@ -1491,8 +1506,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) ?
+				opd->pu_bit : opd->pd_bit;
 
 			err = ocelot_hw_set_value(info, pin, PINCONF_BIAS, arg);
 			if (err)
@@ -1501,7 +1516,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 (!opd->schmitt_bit)
+				return -EOPNOTSUPP;
+
+			arg = arg ? opd->schmitt_bit : 0;
 			err = ocelot_hw_set_value(info, pin, PINCONF_SCHMITT,
 						  arg);
 			if (err)
@@ -1562,69 +1580,94 @@ 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),
+		.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 +1956,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 +1973,16 @@ 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(*info->desc),
+				  GFP_KERNEL);
+	if (!info->desc)
+		return -ENOMEM;
+
+	info->pincfg_data = &data->pincfg_data;
 
 	reset = devm_reset_control_get_optional_shared(dev, "switch");
 	if (IS_ERR(reset))
-- 
2.33.0

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

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 9:34 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.

I think there is still room for improvement (like refactoring all this
->get() and ->set() to avoid multiple parameter check, etc), but it is
out of scope and can be done later on. That said,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: 531d6ab36571 ("pinctrl: ocelot: Extend support for lan966x")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/pinctrl/pinctrl-ocelot.c | 195 ++++++++++++++++++++-----------
>  1 file changed, 124 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-ocelot.c b/drivers/pinctrl/pinctrl-ocelot.c
> index 5f4a8c5c6650..c6e0232770b7 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,13 @@ 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;
> +};
> +
>  struct ocelot_pinctrl {
>         struct device *dev;
>         struct pinctrl_dev *pctl;
> @@ -328,10 +328,16 @@ struct ocelot_pinctrl {
>         struct regmap *map;
>         struct regmap *pincfg;
>         struct pinctrl_desc *desc;
> +       const 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,                                                       \
> @@ -1325,6 +1331,7 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
>         int ret = -EOPNOTSUPP;
>
>         if (info->pincfg) {
> +               const struct ocelot_pincfg_data *opd = info->pincfg_data;
>                 u32 regcfg;
>
>                 ret = regmap_read(info->pincfg, pin, &regcfg);
> @@ -1334,15 +1341,15 @@ static int ocelot_hw_get_value(struct ocelot_pinctrl *info,
>                 ret = 0;
>                 switch (reg) {
>                 case PINCONF_BIAS:
> -                       *val = regcfg & BIAS_BITS;
> +                       *val = regcfg & (opd->pd_bit | opd->pu_bit);
>                         break;
>
>                 case PINCONF_SCHMITT:
> -                       *val = regcfg & SCHMITT_BIT;
> +                       *val = regcfg & opd->schmitt_bit;
>                         break;
>
>                 case PINCONF_DRIVE_STRENGTH:
> -                       *val = regcfg & DRIVE_BITS;
> +                       *val = regcfg & opd->drive_bits;
>                         break;
>
>                 default:
> @@ -1379,23 +1386,27 @@ static int ocelot_hw_set_value(struct ocelot_pinctrl *info,
>         int ret = -EOPNOTSUPP;
>
>         if (info->pincfg) {
> +               const struct ocelot_pincfg_data *opd = info->pincfg_data;
>
>                 ret = 0;
>                 switch (reg) {
>                 case PINCONF_BIAS:
> -                       ret = ocelot_pincfg_clrsetbits(info, pin, BIAS_BITS,
> +                       ret = ocelot_pincfg_clrsetbits(info, pin,
> +                                                      opd->pd_bit | opd->pu_bit,
>                                                        val);
>                         break;
>
>                 case PINCONF_SCHMITT:
> -                       ret = ocelot_pincfg_clrsetbits(info, pin, SCHMITT_BIT,
> +                       ret = ocelot_pincfg_clrsetbits(info, pin,
> +                                                      opd->schmitt_bit,
>                                                        val);
>                         break;
>
>                 case PINCONF_DRIVE_STRENGTH:
>                         if (val <= 3)
>                                 ret = ocelot_pincfg_clrsetbits(info, pin,
> -                                                              DRIVE_BITS, val);
> +                                                              opd->drive_bits,
> +                                                              val);
>                         else
>                                 ret = -EINVAL;
>                         break;
> @@ -1425,17 +1436,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->schmitt_bit)
> +                       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:
> @@ -1479,6 +1493,7 @@ static int ocelot_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>                               unsigned long *configs, unsigned int num_configs)
>  {
>         struct ocelot_pinctrl *info = pinctrl_dev_get_drvdata(pctldev);
> +       const struct ocelot_pincfg_data *opd = info->pincfg_data;
>         u32 param, arg, p;
>         int cfg, err = 0;
>
> @@ -1491,8 +1506,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) ?
> +                               opd->pu_bit : opd->pd_bit;
>
>                         err = ocelot_hw_set_value(info, pin, PINCONF_BIAS, arg);
>                         if (err)
> @@ -1501,7 +1516,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 (!opd->schmitt_bit)
> +                               return -EOPNOTSUPP;
> +
> +                       arg = arg ? opd->schmitt_bit : 0;
>                         err = ocelot_hw_set_value(info, pin, PINCONF_SCHMITT,
>                                                   arg);
>                         if (err)
> @@ -1562,69 +1580,94 @@ 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),
> +               .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 +1956,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 +1973,16 @@ 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(*info->desc),
> +                                 GFP_KERNEL);
> +       if (!info->desc)
> +               return -ENOMEM;
> +
> +       info->pincfg_data = &data->pincfg_data;
>
>         reset = devm_reset_control_get_optional_shared(dev, "switch");
>         if (IS_ERR(reset))
> --
> 2.33.0
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 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")
Acked-by: Colin Foster <colin.foster@in-advantage.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
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 c6e0232770b7..dfc8ea9f3843 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,
 		const struct ocelot_pincfg_data *opd = info->pincfg_data;
 		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;
 
@@ -1366,14 +1368,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;
 }
@@ -1933,7 +1939,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 platform_device *pdev,
+						   const struct ocelot_pinctrl *info)
 {
 	void __iomem *base;
 
@@ -1941,7 +1948,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",
 	};
 
@@ -2009,7 +2016,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(pdev, info);
 		if (IS_ERR(pincfg))
 			dev_dbg(dev, "Failed to create pincfg regmap\n");
 		else
-- 
2.33.0

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

[Linus Walleij]

On Wed, Jul 13, 2022 at 9:33 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:

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

v5 applied for fixes!

Thanks for fixing this Horatiu!

Thanks to Andy for the usual first-class review as well.

Yours,
Linus Walleij