linux-kernel - Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <330174f9-bead-4411-b05e-ea9c009f765c@gmail.com>
Date: Sat, 7 Sep 2024 14:02:20 +0530
From: Tejas Vipin <tejasvipin76@...il.com>
To: Jessica Zhang <quic_jesszhan@...cinc.com>, neil.armstrong@...aro.org,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 airlied@...il.com, daniel@...ll.ch
Cc: dianders@...omium.org, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/panel: himax-hx83112a: transition to mipi_dsi wrapped
 functions



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@...il.com>
>>
>> Reviewed-by: Jessica Zhang <quic_jesszhan@...cinc.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