[PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Dmitry Baryshkov]

Add a fat warning against adding new panel compatibles to the panel-edp
driver. All new users of the eDP panels are supposed to use the generic
"edp-panel" compatible device on the AUX bus. The remaining compatibles
are either used by the existing DT or were used previously and are
retained for backwards compatibility.

Suggested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
The following compatibles were never used by the devices supported by
the upstream kernel and are a subject to possible removal:

- auo,b133han05
- auo,b140han06
- ivo,m133nwf4-r0
- lg,lp097qx1-spa1
- lg,lp129qe
- samsung,lsn122dl01-c01
- samsung,ltn140at29-301
- sharp,ld-d5116z01b
- sharp,lq140m1jw46
- starry,kr122ea0sra

I'm considering dropping them, unless there is a good reason not to do
so.
---
 drivers/gpu/drm/panel/panel-edp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 6db277efcbb7..95b25ec67168 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = {
 	{
 		/* Must be first */
 		.compatible = "edp-panel",
-	}, {
+	},
+	/*
+	 * Do not add panels to the list below unless they cannot be handled by
+	 * the generic edp-panel compatible.
+	 *
+	 * The only two valid reasons are:
+	 * - because of the panel issues (e.g. broken EDID or broken
+	 *   identification),
+	 * - because the platform which uses the panel didn't wire up the AUX
+	 *   bus properly.
+	 *
+	 * In all other cases the platform should use the aux-bus and declare
+	 * the panel using the 'edp-panel' compatible as a device on the AUX
+	 * bus. The lack of the aux-bus support is not a valid case. Platforms
+	 * are urged to be converted to declaring panels in a proper way.
+	 */
+	{
 		.compatible = "auo,b101ean01",
 		.data = &auo_b101ean01,
 	}, {

---
base-commit: 8314289a8d50a4e05d8ece1ae0445a3b57bb4d3b
change-id: 20240523-edp-panel-drop-00aa239b3c6b

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Re: [PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Neil Armstrong]

On 23/05/2024 00:07, Dmitry Baryshkov wrote:
> Add a fat warning against adding new panel compatibles to the panel-edp
> driver. All new users of the eDP panels are supposed to use the generic
> "edp-panel" compatible device on the AUX bus. The remaining compatibles
> are either used by the existing DT or were used previously and are
> retained for backwards compatibility.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> The following compatibles were never used by the devices supported by
> the upstream kernel and are a subject to possible removal:
> 
> - auo,b133han05
> - auo,b140han06
> - ivo,m133nwf4-r0
> - lg,lp097qx1-spa1
> - lg,lp129qe
> - samsung,lsn122dl01-c01
> - samsung,ltn140at29-301
> - sharp,ld-d5116z01b
> - sharp,lq140m1jw46
> - starry,kr122ea0sra
> 
> I'm considering dropping them, unless there is a good reason not to do
> so.
> ---
>   drivers/gpu/drm/panel/panel-edp.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 6db277efcbb7..95b25ec67168 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = {
>   	{
>   		/* Must be first */
>   		.compatible = "edp-panel",
> -	}, {
> +	},
> +	/*
> +	 * Do not add panels to the list below unless they cannot be handled by
> +	 * the generic edp-panel compatible.
> +	 *
> +	 * The only two valid reasons are:
> +	 * - because of the panel issues (e.g. broken EDID or broken
> +	 *   identification),
> +	 * - because the platform which uses the panel didn't wire up the AUX
> +	 *   bus properly.
> +	 *
> +	 * In all other cases the platform should use the aux-bus and declare
> +	 * the panel using the 'edp-panel' compatible as a device on the AUX
> +	 * bus. The lack of the aux-bus support is not a valid case. Platforms
> +	 * are urged to be converted to declaring panels in a proper way.
> +	 */
> +	{
>   		.compatible = "auo,b101ean01",
>   		.data = &auo_b101ean01,
>   	}, {
> 
> ---
> base-commit: 8314289a8d50a4e05d8ece1ae0445a3b57bb4d3b
> change-id: 20240523-edp-panel-drop-00aa239b3c6b
> 
> Best regards,

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Doug Anderson]

Hi,

On Wed, May 22, 2024 at 3:07 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Add a fat warning against adding new panel compatibles to the panel-edp
> driver. All new users of the eDP panels are supposed to use the generic
> "edp-panel" compatible device on the AUX bus. The remaining compatibles
> are either used by the existing DT or were used previously and are
> retained for backwards compatibility.
>
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> The following compatibles were never used by the devices supported by
> the upstream kernel and are a subject to possible removal:
>
> - auo,b133han05

Ack. Looks like this was added by Bjorn but by the time the dts for
the board (from context, sc8180x-primus) using it made it upstream it
was using "edp-panel".


> - auo,b140han06

Ack. Same as above, but this time the board was "sc8180x-lenovo-flex-5g".


> - ivo,m133nwf4-r0

Ack. Also added by Bjorn but not easy to tell from context which board
it was from. "m133nwf4" never shows up in the history of arm64 qcom
dts files.


> - lg,lp097qx1-spa1

Maybe. Added by Yakir at Rockchip. I don't think this was ChromeOS
related so I don't have any inside knowledge. Presumably this is for
some other hardware they were using. Probably worth CCing Rockchip
mailing list. It's entirely possible that they have downstream dts
files using this and there's no requirement to upstream dts files that
I'm aware of.


> - lg,lp129qe

NAK. See "arch/arm/boot/dts/nvidia/tegra124-venice2.dts"


> - samsung,lsn122dl01-c01

Maybe. Also by Yakir at Rockchip and also doesn't appear to be
ChromeOS, so you should ask them.


> - samsung,ltn140at29-301

NAK. arch/arm/boot/dts/nvidia/tegra124-nyan-blaze.dts


> - sharp,ld-d5116z01b

Added by Jeffrey Hugo. Maybe Cc him to make sure it's OK to delete?


> - sharp,lq140m1jw46

Ack. Feel free to delete. This was used in the sc7280 reference board
(hoglin/zoglin) and by the time the dts made it upstream it was
already using generic edp-panel.


> - starry,kr122ea0sra

Ack. From my previous notes: "starry,kr122ea0sra - rk3399-gru-gru
(reference board, not upstream)". Nobody needs this.


> I'm considering dropping them, unless there is a good reason not to do
> so.
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 6db277efcbb7..95b25ec67168 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = {
>         {
>                 /* Must be first */
>                 .compatible = "edp-panel",
> -       }, {
> +       },
> +       /*
> +        * Do not add panels to the list below unless they cannot be handled by
> +        * the generic edp-panel compatible.
> +        *
> +        * The only two valid reasons are:
> +        * - because of the panel issues (e.g. broken EDID or broken
> +        *   identification),
> +        * - because the platform which uses the panel didn't wire up the AUX
> +        *   bus properly.

You mean that they physically didn't wire up the AUX bus? I don't
think that's actually possible. I don't believe you can use an eDP
panel without this working. Conceivably a reason to add here is if
there's some old platform that hasn't coded up support for AUX bus.
...but it would be much preferred to update the platform driver to
support it.

-Doug

Re: [PATCH RFC] drm/panel-edp: add fat warning against adding new panel compatibles

[Dmitry Baryshkov]

On Thu, May 23, 2024 at 08:36:39AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, May 22, 2024 at 3:07 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > Add a fat warning against adding new panel compatibles to the panel-edp
> > driver. All new users of the eDP panels are supposed to use the generic
> > "edp-panel" compatible device on the AUX bus. The remaining compatibles
> > are either used by the existing DT or were used previously and are
> > retained for backwards compatibility.
> >
> > Suggested-by: Doug Anderson <dianders@chromium.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > The following compatibles were never used by the devices supported by
> > the upstream kernel and are a subject to possible removal:
> >
> > - auo,b133han05
> 
> Ack. Looks like this was added by Bjorn but by the time the dts for
> the board (from context, sc8180x-primus) using it made it upstream it
> was using "edp-panel".
> 
> 
> > - auo,b140han06
> 
> Ack. Same as above, but this time the board was "sc8180x-lenovo-flex-5g".
> 
> 
> > - ivo,m133nwf4-r0
> 
> Ack. Also added by Bjorn but not easy to tell from context which board
> it was from. "m133nwf4" never shows up in the history of arm64 qcom
> dts files.
> 
> 
> > - lg,lp097qx1-spa1
> 
> Maybe. Added by Yakir at Rockchip. I don't think this was ChromeOS
> related so I don't have any inside knowledge. Presumably this is for
> some other hardware they were using. Probably worth CCing Rockchip
> mailing list. It's entirely possible that they have downstream dts
> files using this and there's no requirement to upstream dts files that
> I'm aware of.

I see. Adding him to the CC list.

> 
> 
> > - lg,lp129qe
> 
> NAK. See "arch/arm/boot/dts/nvidia/tegra124-venice2.dts"

Yes. I even had a comment next to it. And then blindly posted it here.

> 
> 
> > - samsung,lsn122dl01-c01
> 
> Maybe. Also by Yakir at Rockchip and also doesn't appear to be
> ChromeOS, so you should ask them.
> 
> 
> > - samsung,ltn140at29-301
> 
> NAK. arch/arm/boot/dts/nvidia/tegra124-nyan-blaze.dts
> 
> 
> > - sharp,ld-d5116z01b
> 
> Added by Jeffrey Hugo. Maybe Cc him to make sure it's OK to delete?

I pinged him on IRC.

> 
> 
> > - sharp,lq140m1jw46
> 
> Ack. Feel free to delete. This was used in the sc7280 reference board
> (hoglin/zoglin) and by the time the dts made it upstream it was
> already using generic edp-panel.
> 
> 
> > - starry,kr122ea0sra
> 
> Ack. From my previous notes: "starry,kr122ea0sra - rk3399-gru-gru
> (reference board, not upstream)". Nobody needs this.
> 
> 
> > I'm considering dropping them, unless there is a good reason not to do
> > so.
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> > index 6db277efcbb7..95b25ec67168 100644
> > --- a/drivers/gpu/drm/panel/panel-edp.c
> > +++ b/drivers/gpu/drm/panel/panel-edp.c
> > @@ -1776,7 +1776,23 @@ static const struct of_device_id platform_of_match[] = {
> >         {
> >                 /* Must be first */
> >                 .compatible = "edp-panel",
> > -       }, {
> > +       },
> > +       /*
> > +        * Do not add panels to the list below unless they cannot be handled by
> > +        * the generic edp-panel compatible.
> > +        *
> > +        * The only two valid reasons are:
> > +        * - because of the panel issues (e.g. broken EDID or broken
> > +        *   identification),
> > +        * - because the platform which uses the panel didn't wire up the AUX
> > +        *   bus properly.
> 
> You mean that they physically didn't wire up the AUX bus? I don't
> think that's actually possible. I don't believe you can use an eDP
> panel without this working. Conceivably a reason to add here is if
> there's some old platform that hasn't coded up support for AUX bus.
> ...but it would be much preferred to update the platform driver to
> support it.

I was thinking about the DT/driver side of the story. Let me rephrase it
in a cleaner way.

-- 
With best wishes
Dmitry