Messages in this thread | | | Date | Wed, 8 May 2024 17:38:35 +0800 | Subject | Re: [PATCH v8 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO driver | From | Jacky Huang <> |
| |
Dear Christophe,
Thanks for the review.
On 2024/5/8 下午 04:38, Christophe JAILLET wrote: > 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) >
Yes, I will just drop the whole "fail:" case and make it return from "goto fail" with the error code.
>> + 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; ?
I will fix it.
> >> + } >> + } >> + } >> + 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) >
Yes, I will add fwnode_handle_put(child) here.
>> + return ret; >> + } >> + } >> + return 0; >> + > > ... > > CJ
Best Regards, Jacky Huang
| |