lkml.org 
[lkml]   [2023]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 2/3] pinctrl: add NXP S32 SoC family support
    Hi Andy,

    Thanks for reviewing this patch!

    On Tue, Mar 07, 2023 at 01:28:09AM +0200, andy.shevchenko@gmail.com 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@nxp.com>
    > > Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
    > > Signed-off-by: Stefan-Gabriel Mirea <stefan-gabriel.mirea@nxp.com>
    > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
    > > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com>
    > > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@nxp.com>
    > > Signed-off-by: Radu Pirea <radu-nicolae.pirea@nxp.com>
    > > Signed-off-by: Chester Lin <clin@suse.com>
    >
    > 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.
    >

    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
    >
    >

    \
     
     \ /
      Last update: 2023-03-27 00:48    [W:2.300 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site