Re: [PATCH v3 3/7] drm: Add driver for Solomon SSD130X OLED displays

[Javier Martinez Canillas <javierm@redhat.com>]

On 2/9/22 17:08, Andy Shevchenko wrote:

[snip]

>> Agreed, as mentioned I'll give it a try to sending all the data as a
>> bulk write with regmap.
> 
> Ah, it might be that it should be noinc bulk op. Need to be checked anyway.
>

Yeah, I'll give it a try for v4. Let's see how it goes.

[snip]

>>>
>>> A-ha, why not in the callee?
>>>
>>
>> I think is easier to read when doing it in the caller, specially since there
>> is only a single call. Than calling it unconditionally and making it a no-op
>> if there isn't a reset GPIO.
> 
> It doesn't matter where the check is and checking that in the callee seems
> better as it relies on the reset functionality. Caller in such case shouldn't
> even think if it's supported or not, not their business.
>

Ok, no strong opinions really so I will change it.
 
> Last, but not least, if you think of power management, you probably want to
> assert reset there as well, means additional checks?
> 
>>>> +		ssd130x_reset(ssd130x);
> 
> ...
> 
>>>> +	if (IS_ERR(ssd130x)) {
>>>
>>>> +		dev_err(dev, "Failed to allocate DRM device: %d\n", ret);
>>>> +		return ssd130x;
>>>
>>> return dev_err_probe() ?
>>>
>>
>> No, because this isn't a resource provided by other driver. If this
>> failed is mostly due a memory allocation error.
> 
> Is it a problem? dev_err_probe() got update in the documentation explaining
> that's fine to call even in such cases. The outcome is less amount of LOCs.
>

Thanks for pointing out. In my mind that was a way to denote in the code that
a probe deferral was possible and that the message should be debug but now I
went and read the comment as you suggested:

 * Note that it is deemed acceptable to use this function for error
 * prints during probe even if the @err is known to never be -EPROBE_DEFER.
 * The benefit compared to a normal dev_err() is the standardized format
 * of the error code and the fact that the error code is returned

So you are correct and using it is preferred even when no probe defer error
will be returned. I'll do that here and in the other places you mentioned.

[snip] 

>>> I have feelings that half of my comments were ignored...
>>> Maybe I missed the discussion(s).
>>
>> I assure you that no comments from you or anyone were ignored.
>>
>> I may had missed something but if if I did was a mistake and
>> not intentionally. I keep a changelog for each revision in
>> the patches with the name of the reviewer so people can check
>> and compare.
>>
>> If something that you mentioned is not there, I apologize and
>> please point me out so I can address it in v4.
> 
> It's just a feeling, because I repeating that dev_err_probe() a lot :-)
> Nevertheless, now I see at least your point why you went that way.
> But see my comments on it.

And I did use dev_err_probe() in the places that could cause a probe
deferral so it wasn't (completely) ignored.

On that topic, I even typed a SPI driver because of your feedback :)

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat