On Wed, Mar 08, 2023 at 01:03:06PM +0800, Chester Lin wrote: > Hi Andy, > > Thanks for reviewing this patch! > > On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@xxxxxxxxx wrote: > > Mon, Feb 20, 2023 at 10:33:19AM +0800, Chester Lin kirjoitti: > > > Add the pinctrl driver for NXP S32 SoC family. This driver is mainly based > > > on NXP's downstream implementation on nxp-auto-linux repo[1]. > > > > Seems Linus already applied this, but still some comments for the future, some > > for the followups. However, personally I prefer this to be dropped and redone > > because too many things here and there. > > > > > [1] https://github.com/nxp-auto-linux/linux/tree/bsp35.0-5.15.73-rt/drivers/pinctrl/freescale > > > > This can be transformed to Link: tag. > > > > > Signed-off-by: Matthew Nunez <matthew.nunez@xxxxxxx> > > > Signed-off-by: Phu Luu An <phu.luuan@xxxxxxx> > > > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@xxxxxxx> > > > Signed-off-by: Larisa Grigore <larisa.grigore@xxxxxxx> > > > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxxxxxx> > > > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxx> > > > Signed-off-by: Radu Pirea <radu-nicolae.pirea@xxxxxxx> > > > Signed-off-by: Chester Lin <clin@xxxxxxxx> > > > > Is it for real?! > > Quite a long chain and none of Co-developed-by. > > > > They are developers who contribute codes and have Signed-off-bys in NXP's downstream > version. Since their implementations can still be seen in this upstream one, I > prefer to list them all. Indeed a part of them who also actively work with me on > upstreaming this driver can be listed as Co-developed-by, but the driver patch > has been merged into the maintainer's for-next so I would not change this part > unless the driver patch needs to be reverted and re-submitted in the end. > > Sorry for the patch header that I mess up anyway. > > > ... > > > > > + depends on ARCH_S32 && OF > > > > Is OF necessary? Can it be > > I think it's required since the driver file refers to of_* APIs. > > > > > depends OF || COMPILE_TEST > > > > ? > > > > ... > > > > > + depends on ARCH_S32 && OF > > > > Ditto. > > > > ... > > > > > +/** > > > + * struct s32_pin_group - describes an S32 pin group > > > + * @name: the name of this specific pin group > > > + * @npins: the number of pins in this group array, i.e. the number of > > > + * elements in pin_ids and pin_sss so we can iterate over that array > > > + * @pin_ids: an array of pin IDs in this group > > > + * @pin_sss: an array of source signal select configs paired with pin_ids > > > + */ > > > +struct s32_pin_group { > > > + const char *name; > > > + unsigned int npins; > > > + unsigned int *pin_ids; > > > + unsigned int *pin_sss; > > > > Why didn't you embed struct pingroup? > > > > I did think about that but there's an additional 'pin_sss' which could make code > a bit messy. For example: > > s32_regmap_update(pctldev, grp->grp.pins[i], > S32_MSCR_SSS_MASK, grp->pin_sss[i]); > > > > +}; > > > + > > > +/** > > > + * struct s32_pmx_func - describes S32 pinmux functions > > > + * @name: the name of this specific function > > > + * @groups: corresponding pin groups > > > + * @num_groups: the number of groups > > > + */ > > > +struct s32_pmx_func { > > > + const char *name; > > > + const char **groups; > > > + unsigned int num_groups; > > > +}; > > > > struct pinfunction. > > > > Thanks for your information. I was not aware of this new struct since it just got > merged recently. > > > ... > > > > > +#ifdef CONFIG_PM_SLEEP > > > +int __maybe_unused s32_pinctrl_resume(struct device *dev); > > > +int __maybe_unused s32_pinctrl_suspend(struct device *dev); > > > +#endif > > > > Please, consider using new PM macros, like pm_ptr(). > > > > Maybe pm_sleep_ptr() is more accurate? > > > ... > > > > > +static u32 get_pin_no(u32 pinmux) > > > +{ > > > + return (pinmux & S32_PIN_ID_MASK) >> __ffs(S32_PIN_ID_MASK); > > > > Oh, don't you have MASK to be 2^x - 1? Why to drag this with __ffs()? > > Just define a complement _SHIFT. > > > > Will fix it. > > > > +} > > > > ... > > > > > + n_pins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32)); > > > + if (n_pins < 0) { > > > + dev_warn(dev, "Unable to find 'pinmux' property in node %s.\n", > > > + np->name); > > > > Use %pOFn instead of %s. > > > > Will change it. > > > > + } else if (!n_pins) { > > > + return -EINVAL; > > > + } > > > > ... > > > > > + for (i = 0; i < grp->npins; ++i) { > > > > Why pre-increment? > > Will change that to keep code style consistency. > > > > > > + if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) { > > > + dev_err(info->dev, "invalid pin: %d in group: %d\n", > > > + grp->pin_ids[i], group); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + for (i = 0, ret = 0; i < grp->npins && !ret; ++i) { > > > > Ditto. > > > > > + ret = s32_regmap_update(pctldev, grp->pin_ids[i], > > > + S32_MSCR_SSS_MASK, grp->pin_sss[i]); > > > > Traditional pattern is > > > > if (ret) > > return ret; > > > > This avoids first assignment of the ret. > > > > > + } > > > + > > > + return ret; > > > > return 0; > > > > > +} > > > > ... > > > > > + ret = s32_regmap_read(pctldev, offset, &config); > > > + if (ret != 0) > > > + return -EINVAL; > > > > Why not > > > > if (ret) > > return ret; > > > > Will fix this and the following error code shadowings, return handlings and blanks. > > Thanks for your reminder. > > > ... > > > > > + list_add(&(gpio_pin->list), &(ipctl->gpio_configs)); > > > > Too many parentheses. > > > > ... > > > > > + list_for_each_safe(pos, tmp, &ipctl->gpio_configs) { > > > + gpio_pin = list_entry(pos, struct gpio_pin_config, list); > > > > Why not list_for_each_entry_safe() ? > > Will change it. > > > > > > > > + if (gpio_pin->pin_id == offset) { > > > + ret = s32_regmap_write(pctldev, gpio_pin->pin_id, > > > + gpio_pin->config); > > > + if (ret != 0) > > > + goto unlock; > > > + > > > + list_del(pos); > > > + kfree(gpio_pin); > > > + break; > > > + } > > > + } > > > > ... > > > > > +static int s32_get_slew_regval(int arg) > > > +{ > > > + int i; > > > > unsigned int i; > > > > + Blank line. > > > > > + /* Translate a real slew rate (MHz) to a register value */ > > > + for (i = 0; i < ARRAY_SIZE(support_slew); i++) { > > > + if (arg == support_slew[i]) > > > + return i; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > > ... > > > > > + case PIN_CONFIG_BIAS_PULL_UP: > > > + if (arg) > > > + *config |= S32_MSCR_PUS; > > > + else > > > + *config &= ~S32_MSCR_PUS; > > > > > + fallthrough; > > > > It's quite easy to miss this and tell us about how is it supposed to work with PU + PD? > > > I admit that it's ambiguous and should be improved in order to have better readability. > > In a S32G2 MSCR register, there are two register bits related to pull up/down functions: > > PUE (Pull Enable, MSCR[13]): 0'b: Disabled, 1'b: Enabled > PUS (Pull Select, MSCR[12]): 0'b: Pull Down, 1'b: Pull Up > > The dt properties could be like these: > > 1) 'bias-pull-up' or 'bias-pull-up: true' --> arg = 1 > In this case both PUE and PUS are set. > > 2) 'bias-pull-up: false' --> arg = 0 > In this case both PUE and PUS are cleared since the pull-up function must be disabled. > > > > + case PIN_CONFIG_BIAS_PULL_DOWN: > > > + if (arg) > > > + *config |= S32_MSCR_PUE; > > > + else > > > + *config &= ~S32_MSCR_PUE; > > > + *mask |= S32_MSCR_PUE | S32_MSCR_PUS; > > > + break; > > > + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE: > > > + *config &= ~(S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE); > > > + *mask |= S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE; > > > + fallthrough; > > > > Ditto. > > Sorry that I missed the case of PIN_CONFIG_BIAS_HIGH_IMPEDANCE: As you can see that the pull function must be disabled as well in order to make the pin floating. Regards, Chester > > It's similar to the case 'PIN_CONFIG_BIAS_PULL_UP' although the PUS bit is assumed > as 0 via the config variable so only the PUE bit needs to be configured, for example: > > 1) 'bias-pull-down' or 'bias-pull-down: true' --> arg = 1 > PUE is set and PUS is cleared. > > 2) 'bias-pull-down: false' --> arg = 0 > In this case both PUE and PUS are cleared since the pull-down function must be disabled. > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > + *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE); > > > + *mask |= S32_MSCR_PUS | S32_MSCR_PUE; > > > + break; > > > > ... > > > > > + if (s32_check_pin(pctldev, pin_id) != 0) > > > > Shadowing an error? > > > > > + return -EINVAL; > > > > ... > > > > > + ret = s32_regmap_update(pctldev, pin_id, mask, config); > > > + > > > + dev_dbg(ipctl->dev, "update: pin %d cfg 0x%x\n", pin_id, config); > > > + > > > + return ret; > > > > You are not using ret in between, hence > > > > return _regmap_update(...); > > > > ... > > > > > +static void s32_pinconf_dbg_show(struct pinctrl_dev *pctldev, > > > + struct seq_file *s, unsigned int pin_id) > > > +{ > > > + unsigned int config; > > > + int ret = s32_regmap_read(pctldev, pin_id, &config); > > > + > > > + if (!ret) > > > + seq_printf(s, "0x%x", config); > > > > > > int ret; > > > > ret = s32_regmap_read(pctldev, pin_id, &config); > > if (ret) > > return; > > > > seq_printf(s, "0x%x", config); > > > > > +} > > > > ... > > > > > + npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32)); > > > > > + > > > > Unneccessary blank line. > > > > > + if (npins < 0) { > > > + dev_err(dev, "Failed to read 'pinmux' property in node %s.\n", > > > + np->name); > > > + return; > > > + } > > > + if (!npins) { > > > + dev_err(dev, "The group %s has no pins.\n", np->name); > > > + return; > > > + } > > > > ... > > > > > + grp->pin_ids = devm_kcalloc(info->dev, grp->npins, > > > + sizeof(unsigned int), GFP_KERNEL); > > > + grp->pin_sss = devm_kcalloc(info->dev, grp->npins, > > > + sizeof(unsigned int), GFP_KERNEL); > > > > > + > > > > Ditto. > > > > > + if (!grp->pin_ids || !grp->pin_sss) { > > > > > + dev_err(dev, "Failed to allocate memory for the group %s.\n", > > > + np->name); > > > > We do not print ENOMEM error messages. > > > > Will drop it. > > > > + return; > > > + } > > > > ... > > > > > + func->groups = devm_kzalloc(info->dev, > > > + func->num_groups * sizeof(char *), GFP_KERNEL); > > > > First of all, this is devm_kcalloc(). > > Second, where is the error check? > > > > Will fix it. > > > > + for_each_child_of_node(np, child) { > > > + func->groups[i] = child->name; > > > + grp = &info->groups[info->grp_index++]; > > > + s32_pinctrl_parse_groups(child, grp, info); > > > + i++; > > > + } > > > > ... > > > > > + ipctl->regions = devm_kzalloc(&pdev->dev, > > > + mem_regions * sizeof(*(ipctl->regions)), > > > + GFP_KERNEL); > > > > devm_kcalloc() > > > > > + if (!ipctl->regions) > > > + return -ENOMEM; > > > > ... > > > > > + info->functions = devm_kzalloc(&pdev->dev, > > > + nfuncs * sizeof(struct s32_pmx_func), > > > + GFP_KERNEL); > > > > Ditto. > > > > > + if (!info->functions) > > > + return -ENOMEM; > > > > ... > > > > > + info->groups = devm_kzalloc(&pdev->dev, > > > + info->ngroups * sizeof(struct s32_pin_group), > > > + GFP_KERNEL); > > > > Ditto. > > > > > + if (!info->groups) > > > + return -ENOMEM; > > > > ... > > > > > + ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc, > > > + ipctl); > > > > > + > > > > Unneeded blank line. > > > > > + if (IS_ERR(ipctl->pctl)) { > > > > > + dev_err(&pdev->dev, "could not register s32 pinctrl driver\n"); > > > + return PTR_ERR(ipctl->pctl); > > > > return dev_err_probe(...); > > > > Will change it, thanks! > > > > + } > > > > ... > > > > > diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c > > > > Similar issues has to be addressed, if any. > > > > ... > > > > > + return s32_pinctrl_probe > > > + (pdev, (struct s32_pinctrl_soc_info *) of_id->data); > > > > Broken indentation. > > > > ... > > > > The checkpatch.pl seems not to warn this but I will definitely improve it. > > > > +static const struct dev_pm_ops s32g_pinctrl_pm_ops = { > > > + SET_LATE_SYSTEM_SLEEP_PM_OPS(s32_pinctrl_suspend, > > > + s32_pinctrl_resume) > > > +}; > > > > Consider using DEFINE_* PM macros. > > > > You probably mean DEFINE_SIMPLE_DEV_PM_OPS. Will change it. > > > ... > > > > > +static struct platform_driver s32g_pinctrl_driver = { > > > + .driver = { > > > + .name = "s32g-siul2-pinctrl", > > > > > + .owner = THIS_MODULE, > > > > Duplicate assignment. > > Will drop it. > > > > > > + .of_match_table = s32_pinctrl_of_match, > > > + .pm = &s32g_pinctrl_pm_ops, > > > + .suppress_bind_attrs = true, > > > + }, > > > + .probe = s32g_pinctrl_probe, > > > +}; > > > > > + > > > > Unnecessary blank line. > > > > > +builtin_platform_driver(s32g_pinctrl_driver); > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
