Received-SPF: pass (google.com: domain of linux-kernel+bounces-172974-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3ee92974-6f55-426d-8cd6-bb871a23499f@gmail.com>
Date: Wed, 8 May 2024 17:38:35 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v8 3/3] pinctrl: nuvoton: Add ma35d1 pinctrl and GPIO
 driver
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
 linus.walleij@linaro.org, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
 p.zabel@pengutronix.de, j.neuschaefer@gmx.net
Cc: linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 ychuang3@nuvoton.com, schung@nuvoton.com
References: <20240508065141.565848-1-ychuang570808@gmail.com>
 <20240508065141.565848-4-ychuang570808@gmail.com>
 <39ff586c-24fc-4987-bf5b-c99f0b2ae8f9@wanadoo.fr>
Content-Language: en-US
From: Jacky Huang <ychuang570808@gmail.com>
In-Reply-To: <39ff586c-24fc-4987-bf5b-c99f0b2ae8f9@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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