| Message ID | 20240529-edp-panel-drop-v2-1-fcfc457fc8dd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel-edp: remove several legacy compatibles used by the driver | expand |
Hi, On Tue, May 28, 2024 at 4:52 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> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > 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. I'm still at least slightly confused by the wording. What is "the lack of the aux-bus support". I guess this is different from the valid reason of "the platform which uses the panel didn't wire up the AUX bus properly" but I'm not sure how. Maybe you can explain? I guess I'd write it like this: /* * 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. NOTE that, though this is a marginally valid reason, * some justification needs to be made for why the platform can'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. */ What do you think? In any case, it probably doesn't matter much. The important thing is some sort of warning here telling people not to add to the table. In that sense: Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, May 30, 2024 at 07:33:59AM -0700, Doug Anderson wrote: > Hi, > > On Tue, May 28, 2024 at 4:52 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> > > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > 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. > > I'm still at least slightly confused by the wording. What is "the lack > of the aux-bus support". I guess this is different from the valid > reason of "the platform which uses the panel didn't wire up the AUX > bus properly" but I'm not sure how. Maybe you can explain? > > I guess I'd write it like this: > > /* > * 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. NOTE that, though this is a marginally valid reason, > * some justification needs to be made for why the platform can'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. > */ > > What do you think? In any case, it probably doesn't matter much. The > important thing is some sort of warning here telling people not to add > to the table. In that sense: Ack, I'l update the wording in a similar way. > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
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, }, {