2019-09-27 05:04:55

by Light Hsieh (謝明燈)

[permalink] [raw]
Subject: [PATCH v6 1/5] pinctrl: mediatek: Check gpio pin number and use binary search in mtk_hw_pin_field_lookup()

1. Check if gpio pin number is in valid range to prevent from get invalid
pointer 'desc' in the following code:
desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];

2. Use binary search in mtk_hw_pin_field_lookup()
Modify mtk_hw_pin_field_lookup() to use binary search for accelerating
search.

---
drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 25 +++++++++++++++++++-----
drivers/pinctrl/mediatek/pinctrl-paris.c | 25 ++++++++++++++++++++++++
2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
index 20e1c89..8077855 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
@@ -68,7 +68,8 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
{
const struct mtk_pin_field_calc *c, *e;
const struct mtk_pin_reg_calc *rc;
- u32 bits;
+ u32 bits, found = 0;
+ int start = 0, end, check;

if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) {
rc = &hw->soc->reg_cal[field];
@@ -79,21 +80,32 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
return -ENOTSUPP;
}

+ end = rc->nranges - 1;
c = rc->range;
e = c + rc->nranges;

- while (c < e) {
- if (desc->number >= c->s_pin && desc->number <= c->e_pin)
+ while (start <= end) {
+ check = (start + end) >> 1;
+ if (desc->number >= rc->range[check].s_pin
+ && desc->number <= rc->range[check].e_pin) {
+ found = 1;
break;
- c++;
+ } else if (start == end)
+ break;
+ else if (desc->number < rc->range[check].s_pin)
+ end = check - 1;
+ else
+ start = check + 1;
}

- if (c >= e) {
+ if (!found) {
dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n",
field, desc->number, desc->name);
return -ENOTSUPP;
}

+ c = rc->range + check;
+
if (c->i_base > hw->nbase - 1) {
dev_err(hw->dev,
"Invalid base for field %d for pin = %d (%s)\n",
@@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc,
if (err)
return err;

+ if (value < 0 || value > pf.mask)
+ return -EINVAL;
+
if (!pf.next)
mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos,
(value & pf.mask) << pf.bitpos);
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 923264d..3e13ae7 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
int val, val2, err, reg, ret = 1;
const struct mtk_pin_desc *desc;

+ if (pin >= hw->soc->npins)
+ return -EINVAL;
desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];

switch (param) {
@@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
int err = 0;
u32 reg;

+ if (pin >= hw->soc->npins) {
+ err = -EINVAL;
+ goto err;
+ }
desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];

switch ((u32)param) {
@@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
const struct mtk_pin_desc *desc;
int value, err;

+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];

err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value);
@@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio)
const struct mtk_pin_desc *desc;
int value, err;

+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];

err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value);
@@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
struct mtk_pinctrl *hw = gpiochip_get_data(chip);
const struct mtk_pin_desc *desc;

+ if (gpio > hw->soc->npins)
+ return;
+
desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];

mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value);
@@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)

static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
{
+ struct mtk_pinctrl *hw = gpiochip_get_data(chip);
+
+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
return pinctrl_gpio_direction_input(chip->base + gpio);
}

static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio,
int value)
{
+ struct mtk_pinctrl *hw = gpiochip_get_data(chip);
+
+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
mtk_gpio_set(chip, gpio, value);

return pinctrl_gpio_direction_output(chip->base + gpio);
--
1.8.1.1.dirty


2019-09-27 05:08:13

by Light Hsieh (謝明燈)

[permalink] [raw]
Subject: [PATCH v6 5/5] pinctrl: mediatek: Add support for pin configuration dump via debugfs.

Add support for pin configuration dump via catting
/sys/kernel/debug/pinctrl/$platform_dependent_path/pinconf-pins.
pinctrl framework had already support such dump. This patch implement the
operation function pointer to fullfill this dump.

---
drivers/pinctrl/mediatek/pinctrl-paris.c | 88 ++++++++++++++++++++++++++++++++
drivers/pinctrl/mediatek/pinctrl-paris.h | 30 +++++++++++
2 files changed, 118 insertions(+)

diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
index 2a47c45..f531908 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.c
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
@@ -538,12 +538,99 @@ static int mtk_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
return 0;
}

