[PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Marek Vasut]

Decoder input LVDS format is a property of the decoder chip or even
its strapping. Add DT property data-mapping the same way lvds-panel
does, to define the LVDS data mapping.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: devicetree@vger.kernel.org
To: dri-devel@lists.freedesktop.org
---
 .../bindings/display/bridge/lvds-codec.yaml   | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index f4dd16bd69d2..f0abb94f8f2e 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -64,6 +64,15 @@ properties:
       - port@0
       - port@1
 
+  data-mapping:
+    enum:
+      - jeida-18
+      - jeida-24
+      - vesa-24
+    description: |
+      The color signals mapping order. See details in
+      Documentation/devicetree/bindings/display/panel/lvds.yaml
+
   pclk-sample:
     description:
       Data sampling on rising or falling edge.
@@ -79,6 +88,16 @@ properties:
 
   power-supply: true
 
+if:
+  not:
+    properties:
+      compatible:
+        contains:
+          const: lvds-decoder
+then:
+  properties:
+    data-mapping: false
+
 if:
   not:
     properties:
-- 
2.30.2

[PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

[Marek Vasut]

Decoder input LVDS format is a property of the decoder chip or even
its strapping. Handle data-mapping the same way lvds-panel does. In
case data-mapping is not present, do nothing, since there are still
legacy bindings which do not specify this property.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
To: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index 8a7cb267ab14..33f992d52902 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -23,6 +23,7 @@ struct lvds_codec {
 	struct regulator *vcc;
 	struct gpio_desc *powerdown_gpio;
 	u32 connector_type;
+	unsigned int bus_format;
 };
 
 static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge)
@@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge)
 			"Failed to disable regulator \"vcc\": %d\n", ret);
 }
 
+static bool lvds_codec_mode_fixup(struct drm_bridge *bridge,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adj)
+{
+	struct lvds_codec *lvds_codec = to_lvds_codec(bridge);
+	struct drm_encoder *encoder = bridge->encoder;
+	struct drm_device *ddev = encoder->dev;
+	struct drm_connector *connector;
+
+	/* If 'data-mapping' was not specified, do nothing. */
+	if (!lvds_codec->bus_format)
+		return true;
+
+	/* Patch in the LVDS format */
+	list_for_each_entry(connector, &ddev->mode_config.connector_list, head) {
+		drm_display_info_set_bus_formats(&connector->display_info,
+						 &lvds_codec->bus_format, 1);
+	}
+
+	return true;
+}
+
 static const struct drm_bridge_funcs funcs = {
 	.attach = lvds_codec_attach,
 	.enable = lvds_codec_enable,
 	.disable = lvds_codec_disable,
+	.mode_fixup = lvds_codec_mode_fixup,
 };
 
 static int lvds_codec_probe(struct platform_device *pdev)
@@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
 	struct device_node *panel_node;
 	struct drm_panel *panel;
 	struct lvds_codec *lvds_codec;
+	const char *mapping;
 	u32 val;
 
 	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
@@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev)
 			DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
 	}
 
