[PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Marek Vasut]

The mode parsing is currently implemented in three steps:
of_get_display_timing() - DT panel-timing to struct display_timing
videomode_from_timing() - struct display_timing to struct videomode
drm_display_mode_from_videomode() - struct videomode to struct drm_display_mode

Replace all that with simple of_get_drm_panel_display_mode() call,
which already populates struct drm_display_mode and then duplicate
that mode in panel_lvds_get_modes() each time, since the mode does
not change.

Nice bonus is the bus_flags parsed by of_get_drm_panel_display_mode()
out of panel-timing DT node, which is used in subsequent patch to fix
handling of 'de-active' DT property.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dmitry Osipenko <digetx@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/panel/panel-lvds.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 27a1c9923b09..65c6a6e9e223 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -30,7 +30,8 @@ struct panel_lvds {
 	const char *label;
 	unsigned int width;
 	unsigned int height;
-	struct videomode video_mode;
+	struct drm_display_mode dmode;
+	u32 bus_flags;
 	unsigned int bus_format;
 	bool data_mirror;
 
@@ -87,16 +88,15 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
 	struct panel_lvds *lvds = to_panel_lvds(panel);
 	struct drm_display_mode *mode;
 
-	mode = drm_mode_create(connector->dev);
+	mode = drm_mode_duplicate(connector->dev, &lvds->dmode);
 	if (!mode)
 		return 0;
 
-	drm_display_mode_from_videomode(&lvds->video_mode, mode);
 	mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
 	drm_mode_probed_add(connector, mode);
 
-	connector->display_info.width_mm = lvds->width;
-	connector->display_info.height_mm = lvds->height;
+	connector->display_info.width_mm = lvds->dmode.width_mm;
+	connector->display_info.height_mm = lvds->dmode.height_mm;
 	drm_display_info_set_bus_formats(&connector->display_info,
 					 &lvds->bus_format, 1);
 	connector->display_info.bus_flags = lvds->data_mirror
@@ -116,7 +116,6 @@ static const struct drm_panel_funcs panel_lvds_funcs = {
 static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 {
 	struct device_node *np = lvds->dev->of_node;
-	struct display_timing timing;
 	int ret;
 
 	ret = of_drm_get_panel_orientation(np, &lvds->orientation);
@@ -125,28 +124,13 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 		return ret;
 	}
 
-	ret = of_get_display_timing(np, "panel-timing", &timing);
+	ret = of_get_drm_panel_display_mode(np, &lvds->dmode, &lvds->bus_flags);
 	if (ret < 0) {
 		dev_err(lvds->dev, "%pOF: problems parsing panel-timing (%d)\n",
 			np, ret);
 		return ret;
 	}
 
-	videomode_from_timing(&timing, &lvds->video_mode);
-
-	ret = of_property_read_u32(np, "width-mm", &lvds->width);
-	if (ret < 0) {
-		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
-			np, "width-mm");
-		return -ENODEV;
-	}
-	ret = of_property_read_u32(np, "height-mm", &lvds->height);
-	if (ret < 0) {
-		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
-			np, "height-mm");
-		return -ENODEV;
-	}
-
 	of_property_read_string(np, "label", &lvds->label);
 
 	ret = drm_of_lvds_get_data_mapping(np);
-- 
2.35.1

[PATCH 2/2] drm/panel: lvds: Use bus_flags from DT panel-timing property

[Marek Vasut]

This driver currently rewrites bus_flags based solely on the value of
DT property 'data-mirror' and ignores bus_flags which might have been
set in DT panel-timing node. Specificaly, the 'de-active' DT property
sets DRM_BUS_FLAG_DE_ bus_flags.

Since of_get_drm_panel_display_mode() conveniently parses the bus_flags
out of DT panel-timing property, just ORR them with bus_flags inferred
from 'data-mirror' DT property and use the result as panel bus_flags.

