| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXv+5Fp0coJn1zVHZU_TWQyqtuc0EEPU4a7uaEsjX6u6EEhzA@mail.gmail.com>
Date: Thu, 23 Sep 2021 15:35:49 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Zhiyong Tao <zhiyong.tao@...iatek.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
Mark Rutland <mark.rutland@....com>,
Matthias Brugger <matthias.bgg@...il.com>,
Sean Wang <sean.wang@...nel.org>,
srv_heupstream <srv_heupstream@...iatek.com>,
hui.liu@...iatek.com, Light Hsieh <light.hsieh@...iatek.com>,
Biao Huang <biao.huang@...iatek.com>,
Hongzhou Yang <hongzhou.yang@...iatek.com>,
Sean Wang <sean.wang@...iatek.com>,
Seiya Wang <seiya.wang@...iatek.com>,
Devicetree List <devicetree@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v13 4/5] pinctrl: mediatek: support rsel feature
Hi,
On Wed, Sep 22, 2021 at 10:59 AM Zhiyong Tao <zhiyong.tao@...iatek.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@...iatek.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.
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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Powered by blists - more mailing lists