+	/*
+	 * Decoder input LVDS format is a property of the decoder chip or even
+	 * its strapping. Handle data-mapping the same way lvds-panel does. In
+	 * case data-mapping is not present, do nothing, since there are still
+	 * legacy bindings which do not specify this property.
+	 */
+	if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) {
+		ret = of_property_read_string(dev->of_node, "data-mapping",
+					      &mapping);
+		if (ret < 0) {
+			dev_err(dev, "missing 'data-mapping' DT property\n");
+		} else {
+			if (!strcmp(mapping, "jeida-18")) {
+				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
+			} else if (!strcmp(mapping, "jeida-24")) {
+				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+			} else if (!strcmp(mapping, "vesa-24")) {
+				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+			} else {
+				dev_err(dev, "invalid 'data-mapping' DT property\n");
+				return -EINVAL;
+			}
+		}
+	}
+
 	/*
 	 * The panel_bridge bridge is attached to the panel's of_node,
 	 * but we need a bridge attached to our of_node for our user
-- 
2.30.2

Re: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Sat, May 15, 2021 at 10:46:56PM +0200, Marek Vasut wrote:
> Decoder input LVDS format is a property of the decoder chip or even
> its strapping. Handle data-mapping the same way lvds-panel does. In
> case data-mapping is not present, do nothing, since there are still
> legacy bindings which do not specify this property.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> To: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/lvds-codec.c | 50 +++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index 8a7cb267ab14..33f992d52902 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -23,6 +23,7 @@ struct lvds_codec {
>  	struct regulator *vcc;
>  	struct gpio_desc *powerdown_gpio;
>  	u32 connector_type;
> +	unsigned int bus_format;
>  };
>  
>  static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge)
> @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge)
>  			"Failed to disable regulator \"vcc\": %d\n", ret);
>  }
>  
> +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge,
> +				const struct drm_display_mode *mode,
> +				struct drm_display_mode *adj)
> +{
> +	struct lvds_codec *lvds_codec = to_lvds_codec(bridge);
> +	struct drm_encoder *encoder = bridge->encoder;
> +	struct drm_device *ddev = encoder->dev;
> +	struct drm_connector *connector;
> +
> +	/* If 'data-mapping' was not specified, do nothing. */
> +	if (!lvds_codec->bus_format)
> +		return true;
> +
> +	/* Patch in the LVDS format */
> +	list_for_each_entry(connector, &ddev->mode_config.connector_list, head) {
> +		drm_display_info_set_bus_formats(&connector->display_info,
> +						 &lvds_codec->bus_format, 1);
> +	}

This part bothers me, as the format at the input of the LVDS decoder
doesn't match the format on the connector. Shouldn't you implement
.atomic_get_output_bus_fmts() instead ?

> +
> +	return true;
> +}
> +
>  static const struct drm_bridge_funcs funcs = {
>  	.attach = lvds_codec_attach,
>  	.enable = lvds_codec_enable,
>  	.disable = lvds_codec_disable,
> +	.mode_fixup = lvds_codec_mode_fixup,
>  };
>  
>  static int lvds_codec_probe(struct platform_device *pdev)
> @@ -81,6 +105,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  	struct device_node *panel_node;
>  	struct drm_panel *panel;
>  	struct lvds_codec *lvds_codec;
> +	const char *mapping;

I would have moved this variable to the if () { ... } below, but maybe
that's just me.