This fixes handling of panels with 'panel-timing { de-active = <1>; };'.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dmitry Osipenko <digetx@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
To: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/panel/panel-lvds.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 65c6a6e9e223..f11252fb00fe 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -33,7 +33,6 @@ struct panel_lvds {
 	struct drm_display_mode dmode;
 	u32 bus_flags;
 	unsigned int bus_format;
-	bool data_mirror;
 
 	struct regulator *supply;
 
@@ -99,9 +98,7 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
 	connector->display_info.height_mm = lvds->dmode.height_mm;
 	drm_display_info_set_bus_formats(&connector->display_info,
 					 &lvds->bus_format, 1);
-	connector->display_info.bus_flags = lvds->data_mirror
-					  ? DRM_BUS_FLAG_DATA_LSB_TO_MSB
-					  : DRM_BUS_FLAG_DATA_MSB_TO_LSB;
+	connector->display_info.bus_flags = lvds->bus_flags;
 	drm_connector_set_panel_orientation(connector, lvds->orientation);
 
 	return 1;
@@ -142,7 +139,9 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 
 	lvds->bus_format = ret;
 
-	lvds->data_mirror = of_property_read_bool(np, "data-mirror");
+	lvds->bus_flags |= of_property_read_bool(np, "data-mirror") ?
+			   DRM_BUS_FLAG_DATA_LSB_TO_MSB :
+			   DRM_BUS_FLAG_DATA_MSB_TO_LSB;
 
 	return 0;
 }
-- 
2.35.1

RE: [PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Christoph Niedermaier]

From: Marek Vasut [mailto:marex@denx.de]
Sent: Thursday, March 31, 2022 9:24 PM
> The mode parsing is currently implemented in three steps:
> of_get_display_timing() - DT panel-timing to struct display_timing
> videomode_from_timing() - struct display_timing to struct videomode
> drm_display_mode_from_videomode() - struct videomode to struct
> drm_display_mode
> 
> Replace all that with simple of_get_drm_panel_display_mode() call,
> which already populates struct drm_display_mode and then duplicate
> that mode in panel_lvds_get_modes() each time, since the mode does
> not change.
> 
> Nice bonus is the bus_flags parsed by of_get_drm_panel_display_mode()
> out of panel-timing DT node, which is used in subsequent patch to fix
> handling of 'de-active' DT property.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/panel/panel-lvds.c | 28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-lvds.c
> b/drivers/gpu/drm/panel/panel-lvds.c
> index 27a1c9923b09..65c6a6e9e223 100644
> --- a/drivers/gpu/drm/panel/panel-lvds.c
> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> @@ -30,7 +30,8 @@ struct panel_lvds {
>         const char *label;
>         unsigned int width;
>         unsigned int height;
> -       struct videomode video_mode;
> +       struct drm_display_mode dmode;
> +       u32 bus_flags;
>         unsigned int bus_format;
>         bool data_mirror;
> 
> @@ -87,16 +88,15 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
>         struct panel_lvds *lvds = to_panel_lvds(panel);
>         struct drm_display_mode *mode;
> 
> -       mode = drm_mode_create(connector->dev);
> +       mode = drm_mode_duplicate(connector->dev, &lvds->dmode);
>         if (!mode)
>                 return 0;
> 
> -       drm_display_mode_from_videomode(&lvds->video_mode, mode);
>         mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>         drm_mode_probed_add(connector, mode);
> 
> -       connector->display_info.width_mm = lvds->width;
> -       connector->display_info.height_mm = lvds->height;
> +       connector->display_info.width_mm = lvds->dmode.width_mm;
> +       connector->display_info.height_mm = lvds->dmode.height_mm;
>         drm_display_info_set_bus_formats(&connector->display_info,
>                                          &lvds->bus_format, 1);
>         connector->display_info.bus_flags = lvds->data_mirror
> @@ -116,7 +116,6 @@ static const struct drm_panel_funcs panel_lvds_funcs = {
>  static int panel_lvds_parse_dt(struct panel_lvds *lvds)
>  {
>         struct device_node *np = lvds->dev->of_node;
> -       struct display_timing timing;
>         int ret;
> 
>         ret = of_drm_get_panel_orientation(np, &lvds->orientation);
> @@ -125,28 +124,13 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
>                 return ret;
>         }
> 
> -       ret = of_get_display_timing(np, "panel-timing", &timing);
> +       ret = of_get_drm_panel_display_mode(np, &lvds->dmode, &lvds->bus_flags);
>         if (ret < 0) {
>                 dev_err(lvds->dev, "%pOF: problems parsing panel-timing (%d)\n",
>                         np, ret);
>                 return ret;
>         }
> 
> -       videomode_from_timing(&timing, &lvds->video_mode);
> -
> -       ret = of_property_read_u32(np, "width-mm", &lvds->width);
> -       if (ret < 0) {
> -               dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> -                       np, "width-mm");
> -               return -ENODEV;
> -       }
> -       ret = of_property_read_u32(np, "height-mm", &lvds->height);
> -       if (ret < 0) {
> -               dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> -                       np, "height-mm");
> -               return -ENODEV;
> -       }
> -
>         of_property_read_string(np, "label", &lvds->label);
> 
>         ret = drm_of_lvds_get_data_mapping(np);

