Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped functions

[Tejas Vipin <tejasvipin76@gmail.com>]

On 9/7/24 3:53 AM, Jessica Zhang wrote:
> 
> 
> On 9/6/2024 3:14 PM, Jessica Zhang wrote:
>>
>>
>> On 9/4/2024 7:15 AM, Tejas Vipin wrote:
>>> Changes the himax-hx83112a panel to use multi style functions for
>>> improved error handling.
>>>
>>> Signed-off-by: Tejas Vipin <tejasvipin76@gmail.com>
>>
>> Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> 
> Hi Tejas,
> 
> Just a heads up, it seems that this might be a duplicate of this change [1]?
> 
> Thanks,
> 
> Jessica Zhang
> 
> [1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1

Ah, thanks for letting me know. I hadn't realized someone else had
started working on this too. 

However, I would argue that my patch [2] is a better candidate for merging
because of the following reasons:

1) Removes unnecessary error printing:
The mipi_dsi_*_multi() functions all have inbuilt error printing which
makes printing errors after hx83112a_on unnecessary as is addressed in
[2] like so:

> -	ret = hx83112a_on(ctx);
> +	ret = hx83112a_on(ctx->dsi);
>  	if (ret < 0) {
> -		dev_err(dev, "Failed to initialize panel: %d\n", ret);
>  		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>  		regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -		return ret;
>  	}

[2] also removes the unnecessary dev_err after regulator_bulk_enable as was
addressed in [3] like so:

>  	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to enable regulators: %d\n", ret);
> +	if (ret < 0)
>  		return ret;
> -	}

2) Better formatting

The mipi_dsi_dcs_write_seq_multi statements in [1] aren't formatted
quite right according to what has been done so far. They are written as
such in [1]:

> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
>  			       0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4, 0x0e);

Where they should be written as such in [2]:

> +	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, HX83112A_SETTP1,
> +				     0x02, 0x00, 0xa8, 0x01, 0xa8, 0x0d, 0xa4, 0x0e);

All in all, the module generated using my patch ends up being a teensy
bit smaller (1% smaller). But if chronology is what is important, then
it would at least be nice to see the above changes as part of Abhishek's
patch too. And I'll be sure to look at the mail in the drm inbox now
onwards :p

[1] https://patchwork.freedesktop.org/patch/612367/?series=138155&rev=1
[2] https://lore.kernel.org/all/20240904141521.554451-1-tejasvipin76@gmail.com/
[3] https://lore.kernel.org/all/CAD=FV=XRZKL_ppjUKDK61fQkWhHiQCJLfmVBS7wSo4sUux2g8Q@mail.gmail.com/
-- 
Tejas Vipin