Re: [PATCH v13 4/5] pinctrl: mediatek: support rsel feature

From: Chen-Yu Tsai
Date: Thu Sep 23 2021 - 03:36:05 EST

Next message: Leonard Crestez: "Re: [PATCH 00/19] tcp: Initial support for RFC5925 auth option"
Previous message: Laurent Vivier: "Re: [PATCH 1/4] hwrng: virtio - add an internal buffer"
In reply to: Zhiyong Tao: "[PATCH v13 4/5] pinctrl: mediatek: support rsel feature"
Next in thread: Zhiyong Tao: "[PATCH v13 2/5] dt-bindings: pinctrl: mt8195: change pull up/down description"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Wed, Sep 22, 2021 at 10:59 AM Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx> wrote:
>
> This patch supports rsel(resistance selection) feature for I2C pins.
> It provides more resistance selection solution in different ICs.
> It provides rsel define and si unit solution by identifying
> "mediatek,rsel_resistance_in_si_unit" property in pio dtsi node.
>
> Signed-off-by: Zhiyong Tao <zhiyong.tao@xxxxxxxxxxxx>
> ---
> .../pinctrl/mediatek/pinctrl-mtk-common-v2.c | 231 +++++++++++++++---
> .../pinctrl/mediatek/pinctrl-mtk-common-v2.h | 45 ++++
> drivers/pinctrl/mediatek/pinctrl-paris.c | 60 +++--
> 3 files changed, 288 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index 5b3b048725cc..e84001923aaf 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -661,6 +661,181 @@ static int mtk_pinconf_bias_set_pupd_r1_r0(struct mtk_pinctrl *hw,
> return err;
> }
>
> +static int mtk_hw_pin_rsel_lookup(struct mtk_pinctrl *hw,
> + const struct mtk_pin_desc *desc,
> + u32 pullup, u32 arg, u32 *rsel_val)
> +{
> + const struct mtk_pin_rsel *rsel;
> + int check;
> + bool found = false;
> +
> + rsel = hw->soc->pin_rsel;
> +
> + for (check = 0; check <= hw->soc->npin_rsel - 1; check++) {
> + if (desc->number >= rsel[check].s_pin &&
> + desc->number <= rsel[check].e_pin) {
> + if (pullup) {
> + if (rsel[check].up_rsel == arg) {
> + found = true;
> + *rsel_val = rsel[check].rsel_index;
> + break;

The code could simply `return 0` after setting *rsel_val, then we don't have
to have the `found` variable.

Unless your goal is to use the last matching value in the case of duplicates,
instead of the first. If that is the case you should add a comment stating
so along with the reason,

And the structure could be written as

if (pin not in range)
continue;

... check value ...

which would decrease the nesting level. Mostly stylistic though.

> + }
> + } else {
> + if (rsel[check].down_rsel == arg) {
> + found = true;
> + *rsel_val = rsel[check].rsel_index;
> + break;
> + }
> + }
> + }
> + }
> +
> + if (!found) {
> + dev_err(hw->dev, "Not support rsel value %d Ohm for pin = %d (%s)\n",
> + arg, desc->number, desc->name);
> + return -ENOTSUPP;
> + }
> +
> + return 0;
> +}
> +

[...]

> +static int mtk_pinconf_bias_get_rsel(struct mtk_pinctrl *hw,
> + const struct mtk_pin_desc *desc,
> + u32 *pullup, u32 *enable)
> +{
> + int pu, pd, rsel, err;
> +
> + err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_RSEL, &rsel);
> + if (err)
> + goto out;
> +
> + err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_PU, &pu);
> + if (err)
> + goto out;
> +
> + err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_PD, &pd);

mtk_pinconf_bias_get_pu_pd() couldn't be reused?

> +
> + if (pu == 0 && pd == 0) {
> + *pullup = 0;
> + *enable = MTK_DISABLE;
> + } else if (pu == 1 && pd == 0) {
> + *pullup = 1;
> + if (hw->rsel_si_unit)
> + mtk_rsel_get_si_unit(hw, desc, *pullup, rsel, enable);
> + else
> + *enable = rsel + MTK_PULL_SET_RSEL_000;
> + } else if (pu == 0 && pd == 1) {
> + *pullup = 0;
> + if (hw->rsel_si_unit)
> + mtk_rsel_get_si_unit(hw, desc, *pullup, rsel, enable);
> + else
> + *enable = rsel + MTK_PULL_SET_RSEL_000;
> + } else {
> + err = -EINVAL;
> + goto out;
> + }
> +
> +out:
> + return err;
> +}
> +
> static int mtk_pinconf_bias_get_pu_pd(struct mtk_pinctrl *hw,
> const struct mtk_pin_desc *desc,
> u32 *pullup, u32 *enable)
> @@ -742,44 +917,40 @@ static int mtk_pinconf_bias_get_pupd_r1_r0(struct mtk_pinctrl *hw,
> return err;
> }
>
> -int mtk_pinconf_bias_set_combo(struct mtk_pinctrl *hw,
> - const struct mtk_pin_desc *desc,
> - u32 pullup, u32 arg)
> -{
> - int err;
> -
> - err = mtk_pinconf_bias_set_pu_pd(hw, desc, pullup, arg);
> - if (!err)
> - goto out;
> -
> - err = mtk_pinconf_bias_set_pullsel_pullen(hw, desc, pullup, arg);
> - if (!err)
> - goto out;
> -
> - err = mtk_pinconf_bias_set_pupd_r1_r0(hw, desc, pullup, arg);
> -
> -out:
> - return err;
> -}
> -EXPORT_SYMBOL_GPL(mtk_pinconf_bias_set_combo);
> -
> int mtk_pinconf_bias_get_combo(struct mtk_pinctrl *hw,
> const struct mtk_pin_desc *desc,
> u32 *pullup, u32 *enable)
> {
> - int err;
> + int err = -ENOTSUPP;
> + u32 try_all_type;
>
> - err = mtk_pinconf_bias_get_pu_pd(hw, desc, pullup, enable);
> - if (!err)
> - goto out;
> + if (hw->soc->pull_type)
> + try_all_type = hw->soc->pull_type[desc->number];
> + else
> + try_all_type = MTK_PULL_TYPE_MASK;
>
> - err = mtk_pinconf_bias_get_pullsel_pullen(hw, desc, pullup, enable);
> - if (!err)
> - goto out;
> + if (try_all_type & MTK_PULL_RSEL_TYPE) {
> + err = mtk_pinconf_bias_get_rsel(hw, desc, pullup, enable);
> + if (!err)
> + return err;
> + }
>
> - err = mtk_pinconf_bias_get_pupd_r1_r0(hw, desc, pullup, enable);
> + if (try_all_type & MTK_PULL_PU_PD_TYPE) {
> + err = mtk_pinconf_bias_get_pu_pd(hw, desc, pullup, enable);
> + if (!err)
> + return err;
> + }
> +
> + if (try_all_type & MTK_PULL_PULLSEL_TYPE) {
> + err = mtk_pinconf_bias_get_pullsel_pullen(hw, desc,
> + pullup, enable);
> + if (!err)
> + return err;
> + }
> +
> + if (try_all_type & MTK_PULL_PUPD_R1R0_TYPE)
> + err = mtk_pinconf_bias_get_pupd_r1_r0(hw, desc, pullup, enable);
>
> -out:
> return err;
> }
> EXPORT_SYMBOL_GPL(mtk_pinconf_bias_get_combo);
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h
> index a6f1bdb2083b..4908c7aedbe0 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.h
> @@ -17,6 +17,22 @@
> #define MTK_ENABLE 1
> #define MTK_PULLDOWN 0
> #define MTK_PULLUP 1
> +#define MTK_PULL_PU_PD_TYPE BIT(0)
> +#define MTK_PULL_PULLSEL_TYPE BIT(1)
> +#define MTK_PULL_PUPD_R1R0_TYPE BIT(2)
> +/* MTK_PULL_RSEL_TYPE can select resistance and can be
> + * turned on/off itself. But it can't be selected pull up/down
> + */
> +#define MTK_PULL_RSEL_TYPE BIT(3)
> +/* MTK_PULL_PU_PD_RSEL_TYPE is a type which is controlled by
> + * MTK_PULL_PU_PD_TYPE and MTK_PULL_RSEL_TYPE.
> + */
> +#define MTK_PULL_PU_PD_RSEL_TYPE (MTK_PULL_PU_PD_TYPE \
> + | MTK_PULL_RSEL_TYPE)
> +#define MTK_PULL_TYPE_MASK (MTK_PULL_PU_PD_TYPE |\
> + MTK_PULL_PULLSEL_TYPE |\
> + MTK_PULL_PUPD_R1R0_TYPE |\
> + MTK_PULL_RSEL_TYPE)
>
> #define EINT_NA U16_MAX
> #define NO_EINT_SUPPORT EINT_NA
> @@ -42,6 +58,14 @@
> PIN_FIELD_CALC(_s_pin, _e_pin, 0, _s_addr, _x_addrs, _s_bit, \
> _x_bits, 32, 1)
>
> +#define PIN_RSEL(_s_pin, _e_pin, _rsel_index, _up_resl, _down_rsel) { \
> + .s_pin = _s_pin, \
> + .e_pin = _e_pin, \
> + .rsel_index = _rsel_index, \
> + .up_rsel = _up_resl, \
> + .down_rsel = _down_rsel, \
> + }
> +
> /* List these attributes which could be modified for the pin */
> enum {
> PINCTRL_PIN_REG_MODE,
> @@ -67,6 +91,7 @@ enum {
> PINCTRL_PIN_REG_DRV_E0,
> PINCTRL_PIN_REG_DRV_E1,
> PINCTRL_PIN_REG_DRV_ADV,
> + PINCTRL_PIN_REG_RSEL,
> PINCTRL_PIN_REG_MAX,
> };
>
> @@ -129,6 +154,21 @@ struct mtk_pin_field_calc {
> u8 fixed;
> };
>
> +/* struct mtk_pin_rsel - the structure that provides bias resistance selection.

Since you went through the trouble of documenting all the fields, would
you consider changing this to a kernel-doc style comment? It is similar
to Java-doc, and would be like:

/**
* struct mtk_pin_rsel ......
* @s_pin: ....
* ...
*/

Only the start of the comment block needs to be changed.
See Documentation/doc-guide/kernel-doc.rst if you are unsure.

> + * @s_pin: the start pin within the rsel range
> + * @e_pin: the end pin within the rsel range
> + * @rsel_index: the rsel bias resistance index
> + * @up_rsel: the pullup rsel bias resistance value
> + * @down_rsel: the pulldown rsel bias resistance value
> + */
> +struct mtk_pin_rsel {
> + u16 s_pin;
> + u16 e_pin;
> + u16 rsel_index;
> + u32 up_rsel;
> + u32 down_rsel;
> +};
> +
> /* struct mtk_pin_reg_calc - the structure that holds all ranges used to
> * determine which register the pin would make use of
> * for certain pin attribute.
> @@ -206,6 +246,9 @@ struct mtk_pin_soc {
> bool ies_present;
> const char * const *base_names;
> unsigned int nbase_names;
> + const unsigned int *pull_type;
> + const struct mtk_pin_rsel *pin_rsel;
> + unsigned int npin_rsel;
>
> /* Specific pinconfig operations */
> int (*bias_disable_set)(struct mtk_pinctrl *hw,
> @@ -254,6 +297,8 @@ struct mtk_pinctrl {
> const char **grp_names;
> /* lock pin's register resource to avoid multiple threads issue*/
> spinlock_t lock;
> + /* identify rsel setting by si unit or rsel define in dts node */
> + bool rsel_si_unit;
> };
>
> void mtk_rmw(struct mtk_pinctrl *pctl, u8 i, u32 reg, u32 mask, u32 set);
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 38aec0177d15..d4e02c5d74a8 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -579,8 +579,9 @@ static int mtk_hw_get_value_wrap(struct mtk_pinctrl *hw, unsigned int gpio, int
> ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> unsigned int gpio, char *buf, unsigned int buf_len)
> {
> - int pinmux, pullup, pullen, len = 0, r1 = -1, r0 = -1;
> + int pinmux, pullup, pullen, len = 0, r1 = -1, r0 = -1, rsel = -1;
> const struct mtk_pin_desc *desc;
> + u32 try_all_type;
>
> if (gpio >= hw->soc->npins)
> return -EINVAL;
> @@ -591,24 +592,39 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> pinmux -= hw->soc->nfuncs;
>
> mtk_pinconf_bias_get_combo(hw, desc, &pullup, &pullen);
> - if (pullen == MTK_PUPD_SET_R1R0_00) {
> - pullen = 0;
> - r1 = 0;
> - r0 = 0;
> - } else if (pullen == MTK_PUPD_SET_R1R0_01) {
> - pullen = 1;
> - r1 = 0;
> - r0 = 1;
> - } else if (pullen == MTK_PUPD_SET_R1R0_10) {
> - pullen = 1;
> - r1 = 1;
> - r0 = 0;
> - } else if (pullen == MTK_PUPD_SET_R1R0_11) {
> +
> + if (hw->soc->pull_type)
> + try_all_type = hw->soc->pull_type[desc->number];
> +
> + if (hw->rsel_si_unit && (try_all_type & MTK_PULL_RSEL_TYPE)) {
> + rsel = pullen;
> pullen = 1;
> - r1 = 1;
> - r0 = 1;
> - } else if (pullen != MTK_DISABLE && pullen != MTK_ENABLE) {
> - pullen = 0;
> + } else {
> + /* Case for: R1R0 */
> + if (pullen == MTK_PUPD_SET_R1R0_00) {
> + pullen = 0;
> + r1 = 0;
> + r0 = 0;
> + } else if (pullen == MTK_PUPD_SET_R1R0_01) {
> + pullen = 1;
> + r1 = 0;
> + r0 = 1;
> + } else if (pullen == MTK_PUPD_SET_R1R0_10) {
> + pullen = 1;
> + r1 = 1;
> + r0 = 0;
> + } else if (pullen == MTK_PUPD_SET_R1R0_11) {
> + pullen = 1;
> + r1 = 1;
> + r0 = 1;
> + }
> +
> + /* Case for: RSEL */
> + if (pullen >= MTK_PULL_SET_RSEL_000 &&
> + pullen <= MTK_PULL_SET_RSEL_111) {
> + rsel = pullen - MTK_PULL_SET_RSEL_000;
> + pullen = 1;
> + }
> }
> len += scnprintf(buf + len, buf_len - len,
> "%03d: %1d%1d%1d%1d%02d%1d%1d%1d%1d",
> @@ -626,6 +642,8 @@ ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> if (r1 != -1) {
> len += scnprintf(buf + len, buf_len - len, " (%1d %1d)\n",
> 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");
> }
> @@ -970,6 +988,12 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev,
>
> hw->nbase = hw->soc->nbase_names;
>
> + if (of_find_property(hw->dev->of_node,
> + "mediatek,rsel_resistance_in_si_unit", NULL))

This new property should be documented in the bindings.

Regards
ChenYu

> + hw->rsel_si_unit = true;
> + else
> + hw->rsel_si_unit = false;
> +
> spin_lock_init(&hw->lock);
>
> err = mtk_pctrl_build_state(pdev);
> --
> 2.25.1
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Next message: Leonard Crestez: "Re: [PATCH 00/19] tcp: Initial support for RFC5925 auth option"
Previous message: Laurent Vivier: "Re: [PATCH 1/4] hwrng: virtio - add an internal buffer"
In reply to: Zhiyong Tao: "[PATCH v13 4/5] pinctrl: mediatek: support rsel feature"
Next in thread: Zhiyong Tao: "[PATCH v13 2/5] dt-bindings: pinctrl: mt8195: change pull up/down description"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]