[PATCH v2 0/4] Add drive-strength in Meson pinctrl driver

[PATCH v2 0/4] Add drive-strength in Meson pinctrl driver

[Guillaume La Roque]

The purpose of this patchset is to add drive-strength support in meson pinconf
driver. This is a new feature that was added on the g12a. It is critical for us
to support this since many functions are failing with default pad drive-strength.

The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
'drive-strength' is expressed in mA.
So this patch add another generic property "drive-strength-uA". The change to do so
would be minimal and could be benefit to other platforms later on.

Cheers
Guillaume

Changes since v1:
- fix missing break
- implement new pinctrl generic property "drive-strength-uA"

[1] https://lkml.kernel.org/r/20190314163725.7918-1-jbrunet@baylibre.com

Guillaume La Roque (4):
  dt-bindings: pinctrl: add a 'drive-strength-uA' property
  pinctrl: generic: add new 'drive-strength-uA' property support
  dt-bindings: pinctrl: meson: Add drive-strength-uA property
  pinctrl: meson: add support of drive-strength-uA

 .../bindings/pinctrl/meson,pinctrl.txt        |   3 +
 .../bindings/pinctrl/pinctrl-bindings.txt     |   3 +
 drivers/pinctrl/meson/pinctrl-meson-g12a.c    |  36 ++--
 drivers/pinctrl/meson/pinctrl-meson.c         | 166 ++++++++++++++----
 drivers/pinctrl/meson/pinctrl-meson.h         |  20 ++-
 drivers/pinctrl/pinconf-generic.c             |   2 +
 include/linux/pinctrl/pinconf-generic.h       |   3 +
 7 files changed, 174 insertions(+), 59 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[Guillaume La Roque]

This property allow drive-strength parameter in uA instead of mA.

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index cef2b5855d60..fc7018459aa2 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -258,6 +258,7 @@ drive-push-pull		- drive actively high and low
 drive-open-drain	- drive with open drain
 drive-open-source	- drive with open source
 drive-strength		- sink or source at most X mA
