Re: [PATCH v6 3/5] pinctrl: mediatek: Refine mtk_pinconf_get() and mtk_pinconf_set()
Hi,
On Thu, Sep 26, 2019 at 10:02 PM Light Hsieh wrote:
>
> 1.Refine mtk_pinconf_get():
> 1.1 Use only one occurrence of return at end of this function.
> 1.2 Correct cases for PIN_CONFIG_SLEW_RATE, PIN_CONFIG_INPUT_SCHMITT_ENABLE,
If you want to fix it a bug, you should submit a separate patch for
that and don't mix fixups and improvements in one.
> and PIN_CONFIG_OUTPUT_ENABLE -
> Use variable ret to receive value in mtk_hw_get_value() (instead of
> variable val) since pinconf_to_config_packed() at end of this function
> use variable ret to pack config value.
Is that a fixup or an improvement?
>
> 2.Refine mtk_pinconf_set():
> 2.1 Use only one occurrence of return at end of this function.
> 2.2 Modify case of PIN_CONFIG_INPUT_ENABLE -
> Remove check of ies_present flag and always invoke mtk_hw_set_value()
> since mtk_hw_pin_field_lookup() invoked inside mtk_hw_set_value() has
> the same effect of checking if ies control is supported.
> [The rationale is that: available of a control is always checked
> in mtk_hw_pin_field_lookup() and no need to add ies_present flag
> specially for ies control.]
> 2.3 Simply code logic for case of PIN_CONFIG_INPUT_SCHMITT.
> 2.4 Add case for PIN_CONFIG_INPUT_SCHMITT_ENABLE and process it with the
> same code for case of PIN_CONFIG_INPUT_SCHMITT.
Remember that one patch only does one thing so that please split the
patch you proposed here to smaller patches in the appropriate group
which are pointed out by that is either a fixup and an improvement.
>
There are many missing tags
> ---
> drivers/pinctrl/mediatek/pinctrl-mt6765.c | 1 -
> drivers/pinctrl/mediatek/pinctrl-paris.c | 211
> +++---
> 2 files changed, 79 insertions(+), 133 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6765.c
> b/drivers/pinctrl/mediatek/pinctrl-mt6765.c
> index e024ebc..bada37f 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt6765.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt6765.c
> @@ -1070,7 +1070,6 @@
> .ngrps = ARRAY_SIZE(mtk_pins_mt6765),
> .eint_hw = _eint_hw,
> .gpio_m = 0,
> - .ies_present = true,
> .base_names = mt6765_pinctrl_register_base_names,
> .nbase_names = ARRAY_SIZE(mt6765_pinctrl_register_base_names),
> .bias_disable_set = mtk_pinconf_bias_disable_set,
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
> b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 5217f76..54f069b 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -78,95 +78,79 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> {
> struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
> u32 param = pinconf_to_config_param(*config);
> - int val, val2, err, reg, ret = 1;
> + int err, reg, ret = 1;
> const struct mtk_pin_desc *desc;
>
> - if (pin >= hw->soc->npins)
> - return -EINVAL;
> + if (pin >= hw->soc->npins) {
> + err = -EINVAL;
> + goto out;
> + }
> desc = (const struct mtk_pin_desc *)>soc->pins[pin];
>
> switch (param) {
> case PIN_CONFIG_BIAS_DISABLE:
> - if (hw->soc->bias_disable_get) {
> + if (hw->soc->bias_disable_get)
> err = hw->soc->bias_disable_get(hw, desc, );
> - if (err)
> - return err;
> - } else {
> - return -ENOTSUPP;
> - }
> + else
> + err = -ENOTSUPP;
> break;
> case PIN_CONFIG_BIAS_PULL_UP:
> - if (hw->soc->bias_get) {
> + if (hw->soc->bias_get)
> err = hw->soc->bias_get(hw, desc, 1, );
> - if (err)
> - return err;
> - } else {
> - return -ENOTSUPP;
> - }
> + else
> + err = -ENOTSUPP;
> break;
> case PIN_CONFIG_BIAS_PULL_DOWN:
> - if (hw->soc->bias_get) {
> + if (hw->soc->bias_get)
> err = hw->soc->bias_get(hw, desc, 0, );
> - if (err)
> - return err;
> - } else {
> - return -ENOTSUPP;
> - }
> + else
> + err = -ENOTSUPP;
> break;
> case PIN_CONFIG_SLEW_RATE:
> - err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, );
> - if (err)
> - return err;
> -
> - if (!val)
> - return -EINVAL;
> -
> + err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, );
> break;
> case PIN_CONFIG_INPUT_ENABLE:
>
[PATCH v6 3/5] pinctrl: mediatek: Refine mtk_pinconf_get() and mtk_pinconf_set()
1.Refine mtk_pinconf_get():
1.1 Use only one occurrence of return at end of this function.
1.2 Correct cases for PIN_CONFIG_SLEW_RATE, PIN_CONFIG_INPUT_SCHMITT_ENABLE,
and PIN_CONFIG_OUTPUT_ENABLE -
Use variable ret to receive value in mtk_hw_get_value() (instead of
variable val) since pinconf_to_config_packed() at end of this function
use variable ret to pack config value.
2.Refine mtk_pinconf_set():
2.1 Use only one occurrence of return at end of this function.
2.2 Modify case of PIN_CONFIG_INPUT_ENABLE -
Remove check of ies_present flag and always invoke mtk_hw_set_value()
since mtk_hw_pin_field_lookup() invoked inside mtk_hw_set_value() has
the same effect of checking if ies control is supported.
[The rationale is that: available of a control is always checked
in mtk_hw_pin_field_lookup() and no need to add ies_present flag
specially for ies control.]
2.3 Simply code logic for case of PIN_CONFIG_INPUT_SCHMITT.
2.4 Add case for PIN_CONFIG_INPUT_SCHMITT_ENABLE and process it with the
same code for case of PIN_CONFIG_INPUT_SCHMITT.
---
drivers/pinctrl/mediatek/pinctrl-mt6765.c | 1 -
drivers/pinctrl/mediatek/pinctrl-paris.c | 211 +++---
2 files changed, 79 insertions(+), 133 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-mt6765.c
b/drivers/pinctrl/mediatek/pinctrl-mt6765.c
index e024ebc..bada37f 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mt6765.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mt6765.c
@@ -1070,7 +1070,6 @@
.ngrps = ARRAY_SIZE(mtk_pins_mt6765),
.eint_hw = _eint_hw,
.gpio_m = 0,
- .ies_present = true,
.base_names = mt6765_pinctrl_register_base_names,
.nbase_names = ARRAY_SIZE(mt6765_pinctrl_register_base_names),
.bias_disable_set = mtk_pinconf_bias_disable_set,
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c
b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 5217f76..54f069b 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -78,95 +78,79 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
{
struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
u32 param = pinconf_to_config_param(*config);
- int val, val2, err, reg, ret = 1;
+ int err, reg, ret = 1;
const struct mtk_pin_desc *desc;
- if (pin >= hw->soc->npins)
- return -EINVAL;
+ if (pin >= hw->soc->npins) {
+ err = -EINVAL;
+ goto out;
+ }
desc = (const struct mtk_pin_desc *)>soc->pins[pin];
switch (param) {
case PIN_CONFIG_BIAS_DISABLE:
- if (hw->soc->bias_disable_get) {
+ if (hw->soc->bias_disable_get)
err = hw->soc->bias_disable_get(hw, desc, );
- if (err)
- return err;
- } else {
- return -ENOTSUPP;
- }
+ else
+ err = -ENOTSUPP;
break;
case PIN_CONFIG_BIAS_PULL_UP:
- if (hw->soc->bias_get) {
+ if (hw->soc->bias_get)
err = hw->soc->bias_get(hw, desc, 1, );
- if (err)
- return err;
- } else {
- return -ENOTSUPP;
- }
+ else
+ err = -ENOTSUPP;
break;
case PIN_CONFIG_BIAS_PULL_DOWN:
- if (hw->soc->bias_get) {
+ if (hw->soc->bias_get)
err = hw->soc->bias_get(hw, desc, 0, );
- if (err)
- return err;
- } else {
- return -ENOTSUPP;
- }
+ else
+ err = -ENOTSUPP;
break;
case PIN_CONFIG_SLEW_RATE:
- err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, );
- if (err)
- return err;
-
- if (!val)
- return -EINVAL;
-
+ err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_SR, );
break;
case PIN_CONFIG_INPUT_ENABLE:
case PIN_CONFIG_OUTPUT_ENABLE:
- err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, );
+ err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, );
if (err)
- return err;
-
- /* HW takes input mode as zero; output mode as non-zero */
- if ((val && param == PIN_CONFIG_INPUT_ENABLE) ||
- (!val && param == PIN_CONFIG_OUTPUT_ENABLE))
- return -EINVAL;
+ goto out;
+ /* CONFIG Current direction return value
+* - - --
+
