Re: [PATCH v1 2/2] drm/panel-edp: Add sharp panel support for sc7280

[Doug Anderson]

Hi,

On Mon, Jan 24, 2022 at 10:16 AM Sankeerth Billakanti
<quic_sbillaka@xxxxxxxxxxx> wrote:
>
> Add eDP panel support for sc7280 CRD platform.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 176ef0c..bb2e346 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -1605,6 +1605,14 @@ static const struct panel_desc sharp_lq123p1jx31 = {
>         },
>  };
>
> +static const struct panel_desc sharp_lq140m1jw46 = {
> +       .bpc = 8,
> +       .size = {
> +               .width = 309,
> +               .height = 173,
> +       },

Where are your delays? I very much doubt that they are all 0.

I guess you're also not putting timings in here and you're relying on
these coming from the EDID? My own preference would be:

1. If you are relying on a reliable way to read the EDID of the panel
then you shouldn't even need to add anything to this section of the
file. You should use the "edp-panel" compatible string and then add an
entry to the "edp_panels" structure.

2. If you want to support folks that don't have a reliable way to read
the EDID then you can add things here, but you should add a mode.


> +};
> +
>  static const struct drm_display_mode starry_kr122ea0sra_mode = {
>         .clock = 147000,
>         .hdisplay = 1920,
> @@ -1719,6 +1727,9 @@ static const struct of_device_id platform_of_match[] = {
>                 .compatible = "sharp,lq123p1jx31",
>                 .data = &sharp_lq123p1jx31,
>         }, {
> +               .compatible = "sharp_lq140m1jw46",
> +               .data = &sharp_lq140m1jw46,

Two problems:

1. You should use a ",", not a "_" to separate the vendor from the model.

2. You need to post device tree bindings for this.


NOTE: if instead your eDP controller supports DP AUX bus then you
don't need to add to this table at all and you don't need to add
bindings. Instead, you'd add your EDID panel ID to the "edp_panels"
structure.