Tested-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>

Regards
Christoph

RE: [PATCH 2/2] drm/panel: lvds: Use bus_flags from DT panel-timing property

[Christoph Niedermaier]

From: Marek Vasut [mailto:marex@denx.de]
Sent: Thursday, March 31, 2022 9:24 PM
> This driver currently rewrites bus_flags based solely on the value of
> DT property 'data-mirror' and ignores bus_flags which might have been
> set in DT panel-timing node. Specificaly, the 'de-active' DT property
> sets DRM_BUS_FLAG_DE_ bus_flags.
> 
> Since of_get_drm_panel_display_mode() conveniently parses the bus_flags
> out of DT panel-timing property, just ORR them with bus_flags inferred
> from 'data-mirror' DT property and use the result as panel bus_flags.
> 
> This fixes handling of panels with 'panel-timing { de-active = <1>; };'.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/panel/panel-lvds.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-lvds.c
> b/drivers/gpu/drm/panel/panel-lvds.c
> index 65c6a6e9e223..f11252fb00fe 100644
> --- a/drivers/gpu/drm/panel/panel-lvds.c
> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> @@ -33,7 +33,6 @@ struct panel_lvds {
>         struct drm_display_mode dmode;
>         u32 bus_flags;
>         unsigned int bus_format;
> -       bool data_mirror;
> 
>         struct regulator *supply;
> 
> @@ -99,9 +98,7 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
>         connector->display_info.height_mm = lvds->dmode.height_mm;
>         drm_display_info_set_bus_formats(&connector->display_info,
>                                          &lvds->bus_format, 1);
> -       connector->display_info.bus_flags = lvds->data_mirror
> -                                         ? DRM_BUS_FLAG_DATA_LSB_TO_MSB
> -                                         : DRM_BUS_FLAG_DATA_MSB_TO_LSB;
> +       connector->display_info.bus_flags = lvds->bus_flags;
>         drm_connector_set_panel_orientation(connector, lvds->orientation);
> 
>         return 1;
> @@ -142,7 +139,9 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
> 
>         lvds->bus_format = ret;
> 
> -       lvds->data_mirror = of_property_read_bool(np, "data-mirror");
> +       lvds->bus_flags |= of_property_read_bool(np, "data-mirror") ?
> +                          DRM_BUS_FLAG_DATA_LSB_TO_MSB :
> +                          DRM_BUS_FLAG_DATA_MSB_TO_LSB;
> 
>         return 0;
>  }


Tested-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>

Regards
Christoph

