Messages in this thread | | | Date | Wed, 8 May 2024 10:38:23 +0200 | Subject | Re: [PATCH v8 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver | From | Christophe JAILLET <> |
| |
Le 08/05/2024 à 08:51, Jacky Huang a écrit : > From: Jacky Huang <ychuang3@nuvoton.com> > > Add common pinctrl and GPIO driver for Nuvoton MA35 series SoC, and > add support for ma35d1 pinctrl. > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> > ---
..
> +static int ma35_gpiolib_register(struct platform_device *pdev, struct ma35_pinctrl *npctl) > +{ > + struct ma35_pin_ctrl *ctrl = npctl->ctrl; > + struct ma35_pin_bank *bank = ctrl->pin_banks; > + int ret; > + int i; > + > + for (i = 0; i < ctrl->nr_banks; i++, bank++) { > + if (!bank->valid) { > + dev_warn(&pdev->dev, "%pfw: bank is not valid\n", bank->fwnode); > + continue; > + } > + bank->irqtype = 0; > + bank->irqinten = 0; > + bank->chip.label = bank->name; > + bank->chip.of_gpio_n_cells = 2; > + bank->chip.parent = &pdev->dev; > + bank->chip.request = ma35_gpio_core_to_request; > + bank->chip.direction_input = ma35_gpio_core_direction_in; > + bank->chip.direction_output = ma35_gpio_core_direction_out; > + bank->chip.get = ma35_gpio_core_get; > + bank->chip.set = ma35_gpio_core_set; > + bank->chip.base = -1; > + bank->chip.ngpio = bank->nr_pins; > + bank->chip.can_sleep = false; > + > + if (bank->irq > 0) { > + struct gpio_irq_chip *girq; > + > + girq = &bank->chip.irq; > + gpio_irq_chip_set_chip(girq, &ma35_gpio_irqchip); > + girq->parent_handler = ma35_irq_demux_intgroup; > + girq->num_parents = 1; > + > + girq->parents = devm_kcalloc(&pdev->dev, girq->num_parents, > + sizeof(*girq->parents), GFP_KERNEL); > + if (!girq->parents) { > + ret = -ENOMEM; > + goto fail; > + } > + > + girq->parents[0] = bank->irq; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_bad_irq; > + } > + > + ret = devm_gpiochip_add_data(&pdev->dev, &bank->chip, bank); > + if (ret) { > + dev_err(&pdev->dev, "failed to register gpio_chip %s, error code: %d\n", > + bank->chip.label, ret); > + goto fail; > + } > + } > + return 0; > + > +fail: > + while (i--) { > + bank--; > + if (!bank->valid) > + continue; > + gpiochip_remove(&bank->chip); > + }
I don't think this is correct. This is to undo the devm_gpiochip_add_data(), right? Because of the devm_, no need to explicityl call gpiochip_remove()
(more over, it should have been --i and not i-- in the while)
> + return ret; > +}
..
> +static int ma35_pinconf_set_drive_strength(struct ma35_pinctrl *npctl, unsigned int pin, > + int strength) > +{ > + unsigned int port, group_num; > + void __iomem *base; > + int i, ds_val = -1; > + u32 regval; > + > + if (ma35_pinconf_get_power_source(npctl, pin) == MVOLT_1800) { > + for (i = 0; i < ARRAY_SIZE(ds_1800mv_tbl); i++) { > + if (ds_1800mv_tbl[i] == strength) { > + ds_val = i; > + break; > + } > + } > + } else { > + for (i = 0; i < ARRAY_SIZE(ds_3300mv_tbl); i++) { > + if (ds_3300mv_tbl[i] == strength) { > + ds_val = i; > + continue;
break; ?
> + } > + } > + } > + if (ds_val == -1) > + return -EINVAL; > + > + ma35_gpio_cla_port(pin, &group_num, &port); > + base = npctl->ctrl->pin_banks[group_num].reg_base; > + > + regval = readl(base + MA35_GP_DS_REG(port)); > + regval &= ~MA35_GP_DS_MASK(port); > + regval |= field_prep(MA35_GP_DS_MASK(port), ds_val); > + > + writel(regval, base + MA35_GP_DS_REG(port)); > + > + return 0; > +}
..
> +static int ma35_pinctrl_probe_dt(struct platform_device *pdev, struct ma35_pinctrl *npctl) > +{ > + struct fwnode_handle *child; > + u32 idx = 0; > + int ret; > + > + device_for_each_child_node(&pdev->dev, child) { > + if (fwnode_property_present(child, "gpio-controller")) > + continue; > + npctl->nfunctions++; > + npctl->ngroups += of_get_child_count(to_of_node(child)); > + } > + > + if (!npctl->nfunctions) > + return -EINVAL; > + > + npctl->functions = devm_kcalloc(&pdev->dev, npctl->nfunctions, > + sizeof(*npctl->functions), GFP_KERNEL); > + if (!npctl->functions) > + return -ENOMEM; > + > + npctl->groups = devm_kcalloc(&pdev->dev, npctl->ngroups, > + sizeof(*npctl->groups), GFP_KERNEL); > + if (!npctl->groups) > + return -ENOMEM; > + > + device_for_each_child_node(&pdev->dev, child) { > + if (fwnode_property_present(child, "gpio-controller")) > + continue; > + > + ret = ma35_pinctrl_parse_functions(to_of_node(child), npctl, idx++); > + if (ret) { > + dev_err(&pdev->dev, "failed to parse function\n");
Missing fwnode_handle_put(child); ? (or a scoped version of device_for_each_child_node() if it exists)
> + return ret; > + } > + } > + return 0; > +
..
CJ
| |