+drive-strength-uA	- sink or source at most X uA
 input-enable		- enable input on pin (no effect on output, such as
 			  enabling an input buffer)
 input-disable		- disable input on pin (no effect on output, such as
@@ -326,6 +327,8 @@ arguments are described below.
 
 - drive-strength takes as argument the target strength in mA.
 
+- drive-strength-uA takes as argument the target strength in uA.
+
 - input-debounce takes the debounce time in usec as argument
   or 0 to disable debouncing
 
-- 
2.17.1

Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[Linus Walleij]

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:

> This property allow drive-strength parameter in uA instead of mA.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>

I'm fine with this but I need the DT maintainers to ACK it explicitly.

Yours,
Linus Walleij

Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[Martin Blumenstingl]

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> This property allow drive-strength parameter in uA instead of mA.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
personally I'm happy with this if the DT maintainers give their ACK.
based on that:
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[Kevin Hilman]

Rob,

Guillaume La Roque <glaroque@baylibre.com> writes:

> This property allow drive-strength parameter in uA instead of mA.
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index cef2b5855d60..fc7018459aa2 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -258,6 +258,7 @@ drive-push-pull		- drive actively high and low
>  drive-open-drain	- drive with open drain
>  drive-open-source	- drive with open source
>  drive-strength		- sink or source at most X mA
> +drive-strength-uA	- sink or source at most X uA
>  input-enable		- enable input on pin (no effect on output, such as
>  			  enabling an input buffer)
>  input-disable		- disable input on pin (no effect on output, such as
> @@ -326,6 +327,8 @@ arguments are described below.
>  
>  - drive-strength takes as argument the target strength in mA.
>  
> +- drive-strength-uA takes as argument the target strength in uA.
> +
>  - input-debounce takes the debounce time in usec as argument
>    or 0 to disable debouncing

Can we get your input on this?

Linus W. is OK with this[1], but wants opinion/approval from DT
maintainers first.

Thanks,

Kevin

[1] https://lore.kernel.org/lkml/CACRpkdZ2dPzrtJQkxmN7V=f6+qYZAvrF+b0J77cN9hoRAgFqrw@mail.gmail.com/T/#u

Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[Rob Herring]

On Thu, Apr 18, 2019 at 02:47:55PM +0200, Guillaume La Roque wrote:
> This property allow drive-strength parameter in uA instead of mA.
> 
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> index cef2b5855d60..fc7018459aa2 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> @@ -258,6 +258,7 @@ drive-push-pull		- drive actively high and low
>  drive-open-drain	- drive with open drain
>  drive-open-source	- drive with open source
>  drive-strength		- sink or source at most X mA
> +drive-strength-uA	- sink or source at most X uA
>  input-enable		- enable input on pin (no effect on output, such as
>  			  enabling an input buffer)
>  input-disable		- disable input on pin (no effect on output, such as
> @@ -326,6 +327,8 @@ arguments are described below.
>  
>  - drive-strength takes as argument the target strength in mA.
>  
> +- drive-strength-uA takes as argument the target strength in uA.
> +

We have standard unit suffixes defined in bindings/property-units.txt. 
Use them please.

Rob

Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property

[guillaume La Roque]

Hi Rob,


On 4/30/19 5:12 PM, Rob Herring wrote:
> On Thu, Apr 18, 2019 at 02:47:55PM +0200, Guillaume La Roque wrote:
>> This property allow drive-strength parameter in uA instead of mA.
>>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> index cef2b5855d60..fc7018459aa2 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> @@ -258,6 +258,7 @@ drive-push-pull		- drive actively high and low
>>  drive-open-drain	- drive with open drain
>>  drive-open-source	- drive with open source
>>  drive-strength		- sink or source at most X mA
>> +drive-strength-uA	- sink or source at most X uA
>>  input-enable		- enable input on pin (no effect on output, such as
>>  			  enabling an input buffer)
>>  input-disable		- disable input on pin (no effect on output, such as
>> @@ -326,6 +327,8 @@ arguments are described below.
>>  
>>  - drive-strength takes as argument the target strength in mA.
>>  
>> +- drive-strength-uA takes as argument the target strength in uA.
>> +
> We have standard unit suffixes defined in bindings/property-units.txt. 
> Use them please.


thanks for your feedback and sorry i don't see this doc.

According to it i will update patch series with drive-strength-microamp


> Rob
Guillaume

[PATCH v2 2/4] pinctrl: generic: add new 'drive-strength-uA' property support

[Guillaume La Roque]

Add drive-strength-uA property support to allow drive strength in uA

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 drivers/pinctrl/pinconf-generic.c       | 2 ++
 include/linux/pinctrl/pinconf-generic.h | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index b4f7f8a458ea..87393e9452b4 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -39,6 +39,7 @@ static const struct pin_config_item conf_items[] = {
 	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
 	PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
 	PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH, "output drive strength", "mA", true),
+	PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH_UA, "output drive strength", "uA", true),
 	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "usec", true),
 	PCONFDUMP(PIN_CONFIG_INPUT_ENABLE, "input enabled", NULL, false),
 	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL, false),
@@ -167,6 +168,7 @@ static const struct pinconf_generic_params dt_params[] = {
 	{ "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 },
 	{ "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 },
 	{ "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 },
+	{ "drive-strength-uA", PIN_CONFIG_DRIVE_STRENGTH_UA, 0 },
 	{ "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 },
 	{ "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 },
 	{ "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 },
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 6c0680641108..72d06d6a3099 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -55,6 +55,8 @@
  *	push-pull mode, the argument is ignored.
  * @PIN_CONFIG_DRIVE_STRENGTH: the pin will sink or source at most the current
  *	passed as argument. The argument is in mA.
+ * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current
+ *	passed as argument. The argument is in uA.
  * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
  *	which means it will wait for signals to settle when reading inputs. The
  *	argument gives the debounce time in usecs. Setting the
@@ -112,6 +114,7 @@ enum pin_config_param {
 	PIN_CONFIG_DRIVE_OPEN_SOURCE,
 	PIN_CONFIG_DRIVE_PUSH_PULL,
 	PIN_CONFIG_DRIVE_STRENGTH,
+	PIN_CONFIG_DRIVE_STRENGTH_UA,
 	PIN_CONFIG_INPUT_DEBOUNCE,
 	PIN_CONFIG_INPUT_ENABLE,
 	PIN_CONFIG_INPUT_SCHMITT,
-- 
2.17.1

[PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property

[Guillaume La Roque]

Add optional drive-strength-uA property

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
index a47dd990a8d3..b3e4be696ddc 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable",
 "bias-pull-up" and "bias-pull-down", described in file
 pinctrl-bindings.txt
 
+Optional properties :
+ - drive-strength-uA: Drive strength for the specified pins in uA.
+
 === Example ===
 
 	pinctrl: pinctrl@c1109880 {
-- 
2.17.1

Re: [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property

[Martin Blumenstingl]

Hi Guillaume,

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Add optional drive-strength-uA property
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
> index a47dd990a8d3..b3e4be696ddc 100644
> --- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
> @@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable",
>  "bias-pull-up" and "bias-pull-down", described in file
>  pinctrl-bindings.txt
>
> +Optional properties :
> + - drive-strength-uA: Drive strength for the specified pins in uA.
if you have to re-send this series for whatever reason then please
mention that drive-strength-uA is only valid for G12A and newer

otherwise:
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Re: [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property

[guillaume La Roque]

Hi Martin,

On 4/27/19 9:21 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Add optional drive-strength-uA property
>>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>> ---
>>  Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
>> index a47dd990a8d3..b3e4be696ddc 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
>> @@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable",
>>  "bias-pull-up" and "bias-pull-down", described in file
>>  pinctrl-bindings.txt
>>
>> +Optional properties :
>> + - drive-strength-uA: Drive strength for the specified pins in uA.
> if you have to re-send this series for whatever reason then please
> mention that drive-strength-uA is only valid for G12A and newer

thanks for your review, i will do if i send new series.


> otherwise:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

[PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA

[Guillaume La Roque]

drive-strength-uA is a new feature needed for G12A SoC.
the default DS setting after boot is usually 500uA and it is not enough for
many functions. We need to be able to set the drive strength to reliably
enable things like MMC, I2C, etc ...

Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
---
 drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
 drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
 drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
 3 files changed, 163 insertions(+), 59 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
index d494492e98e9..3475cd7bd2af 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
@@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = {
 };
 
 static struct meson_bank meson_g12a_periphs_banks[] = {
-	/* name  first  last  irq  pullen  pull  dir  out  in */
-	BANK("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
-	     4,  0,  4,  0,  12,  0,  13, 0,  14, 0),
-	BANK("H",    GPIOH_0,    GPIOH_8, 28, 36,
-	     3,  0,  3,  0,  9,  0,  10,  0,  11,  0),
-	BANK("BOOT", BOOT_0,     BOOT_15,  37, 52,
-	     0,  0,  0,  0,  0, 0,  1, 0,  2, 0),
-	BANK("C",    GPIOC_0,    GPIOC_7,  53, 60,
-	     1,  0,  1,  0,  3, 0,  4, 0,  5, 0),
-	BANK("A",    GPIOA_0,    GPIOA_15,  61, 76,
-	     5,  0,  5,  0,  16,  0,  17,  0,  18,  0),
-	BANK("X",    GPIOX_0,    GPIOX_19,   77, 96,
-	     2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
+	/* name  first  last  irq  pullen  pull  dir  out  in  ds */
+	BANK_DS("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
+		4,  0,  4,  0,  12,  0,  13, 0,  14, 0, 5, 0),
+	BANK_DS("H",    GPIOH_0,    GPIOH_8, 28, 36,
+		3,  0,  3,  0,  9,  0,  10,  0,  11,  0, 4, 0),
+	BANK_DS("BOOT", BOOT_0,     BOOT_15,  37, 52,
+		0,  0,  0,  0,  0, 0,  1, 0,  2, 0, 0, 0),
+	BANK_DS("C",    GPIOC_0,    GPIOC_7,  53, 60,
+		1,  0,  1,  0,  3, 0,  4, 0,  5, 0, 1, 0),
+	BANK_DS("A",    GPIOA_0,    GPIOA_15,  61, 76,
+		5,  0,  5,  0,  16,  0,  17,  0,  18,  0, 6, 0),
+	BANK_DS("X",    GPIOX_0,    GPIOX_19,   77, 96,
+		2,  0,  2,  0,  6,  0,  7,  0,  8,  0, 2, 0),
 };
 
 static struct meson_bank meson_g12a_aobus_banks[] = {
-	/* name  first  last  irq  pullen  pull  dir  out  in  */
-	BANK("AO",   GPIOAO_0,  GPIOAO_11,  0, 11,
-	     3,  0,  2, 0,  0,  0,  4, 0,  1,  0),
+	/* name  first  last  irq  pullen  pull  dir  out  in  ds */
+	BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0,
+		0, 0),
 	/* GPIOE actually located in the AO bank */
-	BANK("E",   GPIOE_0,  GPIOE_2,   97, 99,
-	     3,  16,  2, 16,  0,  16,  4, 16,  1,  16),
+	BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1,
+		16, 1, 0),
 };
 
 static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = {
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 96a4a72708e4..5108e5aa6514 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
 	return 0;
 }
 
-static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
-			     unsigned long *configs, unsigned num_configs)
+static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
+				  enum pin_config_param conf)
+{
+	struct meson_bank *bank;
+	unsigned int reg, bit, val = 0;
+	int ret;
+
+	ret = meson_get_bank(pc, pin, &bank);
+	if (ret)
+		return ret;
+
+	meson_calc_reg_and_bit(bank, pin, REG_PULLEN, &reg, &bit);
+
+	if (conf == PIN_CONFIG_BIAS_DISABLE) {
+		ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit),	0);
+		if (ret)
+			return ret;
+	} else {
+		meson_calc_reg_and_bit(bank, pin, REG_PULL, &reg, &bit);
+		if (conf == PIN_CONFIG_BIAS_PULL_UP)
+			val = BIT(bit);
+
+		ret = regmap_update_bits(pc->reg_pull, reg, BIT(bit), val);
+		if (ret)
+			return ret;
+
+		meson_calc_reg_and_bit(bank, pin, REG_PULLEN, &reg, &bit);
+		ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit),
+					 BIT(bit));
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
+					    unsigned int pin, u16 arg)
 {
-	struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
 	struct meson_bank *bank;
-	enum pin_config_param param;
 	unsigned int reg, bit;
-	int i, ret;
+	unsigned int ds_val;
+	int ret;
+
+	if (!pc->reg_ds) {
+		dev_err(pc->dev, "drive-strength not supported\n");
+		return -ENOTSUPP;
+	}
 
 	ret = meson_get_bank(pc, pin, &bank);
 	if (ret)
 		return ret;
 
+	meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
+	bit = bit << 1;
+
+	if (arg <= 500) {
+		ds_val = MESON_PINCONF_DRV_500UA;
+	} else if (arg <= 2500) {
+		ds_val = MESON_PINCONF_DRV_2500UA;
+	} else if (arg <= 3000) {
+		ds_val = MESON_PINCONF_DRV_3000UA;
+	} else if (arg <= 4000) {
+		ds_val = MESON_PINCONF_DRV_4000UA;
+	} else {
+		dev_warn_once(pc->dev,
+			      "pin %u: invalid drive-strength : %d , default to 4mA\n",
+			      pin, arg);
+		ds_val = MESON_PINCONF_DRV_4000UA;
+	}
+
+	ret = regmap_update_bits(pc->reg_ds, reg, 0x3 << bit, ds_val << bit);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
+			     unsigned long *configs, unsigned num_configs)
+{
+	struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
+	enum pin_config_param param;
+	unsigned int arg;
+	int i, ret;
+
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(configs[i]);
 
 		switch (param) {
 		case PIN_CONFIG_BIAS_DISABLE:
-			dev_dbg(pc->dev, "pin %u: disable bias\n", pin);
-
-			meson_calc_reg_and_bit(bank, pin, REG_PULLEN, &reg,
-					       &bit);
-			ret = regmap_update_bits(pc->reg_pullen, reg,
-						 BIT(bit), 0);
-			if (ret)
-				return ret;
-			break;
 		case PIN_CONFIG_BIAS_PULL_UP:
-			dev_dbg(pc->dev, "pin %u: enable pull-up\n", pin);
-
-			meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
-					       &reg, &bit);
-			ret = regmap_update_bits(pc->reg_pullen, reg,
-						 BIT(bit), BIT(bit));
-			if (ret)
-				return ret;
-
-			meson_calc_reg_and_bit(bank, pin, REG_PULL, &reg, &bit);
-			ret = regmap_update_bits(pc->reg_pull, reg,
-						 BIT(bit), BIT(bit));
-			if (ret)
-				return ret;
-			break;
 		case PIN_CONFIG_BIAS_PULL_DOWN:
-			dev_dbg(pc->dev, "pin %u: enable pull-down\n", pin);
-
-			meson_calc_reg_and_bit(bank, pin, REG_PULLEN,
-					       &reg, &bit);
-			ret = regmap_update_bits(pc->reg_pullen, reg,
-						 BIT(bit), BIT(bit));
+			ret = meson_pinconf_set_bias(pc, pin, param);
 			if (ret)
 				return ret;
-
-			meson_calc_reg_and_bit(bank, pin, REG_PULL, &reg, &bit);
-			ret = regmap_update_bits(pc->reg_pull, reg,
-						 BIT(bit), 0);
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH_UA:
+			arg = pinconf_to_config_argument(configs[i]);
+			ret = meson_pinconf_set_drive_strength(pc, pin, arg);
 			if (ret)
 				return ret;
 			break;
@@ -275,12 +319,51 @@ static int meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
 	return conf;
 }
 
+static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
+					    unsigned int pin, u16 *arg)
+{
+	struct meson_bank *bank;
+	unsigned int reg, bit;
+	unsigned int val;
+	int ret;
+
+	ret = meson_get_bank(pc, pin, &bank);
+	if (ret)
+		return ret;
+
+	meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
+
+	ret = regmap_read(pc->reg_ds, reg, &val);
+	if (ret)
+		return ret;
+
+	switch ((val >> bit) & 0x3) {
+	case MESON_PINCONF_DRV_500UA:
+		*arg = 500;
+		break;
+	case MESON_PINCONF_DRV_2500UA:
+		*arg = 2500;
+		break;
+	case MESON_PINCONF_DRV_3000UA:
+		*arg = 3000;
+		break;
+	case MESON_PINCONF_DRV_4000UA:
+		*arg = 4000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int meson_pinconf_get(struct pinctrl_dev *pcdev, unsigned int pin,
 			     unsigned long *config)
 {
 	struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
 	enum pin_config_param param = pinconf_to_config_param(*config);
 	u16 arg;
+	int ret;
 
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
@@ -291,6 +374,11 @@ static int meson_pinconf_get(struct pinctrl_dev *pcdev, unsigned int pin,
 		else
 			return -EINVAL;
 		break;
+	case PIN_CONFIG_DRIVE_STRENGTH_UA:
+		ret = meson_pinconf_get_drive_strength(pc, pin, &arg);
+		if (ret)
+			return ret;
+		break;
 	default:
 		return -ENOTSUPP;
 	}
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 5eaab925f427..1a88103dcb9b 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -71,9 +71,20 @@ enum meson_reg_type {
 	REG_DIR,
 	REG_OUT,
 	REG_IN,
+	REG_DS,
 	NUM_REG,
 };
 
+/**
+ * enum meson_pinconf_drv - value of drive-strength supported
+ */
+enum meson_pinconf_drv {
+	MESON_PINCONF_DRV_500UA,
+	MESON_PINCONF_DRV_2500UA,
+	MESON_PINCONF_DRV_3000UA,
+	MESON_PINCONF_DRV_4000UA,
+};
+
 /**
  * struct meson bank
  *
@@ -132,7 +143,8 @@ struct meson_pinctrl {
 		.num_groups = ARRAY_SIZE(fn ## _groups),		\
 	}
 
-#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib)	\
+#define BANK_DS(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib,     \
+		dsr, dsb)                                                      \
 	{								\
 		.name		= n,					\
 		.first		= f,					\
@@ -145,8 +157,12 @@ struct meson_pinctrl {
 			[REG_DIR]	= { dr, db },			\
 			[REG_OUT]	= { or, ob },			\
 			[REG_IN]	= { ir, ib },			\
+			[REG_DS]	= { dsr, dsb },			\
 		},							\
-	 }
+	}
+
+#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib) \
+	BANK_DS(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib, 0, 0)
 
 #define MESON_PIN(x) PINCTRL_PIN(x, #x)
 
-- 
2.17.1

Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA

[Martin Blumenstingl]

Hi Guillaume,

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> drive-strength-uA is a new feature needed for G12A SoC.
> the default DS setting after boot is usually 500uA and it is not enough for
> many functions. We need to be able to set the drive strength to reliably
> enable things like MMC, I2C, etc ...
>
> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
I gave this a go on Meson8m2 (meaning I applied all four patches from
this series and booted the result on my board):
[Meson8m2 doesn't support drive strength and still boots without any
crashes or obvious regressions]
Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

> ---
>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
personally I would have split this into two separate patches:
- one for the generic pinctrl-meson part which adds drive-strength-uA support
- another patch for enabling this on G12A

if nobody else wants you to split this then it's fine for me as well

>  3 files changed, 163 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> index d494492e98e9..3475cd7bd2af 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
> @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = {
>  };
>
>  static struct meson_bank meson_g12a_periphs_banks[] = {
> -       /* name  first  last  irq  pullen  pull  dir  out  in */
> -       BANK("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
> -            4,  0,  4,  0,  12,  0,  13, 0,  14, 0),
> -       BANK("H",    GPIOH_0,    GPIOH_8, 28, 36,
> -            3,  0,  3,  0,  9,  0,  10,  0,  11,  0),
> -       BANK("BOOT", BOOT_0,     BOOT_15,  37, 52,
> -            0,  0,  0,  0,  0, 0,  1, 0,  2, 0),
> -       BANK("C",    GPIOC_0,    GPIOC_7,  53, 60,
> -            1,  0,  1,  0,  3, 0,  4, 0,  5, 0),
> -       BANK("A",    GPIOA_0,    GPIOA_15,  61, 76,
> -            5,  0,  5,  0,  16,  0,  17,  0,  18,  0),
> -       BANK("X",    GPIOX_0,    GPIOX_19,   77, 96,
> -            2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
> +       BANK_DS("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
> +               4,  0,  4,  0,  12,  0,  13, 0,  14, 0, 5, 0),
> +       BANK_DS("H",    GPIOH_0,    GPIOH_8, 28, 36,
> +               3,  0,  3,  0,  9,  0,  10,  0,  11,  0, 4, 0),
> +       BANK_DS("BOOT", BOOT_0,     BOOT_15,  37, 52,
> +               0,  0,  0,  0,  0, 0,  1, 0,  2, 0, 0, 0),
> +       BANK_DS("C",    GPIOC_0,    GPIOC_7,  53, 60,
> +               1,  0,  1,  0,  3, 0,  4, 0,  5, 0, 1, 0),
> +       BANK_DS("A",    GPIOA_0,    GPIOA_15,  61, 76,
> +               5,  0,  5,  0,  16,  0,  17,  0,  18,  0, 6, 0),
> +       BANK_DS("X",    GPIOX_0,    GPIOX_19,   77, 96,
> +               2,  0,  2,  0,  6,  0,  7,  0,  8,  0, 2, 0),
>  };
>
>  static struct meson_bank meson_g12a_aobus_banks[] = {
> -       /* name  first  last  irq  pullen  pull  dir  out  in  */
> -       BANK("AO",   GPIOAO_0,  GPIOAO_11,  0, 11,
> -            3,  0,  2, 0,  0,  0,  4, 0,  1,  0),
> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
> +       BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0,
> +               0, 0),
>         /* GPIOE actually located in the AO bank */
> -       BANK("E",   GPIOE_0,  GPIOE_2,   97, 99,
> -            3,  16,  2, 16,  0,  16,  4, 16,  1,  16),
> +       BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1,
> +               16, 1, 0),
>  };
these definitions are really hard to read, but it's been like this
even before your patch

>  static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = {
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 96a4a72708e4..5108e5aa6514 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
>         return 0;
>  }
>
> -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
> -                            unsigned long *configs, unsigned num_configs)
> +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
> +                                 enum pin_config_param conf)
can you please confirm that I understood the purpose of this correctly:
I think you introduce this to make setting the bias consistent with
how you set the drive-strength.
if so then it would be great to have a separate patch which describes
that it's only a code-style change and a functional no-op

additionally the function arguments are not consistent with
meson_pinconf_get_drive_strength():
- here you pass the pinctrl subsystem specific parameters (enum
pin_config_param conf)
- in meson_pinconf_get_drive_strength the conversion for pinctrl
subsystem specific values (pinconf_to_config_argument) is part of
meson_pinconf_set
I'm wondering whether two separate functions
(meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
things easier to read. I haven't tried whether this would really make
things better, so I'd like to hear your opinion on this Guillaume!

[...]
> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
> +                                           unsigned int pin, u16 arg)
>  {
> -       struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
>         struct meson_bank *bank;
> -       enum pin_config_param param;
>         unsigned int reg, bit;
> -       int i, ret;
> +       unsigned int ds_val;
> +       int ret;
> +
> +       if (!pc->reg_ds) {
> +               dev_err(pc->dev, "drive-strength not supported\n");
> +               return -ENOTSUPP;
in meson_pinconf_set() we don't complain (with a dev_err) for this case.
I'm not sure what the best-practice is for the pinctrl subsystem,
maybe Linus can comment on this

> +       }
>
>         ret = meson_get_bank(pc, pin, &bank);
>         if (ret)
>                 return ret;
>
> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
> +       bit = bit << 1;
> +
> +       if (arg <= 500) {
> +               ds_val = MESON_PINCONF_DRV_500UA;
> +       } else if (arg <= 2500) {
> +               ds_val = MESON_PINCONF_DRV_2500UA;
> +       } else if (arg <= 3000) {
> +               ds_val = MESON_PINCONF_DRV_3000UA;
> +       } else if (arg <= 4000) {
> +               ds_val = MESON_PINCONF_DRV_4000UA;
> +       } else {
> +               dev_warn_once(pc->dev,
> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
> +                             pin, arg);
> +               ds_val = MESON_PINCONF_DRV_4000UA;
why not return -EINVAL here? (my assumption is that the pinctrl
subsystem would like to have -EINVAL instead of drivers doing
fallbacks if the values are out-of-range, but I'm not 100% sure about
this)

[...]
> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
> +                                           unsigned int pin, u16 *arg)
> +{
> +       struct meson_bank *bank;
> +       unsigned int reg, bit;
> +       unsigned int val;
> +       int ret;
> +
do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
what you already have in meson_pinconf_set_drive_strength()?


Regards
Martin

Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA

[guillaume La Roque]

Hi Martin,


thanks for your feedback.


On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> drive-strength-uA is a new feature needed for G12A SoC.
>> the default DS setting after boot is usually 500uA and it is not enough for
>> many functions. We need to be able to set the drive strength to reliably
>> enable things like MMC, I2C, etc ...
>>
>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> I gave this a go on Meson8m2 (meaning I applied all four patches from
> this series and booted the result on my board):
> [Meson8m2 doesn't support drive strength and still boots without any
> crashes or obvious regressions]
> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
>> ---
>>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
> personally I would have split this into two separate patches:
> - one for the generic pinctrl-meson part which adds drive-strength-uA support
> - another patch for enabling this on G12A
>
> if nobody else wants you to split this then it's fine for me as well


why not if i send new series i will do.


>
>>  3 files changed, 163 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> index d494492e98e9..3475cd7bd2af 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c
>> @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = {
>>  };
>>
>>  static struct meson_bank meson_g12a_periphs_banks[] = {
>> -       /* name  first  last  irq  pullen  pull  dir  out  in */
>> -       BANK("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
>> -            4,  0,  4,  0,  12,  0,  13, 0,  14, 0),
>> -       BANK("H",    GPIOH_0,    GPIOH_8, 28, 36,
>> -            3,  0,  3,  0,  9,  0,  10,  0,  11,  0),
>> -       BANK("BOOT", BOOT_0,     BOOT_15,  37, 52,
>> -            0,  0,  0,  0,  0, 0,  1, 0,  2, 0),
>> -       BANK("C",    GPIOC_0,    GPIOC_7,  53, 60,
>> -            1,  0,  1,  0,  3, 0,  4, 0,  5, 0),
>> -       BANK("A",    GPIOA_0,    GPIOA_15,  61, 76,
>> -            5,  0,  5,  0,  16,  0,  17,  0,  18,  0),
>> -       BANK("X",    GPIOX_0,    GPIOX_19,   77, 96,
>> -            2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
>> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
>> +       BANK_DS("Z",    GPIOZ_0,    GPIOZ_15, 12, 27,
>> +               4,  0,  4,  0,  12,  0,  13, 0,  14, 0, 5, 0),
>> +       BANK_DS("H",    GPIOH_0,    GPIOH_8, 28, 36,
>> +               3,  0,  3,  0,  9,  0,  10,  0,  11,  0, 4, 0),
>> +       BANK_DS("BOOT", BOOT_0,     BOOT_15,  37, 52,
>> +               0,  0,  0,  0,  0, 0,  1, 0,  2, 0, 0, 0),
>> +       BANK_DS("C",    GPIOC_0,    GPIOC_7,  53, 60,
>> +               1,  0,  1,  0,  3, 0,  4, 0,  5, 0, 1, 0),
>> +       BANK_DS("A",    GPIOA_0,    GPIOA_15,  61, 76,
>> +               5,  0,  5,  0,  16,  0,  17,  0,  18,  0, 6, 0),
>> +       BANK_DS("X",    GPIOX_0,    GPIOX_19,   77, 96,
>> +               2,  0,  2,  0,  6,  0,  7,  0,  8,  0, 2, 0),
>>  };
>>
>>  static struct meson_bank meson_g12a_aobus_banks[] = {
>> -       /* name  first  last  irq  pullen  pull  dir  out  in  */
>> -       BANK("AO",   GPIOAO_0,  GPIOAO_11,  0, 11,
>> -            3,  0,  2, 0,  0,  0,  4, 0,  1,  0),
>> +       /* name  first  last  irq  pullen  pull  dir  out  in  ds */
>> +       BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0,
>> +               0, 0),
>>         /* GPIOE actually located in the AO bank */
>> -       BANK("E",   GPIOE_0,  GPIOE_2,   97, 99,
>> -            3,  16,  2, 16,  0,  16,  4, 16,  1,  16),
>> +       BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1,
>> +               16, 1, 0),
>>  };
> these definitions are really hard to read, but it's been like this
> even before your patch
>
>>  static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = {
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 96a4a72708e4..5108e5aa6514 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.c
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
>> @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector,
>>         return 0;
>>  }
>>
>> -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
>> -                            unsigned long *configs, unsigned num_configs)
>> +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
>> +                                 enum pin_config_param conf)
> can you please confirm that I understood the purpose of this correctly:
> I think you introduce this to make setting the bias consistent with
> how you set the drive-strength.
> if so then it would be great to have a separate patch which describes
> that it's only a code-style change and a functional no-op


you  are right, i will separate this changes.


>
> additionally the function arguments are not consistent with
> meson_pinconf_get_drive_strength():
> - here you pass the pinctrl subsystem specific parameters (enum
> pin_config_param conf)
> - in meson_pinconf_get_drive_strength the conversion for pinctrl
> subsystem specific values (pinconf_to_config_argument) is part of
> meson_pinconf_set


for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.

if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.


> I'm wondering whether two separate functions
> (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
> things easier to read. I haven't tried whether this would really make
> things better, so I'd like to hear your opinion on this Guillaume!


no special opinion on this, if you think it's better for understanding i can separate  set_bias function.


>
> [...]
>> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
>> +                                           unsigned int pin, u16 arg)
>>  {
>> -       struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
>>         struct meson_bank *bank;
>> -       enum pin_config_param param;
>>         unsigned int reg, bit;
>> -       int i, ret;
>> +       unsigned int ds_val;
>> +       int ret;
>> +
>> +       if (!pc->reg_ds) {
>> +               dev_err(pc->dev, "drive-strength not supported\n");
>> +               return -ENOTSUPP;
> in meson_pinconf_set() we don't complain (with a dev_err) for this case.
> I'm not sure what the best-practice is for the pinctrl subsystem,
> maybe Linus can comment on this
>

this check is to be sure it's possible to set drive stength,

if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error

because something is wrong.


>> +       }
>>
>>         ret = meson_get_bank(pc, pin, &bank);
>>         if (ret)
>>                 return ret;
>>
>> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
>> +       bit = bit << 1;
>> +
>> +       if (arg <= 500) {
>> +               ds_val = MESON_PINCONF_DRV_500UA;
>> +       } else if (arg <= 2500) {
>> +               ds_val = MESON_PINCONF_DRV_2500UA;
>> +       } else if (arg <= 3000) {
>> +               ds_val = MESON_PINCONF_DRV_3000UA;
>> +       } else if (arg <= 4000) {
>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>> +       } else {
>> +               dev_warn_once(pc->dev,
>> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
>> +                             pin, arg);
>> +               ds_val = MESON_PINCONF_DRV_4000UA;
> why not return -EINVAL here? (my assumption is that the pinctrl
> subsystem would like to have -EINVAL instead of drivers doing
> fallbacks if the values are out-of-range, but I'm not 100% sure about
> this)
>
> [...]


i choose to set a default value instead of generating an error,

in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.


>> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
>> +                                           unsigned int pin, u16 *arg)
>> +{
>> +       struct meson_bank *bank;
>> +       unsigned int reg, bit;
>> +       unsigned int val;
>> +       int ret;
>> +
> do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
> what you already have in meson_pinconf_set_drive_strength()?

depending of linux comment on your feedback on meson_pinconf_set_drive_strength

i will update this part .


>
>
> Regards
> Martin


Regards

Guillaume

Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA

[Martin Blumenstingl]

Hi Guillaume,

On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque
<glaroque@baylibre.com> wrote:
>
> Hi Martin,
>
>
> thanks for your feedback.
>
>
> On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
> > Hi Guillaume,
> >
> > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> > <glaroque@baylibre.com> wrote:
> >> drive-strength-uA is a new feature needed for G12A SoC.
> >> the default DS setting after boot is usually 500uA and it is not enough for
> >> many functions. We need to be able to set the drive strength to reliably
> >> enable things like MMC, I2C, etc ...
> >>
> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
> > I gave this a go on Meson8m2 (meaning I applied all four patches from
> > this series and booted the result on my board):
> > [Meson8m2 doesn't support drive strength and still boots without any
> > crashes or obvious regressions]
> > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >
> >> ---
> >>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
> >>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
> >>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
> > personally I would have split this into two separate patches:
> > - one for the generic pinctrl-meson part which adds drive-strength-uA support
> > - another patch for enabling this on G12A
> >
> > if nobody else wants you to split this then it's fine for me as well
>
>
> why not if i send new series i will do.
great, thank you

[...]
> > additionally the function arguments are not consistent with
> > meson_pinconf_get_drive_strength():
> > - here you pass the pinctrl subsystem specific parameters (enum
> > pin_config_param conf)
> > - in meson_pinconf_get_drive_strength the conversion for pinctrl
> > subsystem specific values (pinconf_to_config_argument) is part of
> > meson_pinconf_set
>
>
> for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.
>
> if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.
let me use an example to better explain what I mean.

meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
unsigned long *configs, unsigned num_configs)
-> this uses parameters from the pinctrl subsystem only (struct
pinctrl_dev, configs/pinconf_to_config_argument)

meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned
int pin, u16 drive_strength_ua)
-> these use hardware-specific parameters only (struct meson_pinctrl,
[drive strength in uA])

meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
enum pin_config_param conf)
-> this mixes hardware-specific parameters (struct meson_pinctrl) with
parameters from the pinctrl subsystem (enum pin_config_param conf)

> > I'm wondering whether two separate functions
> > (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
> > things easier to read. I haven't tried whether this would really make
> > things better, so I'd like to hear your opinion on this Guillaume!
>
>
> no special opinion on this, if you think it's better for understanding i can separate  set_bias function.
this goes with the mixed parameters from my previous comment: if we
stick to the way these "private" functions are defined the "set bias"
function parameters should be hardware/driver specific (in other
words: don't use enum pin_config_param conf)
so my initial idea was to keep the switch/case for enum
pin_config_param in meson_pinconf_set and to have these function
declarations:
- meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin)
- meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int
pin, bool pull_up)

and to make it clear: I haven't tested whether this *really* looks
better when fully implemented.
let me know what you think - I'm happy with anything that others will
understand in the end (to make easy to catch potential breakage in a
code-review ;))

>
> >
> > [...]
> >> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
> >> +                                           unsigned int pin, u16 arg)
> >>  {
> >> -       struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
> >>         struct meson_bank *bank;
> >> -       enum pin_config_param param;
> >>         unsigned int reg, bit;
> >> -       int i, ret;
> >> +       unsigned int ds_val;
> >> +       int ret;
> >> +
> >> +       if (!pc->reg_ds) {
> >> +               dev_err(pc->dev, "drive-strength not supported\n");
> >> +               return -ENOTSUPP;
> > in meson_pinconf_set() we don't complain (with a dev_err) for this case.
> > I'm not sure what the best-practice is for the pinctrl subsystem,
> > maybe Linus can comment on this
> >
>
> this check is to be sure it's possible to set drive stength,
>
> if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error
>
> because something is wrong.
OK, I see, there are two different use-cases:
- meson_pinconf_set returns -ENOTSUPP in the "default" case if the
pin_config_param is not supported but no error message
- we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is
always delegated from meson_pinconf_set() to
meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and
print an error message

what I meant with my original comment is that there are two different
"-ENOTSUPP" cases but only one prints an error.
I don't know if there are any rules in the pinctrl subsystem how these
cases should be implemented, maybe Linus W. can give his feedback on
this topic.

>
> >> +       }
> >>
> >>         ret = meson_get_bank(pc, pin, &bank);
> >>         if (ret)
> >>                 return ret;
> >>
> >> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
> >> +       bit = bit << 1;
> >> +
> >> +       if (arg <= 500) {
> >> +               ds_val = MESON_PINCONF_DRV_500UA;
> >> +       } else if (arg <= 2500) {
> >> +               ds_val = MESON_PINCONF_DRV_2500UA;
> >> +       } else if (arg <= 3000) {
> >> +               ds_val = MESON_PINCONF_DRV_3000UA;
> >> +       } else if (arg <= 4000) {
> >> +               ds_val = MESON_PINCONF_DRV_4000UA;
> >> +       } else {
> >> +               dev_warn_once(pc->dev,
> >> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
> >> +                             pin, arg);
> >> +               ds_val = MESON_PINCONF_DRV_4000UA;
> > why not return -EINVAL here? (my assumption is that the pinctrl
> > subsystem would like to have -EINVAL instead of drivers doing
> > fallbacks if the values are out-of-range, but I'm not 100% sure about
> > this)
> >
> > [...]
>
>
> i choose to set a default value instead of generating an error,
>
> in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.
in that case I'm fine with it if Linus W. is happy as well (I'm not
sure if there are any rules about "fallback values" in the pinctrl
subsystem)

>
> >> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
> >> +                                           unsigned int pin, u16 *arg)
> >> +{
> >> +       struct meson_bank *bank;
> >> +       unsigned int reg, bit;
> >> +       unsigned int val;
> >> +       int ret;
> >> +
> > do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
> > what you already have in meson_pinconf_set_drive_strength()?
>
> depending of linux comment on your feedback on meson_pinconf_set_drive_strength
great, thank you!


Martin

Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA

[guillaume La Roque]

hi Martin,


On 4/30/19 10:28 PM, Martin Blumenstingl wrote:
> Hi Guillaume,
>
> On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque
> <glaroque@baylibre.com> wrote:
>> Hi Martin,
>>
>>
>> thanks for your feedback.
>>
>>
>> On 4/27/19 9:44 PM, Martin Blumenstingl wrote:
>>> Hi Guillaume,
>>>
>>> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
>>> <glaroque@baylibre.com> wrote:
>>>> drive-strength-uA is a new feature needed for G12A SoC.
>>>> the default DS setting after boot is usually 500uA and it is not enough for
>>>> many functions. We need to be able to set the drive strength to reliably
>>>> enable things like MMC, I2C, etc ...
>>>>
>>>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com>
>>> I gave this a go on Meson8m2 (meaning I applied all four patches from
>>> this series and booted the result on my board):
>>> [Meson8m2 doesn't support drive strength and still boots without any
>>> crashes or obvious regressions]
>>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>
>>>> ---
>>>>  drivers/pinctrl/meson/pinctrl-meson-g12a.c |  36 ++---
>>>>  drivers/pinctrl/meson/pinctrl-meson.c      | 166 ++++++++++++++++-----
>>>>  drivers/pinctrl/meson/pinctrl-meson.h      |  20 ++-
>>> personally I would have split this into two separate patches:
>>> - one for the generic pinctrl-meson part which adds drive-strength-uA support
>>> - another patch for enabling this on G12A
>>>
>>> if nobody else wants you to split this then it's fine for me as well
>>
>> why not if i send new series i will do.
> great, thank you
>
> [...]
>>> additionally the function arguments are not consistent with
>>> meson_pinconf_get_drive_strength():
>>> - here you pass the pinctrl subsystem specific parameters (enum
>>> pin_config_param conf)
>>> - in meson_pinconf_get_drive_strength the conversion for pinctrl
>>> subsystem specific values (pinconf_to_config_argument) is part of
>>> meson_pinconf_set
>>
>> for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it.
>>
>> if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param.
> let me use an example to better explain what I mean.
>
> meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin,
> unsigned long *configs, unsigned num_configs)
> -> this uses parameters from the pinctrl subsystem only (struct
> pinctrl_dev, configs/pinconf_to_config_argument)
>
> meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin)
> meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned
> int pin, u16 drive_strength_ua)
> -> these use hardware-specific parameters only (struct meson_pinctrl,
> [drive strength in uA])
>
> meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin,
> enum pin_config_param conf)
> -> this mixes hardware-specific parameters (struct meson_pinctrl) with
> parameters from the pinctrl subsystem (enum pin_config_param conf)