+int mtk_hw_get_value_wrap(struct mtk_pinctrl *hw, unsigned int gpio, int field)
+{
+ const struct mtk_pin_desc *desc;
+ int value, err;
+
+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
+ desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
+
+ err = mtk_hw_get_value(hw, desc, field, &value);
+ if (err)
+ return err;
+
+ return value;
+}
+
+ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
+ unsigned int gpio, char *buf, unsigned int bufLen)
+{
+ const struct mtk_pin_desc *desc;
+ int pinmux, pullup, pullen, r1 = -1, r0 = -1, len = 0;
+
+ if (gpio > hw->soc->npins)
+ return -EINVAL;
+
+ desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
+ pinmux = mtk_pctrl_get_pinmux(hw, gpio);
+ if (pinmux >= hw->soc->nfuncs)
+ 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) {
+ pullen = 1;
+ r1 = 1;
+ r0 = 1;
+ } else if (pullen != MTK_DISABLE && pullen != MTK_ENABLE) {
+ pullen = 0;
+ }
+ len += snprintf(buf + len, bufLen - len,
+ "%03d: %1d%1d%1d%1d%02d%1d%1d%1d%1d",
+ gpio,
+ pinmux,
+ mtk_pctrl_get_direction(hw, gpio),
+ mtk_pctrl_get_out(hw, gpio),
+ mtk_pctrl_get_in(hw, gpio),
+ mtk_pctrl_get_driving(hw, gpio),
+ mtk_pctrl_get_smt(hw, gpio),
+ mtk_pctrl_get_ies(hw, gpio),
+ pullen,
+ pullup);
+
+ if (r1 != -1) {
+ len += snprintf(buf + len, bufLen - len, " (%1d %1d)\n",
+ r1, r0);
+ } else {
+ len += snprintf(buf + len, bufLen - len, "\n");
+ }
+
+ return len;
+}
+
+#define PIN_DBG_BUF_SZ 96
+static void mtk_pctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+ unsigned int gpio)
+{
+ struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
+ char buf[PIN_DBG_BUF_SZ];
+
+ (void)mtk_pctrl_show_one_pin(hw, gpio, buf, PIN_DBG_BUF_SZ);
+
+ seq_printf(s, "%s", buf);
+}
+
static const struct pinctrl_ops mtk_pctlops = {
.dt_node_to_map = mtk_pctrl_dt_node_to_map,
.dt_free_map = pinctrl_utils_free_map,
.get_groups_count = mtk_pctrl_get_groups_count,
.get_group_name = mtk_pctrl_get_group_name,
.get_group_pins = mtk_pctrl_get_group_pins,
+ .pin_dbg_show = mtk_pctrl_dbg_show,
};

static int mtk_pmx_get_funcs_cnt(struct pinctrl_dev *pctldev)
@@ -640,6 +727,7 @@ static int mtk_pconf_group_set(struct pinctrl_dev *pctldev, unsigned group,
.pin_config_get = mtk_pinconf_get,
.pin_config_group_get = mtk_pconf_group_get,
.pin_config_group_set = mtk_pconf_group_set,
+ .is_generic = true,
};

static struct pinctrl_desc mtk_desc = {
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.h b/drivers/pinctrl/mediatek/pinctrl-paris.h
index 3d43771..d73f4b6 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.h
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.h
@@ -60,6 +60,36 @@
int mtk_paris_pinctrl_probe(struct platform_device *pdev,
const struct mtk_pin_soc *soc);

+int mtk_hw_get_value_wrap(struct mtk_pinctrl *hw, unsigned int gpio, int field);
+
+#define mtk_pctrl_get_pinmux(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_MODE)
+
+/* MTK HW use 0 as input, 1 for output
+ * This interface is for get direct register value,
+ * so don't reverse
+ */
+#define mtk_pctrl_get_direction(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DIR)
+
+#define mtk_pctrl_get_out(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DO)
+
+#define mtk_pctrl_get_in(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DI)
+
+#define mtk_pctrl_get_smt(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_SMT)
+
+#define mtk_pctrl_get_ies(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_IES)
+
+#define mtk_pctrl_get_driving(hw, gpio) \
+ mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DRV)
+
+ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
+ unsigned int gpio, char *buf, unsigned int bufLen);
+
extern const struct dev_pm_ops mtk_paris_pinctrl_pm_ops;

