| Message ID | 1615476112-113101-1-git-send-email-zhouyanjie@wanyeetech.com |
|---|---|
| Headers | show |
| Series | Fix bugs and add support for new Ingenic SoCs. | expand |
On Thu, Mar 11, 2021 at 5:23 PM 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> wrote: > > v1->v2: > 1.Split [1/3] in v1 to [1/6] [2/6] [3/6] [4/6] in v2. > 2.Fix the uninitialized warning. With the split done the series looks much better! FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> (with the exception of DT bindings) > 周琰杰 (Zhou Yanjie) (6): > pinctrl: Ingenic: Add missing pins to the JZ4770 MAC MII group. > pinctrl: Ingenic: Add support for read the pin configuration of X1830. > pinctrl: Ingenic: Adjust the sequence of X1830 SSI pin groups. > pinctrl: Ingenic: Reformat the code. > dt-bindings: pinctrl: Add bindings for new Ingenic SoCs. > pinctrl: Ingenic: Add support for new Ingenic SoCs. > > .../bindings/pinctrl/ingenic,pinctrl.yaml | 23 +- > drivers/pinctrl/pinctrl-ingenic.c | 1423 ++++++++++++++++++-- > 2 files changed, 1353 insertions(+), 93 deletions(-) > > -- > 2.7.4 > -- With Best Regards, Andy Shevchenko
Hi, Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> a écrit : > The MII group of JZ4770's MAC should have 7 pins, add missing > pins to the MII group. > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> No Fixes: tag? And if the bug wasn't introduced in 5.12-rc1 you'll need to Cc linux-stable as well. > --- > > Notes: > v2: > New patch. > > drivers/pinctrl/pinctrl-ingenic.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-ingenic.c > b/drivers/pinctrl/pinctrl-ingenic.c > index f274612..05dfa0a 100644 > --- a/drivers/pinctrl/pinctrl-ingenic.c > +++ b/drivers/pinctrl/pinctrl-ingenic.c > @@ -667,7 +667,9 @@ static int jz4770_pwm_pwm7_pins[] = { 0x6b, }; > static int jz4770_mac_rmii_pins[] = { > 0xa9, 0xab, 0xaa, 0xac, 0xa5, 0xa4, 0xad, 0xae, 0xa6, 0xa8, > }; > -static int jz4770_mac_mii_pins[] = { 0xa7, 0xaf, }; > +static int jz4770_mac_mii_pins[] = { > + 0x7b, 0x7a, 0x7d, 0x7c, 0xa7, 0x24, 0xaf, Maybe list them in order? And are you sure that's the whole list? The PM (section 12.2 in jz4770_pm_part3.pdf) lists more pins. Cheers, -Paul > +}; > > static const struct group_desc jz4770_groups[] = { > INGENIC_PIN_GROUP("uart0-data", jz4770_uart0_data, 0), > -- > 2.7.4 >
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. > --- > > 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>) > > - 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. > + 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'. > + > + switch (param) { > + case PIN_CONFIG_BIAS_DISABLE: > + if (pull) Here would change to if (pullup || pulldown) > + return -EINVAL; > + break; > + > + case PIN_CONFIG_BIAS_PULL_UP: > + if (!pull || !(jzpc->info->pull_ups[offt] & BIT(idx))) if (!pullup) > + return -EINVAL; > + break; > + > + case PIN_CONFIG_BIAS_PULL_DOWN: > + if (!pull || !(jzpc->info->pull_downs[offt] & BIT(idx))) if (!pulldown) Cheers, -Paul > + return -EINVAL; > + break; > + > + default: > + return -ENOTSUPP; > + } > } > > *config = pinconf_to_config_packed(param, 1); > -- > 2.7.4 >
Le jeu. 11 mars 2021 à 23:21, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> a écrit : > Adjust the sequence of X1830's SSI related codes to make it consistent > with other Ingenic SoCs. > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> Reviewed-by: Paul Cercueil <paul@crapouillou.net> Cheers, -Paul > --- > > Notes: > v2: > New patch. > > drivers/pinctrl/pinctrl-ingenic.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-ingenic.c > b/drivers/pinctrl/pinctrl-ingenic.c > index 0a88aab..607ba0b 100644 > --- a/drivers/pinctrl/pinctrl-ingenic.c > +++ b/drivers/pinctrl/pinctrl-ingenic.c > @@ -1473,16 +1473,16 @@ static int x1830_ssi0_gpc_pins[] = { 0x4d, }; > static int x1830_ssi0_ce0_pins[] = { 0x50, }; > static int x1830_ssi0_ce1_pins[] = { 0x4e, }; > static int x1830_ssi1_dt_c_pins[] = { 0x53, }; > -static int x1830_ssi1_dr_c_pins[] = { 0x54, }; > -static int x1830_ssi1_clk_c_pins[] = { 0x57, }; > -static int x1830_ssi1_gpc_c_pins[] = { 0x55, }; > -static int x1830_ssi1_ce0_c_pins[] = { 0x58, }; > -static int x1830_ssi1_ce1_c_pins[] = { 0x56, }; > static int x1830_ssi1_dt_d_pins[] = { 0x62, }; > +static int x1830_ssi1_dr_c_pins[] = { 0x54, }; > static int x1830_ssi1_dr_d_pins[] = { 0x63, }; > +static int x1830_ssi1_clk_c_pins[] = { 0x57, }; > static int x1830_ssi1_clk_d_pins[] = { 0x66, }; > +static int x1830_ssi1_gpc_c_pins[] = { 0x55, }; > static int x1830_ssi1_gpc_d_pins[] = { 0x64, }; > +static int x1830_ssi1_ce0_c_pins[] = { 0x58, }; > static int x1830_ssi1_ce0_d_pins[] = { 0x67, }; > +static int x1830_ssi1_ce1_c_pins[] = { 0x56, }; > static int x1830_ssi1_ce1_d_pins[] = { 0x65, }; > static int x1830_mmc0_1bit_pins[] = { 0x24, 0x25, 0x20, }; > static int x1830_mmc0_4bit_pins[] = { 0x21, 0x22, 0x23, }; > -- > 2.7.4 >
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 >> >