Re: [PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Thu, Mar 31, 2022 at 09:23:46PM +0200, Marek Vasut wrote:
> The mode parsing is currently implemented in three steps:
> of_get_display_timing() - DT panel-timing to struct display_timing
> videomode_from_timing() - struct display_timing to struct videomode
> drm_display_mode_from_videomode() - struct videomode to struct drm_display_mode
> 
> Replace all that with simple of_get_drm_panel_display_mode() call,
> which already populates struct drm_display_mode and then duplicate
> that mode in panel_lvds_get_modes() each time, since the mode does
> not change.
> 
> Nice bonus is the bus_flags parsed by of_get_drm_panel_display_mode()
> out of panel-timing DT node, which is used in subsequent patch to fix
> handling of 'de-active' DT property.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/panel/panel-lvds.c | 28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
> index 27a1c9923b09..65c6a6e9e223 100644
> --- a/drivers/gpu/drm/panel/panel-lvds.c
> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> @@ -30,7 +30,8 @@ struct panel_lvds {
>  	const char *label;
>  	unsigned int width;
>  	unsigned int height;
> -	struct videomode video_mode;
> +	struct drm_display_mode dmode;

"dmode" sounds a bit weird, I would have gone for just "mode", or
"display_mode", but I don't mind much.

> +	u32 bus_flags;
>  	unsigned int bus_format;
>  	bool data_mirror;
>  
> @@ -87,16 +88,15 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
>  	struct panel_lvds *lvds = to_panel_lvds(panel);
>  	struct drm_display_mode *mode;
>  
> -	mode = drm_mode_create(connector->dev);
> +	mode = drm_mode_duplicate(connector->dev, &lvds->dmode);
>  	if (!mode)
>  		return 0;
>  
> -	drm_display_mode_from_videomode(&lvds->video_mode, mode);
>  	mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>  	drm_mode_probed_add(connector, mode);
>  
> -	connector->display_info.width_mm = lvds->width;
> -	connector->display_info.height_mm = lvds->height;
> +	connector->display_info.width_mm = lvds->dmode.width_mm;
> +	connector->display_info.height_mm = lvds->dmode.height_mm;
>  	drm_display_info_set_bus_formats(&connector->display_info,
>  					 &lvds->bus_format, 1);
>  	connector->display_info.bus_flags = lvds->data_mirror
> @@ -116,7 +116,6 @@ static const struct drm_panel_funcs panel_lvds_funcs = {
>  static int panel_lvds_parse_dt(struct panel_lvds *lvds)
>  {
>  	struct device_node *np = lvds->dev->of_node;
> -	struct display_timing timing;
>  	int ret;
>  
>  	ret = of_drm_get_panel_orientation(np, &lvds->orientation);
> @@ -125,28 +124,13 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
>  		return ret;
>  	}
>  
> -	ret = of_get_display_timing(np, "panel-timing", &timing);
> +	ret = of_get_drm_panel_display_mode(np, &lvds->dmode, &lvds->bus_flags);
>  	if (ret < 0) {
>  		dev_err(lvds->dev, "%pOF: problems parsing panel-timing (%d)\n",
>  			np, ret);
>  		return ret;
>  	}
>  
> -	videomode_from_timing(&timing, &lvds->video_mode);
> -
> -	ret = of_property_read_u32(np, "width-mm", &lvds->width);
> -	if (ret < 0) {
> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> -			np, "width-mm");
> -		return -ENODEV;
> -	}
> -	ret = of_property_read_u32(np, "height-mm", &lvds->height);
> -	if (ret < 0) {
> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> -			np, "height-mm");
> -		return -ENODEV;
> -	}

of_get_drm_panel_display_mode() doesn't consider missing width-mm or
height-mm properties as an error. Should we check here that ->width_mm
and ->height_mm are not 0 ?

> -
>  	of_property_read_string(np, "label", &lvds->label);
>  
>  	ret = drm_of_lvds_get_data_mapping(np);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] drm/panel: lvds: Use bus_flags from DT panel-timing property

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Thu, Mar 31, 2022 at 09:23:47PM +0200, Marek Vasut wrote:
> This driver currently rewrites bus_flags based solely on the value of
> DT property 'data-mirror' and ignores bus_flags which might have been
> set in DT panel-timing node. Specificaly, the 'de-active' DT property
> sets DRM_BUS_FLAG_DE_ bus_flags.
> 
> Since of_get_drm_panel_display_mode() conveniently parses the bus_flags
> out of DT panel-timing property, just ORR them with bus_flags inferred
> from 'data-mirror' DT property and use the result as panel bus_flags.
> 
> This fixes handling of panels with 'panel-timing { de-active = <1>; };'.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dmitry Osipenko <digetx@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> To: dri-devel@lists.freedesktop.org

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/panel/panel-lvds.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
> index 65c6a6e9e223..f11252fb00fe 100644
> --- a/drivers/gpu/drm/panel/panel-lvds.c
> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> @@ -33,7 +33,6 @@ struct panel_lvds {
>  	struct drm_display_mode dmode;
>  	u32 bus_flags;
>  	unsigned int bus_format;
> -	bool data_mirror;
>  
>  	struct regulator *supply;
>  
> @@ -99,9 +98,7 @@ static int panel_lvds_get_modes(struct drm_panel *panel,
>  	connector->display_info.height_mm = lvds->dmode.height_mm;
>  	drm_display_info_set_bus_formats(&connector->display_info,
>  					 &lvds->bus_format, 1);
> -	connector->display_info.bus_flags = lvds->data_mirror
> -					  ? DRM_BUS_FLAG_DATA_LSB_TO_MSB
> -					  : DRM_BUS_FLAG_DATA_MSB_TO_LSB;
> +	connector->display_info.bus_flags = lvds->bus_flags;
>  	drm_connector_set_panel_orientation(connector, lvds->orientation);
>  
>  	return 1;
> @@ -142,7 +139,9 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
>  
>  	lvds->bus_format = ret;
>  
> -	lvds->data_mirror = of_property_read_bool(np, "data-mirror");
> +	lvds->bus_flags |= of_property_read_bool(np, "data-mirror") ?
> +			   DRM_BUS_FLAG_DATA_LSB_TO_MSB :
> +			   DRM_BUS_FLAG_DATA_MSB_TO_LSB;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Marek Vasut]

On 4/1/22 17:48, Laurent Pinchart wrote:

Hi,

[...]

>> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
>> index 27a1c9923b09..65c6a6e9e223 100644
>> --- a/drivers/gpu/drm/panel/panel-lvds.c
>> +++ b/drivers/gpu/drm/panel/panel-lvds.c
>> @@ -30,7 +30,8 @@ struct panel_lvds {
>>   	const char *label;
>>   	unsigned int width;
>>   	unsigned int height;
>> -	struct videomode video_mode;
>> +	struct drm_display_mode dmode;
> 
> "dmode" sounds a bit weird, I would have gone for just "mode", or
> "display_mode", but I don't mind much.

That's how the of_get_drm_panel_display_mode() parameter is called in 
drivers/gpu/drm/drm_modes.c , so I'll just keep it for consistency.

[...]

>> -	videomode_from_timing(&timing, &lvds->video_mode);
>> -
>> -	ret = of_property_read_u32(np, "width-mm", &lvds->width);
>> -	if (ret < 0) {
>> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
>> -			np, "width-mm");
>> -		return -ENODEV;
>> -	}
>> -	ret = of_property_read_u32(np, "height-mm", &lvds->height);
>> -	if (ret < 0) {
>> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
>> -			np, "height-mm");
>> -		return -ENODEV;
>> -	}
> 
> of_get_drm_panel_display_mode() doesn't consider missing width-mm or
> height-mm properties as an error. Should we check here that ->width_mm
> and ->height_mm are not 0 ?

