[PATCH V2 0/2] pinctrl: bcm2835: Implement pin_conf_get

[PATCH V2 0/2] pinctrl: bcm2835: Implement pin_conf_get

[Stefan Wahren]

For years, the Raspberry Pi users relied on userspace programs to read
the pin configuration. In the meantime, it has become apparent that this
approach has reached its limits for various reasons.

This patch series now attempts to improve the debugging possibilities on
the kernel side in order to reduce the dependency on these userspace
programs.

Changes in V2:
- adjust the BCM2711/7211 pull resistor value according to the Broadcom
  datasheet

Stefan Wahren (2):
  pinctrl: bcm2835: Implement bcm2835_pinconf_get
  pinctrl: bcm2835: Implement bcm2711_pinconf_get

 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 78 ++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 2 deletions(-)

--
2.34.1

[PATCH V2 1/2] pinctrl: bcm2835: Implement bcm2835_pinconf_get

[Stefan Wahren]

Even the driver already has implemented pin_dbg_show, it could
be helpful to implement pin_conf_get for a more generic behavior.
Contrary to the BCM2711, the BCM2835 SOC doesn't allow to read
the bias config, so the implementation is limited to the basics.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 35 ++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 1489191a213f..b37c86ec6915 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -1003,7 +1003,40 @@ static const struct pinmux_ops bcm2835_pmx_ops = {
 static int bcm2835_pinconf_get(struct pinctrl_dev *pctldev,
 			unsigned pin, unsigned long *config)
 {
-	/* No way to read back config in HW */
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
+	enum bcm2835_fsel fsel = bcm2835_pinctrl_fsel_get(pc, pin);
+	u32 val;
+
+	/* No way to read back bias config in HW */
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_ENABLE:
+		if (fsel != BCM2835_FSEL_GPIO_IN)
+			return -EINVAL;
+
+		*config = pinconf_to_config_packed(param, 1);
+		return 0;
+
+	case PIN_CONFIG_OUTPUT_ENABLE:
+		if (fsel != BCM2835_FSEL_GPIO_OUT)
+			return -EINVAL;
+
+		*config = pinconf_to_config_packed(param, 1);
+		return 0;
+
+	case PIN_CONFIG_OUTPUT:
+		if (fsel != BCM2835_FSEL_GPIO_OUT)
+			return -EINVAL;
+
+		val = bcm2835_gpio_get_bit(pc, GPLEV0, pin);
+		*config = pinconf_to_config_packed(param, val);
+		return 0;
+
+	default:
+		break;
+	}
+
 	return -ENOTSUPP;
 }

--
2.34.1

[PATCH V2 2/2] pinctrl: bcm2835: Implement bcm2711_pinconf_get

[Stefan Wahren]

The BCM2711 allows to read the bias config. So implement pin_conf_get
accordingly. The pull resistor values has been taken from the
BCM2711/7211 datasheet.

This implementation assumes that BCM7211 behaves the same way.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 43 ++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index b37c86ec6915..fcb9ab6729c0 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -1112,6 +1112,47 @@ static const struct pinconf_ops bcm2835_pinconf_ops = {
 	.pin_config_set = bcm2835_pinconf_set,
 };

+static int bcm2711_pinconf_get(struct pinctrl_dev *pctldev, unsigned pin,
+			       unsigned long *config)
+{
+	struct bcm2835_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 offset, shift, val;
+
+	offset = PUD_2711_REG_OFFSET(pin);
+	shift = PUD_2711_REG_SHIFT(pin);
+	val = bcm2835_gpio_rd(pc, GP_GPIO_PUP_PDN_CNTRL_REG0 + (offset * 4));
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_NONE)
+			return 0;
+		else
+			return -EINVAL;
+
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_UP) {
+			*config = pinconf_to_config_packed(param, 50000);
+			return 0;
+		} else {
+			return -EINVAL;
+		}
+
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_DOWN) {
+			*config = pinconf_to_config_packed(param, 50000);
+			return 0;
+		} else {
+			return -EINVAL;
+		}
+
+	default:
+		break;
+	}
+
+	return bcm2835_pinconf_get(pctldev, pin, config);
+}
+
 static void bcm2711_pull_config_set(struct bcm2835_pinctrl *pc,
 				    unsigned int pin, unsigned int arg)
 {
@@ -1179,7 +1220,7 @@ static int bcm2711_pinconf_set(struct pinctrl_dev *pctldev,

 static const struct pinconf_ops bcm2711_pinconf_ops = {
 	.is_generic = true,
-	.pin_config_get = bcm2835_pinconf_get,
+	.pin_config_get = bcm2711_pinconf_get,
 	.pin_config_set = bcm2711_pinconf_set,
 };

--
2.34.1

Re: [PATCH V2 1/2] pinctrl: bcm2835: Implement bcm2835_pinconf_get

[Andy Shevchenko]

Sat, Mar 02, 2024 at 10:54:29AM +0100, Stefan Wahren kirjoitti:
> Even the driver already has implemented pin_dbg_show, it could
> be helpful to implement pin_conf_get for a more generic behavior.
> Contrary to the BCM2711, the BCM2835 SOC doesn't allow to read
> the bias config, so the implementation is limited to the basics.

> +	switch (param) {
> +	case PIN_CONFIG_INPUT_ENABLE:
> +		if (fsel != BCM2835_FSEL_GPIO_IN)
> +			return -EINVAL;
> +
> +		*config = pinconf_to_config_packed(param, 1);

> +		return 0;

Either use break here (and in similar situations)...

> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		if (fsel != BCM2835_FSEL_GPIO_OUT)
> +			return -EINVAL;
> +
> +		*config = pinconf_to_config_packed(param, 1);
> +		return 0;
> +
> +	case PIN_CONFIG_OUTPUT:
> +		if (fsel != BCM2835_FSEL_GPIO_OUT)
> +			return -EINVAL;
> +
> +		val = bcm2835_gpio_get_bit(pc, GPLEV0, pin);
> +		*config = pinconf_to_config_packed(param, val);
> +		return 0;
> +
> +	default:
> +		break;

...and return from here directly.

> +	}
> +
>  	return -ENOTSUPP;

I.o.w. it's better to have a single point of returning a success code.

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 2/2] pinctrl: bcm2835: Implement bcm2711_pinconf_get

[Andy Shevchenko]

Sat, Mar 02, 2024 at 10:54:30AM +0100, Stefan Wahren kirjoitti:
> The BCM2711 allows to read the bias config. So implement pin_conf_get
> accordingly. The pull resistor values has been taken from the
> BCM2711/7211 datasheet.
> 
> This implementation assumes that BCM7211 behaves the same way.

...

> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_NONE)
> +			return 0;
> +		else
> +			return -EINVAL;

Redundant 'else' in all cases. Also as in previous patch, use single return point of success.

		if (((val >> shift) & PUD_2711_MASK) != BCM2711_PULL_NONE)
			return -EINVAL;
		break;

> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_UP) {
> +			*config = pinconf_to_config_packed(param, 50000);
> +			return 0;
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		if (((val >> shift) & PUD_2711_MASK) == BCM2711_PULL_DOWN) {
> +			*config = pinconf_to_config_packed(param, 50000);
> +			return 0;
> +		} else {
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		break;

Return directly from here.

		return bcm2835_pinconf_get(pctldev, pin, config);

> +	}
> +
> +	return bcm2835_pinconf_get(pctldev, pin, config);

	return 0;

> +}

-- 
With Best Regards,
Andy Shevchenko