>  	u32 val;
>  
>  	lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> @@ -133,6 +158,31 @@ static int lvds_codec_probe(struct platform_device *pdev)
>  			DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
>  	}
>  
> +	/*
> +	 * Decoder input LVDS format is a property of the decoder chip or even
> +	 * its strapping. Handle data-mapping the same way lvds-panel does. In
> +	 * case data-mapping is not present, do nothing, since there are still
> +	 * legacy bindings which do not specify this property.
> +	 */
> +	if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) {
> +		ret = of_property_read_string(dev->of_node, "data-mapping",
> +					      &mapping);
> +		if (ret < 0) {
> +			dev_err(dev, "missing 'data-mapping' DT property\n");
> +		} else {
> +			if (!strcmp(mapping, "jeida-18")) {
> +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
> +			} else if (!strcmp(mapping, "jeida-24")) {
> +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +			} else if (!strcmp(mapping, "vesa-24")) {
> +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> +			} else {
> +				dev_err(dev, "invalid 'data-mapping' DT property\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
>  	/*
>  	 * The panel_bridge bridge is attached to the panel's of_node,
>  	 * but we need a bridge attached to our of_node for our user

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

[Marek Vasut]

On 5/18/21 1:03 AM, Laurent Pinchart wrote:

Hi,

[...]

>> @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge)
>>   			"Failed to disable regulator \"vcc\": %d\n", ret);
>>   }
>>   
>> +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge,
>> +				const struct drm_display_mode *mode,
>> +				struct drm_display_mode *adj)
>> +{
>> +	struct lvds_codec *lvds_codec = to_lvds_codec(bridge);
>> +	struct drm_encoder *encoder = bridge->encoder;
>> +	struct drm_device *ddev = encoder->dev;
>> +	struct drm_connector *connector;
>> +
>> +	/* If 'data-mapping' was not specified, do nothing. */
>> +	if (!lvds_codec->bus_format)
>> +		return true;
>> +
>> +	/* Patch in the LVDS format */
>> +	list_for_each_entry(connector, &ddev->mode_config.connector_list, head) {
>> +		drm_display_info_set_bus_formats(&connector->display_info,
>> +						 &lvds_codec->bus_format, 1);
>> +	}
> 
> This part bothers me, as the format at the input of the LVDS decoder
> doesn't match the format on the connector. Shouldn't you implement
> .atomic_get_output_bus_fmts() instead ?

No, I tried that option before this solution and that didn't work. The 
atomic stuff seems to be separate from what I need to pass here, i.e. 
without this, e.g. the mxsfb scanout engine still receives the wrong format.

[...]

Re: [PATCH 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

[Laurent Pinchart]

Hi Marek,

On Tue, May 25, 2021 at 12:38:47PM +0200, Marek Vasut wrote:
> On 5/18/21 1:03 AM, Laurent Pinchart wrote:
> 
> Hi,
> 
> [...]
> 
> >> @@ -69,10 +70,33 @@ static void lvds_codec_disable(struct drm_bridge *bridge)
> >>   			"Failed to disable regulator \"vcc\": %d\n", ret);
> >>   }
> >>   
> >> +static bool lvds_codec_mode_fixup(struct drm_bridge *bridge,
> >> +				const struct drm_display_mode *mode,
> >> +				struct drm_display_mode *adj)
> >> +{
> >> +	struct lvds_codec *lvds_codec = to_lvds_codec(bridge);
> >> +	struct drm_encoder *encoder = bridge->encoder;
> >> +	struct drm_device *ddev = encoder->dev;
> >> +	struct drm_connector *connector;
> >> +
> >> +	/* If 'data-mapping' was not specified, do nothing. */
> >> +	if (!lvds_codec->bus_format)
> >> +		return true;
> >> +
> >> +	/* Patch in the LVDS format */
> >> +	list_for_each_entry(connector, &ddev->mode_config.connector_list, head) {
> >> +		drm_display_info_set_bus_formats(&connector->display_info,
> >> +						 &lvds_codec->bus_format, 1);
> >> +	}
> > 
> > This part bothers me, as the format at the input of the LVDS decoder
> > doesn't match the format on the connector. Shouldn't you implement
> > .atomic_get_output_bus_fmts() instead ?
> 
> No, I tried that option before this solution and that didn't work. The 
> atomic stuff seems to be separate from what I need to pass here, i.e. 
> without this, e.g. the mxsfb scanout engine still receives the wrong format.

I fear this needs to be investigated then, as the connected format isn't
the same as the LVDS decoder input format, so the above isn't right.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Laurent Pinchart]

Hi Marek,

Thank you for the patch.

On Sat, May 15, 2021 at 10:46:55PM +0200, Marek Vasut wrote:
> Decoder input LVDS format is a property of the decoder chip or even
> its strapping. Add DT property data-mapping the same way lvds-panel
> does, to define the LVDS data mapping.

The information could also be derived by the driver from the compatible
string in the case when this is an intrinsic property of the device (or
when it's configurable by software), but the fact that it can also be
controlled by strapping makes a DT property needed. We may want to limit
the usage of the DT property to the strapping case though, but I don't
have a real preference here, so I'm fine with this approach.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: devicetree@vger.kernel.org
> To: dri-devel@lists.freedesktop.org
> ---
>  .../bindings/display/bridge/lvds-codec.yaml   | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index f4dd16bd69d2..f0abb94f8f2e 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -64,6 +64,15 @@ properties:
>        - port@0
>        - port@1
>  
> +  data-mapping:
> +    enum:
> +      - jeida-18
> +      - jeida-24
> +      - vesa-24
> +    description: |
> +      The color signals mapping order. See details in
> +      Documentation/devicetree/bindings/display/panel/lvds.yaml
> +
>    pclk-sample:
>      description:
>        Data sampling on rising or falling edge.
> @@ -79,6 +88,16 @@ properties:
>  
>    power-supply: true
>  
> +if:
> +  not:
> +    properties:
> +      compatible:
> +        contains:
> +          const: lvds-decoder
> +then:
> +  properties:
> +    data-mapping: false
> +
>  if:
>    not:
>      properties:

Unless I'm mistaken, you can't have identically named properties at the
same level (multiple 'if' at the root level). You can group them in a
'allOf' property.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Laurent Pinchart]

And another thing:

On Tue, May 18, 2021 at 02:02:24AM +0300, Laurent Pinchart wrote:
> On Sat, May 15, 2021 at 10:46:55PM +0200, Marek Vasut wrote:
> > Decoder input LVDS format is a property of the decoder chip or even
> > its strapping. Add DT property data-mapping the same way lvds-panel
> > does, to define the LVDS data mapping.
> 
> The information could also be derived by the driver from the compatible
> string in the case when this is an intrinsic property of the device (or
> when it's configurable by software), but the fact that it can also be
> controlled by strapping makes a DT property needed. We may want to limit
> the usage of the DT property to the strapping case though, but I don't
> have a real preference here, so I'm fine with this approach.
> 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: devicetree@vger.kernel.org
> > To: dri-devel@lists.freedesktop.org
> > ---
> >  .../bindings/display/bridge/lvds-codec.yaml   | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > index f4dd16bd69d2..f0abb94f8f2e 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > @@ -64,6 +64,15 @@ properties:
> >        - port@0
> >        - port@1
> >  
> > +  data-mapping:
> > +    enum:
> > +      - jeida-18
> > +      - jeida-24
> > +      - vesa-24
> > +    description: |
> > +      The color signals mapping order. See details in
> > +      Documentation/devicetree/bindings/display/panel/lvds.yaml
> > +
> >    pclk-sample:
> >      description:
> >        Data sampling on rising or falling edge.
> > @@ -79,6 +88,16 @@ properties:
> >  
> >    power-supply: true
> >  
> > +if:
> > +  not:
> > +    properties:
> > +      compatible:
> > +        contains:
> > +          const: lvds-decoder
> > +then:
> > +  properties:
> > +    data-mapping: false

Should we make the property required for lvds-decoder ? We need to
support backward compatibility in the driver, but from a DT bindings
point of view, should all new DTs require the property ?

> > +
> >  if:
> >    not:
> >      properties:
> 
> Unless I'm mistaken, you can't have identically named properties at the
> same level (multiple 'if' at the root level). You can group them in a
> 'allOf' property.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Marek Vasut]

On 5/18/21 1:03 AM, Laurent Pinchart wrote:

[...]

>>> +if:
>>> +  not:
>>> +    properties:
>>> +      compatible:
>>> +        contains:
>>> +          const: lvds-decoder
>>> +then:
>>> +  properties:
>>> +    data-mapping: false
> 
> Should we make the property required for lvds-decoder ? We need to
> support backward compatibility in the driver, but from a DT bindings
> point of view, should all new DTs require the property ?

I think so.

Re: [PATCH 1/2] dt-bindings: display: bridge: lvds-codec: Document LVDS data mapping select

[Marek Vasut]

On 5/18/21 1:02 AM, Laurent Pinchart wrote:
> Hi Marek,

Hi,

> Thank you for the patch.
> 
> On Sat, May 15, 2021 at 10:46:55PM +0200, Marek Vasut wrote:
>> Decoder input LVDS format is a property of the decoder chip or even
>> its strapping. Add DT property data-mapping the same way lvds-panel
>> does, to define the LVDS data mapping.
> 
> The information could also be derived by the driver from the compatible
> string in the case when this is an intrinsic property of the device (or
> when it's configurable by software), but the fact that it can also be
> controlled by strapping makes a DT property needed. We may want to limit
> the usage of the DT property to the strapping case though, but I don't
> have a real preference here, so I'm fine with this approach.

You'd soon end up piling up compatible strings like the panel-simple 
does, I don't think that scales. Besides, there are bridges where you 
can pick the mapping with a strap, and then the compatible string is no 
longer enough.

[...]