Messages in this thread | | | Subject | Re: [PATCH v2 2/6] pinctrl: Ingenic: Add support for read the pin configuration of X1830. | From | Zhou Yanjie <> | Date | Sat, 13 Mar 2021 16:07:15 +0800 |
| |
Hi Paul,
On 2021/3/12 下午9:31, Paul Cercueil wrote: > Hi Zhou, > > Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie) > <zhouyanjie@wanyeetech.com> a écrit : >> Add X1830 support in "ingenic_pinconf_get()", so that it can read the >> configuration of X1830 SoC correctly. >> >> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > > This is a fix, so it needs a Fixes: tag, and you need to Cc linux-stable. >
Sure.
>> --- >> >> Notes: >> v2: >> New patch. >> >> drivers/pinctrl/pinctrl-ingenic.c | 76 >> +++++++++++++++++++++++++++++---------- >> 1 file changed, 57 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/pinctrl/pinctrl-ingenic.c >> b/drivers/pinctrl/pinctrl-ingenic.c >> index 05dfa0a..0a88aab 100644 >> --- a/drivers/pinctrl/pinctrl-ingenic.c >> +++ b/drivers/pinctrl/pinctrl-ingenic.c >> @@ -2109,31 +2109,69 @@ static int ingenic_pinconf_get(struct >> pinctrl_dev *pctldev, >> enum pin_config_param param = pinconf_to_config_param(*config); >> unsigned int idx = pin % PINS_PER_GPIO_CHIP; >> unsigned int offt = pin / PINS_PER_GPIO_CHIP; >> + unsigned int bias; >> bool pull; >> >> - if (jzpc->info->version >= ID_JZ4770) >> - pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN); >> - else >> - pull = !ingenic_get_pin_config(jzpc, pin, >> JZ4740_GPIO_PULL_DIS); >> + if (jzpc->info->version >= ID_X1830) { >> + unsigned int half = PINS_PER_GPIO_CHIP / 2; >> + unsigned int idxh = pin % half * 2; >> >> - switch (param) { >> - case PIN_CONFIG_BIAS_DISABLE: >> - if (pull) >> - return -EINVAL; >> - break; >> + if (idx < half) >> + regmap_read(jzpc->map, offt * jzpc->info->reg_offset + >> + X1830_GPIO_PEL, &bias); >> + else >> + regmap_read(jzpc->map, offt * jzpc->info->reg_offset + >> + X1830_GPIO_PEH, &bias); >> >> - case PIN_CONFIG_BIAS_PULL_UP: >> - if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx))) >> - return -EINVAL; >> - break; >> + bias = (bias >> idxh) & 3; > > You can do: > > u32 mask = GENMASK(idxh + 1, idxh); > > bias = FIELD_GET(mask, bias); > > (macros in <linux/bitfield.h>) >
Sure.
>> >> - case PIN_CONFIG_BIAS_PULL_DOWN: >> - if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx))) >> - return -EINVAL; >> - break; >> + switch (param) { >> + case PIN_CONFIG_BIAS_DISABLE: >> + if (bias) >> + return -EINVAL; >> + break; >> >> - default: >> - return -ENOTSUPP; >> + case PIN_CONFIG_BIAS_PULL_UP: >> + if ((bias != PIN_CONFIG_BIAS_PULL_UP) || >> + !(jzpc->info->pull_ups[offt] & BIT(idx))) > > "bias" is a 2-bit value (because of the & 3 mask), and > PIN_CONFIG_BIAS_PULL_UP == 5. > > So this clearly won't work. You are comparing hardware values with > public API enums.
OK, I will fix it in the next version.
> >> + return -EINVAL; >> + break; >> + >> + case PIN_CONFIG_BIAS_PULL_DOWN: >> + if ((bias != PIN_CONFIG_BIAS_PULL_DOWN) || >> + !(jzpc->info->pull_downs[offt] & BIT(idx))) >> + return -EINVAL; >> + break; >> + >> + default: >> + return -ENOTSUPP; >> + } >> + >> + } else { >> + if (jzpc->info->version >= ID_JZ4770) >> + pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN); >> + else >> + pull = !ingenic_get_pin_config(jzpc, pin, >> JZ4740_GPIO_PULL_DIS); > > I think you can keep the switch outside the if/else block, if you use > pullup/pulldown variables. > > These can be initialized (in the non-X1830 case) to: > > pullup = pull && (jzpc->info->pull_ups[offt] & BIT(idx)); > pulldown = pull && (jzpc->info->pull_downs[offt] & BIT(idx)); > > In the X1830 case you'd initialize these variables from 'bias'.
Sure, I will do this in the next version.
> >> + >> + switch (param) { >> + case PIN_CONFIG_BIAS_DISABLE: >> + if (pull) > > Here would change to if (pullup || pulldown) >
OK.
>> + return -EINVAL; >> + break; >> + >> + case PIN_CONFIG_BIAS_PULL_UP: >> + if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx))) > > if (!pullup) >
Sure.
>> + return -EINVAL; >> + break; >> + >> + case PIN_CONFIG_BIAS_PULL_DOWN: >> + if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx))) > > if (!pulldown) >
Sure.
Thanks and best regards!
> Cheers, > -Paul > >> + return -EINVAL; >> + break; >> + >> + default: >> + return -ENOTSUPP; >> + } >> } >> >> *config = pinconf_to_config_packed(param, 1); >> -- >> 2.7.4 >> >
| |