#endif /* __PINCTRL_PARIS_H */
--
1.8.1.1.dirty

2019-09-27 05:17:44

by Light Hsieh (謝明燈)

[permalink] [raw]
Subject: Re: [PATCH v6 1/5] pinctrl: mediatek: Check gpio pin number and use binary search in mtk_hw_pin_field_lookup()

Dear reviewers:

Patch v6 improves v5 by:

1.in mtk_pinconf_get() and mtk_pinconf_set() @pinctrl-paris.c:
* check if pin is in range before using pin as array index of
hw->soc->pins[]
2.in mtk_pin_field_lookup() @pinctrl-mtk-common-v2.c:
* declear start, end, check as signed integer instead of unsigned
integer. Otherwise, kernel fault will occur when s_pin field of
first entry of a mtk_pin_field_calc[] array is not 0.

On Fri, 2019-09-27 at 13:02 +0800, Light Hsieh wrote:
> 1. Check if gpio pin number is in valid range to prevent from get invalid
> pointer 'desc' in the following code:
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
>
> 2. Use binary search in mtk_hw_pin_field_lookup()
> Modify mtk_hw_pin_field_lookup() to use binary search for accelerating
> search.
>
> ---
> drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 25 +++++++++++++++++++-----
> drivers/pinctrl/mediatek/pinctrl-paris.c | 25 ++++++++++++++++++++++++
> 2 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index 20e1c89..8077855 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -68,7 +68,8 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
> {
> const struct mtk_pin_field_calc *c, *e;
> const struct mtk_pin_reg_calc *rc;
> - u32 bits;
> + u32 bits, found = 0;
> + int start = 0, end, check;
>
> if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) {
> rc = &hw->soc->reg_cal[field];
> @@ -79,21 +80,32 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
> return -ENOTSUPP;
> }
>
> + end = rc->nranges - 1;
> c = rc->range;
> e = c + rc->nranges;
>
> - while (c < e) {
> - if (desc->number >= c->s_pin && desc->number <= c->e_pin)
> + while (start <= end) {
> + check = (start + end) >> 1;
> + if (desc->number >= rc->range[check].s_pin
> + && desc->number <= rc->range[check].e_pin) {
> + found = 1;
> break;
> - c++;
> + } else if (start == end)
> + break;
> + else if (desc->number < rc->range[check].s_pin)
> + end = check - 1;
> + else
> + start = check + 1;
> }
>
> - if (c >= e) {
> + if (!found) {
> dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n",
> field, desc->number, desc->name);
> return -ENOTSUPP;
> }
>
> + c = rc->range + check;
> +
> if (c->i_base > hw->nbase - 1) {
> dev_err(hw->dev,
> "Invalid base for field %d for pin = %d (%s)\n",
> @@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc,
> if (err)
> return err;
>
> + if (value < 0 || value > pf.mask)
> + return -EINVAL;
> +
> if (!pf.next)
> mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos,
> (value & pf.mask) << pf.bitpos);
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 923264d..3e13ae7 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> int val, val2, err, reg, ret = 1;
> const struct mtk_pin_desc *desc;
>
> + if (pin >= hw->soc->npins)
> + return -EINVAL;
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];
>
> switch (param) {
> @@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> int err = 0;
> u32 reg;
>
> + if (pin >= hw->soc->npins) {
> + err = -EINVAL;
> + goto err;
> + }
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];
>
> switch ((u32)param) {
> @@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
> const struct mtk_pin_desc *desc;
> int value, err;
>
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
>
> err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value);
> @@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio)
> const struct mtk_pin_desc *desc;
> int value, err;
>
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
>
> err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value);
> @@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
> struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> const struct mtk_pin_desc *desc;
>
> + if (gpio > hw->soc->npins)
> + return;
> +
> desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
>
> mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value);
> @@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
>
> static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
> {
> + struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> +
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> return pinctrl_gpio_direction_input(chip->base + gpio);
> }
>
> static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio,
> int value)
> {
> + struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> +
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> mtk_gpio_set(chip, gpio, value);
>
> return pinctrl_gpio_direction_output(chip->base + gpio);


2019-09-27 17:26:47

by Sean Wang

[permalink] [raw]
Subject: Re: [PATCH v6 1/5] pinctrl: mediatek: Check gpio pin number and use binary search in mtk_hw_pin_field_lookup()

On Thu, Sep 26, 2019 at 10:14 PM Light Hsieh <[email protected]> wrote:
>
> Dear reviewers:
>
> Patch v6 improves v5 by:
>
> 1.in mtk_pinconf_get() and mtk_pinconf_set() @pinctrl-paris.c:
> * check if pin is in range before using pin as array index of
> hw->soc->pins[]
> 2.in mtk_pin_field_lookup() @pinctrl-mtk-common-v2.c:
> * declear start, end, check as signed integer instead of unsigned
> integer. Otherwise, kernel fault will occur when s_pin field of
> first entry of a mtk_pin_field_calc[] array is not 0.
>

And for review these patches easily, you should put the changes
history to the cover letter.

> On Fri, 2019-09-27 at 13:02 +0800, Light Hsieh wrote:
> > 1. Check if gpio pin number is in valid range to prevent from get invalid
> > pointer 'desc' in the following code:
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> >
> > 2. Use binary search in mtk_hw_pin_field_lookup()
> > Modify mtk_hw_pin_field_lookup() to use binary search for accelerating
> > search.
> >

You almost forgot to add the tags from you and the reviewers' feedback.

> > ---
> > drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c | 25 +++++++++++++++++++-----
> > drivers/pinctrl/mediatek/pinctrl-paris.c | 25 ++++++++++++++++++++++++
> > 2 files changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > index 20e1c89..8077855 100644
> > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > @@ -68,7 +68,8 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
> > {
> > const struct mtk_pin_field_calc *c, *e;
> > const struct mtk_pin_reg_calc *rc;
> > - u32 bits;
> > + u32 bits, found = 0;

the type of found seems to be a bool

> > + int start = 0, end, check;
> >
> > if (hw->soc->reg_cal && hw->soc->reg_cal[field].range) {
> > rc = &hw->soc->reg_cal[field];
> > @@ -79,21 +80,32 @@ static int mtk_hw_pin_field_lookup(struct mtk_pinctrl *hw,
> > return -ENOTSUPP;
> > }
> >
> > + end = rc->nranges - 1;
> > c = rc->range;
> > e = c + rc->nranges;
> >
> > - while (c < e) {
> > - if (desc->number >= c->s_pin && desc->number <= c->e_pin)
> > + while (start <= end) {
> > + check = (start + end) >> 1;
> > + if (desc->number >= rc->range[check].s_pin
> > + && desc->number <= rc->range[check].e_pin) {
> > + found = 1;
> > break;
> > - c++;
> > + } else if (start == end)
> > + break;
> > + else if (desc->number < rc->range[check].s_pin)
> > + end = check - 1;
> > + else
> > + start = check + 1;
> > }
> >

yuh, it is good to do do a binary search here

> > - if (c >= e) {
> > + if (!found) {
> > dev_dbg(hw->dev, "Not support field %d for pin = %d (%s)\n",
> > field, desc->number, desc->name);
> > return -ENOTSUPP;
> > }
> >
> > + c = rc->range + check;
> > +
> > if (c->i_base > hw->nbase - 1) {
> > dev_err(hw->dev,
> > "Invalid base for field %d for pin = %d (%s)\n",
> > @@ -182,6 +194,9 @@ int mtk_hw_set_value(struct mtk_pinctrl *hw, const struct mtk_pin_desc *desc,
> > if (err)
> > return err;
> >
> > + if (value < 0 || value > pf.mask)
> > + return -EINVAL;
> > +
> > if (!pf.next)
> > mtk_rmw(hw, pf.index, pf.offset, pf.mask << pf.bitpos,
> > (value & pf.mask) << pf.bitpos);
> > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > index 923264d..3e13ae7 100644
> > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> > @@ -81,6 +81,8 @@ static int mtk_pinconf_get(struct pinctrl_dev *pctldev,
> > int val, val2, err, reg, ret = 1;
> > const struct mtk_pin_desc *desc;
> >
> > + if (pin >= hw->soc->npins)
> > + return -EINVAL;
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];
> >
> > switch (param) {
> > @@ -206,6 +208,10 @@ static int mtk_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > int err = 0;
> > u32 reg;
> >
> > + if (pin >= hw->soc->npins) {
> > + err = -EINVAL;
> > + goto err;
> > + }
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[pin];
> >
> > switch ((u32)param) {
> > @@ -693,6 +699,9 @@ static int mtk_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
> > const struct mtk_pin_desc *desc;
> > int value, err;
> >
> > + if (gpio > hw->soc->npins)
> > + return -EINVAL;
> > +
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> >
> > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DIR, &value);
> > @@ -708,6 +717,9 @@ static int mtk_gpio_get(struct gpio_chip *chip, unsigned int gpio)
> > const struct mtk_pin_desc *desc;
> > int value, err;
> >
> > + if (gpio > hw->soc->npins)
> > + return -EINVAL;
> > +
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> >
> > err = mtk_hw_get_value(hw, desc, PINCTRL_PIN_REG_DI, &value);
> > @@ -722,6 +734,9 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
> > struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> > const struct mtk_pin_desc *desc;
> >
> > + if (gpio > hw->soc->npins)
> > + return;
> > +
> > desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> >
> > mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_DO, !!value);
> > @@ -729,12 +744,22 @@ static void mtk_gpio_set(struct gpio_chip *chip, unsigned int gpio, int value)
> >
> > static int mtk_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
> > {
> > + struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> > +
> > + if (gpio > hw->soc->npins)
> > + return -EINVAL;
> > +
> > return pinctrl_gpio_direction_input(chip->base + gpio);
> > }
> >
> > static int mtk_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio,
> > int value)
> > {
> > + struct mtk_pinctrl *hw = gpiochip_get_data(chip);
> > +
> > + if (gpio > hw->soc->npins)
> > + return -EINVAL;
> > +
> > mtk_gpio_set(chip, gpio, value);
> >
> > return pinctrl_gpio_direction_output(chip->base + gpio);
>
>

2019-09-27 22:12:12

by Sean Wang

[permalink] [raw]
Subject: Re: [PATCH v6 5/5] pinctrl: mediatek: Add support for pin configuration dump via debugfs.

Hi,

On Thu, Sep 26, 2019 at 10:02 PM Light Hsieh <[email protected]> wrote:
>
> Add support for pin configuration dump via catting
> /sys/kernel/debug/pinctrl/$platform_dependent_path/pinconf-pins.
> pinctrl framework had already support such dump. This patch implement the
> operation function pointer to fullfill this dump.
>

Here are missing tags too.

> ---
> drivers/pinctrl/mediatek/pinctrl-paris.c | 88 ++++++++++++++++++++++++++++++++
> drivers/pinctrl/mediatek/pinctrl-paris.h | 30 +++++++++++
> 2 files changed, 118 insertions(+)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c
> index 2a47c45..f531908 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c
> @@ -538,12 +538,99 @@ static int mtk_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
> return 0;
> }
>
> +int mtk_hw_get_value_wrap(struct mtk_pinctrl *hw, unsigned int gpio, int field)
> +{
> + const struct mtk_pin_desc *desc;
> + int value, err;
> +
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> +
> + err = mtk_hw_get_value(hw, desc, field, &value);
> + if (err)
> + return err;
> +
> + return value;
> +}
> +
> +ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> + unsigned int gpio, char *buf, unsigned int bufLen)
> +{
> + const struct mtk_pin_desc *desc;
> + int pinmux, pullup, pullen, r1 = -1, r0 = -1, len = 0;

Sort the variable declarations in reverse xmas tree order.

> +
> + if (gpio > hw->soc->npins)
> + return -EINVAL;
> +
> + desc = (const struct mtk_pin_desc *)&hw->soc->pins[gpio];
> + pinmux = mtk_pctrl_get_pinmux(hw, gpio);
> + if (pinmux >= hw->soc->nfuncs)
> + 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) {
> + pullen = 1;
> + r1 = 1;
> + r0 = 1;
> + } else if (pullen != MTK_DISABLE && pullen != MTK_ENABLE) {
> + pullen = 0;
> + }
> + len += snprintf(buf + len, bufLen - len,
> + "%03d: %1d%1d%1d%1d%02d%1d%1d%1d%1d",
> + gpio,
> + pinmux,
> + mtk_pctrl_get_direction(hw, gpio),
> + mtk_pctrl_get_out(hw, gpio),
> + mtk_pctrl_get_in(hw, gpio),
> + mtk_pctrl_get_driving(hw, gpio),
> + mtk_pctrl_get_smt(hw, gpio),
> + mtk_pctrl_get_ies(hw, gpio),
> + pullen,
> + pullup);
> +
> + if (r1 != -1) {
> + len += snprintf(buf + len, bufLen - len, " (%1d %1d)\n",
> + r1, r0);
> + } else {
> + len += snprintf(buf + len, bufLen - len, "\n");
> + }
> +
> + return len;
> +}
> +
> +#define PIN_DBG_BUF_SZ 96
> +static void mtk_pctrl_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
> + unsigned int gpio)
> +{
> + struct mtk_pinctrl *hw = pinctrl_dev_get_drvdata(pctldev);
> + char buf[PIN_DBG_BUF_SZ];
> +
> + (void)mtk_pctrl_show_one_pin(hw, gpio, buf, PIN_DBG_BUF_SZ);
> +
> + seq_printf(s, "%s", buf);
> +}
> +
> static const struct pinctrl_ops mtk_pctlops = {
> .dt_node_to_map = mtk_pctrl_dt_node_to_map,
> .dt_free_map = pinctrl_utils_free_map,
> .get_groups_count = mtk_pctrl_get_groups_count,
> .get_group_name = mtk_pctrl_get_group_name,
> .get_group_pins = mtk_pctrl_get_group_pins,
> + .pin_dbg_show = mtk_pctrl_dbg_show,
> };
>
> static int mtk_pmx_get_funcs_cnt(struct pinctrl_dev *pctldev)
> @@ -640,6 +727,7 @@ static int mtk_pconf_group_set(struct pinctrl_dev *pctldev, unsigned group,
> .pin_config_get = mtk_pinconf_get,
> .pin_config_group_get = mtk_pconf_group_get,
> .pin_config_group_set = mtk_pconf_group_set,
> + .is_generic = true,
> };
>
> static struct pinctrl_desc mtk_desc = {
> diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.h b/drivers/pinctrl/mediatek/pinctrl-paris.h
> index 3d43771..d73f4b6 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-paris.h
> +++ b/drivers/pinctrl/mediatek/pinctrl-paris.h
> @@ -60,6 +60,36 @@
> int mtk_paris_pinctrl_probe(struct platform_device *pdev,
> const struct mtk_pin_soc *soc);
>
> +int mtk_hw_get_value_wrap(struct mtk_pinctrl *hw, unsigned int gpio, int field);
> +
> +#define mtk_pctrl_get_pinmux(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_MODE)
> +
> +/* MTK HW use 0 as input, 1 for output
> + * This interface is for get direct register value,
> + * so don't reverse
> + */

The comment should be removed since that is not really matched with the context.

> +#define mtk_pctrl_get_direction(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DIR)
> +
> +#define mtk_pctrl_get_out(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DO)
> +
> +#define mtk_pctrl_get_in(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DI)
> +
> +#define mtk_pctrl_get_smt(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_SMT)
> +
> +#define mtk_pctrl_get_ies(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_IES)
> +
> +#define mtk_pctrl_get_driving(hw, gpio) \
> + mtk_hw_get_value_wrap(hw, gpio, PINCTRL_PIN_REG_DRV)
> +
> +ssize_t mtk_pctrl_show_one_pin(struct mtk_pinctrl *hw,
> + unsigned int gpio, char *buf, unsigned int bufLen);
> +

Currently, these above functions are not being referred by other users
outside the file so stay them visible inside the file until there are
explicit users present.

> extern const struct dev_pm_ops mtk_paris_pinctrl_pm_ops;
>
> #endif /* __PINCTRL_PARIS_H */
> --
> 1.8.1.1.dirty
>