ok i see, it fix in next series.

>
>>> I'm wondering whether two separate functions
>>> (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make
>>> things easier to read. I haven't tried whether this would really make
>>> things better, so I'd like to hear your opinion on this Guillaume!
>>
>> no special opinion on this, if you think it's better for understanding i can separate  set_bias function.
> this goes with the mixed parameters from my previous comment: if we
> stick to the way these "private" functions are defined the "set bias"
> function parameters should be hardware/driver specific (in other
> words: don't use enum pin_config_param conf)
> so my initial idea was to keep the switch/case for enum
> pin_config_param in meson_pinconf_set and to have these function
> declarations:
> - meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin)
> - meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int
> pin, bool pull_up)
>
> and to make it clear: I haven't tested whether this *really* looks
> better when fully implemented.
> let me know what you think - I'm happy with anything that others will
> understand in the end (to make easy to catch potential breakage in a
> code-review ;))


what i would like to say is it ok for me, it's better to review.

i will do one patch for enable/disable bias function, one for adding drive-strength function and one for adding G12A part.


>>> [...]
>>>> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc,
>>>> +                                           unsigned int pin, u16 arg)
>>>>  {
>>>> -       struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev);
>>>>         struct meson_bank *bank;
>>>> -       enum pin_config_param param;
>>>>         unsigned int reg, bit;
>>>> -       int i, ret;
>>>> +       unsigned int ds_val;
>>>> +       int ret;
>>>> +
>>>> +       if (!pc->reg_ds) {
>>>> +               dev_err(pc->dev, "drive-strength not supported\n");
>>>> +               return -ENOTSUPP;
>>> in meson_pinconf_set() we don't complain (with a dev_err) for this case.
>>> I'm not sure what the best-practice is for the pinctrl subsystem,
>>> maybe Linus can comment on this
>>>
>> this check is to be sure it's possible to set drive stength,
>>
>> if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error
>>
>> because something is wrong.
> OK, I see, there are two different use-cases:
> - meson_pinconf_set returns -ENOTSUPP in the "default" case if the
> pin_config_param is not supported but no error message
> - we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is
> always delegated from meson_pinconf_set() to
> meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and
> print an error message
>
> what I meant with my original comment is that there are two different
> "-ENOTSUPP" cases but only one prints an error.
> I don't know if there are any rules in the pinctrl subsystem how these
> cases should be implemented, maybe Linus W. can give his feedback on
> this topic.


i just add error message for drive strength because it's an optional feature, people can forget to set register part and just set pin part

so they can see why it's not working.


>>>> +       }
>>>>
>>>>         ret = meson_get_bank(pc, pin, &bank);
>>>>         if (ret)
>>>>                 return ret;
>>>>
>>>> +       meson_calc_reg_and_bit(bank, pin, REG_DS, &reg, &bit);
>>>> +       bit = bit << 1;
>>>> +
>>>> +       if (arg <= 500) {
>>>> +               ds_val = MESON_PINCONF_DRV_500UA;
>>>> +       } else if (arg <= 2500) {
>>>> +               ds_val = MESON_PINCONF_DRV_2500UA;
>>>> +       } else if (arg <= 3000) {
>>>> +               ds_val = MESON_PINCONF_DRV_3000UA;
>>>> +       } else if (arg <= 4000) {
>>>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>>>> +       } else {
>>>> +               dev_warn_once(pc->dev,
>>>> +                             "pin %u: invalid drive-strength : %d , default to 4mA\n",
>>>> +                             pin, arg);
>>>> +               ds_val = MESON_PINCONF_DRV_4000UA;
>>> why not return -EINVAL here? (my assumption is that the pinctrl
>>> subsystem would like to have -EINVAL instead of drivers doing
>>> fallbacks if the values are out-of-range, but I'm not 100% sure about
>>> this)
>>>
>>> [...]
>>
>> i choose to set a default value instead of generating an error,
>>
>> in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default.
> in that case I'm fine with it if Linus W. is happy as well (I'm not
> sure if there are any rules about "fallback values" in the pinctrl
> subsystem)
>
>>>> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc,
>>>> +                                           unsigned int pin, u16 *arg)
>>>> +{
>>>> +       struct meson_bank *bank;
>>>> +       unsigned int reg, bit;
>>>> +       unsigned int val;
>>>> +       int ret;
>>>> +
>>> do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to
>>> what you already have in meson_pinconf_set_drive_strength()?
>> depending of linux comment on your feedback on meson_pinconf_set_drive_strength
> great, thank you!
>
>
> Martin


Guillaume

Re: [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver

[Linus Walleij]

On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
<glaroque@baylibre.com> wrote:

> The purpose of this patchset is to add drive-strength support in meson pinconf
> driver. This is a new feature that was added on the g12a. It is critical for us
> to support this since many functions are failing with default pad drive-strength.
>
> The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
> 'drive-strength' is expressed in mA.
> So this patch add another generic property "drive-strength-uA". The change to do so
> would be minimal and could be benefit to other platforms later on.
>
> Cheers
> Guillaume

This needs to be reviewed by Martin Blumenstigl, Martin can you have a look
at this patch set? If you don't have the patches I think Guillaume can
resend them.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver

[Martin Blumenstingl]

Hi Linus,

On Tue, Apr 23, 2019 at 1:13 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque
> <glaroque@baylibre.com> wrote:
>
> > The purpose of this patchset is to add drive-strength support in meson pinconf
> > driver. This is a new feature that was added on the g12a. It is critical for us
> > to support this since many functions are failing with default pad drive-strength.
> >
> > The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property
> > 'drive-strength' is expressed in mA.
> > So this patch add another generic property "drive-strength-uA". The change to do so
> > would be minimal and could be benefit to other platforms later on.
> >
> > Cheers
> > Guillaume
>
> This needs to be reviewed by Martin Blumenstigl, Martin can you have a look
> at this patch set? If you don't have the patches I think Guillaume can
> resend them.
thank you for the reminder - I just sent my feedback.
can you please also have a look at patch 4/4? there are two cases
where I'm not sure what the best practice of the pinctrl subsystem is


Regards
Martin