| Message ID | 20220111112244.1483783-8-wenst@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: mediatek: Fixes and minor improvements | expand |
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 <wenst@chromium.org> > --- > > 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: > 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 <wenst@chromium.org> > --- > > 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 7:23 PM Chen-Yu Tsai <wenst@chromium.org> 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 <wenst@chromium.org> > --- > > 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 >
Hi, On Tue, Jan 18, 2022 at 10:36 AM zhiyong.tao <zhiyong.tao@mediatek.com> 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 <wenst@chromium.org> > > --- > > > > 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) ? >
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) ?
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 <wenst@chromium.org> --- 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(+)