Hi everyone,
Here are some fixes and minor improvements to generic pinconf and the
Mediatek Paris pinctrl driver.
Patch 1 makes the generic pinconf library print out arguments for
PIN_CONFIG_BIAS_PULL_* in debugfs.
Patch 2 fixes bogus readback of PIN_CONFIG_BIAS_DISABLE being always
present.
Patch 3 fixes the type of the "argument" argument in mtk_pinconf_get().
This was erroneously typed as an enum when it should have been u32.
Patch 4 fixes the pingroup config state readback to actually do
readback.
Patch 5 drops an extra newline in the pinconf debugfs output.
Patch 6 cleans up the debugfs output, skipping the custom hardware state
output on the virtual GPIOs, which have no corresponding hardware.
Patch 7 adds support for PIN_CONFIG_DRIVE_STRENGTH_UA (drive-strength-uA)
to the Mediatek Paris pinctrl library. The goal is to replace the vendor
specific "mtk,drive-strength-adv" property with the generic one.
Later on we might want to deprecate "mtk,drive-strength-adv".
Please have a look.
Regards
ChenYu
Chen-Yu Tsai (7):
pinctrl: pinconf-generic: Print arguments for bias-pull-*
pinctrl: mediatek: paris: Fix PIN_CONFIG_BIAS_DISABLE readback
pinctrl: mediatek: paris: Fix "argument" argument type for
mtk_pinconf_get()
pinctrl: mediatek: paris: Fix pingroup pin config state readback
pinctrl: mediatek: paris: Drop extra newline in
mtk_pctrl_show_one_pin()
pinctrl: mediatek: paris: Skip custom extra pin config dump for vrtual
GPIOs
pinctrl: mediatek: paris: Support generic PIN_CONFIG_DRIVE_STRENGTH_UA
drivers/pinctrl/mediatek/pinctrl-paris.c | 106 ++++++++++++++++++++---
drivers/pinctrl/pinconf-generic.c | 6 +-
2 files changed, 98 insertions(+), 14 deletions(-)
--
2.34.1.575.g55b058a8bb-goog
The bias-pull-* properties, or PIN_CONFIG_BIAS_PULL_* pin config
parameters, accept optional arguments in ohms denoting the strength of
the pin bias.
Print these values out in debugfs as well.
Fixes: eec450713e5c ("pinctrl: pinconf-generic: Add flag to print arguments")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/pinconf-generic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index f8edcc88ac01..415d1df8f46a 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -30,10 +30,10 @@ static const struct pin_config_item conf_items[] = {
PCONFDUMP(PIN_CONFIG_BIAS_BUS_HOLD, "input bias bus hold", NULL, false),
PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL, false),
PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL, false),
- PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", NULL, false),
+ PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", "ohms", true),
PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
- "input bias pull to pin specific state", NULL, false),
- PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false),
+ "input bias pull to pin specific state", "ohms", true),
+ PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", "ohms", true),
PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
--
2.34.1.575.g55b058a8bb-goog
When reading back pin bias settings, if the pin is not in a
bias-disabled state, the function should return -EINVAL.
Fix this in the mediatek-paris pinctrl library so that the read back
state is not littered with bogus a "input bias disabled" combined with
"pull up" or "pull down" states.
Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index f9f9110f2107..1ca598ea7ba7 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -97,8 +97,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
if (err)
goto out;
if (param == PIN_CONFIG_BIAS_DISABLE) {
- if (ret == MTK_PUPD_SET_R1R0_00)
- ret = MTK_DISABLE;
+ if (ret != MTK_PUPD_SET_R1R0_00)
+ err = -EINVAL;
} else if (param == PIN_CONFIG_BIAS_PULL_UP) {
/* When desire to get pull-up value, return
* error if current setting is pull-down
--
2.34.1.575.g55b058a8bb-goog
For mtk_pinconf_get(), the "argument" argument is typically returned by
pinconf_to_config_argument(), which holds the value for a given pinconf
parameter. It certainly should not have the type of "enum pin_config_param",
which describes the type of the pinconf parameter itself.
Change the type to u32, which matches the return type of
pinconf_to_config_argument().
Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 1ca598ea7ba7..d720624d8cd2 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -188,8 +188,7 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
}
static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
- enum pin_config_param param,
- enum pin_config_param arg)
+ enum pin_config_param param, u32 arg)
{
struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
const struct mtk_pin_desc *desc;
--
2.34.1.575.g55b058a8bb-goog
mtk_pconf_group_get(), used to read back pingroup pin config state,
simply returns a set of configs saved from a previous invocation of
mtk_pconf_group_set(). This is an unfiltered, unvalidated set passed
in from the pinconf core, which does not match the current hardware
state.
Since the driver library is designed to have a pin per group, pass
through mtk_pconf_group_get() to mtk_pinconf_get(), to read back
the current pin config state of the only pin in the group.
Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index d720624d8cd2..d259f075c62d 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -736,10 +736,10 @@ static int mtk_pconf_group_get(struct pinctrl_dev *pctldev, unsigned group,
unsigned long *config)
{
struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
+ struct mtk_pinctrl_group *grp = &hw->groups[group];
- *config = hw->groups[group].config;
-
- return 0;
+ /* One pin per group only */
+ return mtk_pinconf_get(pctldev, grp->pin, config);
}
static int mtk_pconf_group_set(struct pinctrl_dev *pctldev, unsigned group,
--
2.34.1.575.g55b058a8bb-goog
Virtual GPIOs do not have any hardware state associated with them. Any
attempt to read back hardware state for these pins result in error
codes.
Skip dumping extra pin config information for these virtual GPIOs.
Fixes: 184d8e13f9b1 ("pinctrl: mediatek: Add support for pin configuration dump via debugfs.")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 1bacabfbc183..678c8aa33012 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -585,6 +585,9 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
if (gpio >= hw->soc->npins)
return -EINVAL;
+ if (mtk_is_virt_gpio(hw, gpio))
+ return -EINVAL;
+
desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
pinmux = mtk_pctrl_get_pinmux(hw, gpio);
if (pinmux >= hw->soc->nfuncs)
--
2.34.1.575.g55b058a8bb-goog
The caller of mtk_pctrl_show_one_pin() is responsible for printing the
full line. mtk_pctrl_show_one_pin(), called through mtk_pctrl_dbg_show(),
should only produce a string containing the extra information the driver
wants included.
Drop the extra newlines.
Fixes: 184d8e13f9b1 ("pinctrl: mediatek: Add support for pin configuration dump via debugfs.")
Fixes: fb34a9ae383a ("pinctrl: mediatek: support rsel feature")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index d259f075c62d..1bacabfbc183 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -639,12 +639,10 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
pullup);
if (r1 != -1) {
- len += scnprintf(buf + len, buf_len - len, " (%1d %1d)\n",
+ len += scnprintf(buf + len, buf_len - len, " (%1d %1d)",
r1, r0);
} else if (rsel != -1) {
- len += scnprintf(buf + len, buf_len - len, " (%1d)\n", rsel);
- } else {
- len += scnprintf(buf + len, buf_len - len, "\n");
+ len += scnprintf(buf + len, buf_len - len, " (%1d)", rsel);
}
return len;
--
2.34.1.575.g55b058a8bb-goog
Some of the MediaTek chips that utilize the Paris pinctrl driver library
support a lower drive strength (<= 1mA) than the standard drive strength
settings (2~16 mA) on certain pins. This was previously supported by the
custom MTK_PIN_CONFIG_DRV_ADV parameter along with the
"mediatek,drive-strength-adv" device tree property.
The drive strength values for this hardware are 125, 250, 500, and 1000 mA,
and can be readily described by the existing "drive-strength-microamp",
which then gets parsed by the generic pinconf library into the parameter
PIN_CONFIG_DRIVE_STRENGTH_UA.
Add support for PIN_CONFIG_DRIVE_STRENGTH_UA while keeping the old
custom parameter around for backward compatibility.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
The indentation in the switch/case blocks is getting somewhat out of
control. I also have some cleanup changes to reverse the logic of the
if/break statements. Not sure if it should be done before or after this
patch though.
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 84 ++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 678c8aa33012..5a94903ae372 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -48,6 +48,53 @@ static const char * const mtk_gpio_functions[] = {
"func12", "func13", "func14", "func15",
};
+/*
+ * This section supports converting to/from custom MTK_PIN_CONFIG_DRV_ADV
+ * and standard PIN_CONFIG_DRIVE_STRENGTH_UA pin configs.
+ *
+ * The custom value encodes three hardware bits as follows:
+ *
+ * | Bits |
+ * | 2 (E1) | 1 (E0) | 0 (EN) | drive strength (uA)
+ * ------------------------------------------------
+ * | x | x | 0 | disabled, use standard drive strength
+ * -------------------------------------
+ * | 0 | 0 | 1 | 125 uA
+ * | 0 | 1 | 1 | 250 uA
+ * | 1 | 0 | 1 | 500 uA
+ * | 1 | 1 | 1 | 1000 uA
+ */
+static const int mtk_drv_adv_uA[] = { 125, 250, 500, 1000 };
+
+static int mtk_drv_adv_to_uA(int val)
+{
+ /* This should never happen. */
+ if (WARN_ON_ONCE(val < 0 || val > 7))
+ return -EINVAL;
+
+ /* Bit 0 simply enables this hardware part */
+ if (!(val & BIT(0)))
+ return -EINVAL;
+
+ return mtk_drv_adv_uA[(val >> 1)];
+}
+
+static int mtk_drv_uA_to_adv(int val)
+{
+ switch (val) {
+ case 125:
+ return 0x1;
+ case 250:
+ return 0x3;
+ case 500:
+ return 0x5;
+ case 1000:
+ return 0x7;
+ }
+
+ return -EINVAL;
+}
+
static int mtk_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
struct pinctrl_gpio_range *range,
unsigned int pin)
@@ -151,11 +198,38 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
break;
case PIN_CONFIG_DRIVE_STRENGTH:
+ if (hw->soc->adv_drive_get) {
+ err = hw->soc->adv_drive_get(hw, desc, &ret);
+ if (!err) {
+ err = mtk_drv_adv_to_uA(ret);
+ if (err > 0) {
+ /* PIN_CONFIG_DRIVE_STRENGTH_UA used */
+ err = -EINVAL;
+ break;
+ }
+ }
+ }
+
if (hw->soc->drive_get)
err = hw->soc->drive_get(hw, desc, &ret);
else
err = -ENOTSUPP;
break;
+ case PIN_CONFIG_DRIVE_STRENGTH_UA:
+ if (hw->soc->adv_drive_get) {
+ err = hw->soc->adv_drive_get(hw, desc, &ret);
+ if (err)
+ break;
+ err = mtk_drv_adv_to_uA(ret);
+ if (err < 0)
+ break;
+
+ ret = err;
+ err = 0;
+ } else {
+ err = -ENOTSUPP;
+ }
+ break;
case MTK_PIN_CONFIG_TDSEL:
case MTK_PIN_CONFIG_RDSEL:
reg = (param == MTK_PIN_CONFIG_TDSEL) ?
@@ -271,6 +345,16 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
else
err = -ENOTSUPP;
break;
+ case PIN_CONFIG_DRIVE_STRENGTH_UA:
+ if (hw->soc->adv_drive_set) {
+ err = mtk_drv_uA_to_adv(arg);
+ if (err < 0)
+ break;
+ err = hw->soc->adv_drive_set(hw, desc, err);
+ } else {
+ err = -ENOTSUPP;
+ }
+ break;
case MTK_PIN_CONFIG_TDSEL:
case MTK_PIN_CONFIG_RDSEL:
reg = (param == MTK_PIN_CONFIG_TDSEL) ?
--
2.34.1.575.g55b058a8bb-goog
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> Some of the MediaTek chips that utilize the Paris pinctrl driver library
> support a lower drive strength (<= 1mA) than the standard drive strength
> settings (2~16 mA) on certain pins. This was previously supported by the
> custom MTK_PIN_CONFIG_DRV_ADV parameter along with the
> "mediatek,drive-strength-adv" device tree property.
>
> The drive strength values for this hardware are 125, 250, 500, and 1000 mA,
> and can be readily described by the existing "drive-strength-microamp",
> which then gets parsed by the generic pinconf library into the parameter
> PIN_CONFIG_DRIVE_STRENGTH_UA.
>
> Add support for PIN_CONFIG_DRIVE_STRENGTH_UA while keeping the old
> custom parameter around for backward compatibility.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
>
> The indentation in the switch/case blocks is getting somewhat out of
> control. I also have some cleanup changes to reverse the logic of the
> if/break statements. Not sure if it should be done before or after this
> patch though.
Hello Chen-Yu,
this commit is so nice that:
- My heart says that it's fine as it is, but
- My brain says that it makes a lot more sense if you push the cleanup
changes to reverse that logic before pushing this commit, as to reduce the
count of changed lines (hence, to reduce some noise)...
...so please, can you rebase this commit over the cleanups?
Thanks,
- Angelo
>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 84 ++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 678c8aa33012..5a94903ae372 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -48,6 +48,53 @@ static const char * const mtk_gpio_functions[] = {
> "func12", "func13", "func14", "func15",
> };
>
> +/*
> + * This section supports converting to/from custom MTK_PIN_CONFIG_DRV_ADV
> + * and standard PIN_CONFIG_DRIVE_STRENGTH_UA pin configs.
> + *
> + * The custom value encodes three hardware bits as follows:
> + *
> + * | Bits |
> + * | 2 (E1) | 1 (E0) | 0 (EN) | drive strength (uA)
> + * ------------------------------------------------
> + * | x | x | 0 | disabled, use standard drive strength
> + * -------------------------------------
> + * | 0 | 0 | 1 | 125 uA
> + * | 0 | 1 | 1 | 250 uA
> + * | 1 | 0 | 1 | 500 uA
> + * | 1 | 1 | 1 | 1000 uA
> + */
> +static const int mtk_drv_adv_uA[] = { 125, 250, 500, 1000 };
> +
> +static int mtk_drv_adv_to_uA(int val)
> +{
> + /* This should never happen. */
> + if (WARN_ON_ONCE(val < 0 || val > 7))
> + return -EINVAL;
> +
> + /* Bit 0 simply enables this hardware part */
> + if (!(val & BIT(0)))
> + return -EINVAL;
> +
> + return mtk_drv_adv_uA[(val >> 1)];
> +}
> +
> +static int mtk_drv_uA_to_adv(int val)
> +{
> + switch (val) {
> + case 125:
> + return 0x1;
> + case 250:
> + return 0x3;
> + case 500:
> + return 0x5;
> + case 1000:
> + return 0x7;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int mtk_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
> struct pinctrl_gpio_range *range,
> unsigned int pin)
> @@ -151,11 +198,38 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
>
> break;
> case PIN_CONFIG_DRIVE_STRENGTH:
> + if (hw->soc->adv_drive_get) {
> + err = hw->soc->adv_drive_get(hw, desc, &ret);
> + if (!err) {
> + err = mtk_drv_adv_to_uA(ret);
> + if (err > 0) {
> + /* PIN_CONFIG_DRIVE_STRENGTH_UA used */
> + err = -EINVAL;
> + break;
> + }
> + }
> + }
> +
> if (hw->soc->drive_get)
> err = hw->soc->drive_get(hw, desc, &ret);
> else
> err = -ENOTSUPP;
> break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + if (hw->soc->adv_drive_get) {
> + err = hw->soc->adv_drive_get(hw, desc, &ret);
> + if (err)
> + break;
> + err = mtk_drv_adv_to_uA(ret);
> + if (err < 0)
> + break;
> +
> + ret = err;
> + err = 0;
> + } else {
> + err = -ENOTSUPP;
> + }
> + break;
> case MTK_PIN_CONFIG_TDSEL:
> case MTK_PIN_CONFIG_RDSEL:
> reg = (param == MTK_PIN_CONFIG_TDSEL) ?
> @@ -271,6 +345,16 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> else
> err = -ENOTSUPP;
> break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + if (hw->soc->adv_drive_set) {
> + err = mtk_drv_uA_to_adv(arg);
> + if (err < 0)
> + break;
> + err = hw->soc->adv_drive_set(hw, desc, err);
> + } else {
> + err = -ENOTSUPP;
> + }
> + break;
> case MTK_PIN_CONFIG_TDSEL:
> case MTK_PIN_CONFIG_RDSEL:
> reg = (param == MTK_PIN_CONFIG_TDSEL) ?
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> Virtual GPIOs do not have any hardware state associated with them. Any
> attempt to read back hardware state for these pins result in error
> codes.
>
> Skip dumping extra pin config information for these virtual GPIOs.
>
> Fixes: 184d8e13f9b1 ("pinctrl: mediatek: Add support for pin configuration dump via debugfs.")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Can you please fix the typo in the commit title? "vrtual"->"virtual"
After the typo fix:
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Thank you!
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 1bacabfbc183..678c8aa33012 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -585,6 +585,9 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> if (gpio >= hw->soc->npins)
> return -EINVAL;
>
> + if (mtk_is_virt_gpio(hw, gpio))
> + return -EINVAL;
> +
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> pinmux = mtk_pctrl_get_pinmux(hw, gpio);
> if (pinmux >= hw->soc->nfuncs)
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> When reading back pin bias settings, if the pin is not in a
> bias-disabled state, the function should return -EINVAL.
>
> Fix this in the mediatek-paris pinctrl library so that the read back
> state is not littered with bogus a "input bias disabled" combined with
> "pull up" or "pull down" states.
>
> Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index f9f9110f2107..1ca598ea7ba7 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -97,8 +97,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> if (err)
> goto out;
> if (param == PIN_CONFIG_BIAS_DISABLE) {
> - if (ret == MTK_PUPD_SET_R1R0_00)
> - ret = MTK_DISABLE;
> + if (ret != MTK_PUPD_SET_R1R0_00)
> + err = -EINVAL;
> } else if (param == PIN_CONFIG_BIAS_PULL_UP) {
> /* When desire to get pull-up value, return
> * error if current setting is pull-down
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> The caller of mtk_pctrl_show_one_pin() is responsible for printing the
> full line. mtk_pctrl_show_one_pin(), called through mtk_pctrl_dbg_show(),
> should only produce a string containing the extra information the driver
> wants included.
>
> Drop the extra newlines.
>
> Fixes: 184d8e13f9b1 ("pinctrl: mediatek: Add support for pin configuration dump via debugfs.")
> Fixes: fb34a9ae383a ("pinctrl: mediatek: support rsel feature")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index d259f075c62d..1bacabfbc183 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -639,12 +639,10 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> pullup);
>
> if (r1 != -1) {
> - len += scnprintf(buf + len, buf_len - len, " (%1d %1d)\n",
> + len += scnprintf(buf + len, buf_len - len, " (%1d %1d)",
> r1, r0);
Since you're doing also some nice cleanups, would you mind un-breaking the line
above? That'd be 82 columns, which is fine to have.
In any case:
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> } else if (rsel != -1) {
> - len += scnprintf(buf + len, buf_len - len, " (%1d)\n", rsel);
> - } else {
> - len += scnprintf(buf + len, buf_len - len, "\n");
> + len += scnprintf(buf + len, buf_len - len, " (%1d)", rsel);
> }
>
> return len;
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> mtk_pconf_group_get(), used to read back pingroup pin config state,
> simply returns a set of configs saved from a previous invocation of
> mtk_pconf_group_set(). This is an unfiltered, unvalidated set passed
> in from the pinconf core, which does not match the current hardware
> state.
>
> Since the driver library is designed to have a pin per group, pass
> through mtk_pconf_group_get() to mtk_pinconf_get(), to read back
> the current pin config state of the only pin in the group.
>
> Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index d720624d8cd2..d259f075c62d 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -736,10 +736,10 @@ static int mtk_pconf_group_get(struct pinctrl_dev *pctldev, unsigned group,
> unsigned long *config)
> {
> struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
> + struct mtk_pinctrl_group *grp = &hw->groups[group];
>
> - *config = hw->groups[group].config;
> -
> - return 0;
> + /* One pin per group only */
> + return mtk_pinconf_get(pctldev, grp->pin, config);
> }
>
> static int mtk_pconf_group_set(struct pinctrl_dev *pctldev, unsigned group,
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> For mtk_pinconf_get(), the "argument" argument is typically returned by
> pinconf_to_config_argument(), which holds the value for a given pinconf
> parameter. It certainly should not have the type of "enum pin_config_param",
> which describes the type of the pinconf parameter itself.
>
> Change the type to u32, which matches the return type of
> pinconf_to_config_argument().
>
> Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that implements the vendor dt-bindings")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 1ca598ea7ba7..d720624d8cd2 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -188,8 +188,7 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> }
>
> static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> - enum pin_config_param param,
> - enum pin_config_param arg)
> + enum pin_config_param param, u32 arg)
> {
> struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
> const struct mtk_pin_desc *desc;
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> The bias-pull-* properties, or PIN_CONFIG_BIAS_PULL_* pin config
> parameters, accept optional arguments in ohms denoting the strength of
> the pin bias.
>
> Print these values out in debugfs as well.
>
> Fixes: eec450713e5c ("pinctrl: pinconf-generic: Add flag to print arguments")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
Tested on qcom-SC7180 Trogdor, mtk-MT8173 Elm, and others;
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> ---
> drivers/pinctrl/pinconf-generic.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
> index f8edcc88ac01..415d1df8f46a 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -30,10 +30,10 @@ static const struct pin_config_item conf_items[] = {
> PCONFDUMP(PIN_CONFIG_BIAS_BUS_HOLD, "input bias bus hold", NULL, false),
> PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL, false),
> PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL, false),
> - PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", NULL, false),
> + PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", "ohms", true),
> PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
> - "input bias pull to pin specific state", NULL, false),
> - PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false),
> + "input bias pull to pin specific state", "ohms", true),
> + PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", "ohms", true),
> PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false),
> PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false),
> PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false),
>
Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> Some of the MediaTek chips that utilize the Paris pinctrl driver library
> support a lower drive strength (<= 1mA) than the standard drive strength
> settings (2~16 mA) on certain pins. This was previously supported by the
> custom MTK_PIN_CONFIG_DRV_ADV parameter along with the
> "mediatek,drive-strength-adv" device tree property.
>
> The drive strength values for this hardware are 125, 250, 500, and 1000 mA,
> and can be readily described by the existing "drive-strength-microamp",
> which then gets parsed by the generic pinconf library into the parameter
> PIN_CONFIG_DRIVE_STRENGTH_UA.
>
> Add support for PIN_CONFIG_DRIVE_STRENGTH_UA while keeping the old
> custom parameter around for backward compatibility.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
>
> The indentation in the switch/case blocks is getting somewhat out of
> control. I also have some cleanup changes to reverse the logic of the
> if/break statements. Not sure if it should be done before or after this
> patch though.
Hello Chen-Yu,
this commit is so nice that:
- My heart says that it's fine as it is, but
- My brain says that it makes a lot more sense if you push the cleanup
changes to reverse that logic before pushing this commit, as to reduce the
count of changed lines (hence, to reduce some noise)...
...so please, can you rebase this commit over the cleanups?
Thanks,
- Angelo
On Tue, Jan 11, 2022 at 9:42 PM AngeloGioacchino Del Regno
<[email protected]> wrote:
>
> Il 11/01/22 12:22, Chen-Yu Tsai ha scritto:
> > The bias-pull-* properties, or PIN_CONFIG_BIAS_PULL_* pin config
> > parameters, accept optional arguments in ohms denoting the strength of
> > the pin bias.
> >
> > Print these values out in debugfs as well.
> >
> > Fixes: eec450713e5c ("pinctrl: pinconf-generic: Add flag to print arguments")
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
>
> Tested on qcom-SC7180 Trogdor, mtk-MT8173 Elm, and others;
Cool! I'll take that as a Tested-by tag.
> Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
On Tue, Jan 11, 2022 at 7:23 PM Chen-Yu Tsai <[email protected]> wrote:
>
> Some of the MediaTek chips that utilize the Paris pinctrl driver library
> support a lower drive strength (<= 1mA) than the standard drive strength
> settings (2~16 mA) on certain pins. This was previously supported by the
> custom MTK_PIN_CONFIG_DRV_ADV parameter along with the
> "mediatek,drive-strength-adv" device tree property.
>
> The drive strength values for this hardware are 125, 250, 500, and 1000 mA,
> and can be readily described by the existing "drive-strength-microamp",
> which then gets parsed by the generic pinconf library into the parameter
> PIN_CONFIG_DRIVE_STRENGTH_UA.
So I am actually unsure how to implement support for this properly.
My intention was to map "mediatek,drive-strength-adv" to
"drive-strength-microamp". This implies using the advanced mode if
the property is present, and vice versa.
(Also unsure if such a binding would be acceptable.)
However the pin configs are passed in one-by-one within the driver, so
it doesn't seem viable to check for the absence of a certain parameter.
This might involve a bit more rewriting.
ChenYu
> Add support for PIN_CONFIG_DRIVE_STRENGTH_UA while keeping the old
> custom parameter around for backward compatibility.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
>
> The indentation in the switch/case blocks is getting somewhat out of
> control. I also have some cleanup changes to reverse the logic of the
> if/break statements. Not sure if it should be done before or after this
> patch though.
>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 84 ++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 678c8aa33012..5a94903ae372 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -48,6 +48,53 @@ static const char * const mtk_gpio_functions[] = {
> "func12", "func13", "func14", "func15",
> };
>
> +/*
> + * This section supports converting to/from custom MTK_PIN_CONFIG_DRV_ADV
> + * and standard PIN_CONFIG_DRIVE_STRENGTH_UA pin configs.
> + *
> + * The custom value encodes three hardware bits as follows:
> + *
> + * | Bits |
> + * | 2 (E1) | 1 (E0) | 0 (EN) | drive strength (uA)
> + * ------------------------------------------------
> + * | x | x | 0 | disabled, use standard drive strength
> + * -------------------------------------
> + * | 0 | 0 | 1 | 125 uA
> + * | 0 | 1 | 1 | 250 uA
> + * | 1 | 0 | 1 | 500 uA
> + * | 1 | 1 | 1 | 1000 uA
> + */
> +static const int mtk_drv_adv_uA[] = { 125, 250, 500, 1000 };
> +
> +static int mtk_drv_adv_to_uA(int val)
> +{
> + /* This should never happen. */
> + if (WARN_ON_ONCE(val < 0 || val > 7))
> + return -EINVAL;
> +
> + /* Bit 0 simply enables this hardware part */
> + if (!(val & BIT(0)))
> + return -EINVAL;
> +
> + return mtk_drv_adv_uA[(val >> 1)];
> +}
> +
> +static int mtk_drv_uA_to_adv(int val)
> +{
> + switch (val) {
> + case 125:
> + return 0x1;
> + case 250:
> + return 0x3;
> + case 500:
> + return 0x5;
> + case 1000:
> + return 0x7;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int mtk_pinmux_gpio_request_enable(struct pinctrl_dev *pctldev,
> struct pinctrl_gpio_range *range,
> unsigned int pin)
> @@ -151,11 +198,38 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
>
> break;
> case PIN_CONFIG_DRIVE_STRENGTH:
> + if (hw->soc->adv_drive_get) {
> + err = hw->soc->adv_drive_get(hw, desc, &ret);
> + if (!err) {
> + err = mtk_drv_adv_to_uA(ret);
> + if (err > 0) {
> + /* PIN_CONFIG_DRIVE_STRENGTH_UA used */
> + err = -EINVAL;
> + break;
> + }
> + }
> + }
> +
> if (hw->soc->drive_get)
> err = hw->soc->drive_get(hw, desc, &ret);
> else
> err = -ENOTSUPP;
> break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + if (hw->soc->adv_drive_get) {
> + err = hw->soc->adv_drive_get(hw, desc, &ret);
> + if (err)
> + break;
> + err = mtk_drv_adv_to_uA(ret);
> + if (err < 0)
> + break;
> +
> + ret = err;
> + err = 0;
> + } else {
> + err = -ENOTSUPP;
> + }
> + break;
> case MTK_PIN_CONFIG_TDSEL:
> case MTK_PIN_CONFIG_RDSEL:
> reg = (param == MTK_PIN_CONFIG_TDSEL) ?
> @@ -271,6 +345,16 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> else
> err = -ENOTSUPP;
> break;
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + if (hw->soc->adv_drive_set) {
> + err = mtk_drv_uA_to_adv(arg);
> + if (err < 0)
> + break;
> + err = hw->soc->adv_drive_set(hw, desc, err);
> + } else {
> + err = -ENOTSUPP;
> + }
> + break;
> case MTK_PIN_CONFIG_TDSEL:
> case MTK_PIN_CONFIG_RDSEL:
> reg = (param == MTK_PIN_CONFIG_TDSEL) ?
> --
> 2.34.1.575.g55b058a8bb-goog
>
On Tue, Jan 11, 2022 at 12:22 PM Chen-Yu Tsai <[email protected]> wrote:
> Here are some fixes and minor improvements to generic pinconf and the
> Mediatek Paris pinctrl driver.
Looks good to me, can you rebase this on v5.17-rc1 once it is out
and I will queue it!
Yours,
Linus Walleij
Hi,
On Tue, Jan 18, 2022 at 10:36 AM zhiyong.tao <[email protected]> wrote:
>
> On Tue, 2022-01-11 at 19:22 +0800, Chen-Yu Tsai wrote:
> > Some of the MediaTek chips that utilize the Paris pinctrl driver
> > library
> > support a lower drive strength (<= 1mA) than the standard drive
> > strength
> > settings (2~16 mA) on certain pins. This was previously supported by
> > the
> > custom MTK_PIN_CONFIG_DRV_ADV parameter along with the
> > "mediatek,drive-strength-adv" device tree property.
> >
> > The drive strength values for this hardware are 125, 250, 500, and
> > 1000 mA,
> > and can be readily described by the existing "drive-strength-
> > microamp",
> > which then gets parsed by the generic pinconf library into the
> > parameter
> > PIN_CONFIG_DRIVE_STRENGTH_UA.
> >
> > Add support for PIN_CONFIG_DRIVE_STRENGTH_UA while keeping the old
> > custom parameter around for backward compatibility.
> >
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > ---
> >
> > The indentation in the switch/case blocks is getting somewhat out of
> > control. I also have some cleanup changes to reverse the logic of the
> > if/break statements. Not sure if it should be done before or after
> > this
> > patch though.
> >
> > ---
> > drivers/pinctrl/mediatek/pinctrl-paris.c | 84
> > ++++++++++++++++++++++++
> > 1 file changed, 84 insertions(+)
> >
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
> > b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > index 678c8aa33012..5a94903ae372 100644
> > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > @@ -48,6 +48,53 @@ static const char * const mtk_gpio_functions[] = {
> > "func12", "func13", "func14", "func15",
> > };
> >
> > +/*
> > + * This section supports converting to/from custom
> > MTK_PIN_CONFIG_DRV_ADV
> > + * and standard PIN_CONFIG_DRIVE_STRENGTH_UA pin configs.
> > + *
> > + * The custom value encodes three hardware bits as follows:
> > + *
> > + * | Bits |
> > + * | 2 (E1) | 1 (E0) | 0 (EN) | drive strength (uA)
> > + * ------------------------------------------------
> > + * | x | x | 0 | disabled, use standard drive
> > strength
> > + * -------------------------------------
> > + * | 0 | 0 | 1 | 125 uA
> > + * | 0 | 1 | 1 | 250 uA
> > + * | 1 | 0 | 1 | 500 uA
> > + * | 1 | 1 | 1 | 1000 uA
> > + */
> > +static const int mtk_drv_adv_uA[] = { 125, 250, 500, 1000 };
> > +
> > +static int mtk_drv_adv_to_uA(int val)
> > +{
> > + /* This should never happen. */
> > + if (WARN_ON_ONCE(val < 0 || val > 7))
> > + return -EINVAL;
> > +
> > + /* Bit 0 simply enables this hardware part */
> > + if (!(val & BIT(0)))
> > + return -EINVAL;
> > +
> > + return mtk_drv_adv_uA[(val >> 1)];
> > +}
> > +
> > +static int mtk_drv_uA_to_adv(int val)
> > +{
> > + switch (val) {
> > + case 125:
> > + return 0x1;
> > + case 250:
> > + return 0x3;
> > + case 500:
> > + return 0x5;
> > + case 1000:
> > + return 0x7;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > static int mtk_pinmux_gpio_request_enable(struct pinctrl_dev
> > *pctldev,
> > struct pinctrl_gpio_range
> > *range,
> > unsigned int pin)
> > @@ -151,11 +198,38 @@ static int mtk_pinconf_get(struct pinctrl_dev
> > *pctldev,
> >
> > break;
> > case PIN_CONFIG_DRIVE_STRENGTH:
> > + if (hw->soc->adv_drive_get) {
> > + err = hw->soc->adv_drive_get(hw, desc, &ret);
> > + if (!err) {
> > + err = mtk_drv_adv_to_uA(ret);
> > + if (err > 0) {
> > + /* PIN_CONFIG_DRIVE_STRENGTH_UA
> > used */
> > + err = -EINVAL;
> > + break;
> > + }
> > + }
> > + }
> > +
> Hi Chen-Yu,
>
> PIN_CONFIG_DRIVE_STRENGTH is seems used for 2/4/6/8ma, it is not used
> for 125/250/500/1000ma. why you change here?
If 125/250/500/1000uA is used, that mode takes precedence over 2/4/6/8 mA.
To give a correct readback, if 125/250/500/1000 uA is active, we should
return -EINVAL here to tell the pin config core that PIN_CONFIG_DRIVE_STRENGTH
is not active.
Otherwise when one reads
/sys/kernel/debug/pinctrl/10005000.pinctrl-pinctrl_paris/pinconf-pins
it would return
... output drive strength (X mA), output drive strength (Y uA), ...
where the first "mA" setting isn't actually active in hardware.
> > if (hw->soc->drive_get)
> > err = hw->soc->drive_get(hw, desc, &ret);
> > else
> > err = -ENOTSUPP;
> > break;
> > + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> > + if (hw->soc->adv_drive_get) {
> > + err = hw->soc->adv_drive_get(hw, desc, &ret);
> > + if (err)
> > + break;
> > + err = mtk_drv_adv_to_uA(ret);
> > + if (err < 0)
> > + break;
> > +
> > + ret = err;
> > + err = 0;
> > + } else {
> > + err = -ENOTSUPP;
> > + }
> > + break;
>
> Hi Chen-Yu,
> For PIN_CONFIG_DRIVE_STRENGTH_UA case, How can we use in dts node ?
> Thanks.
My original thought was to have either
drive-strength = <2/4/6/8>;
or
drive-strength-microamp = <125/250/500/1000>;
but not both. However I haven't figured out how to write the binding schema
to have the two properties be mutually exclusive. I'm not sure this would
be accepted either.
At the driver level, it should also have a check for the existence of
"drive-strength-microamp" to enable or disable the special mode.
Regards
ChenYu
> > case MTK_PIN_CONFIG_TDSEL:
> > case MTK_PIN_CONFIG_RDSEL:
> > reg = (param == MTK_PIN_CONFIG_TDSEL) ?
> > @@ -271,6 +345,16 @@ static int mtk_pinconf_set(struct pinctrl_dev
> > *pctldev, unsigned int pin,
> > else
> > err = -ENOTSUPP;
> > break;
> > + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> > + if (hw->soc->adv_drive_set) {
> > + err = mtk_drv_uA_to_adv(arg);
> > + if (err < 0)
> > + break;
> > + err = hw->soc->adv_drive_set(hw, desc, err);
> > + } else {
> > + err = -ENOTSUPP;
> > + }
> > + break;
> > case MTK_PIN_CONFIG_TDSEL:
> > case MTK_PIN_CONFIG_RDSEL:
> > reg = (param == MTK_PIN_CONFIG_TDSEL) ?
>
Hi Linus,
On Sun, Jan 16, 2022 at 8:49 AM Linus Walleij <[email protected]> wrote:
>
> On Tue, Jan 11, 2022 at 12:22 PM Chen-Yu Tsai <[email protected]> wrote:
>
> > Here are some fixes and minor improvements to generic pinconf and the
> > Mediatek Paris pinctrl driver.
>
> Looks good to me, can you rebase this on v5.17-rc1 once it is out
> and I will queue it!
Thanks for the vote of confidence! I think patch 7 still needs some work.
I will likely split that into two parts:
a. pin config readback of the advanced drive strength mode into
PIN_CONFIG_DRIVE_STRENGTH_UA, and
b. Supporting setting advanced drive strength mode via generic pin config
properties.
The latter would require modification of the bindings in a way that might
not be describable. More about that is in my reply to patch 7.
In exchange, I will add on a code style rework requested by Angelo.
Regards
ChenYu
-----Original Message-----
From: Chen-Yu Tsai <[email protected]>
To: Sean Wang <[email protected]>, Linus Walleij <
[email protected]>, Matthias Brugger <[email protected]>
Cc: Chen-Yu Tsai <[email protected]>,
[email protected], [email protected],
[email protected], [email protected],
Zhiyong Tao <[email protected]>, Guodong Liu <
[email protected]>
Subject: [PATCH 2/7] pinctrl: mediatek: paris: Fix
PIN_CONFIG_BIAS_DISABLE readback
Date: Tue, 11 Jan 2022 19:22:39 +0800
When reading back pin bias settings, if the pin is not in a
bias-disabled state, the function should return -EINVAL.
Fix this in the mediatek-paris pinctrl library so that the read back
state is not littered with bogus a "input bias disabled" combined with
"pull up" or "pull down" states.
Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that
implements the vendor dt-bindings")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/pinctrl/mediatek/pinctrl-paris.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
b/drivers/pinctrl/mediatek/pinctrl-paris.c
index f9f9110f2107..1ca598ea7ba7 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -97,8 +97,8 @@ static int mtk_pinconf_get(struct pinctrl_dev
*pctldev,
if (err)
goto out;
if (param == PIN_CONFIG_BIAS_DISABLE) {
- if (ret == MTK_PUPD_SET_R1R0_00)
- ret = MTK_DISABLE;
+ if (ret != MTK_PUPD_SET_R1R0_00)
+ err = -EINVAL;
Hi Chen-Yu
When tht API "hw->soc->bias_get_combo(hw, desc, &pullup, &ret)" is
called,
The ret vaule of may be MTK_DISABLE OR MTK_PUPD_SET_R1R0_00 or (pullen
== 0), All those cases are expected to be as "bias-disable".
We advices to keep original code,
+ if (ret == MTK_PUPD_SET_R1R0_00)
+
ret = MTK_DISABLE;
+ if (ret != MTK_DISABLE)
+ err = -EINVAL;
Thanks
} else if (param == PIN_CONFIG_BIAS_PULL_UP) {
/* When desire to get pull-up value,
return
* error if current setting is pull-
down
On Wed, Jan 19, 2022 at 9:42 AM Guodong Liu <[email protected]> wrote:
>
> -----Original Message-----
> From: Chen-Yu Tsai <[email protected]>
> To: Sean Wang <[email protected]>, Linus Walleij <
> [email protected]>, Matthias Brugger <[email protected]>
> Cc: Chen-Yu Tsai <[email protected]>,
> [email protected], [email protected],
> [email protected], [email protected],
> Zhiyong Tao <[email protected]>, Guodong Liu <
> [email protected]>
> Subject: [PATCH 2/7] pinctrl: mediatek: paris: Fix
> PIN_CONFIG_BIAS_DISABLE readback
> Date: Tue, 11 Jan 2022 19:22:39 +0800
>
> When reading back pin bias settings, if the pin is not in a
> bias-disabled state, the function should return -EINVAL.
>
> Fix this in the mediatek-paris pinctrl library so that the read back
> state is not littered with bogus a "input bias disabled" combined with
> "pull up" or "pull down" states.
>
> Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that
> implements the vendor dt-bindings")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
> b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index f9f9110f2107..1ca598ea7ba7 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -97,8 +97,8 @@ static int mtk_pinconf_get(struct pinctrl_dev
> *pctldev,
> if (err)
> goto out;
> if (param == PIN_CONFIG_BIAS_DISABLE) {
> - if (ret == MTK_PUPD_SET_R1R0_00)
> - ret = MTK_DISABLE;
> + if (ret != MTK_PUPD_SET_R1R0_00)
> + err = -EINVAL;
> Hi Chen-Yu
>
> When tht API "hw->soc->bias_get_combo(hw, desc, &pullup, &ret)" is
> called,
> The ret vaule of may be MTK_DISABLE OR MTK_PUPD_SET_R1R0_00 or (pullen
> == 0), All those cases are expected to be as "bias-disable".
> We advices to keep original code,
> + if (ret == MTK_PUPD_SET_R1R0_00)
> +
> ret = MTK_DISABLE;
> + if (ret != MTK_DISABLE)
> + err = -EINVAL;
IIUC you are suggesting to assign MTK_DISABLE to ret in the other two cases,
and then check if ret == MTK_DISABLE.
Thanks for pointing that out.
ChenYu
> Thanks
> } else if (param == PIN_CONFIG_BIAS_PULL_UP) {
> /* When desire to get pull-up value,
> return
> * error if current setting is pull-
> down
>
-----Original Message-----
From: Chen-Yu Tsai <[email protected]>
To: Guodong Liu <[email protected]>
Cc: Sean Wang <[email protected]>, Linus Walleij <
[email protected]>, Matthias Brugger <[email protected]>,
[email protected], [email protected],
[email protected], [email protected],
Zhiyong Tao <[email protected]>, Hui Liu <[email protected]>,
Light Hsieh <[email protected]>
Subject: Re: [PATCH 2/7] pinctrl: mediatek: paris: Fix
PIN_CONFIG_BIAS_DISABLE readback
Date: Wed, 19 Jan 2022 13:57:18 +0800
On Wed, Jan 19, 2022 at 9:42 AM Guodong Liu <[email protected]>
wrote:
>
> -----Original Message-----
> From: Chen-Yu Tsai <[email protected]>
> To: Sean Wang <[email protected]>, Linus Walleij <
> [email protected]>, Matthias Brugger <[email protected]>
> Cc: Chen-Yu Tsai <[email protected]>,
> [email protected], [email protected],
> [email protected], [email protected],
> Zhiyong Tao <[email protected]>, Guodong Liu <
> [email protected]>
> Subject: [PATCH 2/7] pinctrl: mediatek: paris: Fix
> PIN_CONFIG_BIAS_DISABLE readback
> Date: Tue, 11 Jan 2022 19:22:39 +0800
>
> When reading back pin bias settings, if the pin is not in a
> bias-disabled state, the function should return -EINVAL.
>
> Fix this in the mediatek-paris pinctrl library so that the read back
> state is not littered with bogus a "input bias disabled" combined
> with
> "pull up" or "pull down" states.
>
> Fixes: 805250982bb5 ("pinctrl: mediatek: add pinctrl-paris that
> implements the vendor dt-bindings")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
> b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index f9f9110f2107..1ca598ea7ba7 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -97,8 +97,8 @@ static int mtk_pinconf_get(struct pinctrl_dev
> *pctldev,
> if (err)
> goto out;
> if (param == PIN_CONFIG_BIAS_DISABLE) {
> - if (ret == MTK_PUPD_SET_R1R0_00)
> - ret = MTK_DISABLE;
> + if (ret != MTK_PUPD_SET_R1R0_00)
> + err = -EINVAL;
> Hi Chen-Yu
>
> When the API "hw->soc->bias_get_combo(hw, desc, &pullup, &ret)" is
> called,
> The ret vaule of ret may be MTK_DISABLE or MTK_PUPD_SET_R1R0_00
> or (pullen
> == 0), All those cases are expected to be as "bias-disable".
> We advices to keep original code,
> + if (ret == MTK_PUPD_SET_R1R0_00)
> + ret = MTK_DISABLE;
> + if (ret != MTK_DISABLE)
> + err = -EINVAL;
IIUC you are suggesting to assign MTK_DISABLE to ret in the other two
cases,
and then check if ret == MTK_DISABLE.
Thanks for pointing that out.
ChenYu
> Thanks
Hi Chen-Yu
Yes, just for pins with config of MTK_PUPD_SET_R1R0_00 are required to
do additional assignment operations(ret = MTK_DISABLE;), in the other
two cases, the assignment operations of ret as MTK_DISABLE is obtained
by function call "hw->soc->bias_get_combo(hw, desc, &pullup, &ret)".
Thanks
> } else if (param == PIN_CONFIG_BIAS_PULL_UP)
> {
> /* When desire to get pull-up value,
> return
> * error if current setting is pull-
> down
>