Re: [PATCH 1/3] pinctrl: s32: refine error/return/config checks and simplify driver codes

From: Andy Shevchenko
Date: Tue Mar 14 2023 - 13:09:28 EST

Next message: Serge Semin: "Re: [PATCH net-next 01/16] dt-bindings: net: dwmac: Validate PBL for all IP-cores"
Previous message: Greg Kroah-Hartman: "Re: [PATCH 5.10 1/1] usb: musb: core: drop redundant checks"
In reply to: Chester Lin: "[PATCH 1/3] pinctrl: s32: refine error/return/config checks and simplify driver codes"
Next in thread: Chester Lin: "[PATCH 2/3] pinctrl: s32cc: refactor pin config parsing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Mar 14, 2023 at 3:47 PM Chester Lin <clin@xxxxxxxx> wrote:
>
> Improve error/return code handlings and config checks in order to have
> better reliability and simplify driver codes such as removing/changing
> improper macros, blanks, print formats and helper calls.

I dunno if the maintainer wants this to be split to logically unified
changes, but either way it's fine to me.
Also see below some additional minor things you might be interested in fixing.
With these being addressed:
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Chester Lin <clin@xxxxxxxx>
> ---
> drivers/pinctrl/nxp/pinctrl-s32cc.c | 141 +++++++++++++++-------------
> drivers/pinctrl/nxp/pinctrl-s32g2.c | 8 +-
> 2 files changed, 78 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> index e1da332433a3..7a38e3216b0c 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> @@ -28,7 +28,8 @@
> #include "../pinctrl-utils.h"
> #include "pinctrl-s32.h"
>
> -#define S32_PIN_ID_MASK GENMASK(31, 4)
> +#define S32_PIN_ID_SHIFT 4
> +#define S32_PIN_ID_MASK GENMASK(31, S32_PIN_ID_SHIFT)
>
> #define S32_MSCR_SSS_MASK GENMASK(2, 0)
> #define S32_MSCR_PUS BIT(12)
> @@ -46,7 +47,7 @@ static struct regmap_config s32_regmap_config = {
>
> static u32 get_pin_no(u32 pinmux)
> {
> - return (pinmux & S32_PIN_ID_MASK) >> __ffs(S32_PIN_ID_MASK);
> + return (pinmux & S32_PIN_ID_MASK) >> S32_PIN_ID_SHIFT;
> }
>
> static u32 get_pin_func(u32 pinmux)
> @@ -108,7 +109,7 @@ s32_get_region(struct pinctrl_dev *pctldev, unsigned int pin)
> unsigned int mem_regions = ipctl->info->mem_regions;
> unsigned int i;
>
> - for (i = 0; i < mem_regions; ++i) {
> + for (i = 0; i < mem_regions; i++) {
> pin_range = ipctl->regions[i].pin_range;
> if (pin >= pin_range->start && pin <= pin_range->end)
> return &ipctl->regions[i];
> @@ -224,8 +225,7 @@ static int s32_dt_group_node_to_map(struct pinctrl_dev *pctldev,
>
> 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);
> + dev_warn(dev, "Can't find 'pinmux' property in node %pOFn\n", np);
> } else if (!n_pins) {
> return -EINVAL;
> }
> @@ -317,20 +317,25 @@ static int s32_pmx_set(struct pinctrl_dev *pctldev, unsigned int selector,
> info->functions[selector].name, grp->name);
>
> /* Check beforehand so we don't have a partial config. */
> - for (i = 0; i < grp->npins; ++i) {
> + for (i = 0; i < grp->npins; i++) {
> if (s32_check_pin(pctldev, grp->pin_ids[i]) != 0) {
> - dev_err(info->dev, "invalid pin: %d in group: %d\n",
> + dev_err(info->dev, "invalid pin: %u in group: %u\n",
> grp->pin_ids[i], group);
> return -EINVAL;
> }
> }
>
> - for (i = 0, ret = 0; i < grp->npins && !ret; ++i) {
> + for (i = 0, ret = 0; i < grp->npins && !ret; i++) {
> ret = s32_regmap_update(pctldev, grp->pin_ids[i],
> S32_MSCR_SSS_MASK, grp->pin_sss[i]);
> + if (ret) {
> + dev_err(info->dev, "Failed to set pin %u\n",
> + grp->pin_ids[i]);
> + return ret;
> + }
> }
>
> - return ret;
> + return 0;
> }
>
> static int s32_pmx_get_funcs_count(struct pinctrl_dev *pctldev)
> @@ -375,8 +380,8 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> int ret;
>
> ret = s32_regmap_read(pctldev, offset, &config);
> - if (ret != 0)
> - return -EINVAL;
> + if (ret)
> + return ret;
>
> /* Save current configuration */
> gpio_pin = kmalloc(sizeof(*gpio_pin), GFP_KERNEL);
> @@ -387,7 +392,7 @@ static int s32_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> gpio_pin->config = config;
>
> spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
> - list_add(&(gpio_pin->list), &(ipctl->gpio_configs));
> + list_add(&gpio_pin->list, &ipctl->gpio_configs);
> spin_unlock_irqrestore(&ipctl->gpio_configs_lock, flags);
>
> /* GPIO pin means SSS = 0 */
> @@ -401,15 +406,13 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
> unsigned int offset)
> {
> struct s32_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
> - struct list_head *pos, *tmp;
> - struct gpio_pin_config *gpio_pin;
> + struct gpio_pin_config *gpio_pin, *tmp;
> unsigned long flags;
> int ret;
>
> spin_lock_irqsave(&ipctl->gpio_configs_lock, flags);
>
> - list_for_each_safe(pos, tmp, &ipctl->gpio_configs) {
> - gpio_pin = list_entry(pos, struct gpio_pin_config, list);
> + list_for_each_entry_safe(gpio_pin, tmp, &ipctl->gpio_configs, list) {

>

This blank line now can be removed.

> if (gpio_pin->pin_id == offset) {
> ret = s32_regmap_write(pctldev, gpio_pin->pin_id,
> @@ -417,7 +420,7 @@ static void s32_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
> if (ret != 0)
> goto unlock;
>
> - list_del(pos);
> + list_del(&gpio_pin->list);
> kfree(gpio_pin);
> break;
> }
> @@ -461,7 +464,8 @@ static const int support_slew[] = {208, -1, -1, -1, 166, 150, 133, 83};
>
> static int s32_get_slew_regval(int arg)
> {
> - int i;
> + unsigned int i;
> +
> /* Translate a real slew rate (MHz) to a register value */
> for (i = 0; i < ARRAY_SIZE(support_slew); i++) {
> if (arg == support_slew[i])
> @@ -542,10 +546,11 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
> unsigned int config = 0, mask = 0;
> int i, ret;
>
> - if (s32_check_pin(pctldev, pin_id) != 0)
> - return -EINVAL;
> + ret = s32_check_pin(pctldev, pin_id);
> + if (ret)
> + return ret;
>
> - dev_dbg(ipctl->dev, "pinconf set pin %s with %d configs\n",
> + dev_dbg(ipctl->dev, "pinconf set pin %s with %u configs\n",
> pin_get_name(pctldev, pin_id), num_configs);
>
> for (i = 0; i < num_configs; i++) {
> @@ -559,11 +564,9 @@ static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
> if (!config && !mask)
> return 0;
>
> - ret = s32_regmap_update(pctldev, pin_id, mask, config);
> -
> - dev_dbg(ipctl->dev, "update: pin %d cfg 0x%x\n", pin_id, config);
> + dev_dbg(ipctl->dev, "update: pin %u cfg 0x%x\n", pin_id, config);
>
> - return ret;
> + return s32_regmap_update(pctldev, pin_id, mask, config);
> }
>
> static int s32_pinconf_get(struct pinctrl_dev *pctldev,
> @@ -604,10 +607,13 @@ 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);
> + int ret;
> +
> + ret = s32_regmap_read(pctldev, pin_id, &config);
> + if (ret)
> + return;
>
> - if (!ret)
> - seq_printf(s, "0x%x", config);
> + seq_printf(s, "0x%x", config);
> }
>
> static void s32_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
> @@ -710,7 +716,7 @@ int s32_pinctrl_resume(struct device *dev)
> }
> #endif
>
> -static void s32_pinctrl_parse_groups(struct device_node *np,
> +static int s32_pinctrl_parse_groups(struct device_node *np,
> struct s32_pin_group *grp,
> struct s32_pinctrl_soc_info *info)
> {
> @@ -722,21 +728,20 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
>
> dev = info->dev;
>
> - dev_dbg(dev, "group: %s\n", np->name);
> + dev_dbg(dev, "group: %pOFn\n", np);
>
> /* Initialise group */
> grp->name = np->name;
>
> npins = of_property_count_elems_of_size(np, "pinmux", sizeof(u32));
> -
> if (npins < 0) {
> dev_err(dev, "Failed to read 'pinmux' property in node %s.\n",
> - np->name);
> - return;
> + grp->name);
> + return -EINVAL;
> }
> if (!npins) {
> - dev_err(dev, "The group %s has no pins.\n", np->name);
> - return;
> + dev_err(dev, "The group %s has no pins.\n", grp->name);
> + return -EINVAL;
> }
>
> grp->npins = npins;
> @@ -745,12 +750,8 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
> sizeof(unsigned int), GFP_KERNEL);
> grp->pin_sss = devm_kcalloc(info->dev, grp->npins,
> sizeof(unsigned int), GFP_KERNEL);
> -
> - if (!grp->pin_ids || !grp->pin_sss) {
> - dev_err(dev, "Failed to allocate memory for the group %s.\n",
> - np->name);
> - return;
> - }
> + if (!grp->pin_ids || !grp->pin_sss)
> + return -ENOMEM;
>
> i = 0;
> of_property_for_each_u32(np, "pinmux", prop, p, pinmux) {
> @@ -761,9 +762,11 @@ static void s32_pinctrl_parse_groups(struct device_node *np,
> grp->pin_ids[i], grp->pin_sss[i]);
> i++;
> }
> +
> + return 0;
> }
>
> -static void s32_pinctrl_parse_functions(struct device_node *np,
> +static int s32_pinctrl_parse_functions(struct device_node *np,
> struct s32_pinctrl_soc_info *info,
> u32 index)
> {
> @@ -771,8 +774,9 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
> struct s32_pmx_func *func;
> struct s32_pin_group *grp;
> u32 i = 0;
> + int ret = 0;
>
> - dev_dbg(info->dev, "parse function(%d): %s\n", index, np->name);
> + dev_dbg(info->dev, "parse function(%u): %pOFn\n", index, np);
>
> func = &info->functions[index];
>
> @@ -780,18 +784,24 @@ static void s32_pinctrl_parse_functions(struct device_node *np,
> func->name = np->name;
> func->num_groups = of_get_child_count(np);
> if (func->num_groups == 0) {
> - dev_err(info->dev, "no groups defined in %s\n", np->full_name);
> - return;
> + dev_err(info->dev, "no groups defined in %pOF\n", np);
> + return -EINVAL;
> }
> - func->groups = devm_kzalloc(info->dev,
> - func->num_groups * sizeof(char *), GFP_KERNEL);
> + func->groups = devm_kcalloc(info->dev, func->num_groups,

> + sizeof(char *), GFP_KERNEL);

sizeof(*func->groups) ?

> + if (!func->groups)
> + return -ENOMEM;
>
> 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);
> + ret = s32_pinctrl_parse_groups(child, grp, info);
> + if (ret)
> + return ret;
> i++;
> }
> +
> + return 0;
> }
>
> static int s32_pinctrl_probe_dt(struct platform_device *pdev,
> @@ -804,6 +814,7 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
> struct regmap *map;
> void __iomem *base;
> int mem_regions = info->mem_regions;
> + int ret;
> u32 nfuncs = 0;
> u32 i = 0;
>
> @@ -815,13 +826,12 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
> return -EINVAL;
> }
>
> - ipctl->regions = devm_kzalloc(&pdev->dev,
> - mem_regions * sizeof(*(ipctl->regions)),
> - GFP_KERNEL);
> + ipctl->regions = devm_kcalloc(&pdev->dev, mem_regions,
> + sizeof(*(ipctl->regions)), GFP_KERNEL);

Too many parentheses.

> if (!ipctl->regions)
> return -ENOMEM;
>
> - for (i = 0; i < mem_regions; ++i) {
> + for (i = 0; i < mem_regions; i++) {
> base = devm_platform_get_and_ioremap_resource(pdev, i, &res);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -851,24 +861,26 @@ static int s32_pinctrl_probe_dt(struct platform_device *pdev,
> }
>
> info->nfunctions = nfuncs;
> - info->functions = devm_kzalloc(&pdev->dev,
> - nfuncs * sizeof(struct s32_pmx_func),
> - GFP_KERNEL);
> + info->functions = devm_kcalloc(&pdev->dev, nfuncs,
> + sizeof(struct s32_pmx_func), GFP_KERNEL);

sizeof(*info->functions)

> if (!info->functions)
> return -ENOMEM;
>
> info->ngroups = 0;
> for_each_child_of_node(np, child)
> info->ngroups += of_get_child_count(child);
> - info->groups = devm_kzalloc(&pdev->dev,
> - info->ngroups * sizeof(struct s32_pin_group),
> - GFP_KERNEL);
> +
> + info->groups = devm_kcalloc(&pdev->dev, info->ngroups,
> + sizeof(struct s32_pin_group), GFP_KERNEL);

sizeof (*info->groups) ?

> if (!info->groups)
> return -ENOMEM;
>
> i = 0;
> - for_each_child_of_node(np, child)
> - s32_pinctrl_parse_functions(child, info, i++);
> + for_each_child_of_node(np, child) {
> + ret = s32_pinctrl_parse_functions(child, info, i++);
> + if (ret)
> + return ret;
> + }
>
> return 0;
> }
> @@ -923,12 +935,9 @@ int s32_pinctrl_probe(struct platform_device *pdev,
>
> ipctl->pctl = devm_pinctrl_register(&pdev->dev, s32_pinctrl_desc,
> ipctl);
> -
> - if (IS_ERR(ipctl->pctl)) {
> - dev_err(&pdev->dev, "could not register s32 pinctrl driver\n");
> - return PTR_ERR(ipctl->pctl);
> - }

> -

Don't drop this blank line, see below.

> + if (IS_ERR(ipctl->pctl))
> + return dev_err_probe(&pdev->dev, PTR_ERR(ipctl->pctl),
> + "could not register s32 pinctrl driver\n");

It should be here.

> #ifdef CONFIG_PM_SLEEP
> saved_context = &ipctl->saved_context;
> saved_context->pads =
> diff --git a/drivers/pinctrl/nxp/pinctrl-s32g2.c b/drivers/pinctrl/nxp/pinctrl-s32g2.c
> index 5028f4adc389..0b0b06f12b8a 100644
> --- a/drivers/pinctrl/nxp/pinctrl-s32g2.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32g2.c
> @@ -746,8 +746,8 @@ static int s32g_pinctrl_probe(struct platform_device *pdev)
> if (!of_id)
> return -ENODEV;
>
> - return s32_pinctrl_probe
> - (pdev, (struct s32_pinctrl_soc_info *) of_id->data);
> + return s32_pinctrl_probe(pdev,
> + (struct s32_pinctrl_soc_info *) of_id->data);

No space after casting, but why is it using this interface? Convert to
of_device_get_match_data().
(In a separate patch before this one)

> }
>
> static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
> @@ -757,14 +757,12 @@ static const struct dev_pm_ops s32g_pinctrl_pm_ops = {
> static struct platform_driver s32g_pinctrl_driver = {
> .driver = {
> .name = "s32g-siul2-pinctrl",
> - .owner = THIS_MODULE,
> .of_match_table = s32_pinctrl_of_match,
> - .pm = &s32g_pinctrl_pm_ops,
> + .pm = pm_sleep_ptr(&s32g_pinctrl_pm_ops),
> .suppress_bind_attrs = true,
> },
> .probe = s32g_pinctrl_probe,
> };
> -
> builtin_platform_driver(s32g_pinctrl_driver);
>
> MODULE_AUTHOR("Matthew Nunez <matthew.nunez@xxxxxxx>");
> --
> 2.37.3
>

--
With Best Regards,
Andy Shevchenko

Next message: Serge Semin: "Re: [PATCH net-next 01/16] dt-bindings: net: dwmac: Validate PBL for all IP-cores"
Previous message: Greg Kroah-Hartman: "Re: [PATCH 5.10 1/1] usb: musb: core: drop redundant checks"
In reply to: Chester Lin: "[PATCH 1/3] pinctrl: s32: refine error/return/config checks and simplify driver codes"
Next in thread: Chester Lin: "[PATCH 2/3] pinctrl: s32cc: refactor pin config parsing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]