Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14989C61DA4 for ; Mon, 6 Mar 2023 23:28:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229716AbjCFX2Q (ORCPT ); Mon, 6 Mar 2023 18:28:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42394 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229516AbjCFX2O (ORCPT ); Mon, 6 Mar 2023 18:28:14 -0500 Received: from fgw23-7.mail.saunalahti.fi (fgw23-7.mail.saunalahti.fi [62.142.5.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BFDA0367DA for ; Mon, 6 Mar 2023 15:28:12 -0800 (PST) Received: from localhost (88-113-24-128.elisa-laajakaista.fi [88.113.24.128]) by fgw23.mail.saunalahti.fi (Halon) with ESMTP id 8e0ab352-bc76-11ed-b972-005056bdfda7; Tue, 07 Mar 2023 01:28:09 +0200 (EET) From: andy.shevchenko@gmail.com Date: Tue, 7 Mar 2023 01:28:09 +0200 To: Chester Lin Cc: Linus Walleij , Andreas =?iso-8859-1?Q?F=E4rber?= , s32@nxp.com, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Larisa Grigore , Ghennadi Procopciuc , Andrei Stefanescu , Radu Pirea , Matthias Brugger , Matthew Nunez , Phu Luu An , Stefan-Gabriel Mirea Subject: Re: [PATCH v5 2/3] pinctrl: add NXP S32 SoC family support Message-ID: References: <20230220023320.3499-1-clin@suse.com> <20230220023320.3499-3-clin@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230220023320.3499-3-clin@suse.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Phu Luu An > Signed-off-by: Stefan-Gabriel Mirea > Signed-off-by: Larisa Grigore > Signed-off-by: Ghennadi Procopciuc > Signed-off-by: Andrei Stefanescu > Signed-off-by: Radu Pirea > Signed-off-by: Chester Lin Is it for real?! Quite a long chain and none of Co-developed-by. ... > + depends on ARCH_S32 && OF Is OF necessary? Can it be 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? > +}; > + > +/** > + * 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. ... > +#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(). ... > +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. > +} ... > + 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. > + } else if (!n_pins) { > + return -EINVAL; > + } ... > + for (i = 0; i < grp->npins; ++i) { Why pre-increment? > + 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; ... > + 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() ? > + 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? > + 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. > + 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. > + 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? > + 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(...); > + } ... > 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. ... > +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. ... > +static struct platform_driver s32g_pinctrl_driver = { > + .driver = { > + .name = "s32g-siul2-pinctrl", > + .owner = THIS_MODULE, Duplicate assignment. > + .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