RE: [PATCH 1/2] gpio: realtek: Add GPIO support for RTD SoC variants

[TY_Chang[張子逸] <tychang@xxxxxxxxxxx>]

Hi Krzysztof,

>On 01/11/2023 03:58, Tzuyi Chang wrote:
>> This commit adds GPIO support for Realtek DHC RTD SoCs.
>
>Please do not use "This commit/patch", but imperative mood. See longer
>explanation here:
>https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-
>patches.rst#L95
>

I will remove these words.

>> +static int rtd_gpio_probe(struct platform_device *pdev) {
>> +     struct rtd_gpio *data;
>> +     const struct of_device_id *match;
>> +     struct device_node *node;
>> +     int ret;
>> +     int i;
>> +
>> +     node = pdev->dev.of_node;
>> +     match = of_match_node(rtd_gpio_of_matches, pdev->dev.of_node);
>> +     if (!match || !match->data)
>> +             return -EINVAL;
>> +
>> +     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     data->assert_irq = irq_of_parse_and_map(node, 0);
>> +     if (!data->assert_irq)
>> +             goto deferred;
>> +
>> +     data->deassert_irq = irq_of_parse_and_map(node, 1);
>> +     if (!data->deassert_irq)
>> +             goto deferred;
>
>So this goes to cleanup path...
>

Since there is no need to do devm_free, I will directly return -EPROBE_DEFER here.

>> +
>> +     data->info = match->data;
>> +     spin_lock_init(&data->lock);
>> +
>> +     data->base = of_iomap(node, 0);
>> +     if (!data->base)
>> +             return -ENXIO;
>
>But this does not? What?
>
>> +
>> +     data->irq_base = of_iomap(node, 1);
>> +     if (!data->irq_base)
>> +             return -ENXIO;
>> +
>> +     data->gpio_chip.parent = &pdev->dev;
>> +     data->gpio_chip.label = dev_name(&pdev->dev);
>> +     data->gpio_chip.of_gpio_n_cells = 2;
>> +     data->gpio_chip.base = data->info->gpio_base;
>> +     data->gpio_chip.ngpio = data->info->num_gpios;
>> +     data->gpio_chip.request = rtd_gpio_request;
>> +     data->gpio_chip.free = rtd_gpio_free;
>> +     data->gpio_chip.get_direction = rtd_gpio_get_direction;
>> +     data->gpio_chip.direction_input = rtd_gpio_direction_input;
>> +     data->gpio_chip.direction_output = rtd_gpio_direction_output;
>> +     data->gpio_chip.set = rtd_gpio_set;
>> +     data->gpio_chip.get = rtd_gpio_get;
>> +     data->gpio_chip.set_config = rtd_gpio_set_config;
>> +     data->gpio_chip.to_irq = rtd_gpio_to_irq;
>> +     data->irq_chip = rtd_gpio_irq_chip;
>> +     data->irq_chip.name = data->info->name;
>> +
>> +     ret = devm_gpiochip_add_data(&pdev->dev, &data->gpio_chip, data);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Adding GPIO chip failed (%d)\n",
>> + ret);
>
>And here no cleanup? It's some random choice.
>
>> +             return ret;
>> +     }
>> +
>> +     data->domain = irq_domain_add_linear(node, data->gpio_chip.ngpio,
>> +                             &irq_domain_simple_ops, data);
>> +     if (!data->domain) {
>> +             devm_kfree(&pdev->dev, data);
>
>NAK, test your patch.
>

I will remove it.

>> +             return -ENOMEM;
>> +     }
>> +
>> +     for (i = 0; i < data->gpio_chip.ngpio; i++) {
>> +             int irq = irq_create_mapping(data->domain, i);
>> +
>> +             irq_set_chip_data(irq, data);
>> +             irq_set_chip_and_handler(irq, &data->irq_chip,
>handle_simple_irq);
>> +     }
>> +
>> +     irq_set_chained_handler_and_data(data->assert_irq,
>rtd_gpio_assert_irq_handle, data);
>> +     irq_set_chained_handler_and_data(data->deassert_irq,
>> + rtd_gpio_deassert_irq_handle, data);
>> +
>> +     dev_dbg(&pdev->dev, "probed\n");
>
>Nop, drop all silly, simple entry/exit messages.
>

I will remove it.

>> +
>> +     return 0;
>> +
>> +deferred:
>> +     devm_kfree(&pdev->dev, data);
>
>NAK, test your patch.
>
>> +     return -EPROBE_DEFER;

I will remove this label.

>> +}
>
>Best regards,
>Krzysztof


Thanks,
Tzuyi Chang