I wonder whether we should always require valid width-mm and height-mm 
DT property in of_get_drm_panel_display_mode() instead.

[...]

Re: [PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Laurent Pinchart]

On Fri, Apr 01, 2022 at 06:11:22PM +0200, Marek Vasut wrote:
> On 4/1/22 17:48, Laurent Pinchart wrote:
> 
> Hi,
> 
> [...]
> 
> >> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
> >> index 27a1c9923b09..65c6a6e9e223 100644
> >> --- a/drivers/gpu/drm/panel/panel-lvds.c
> >> +++ b/drivers/gpu/drm/panel/panel-lvds.c
> >> @@ -30,7 +30,8 @@ struct panel_lvds {
> >>   	const char *label;
> >>   	unsigned int width;
> >>   	unsigned int height;
> >> -	struct videomode video_mode;
> >> +	struct drm_display_mode dmode;
> > 
> > "dmode" sounds a bit weird, I would have gone for just "mode", or
> > "display_mode", but I don't mind much.
> 
> That's how the of_get_drm_panel_display_mode() parameter is called in 
> drivers/gpu/drm/drm_modes.c , so I'll just keep it for consistency.
> 
> [...]
> 
> >> -	videomode_from_timing(&timing, &lvds->video_mode);
> >> -
> >> -	ret = of_property_read_u32(np, "width-mm", &lvds->width);
> >> -	if (ret < 0) {
> >> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> >> -			np, "width-mm");
> >> -		return -ENODEV;
> >> -	}
> >> -	ret = of_property_read_u32(np, "height-mm", &lvds->height);
> >> -	if (ret < 0) {
> >> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
> >> -			np, "height-mm");
> >> -		return -ENODEV;
> >> -	}
> > 
> > of_get_drm_panel_display_mode() doesn't consider missing width-mm or
> > height-mm properties as an error. Should we check here that ->width_mm
> > and ->height_mm are not 0 ?
> 
> I wonder whether we should always require valid width-mm and height-mm 
> DT property in of_get_drm_panel_display_mode() instead.

