[PATCH v3] drm/panel: add s6e3ha2 AMOLED panel driver

[Andrzej Hajda]

Hi Thierry,

On 02/03/2015 01:51 PM, Thierry Reding wrote:
> On Tue, Jan 27, 2015 at 10:48:32AM +0900, Hyungwon Hwang wrote:

(...)

>> +
>> +	/* This field is tested by functions directly accessing DSI bus before
>> +	 * transfer, transfer is skipped if it is set. In case of transfer
>> +	 * failure or unexpected response the field is set to error value.
>> +	 * Such construct allows to eliminate many checks in higher level
>> +	 * functions.
>> +	 */
>> +	int error;
> 
> I hate myself for not NAK'ing the first patch that introduced this idiom
> stronger. I think it's a really bad concept and you're not doing
> yourself any favours by using it.

The main favor of using it is much shorter code. The idiom eliminates
redundant error checking in case of nested function calls - and this is
quite common for panel drivers.

Moreover the same idiom is used also in other places. I have not dig too
much but as I remember it is present at least in:
- V4L2 core: struct v4l2_ctrl_handler.error,
- fs/seq_file: struct seq_file overflow handling


(...)

>> +static int s6e3ha2_prepare(struct drm_panel *panel)
>> +{
>> +	struct s6e3ha2 *ctx = panel_to_s6e3ha2(panel);
>> +	int ret;
>> +
>> +	ret = s6e3ha2_power_on(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	s6e3ha2_panel_init(ctx);
>> +	if (ctx->error < 0)
> 
> This is one of the reasons why ctx->error is a bad idea. It's completely
> unintuitive to check ctx->error here because nobody's actually setting
> it in this context.
> 

But this is classical example of passing variable by reference. You pass
ctx to s6e3ha2_panel_init to allow ctx modification. So the natural
thing is to check after the call what was changed, for example by
reading ctx->error.

The advantage of the idiom is that you are not obliged to do it now, it
can be done later. In case of propagating error by return value you
should check the error after every call and it results in much more
bloated code.

Regards
Andrzej