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

zhiyong.tao zhiyong.tao at mediatek.com
Thu Sep 23 01:38:40 PDT 2021


On Thu, 2021-09-23 at 15:35 +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Wed, Sep 22, 2021 at 10:59 AM Zhiyong Tao <
> zhiyong.tao at mediatek.com> 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 at mediatek.com>
> > ---
> >  .../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.

We use "found" variable to identify whether user set the right si unit
value or not . 

> 
> 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?

It couldn't be reused. Because in the api, if user use rsel si unit, it
can return si unit value, if user use rsel define, it can return rsel
define value.

> 
> > +
> > +       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.

we will fix it in the next version.

> 
> > + * @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.
> 
we will add it in the next version.

> 
> 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 at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek


More information about the Linux-mediatek mailing list