If that doesn't introduce any regression, that would be my preference
too.

> [...]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] drm/panel: lvds: Simplify mode parsing

[Marek Vasut]

On 4/1/22 19:02, Laurent Pinchart wrote:
> On Fri, Apr 01, 2022 at 06:11:22PM +0200, Marek Vasut wrote:
>> On 4/1/22 17:48, Laurent Pinchart wrote:
>>
>> Hi,
>>
>> [...]
>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
>>>> index 27a1c9923b09..65c6a6e9e223 100644
>>>> --- a/drivers/gpu/drm/panel/panel-lvds.c
>>>> +++ b/drivers/gpu/drm/panel/panel-lvds.c
>>>> @@ -30,7 +30,8 @@ struct panel_lvds {
>>>>    	const char *label;
>>>>    	unsigned int width;
>>>>    	unsigned int height;
>>>> -	struct videomode video_mode;
>>>> +	struct drm_display_mode dmode;
>>>
>>> "dmode" sounds a bit weird, I would have gone for just "mode", or
>>> "display_mode", but I don't mind much.
>>
>> That's how the of_get_drm_panel_display_mode() parameter is called in
>> drivers/gpu/drm/drm_modes.c , so I'll just keep it for consistency.
>>
>> [...]
>>
>>>> -	videomode_from_timing(&timing, &lvds->video_mode);
>>>> -
>>>> -	ret = of_property_read_u32(np, "width-mm", &lvds->width);
>>>> -	if (ret < 0) {
>>>> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
>>>> -			np, "width-mm");
>>>> -		return -ENODEV;
>>>> -	}
>>>> -	ret = of_property_read_u32(np, "height-mm", &lvds->height);
>>>> -	if (ret < 0) {
>>>> -		dev_err(lvds->dev, "%pOF: invalid or missing %s DT property\n",
>>>> -			np, "height-mm");
>>>> -		return -ENODEV;
>>>> -	}
>>>
>>> of_get_drm_panel_display_mode() doesn't consider missing width-mm or
>>> height-mm properties as an error. Should we check here that ->width_mm
>>> and ->height_mm are not 0 ?
>>
>> I wonder whether we should always require valid width-mm and height-mm
>> DT property in of_get_drm_panel_display_mode() instead.
> 
> If that doesn't introduce any regression, that would be my preference
> too.

I sent out an RFC series, so let's see.