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 <[email protected]>
---
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
On Fri, Jul 8, 2022 at 10:10 PM Horatiu Vultur
<[email protected]> 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
On Mon, Jul 11, 2022 at 8:45 AM Horatiu Vultur
<[email protected]> wrote:
> The 07/08/2022 23:58, Andy Shevchenko wrote:
> > On Fri, Jul 8, 2022 at 10:10 PM Horatiu Vultur
> > <[email protected]> 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