On 07/07/2024 08:53, Krzysztof Wilczyński wrote:
> Hello,
>
> [...]
>> Use dev_err_probe() in all error paths with that construction.
>
> Thank you for this nice refactoring! Much appreciated.
>
> [...]
>> - if (ret > MAX_PCI_SLOTS) {
>> - dev_err(dev, "Too many GPIO clock requests!\n");
>> - return -EINVAL;
>> - }
>> + if (ret > MAX_PCI_SLOTS)
>> + return dev_err_probe(dev, -EINVAL,
>> + "Too many GPIO clock requests!\n");
>
> Something that would be nice to get consistent: adjust all the errors
> capitalisation to make everything consistent, as appropriate, so that it's
> either all lower-case or title case. A mix of both often looks a bit
> sloppy.
>
> Do you think this would be something you would be willing to clean up in
> this series too? Especially since we are touching this code now.
>
>> - if (!dev->of_node) {
>> - dev_err(dev, "NULL node\n");
>> - return -EINVAL;
>> - }
>> + if (!dev->of_node)
>> + return dev_err_probe(dev, -EINVAL, "NULL node\n");
>
> Perhaps -ENODEV would be more appropriate here? Also, the error message is
> not the best, as such, I wonder if we could make it better while we are at
> it, so to speak.
>
> Krzysztof
Sure, I will add that to v2. I have seen that typical error messages in
other drivers for this error are "OF node not found", "Device node not
found" and "This driver needs device tree". Given that "OF data missing"
is used in this driver, I will go for the first one of the list, unless
something different is preferred.
Best regards,
Javier Carrasco
