[PATCH 1/2] drm/bridge: sii902x: Provide data-lines property to input endpoint

[PATCH 1/2] drm/bridge: sii902x: Provide data-lines property to input endpoint

[Wadim Egorov]

Introduce a data-lines property to define the number of parallel RGB
input pins connected to the transmitter. The input bus formats are updated
accordingly. If the property is not specified, default to 24 data lines.

Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
---
 drivers/gpu/drm/bridge/sii902x.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 7f91b0db161e..3565c3533597 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -180,6 +180,8 @@ struct sii902x {
 	struct gpio_desc *reset_gpio;
 	struct i2c_mux_core *i2cmux;
 	bool sink_is_hdmi;
+	u32 pd_lines; /* number of Parallel Port Input Data Lines */
+
 	/*
 	 * Mutex protects audio and video functions from interfering
 	 * each other, by keeping their i2c command sequences atomic.
@@ -477,6 +479,8 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 						     u32 output_fmt,
 						     unsigned int *num_input_fmts)
 {
+
+	struct sii902x *sii902x = bridge_to_sii902x(bridge);
 	u32 *input_fmts;
 
 	*num_input_fmts = 0;
@@ -485,7 +489,19 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 	if (!input_fmts)
 		return NULL;
 
-	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+	switch (sii902x->pd_lines) {
+	case 16:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16;
+		break;
+	case 18:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18;
+		break;
+	default:
+	case 24:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+	}
+
 	*num_input_fmts = 1;
 
 	return input_fmts;
@@ -1167,6 +1183,15 @@ static int sii902x_probe(struct i2c_client *client)
 		return PTR_ERR(sii902x->reset_gpio);
 	}
 
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
+	if (endpoint) {
+		ret = of_property_read_u32(endpoint, "data-lines", &sii902x->pd_lines);
+		if (ret) {
+			dev_dbg(dev, "Could not get data-lines, fallback to 24 data-lines\n");
+			sii902x->pd_lines = 24;
+		}
+	}
+
 	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
 	if (endpoint) {
 		struct device_node *remote = of_graph_get_remote_port_parent(endpoint);
-- 
2.34.1

[PATCH 2/2] dt-bindings: display: bridge: sil,sii9022: Add data-lines

[Wadim Egorov]

The SI9022 HDMI transmitter can be configured with 16, 18, or 24 input
data lines. This commit introduces the data-lines property to the input
endpoint, specifying the number of parallel RGB input pins connected
to the transmitter.

Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
---
 .../bindings/display/bridge/sil,sii9022.yaml        | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
index 5a69547ad3d7..24306f8eb107 100644
--- a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
@@ -81,9 +81,20 @@ properties:
 
     properties:
       port@0:
-        $ref: /schemas/graph.yaml#/properties/port
+        unevaluatedProperties: false
+        $ref: /schemas/graph.yaml#/$defs/port-base
         description: Parallel RGB input port
 
+        properties:
+          endpoint:
+            $ref: /schemas/graph.yaml#/$defs/endpoint-base
+            unevaluatedProperties: false
+
+            properties:
+              data-lines:
+                $ref: /schemas/types.yaml#/definitions/uint32
+                enum: [ 16, 18, 24 ]
+
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: HDMI output port
-- 
2.34.1

Re: [PATCH 2/2] dt-bindings: display: bridge: sil,sii9022: Add data-lines

[Krzysztof Kozlowski]

On 03/10/2024 10:20, Wadim Egorov wrote:
> The SI9022 HDMI transmitter can be configured with 16, 18, or 24 input
> data lines. This commit introduces the data-lines property to the input

lines? lanes? What are lines? like pins?

> endpoint, specifying the number of parallel RGB input pins connected
> to the transmitter.
> 
> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
> ---
>  .../bindings/display/bridge/sil,sii9022.yaml        | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
> index 5a69547ad3d7..24306f8eb107 100644
> --- a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
> @@ -81,9 +81,20 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        unevaluatedProperties: false
> +        $ref: /schemas/graph.yaml#/$defs/port-base
>          description: Parallel RGB input port
>  
> +        properties:
> +          endpoint:
> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lines:

No, this will confuse everyone. Considering lack of description how
anyone would figure out what this means?

I don't know much about RGB parallel port to advice how this should be
called. Can you describe the hardware more?

> +                $ref: /schemas/types.yaml#/definitions/uint32
> +                enum: [ 16, 18, 24 ]
> +
>        port@1:
>          $ref: /schemas/graph.yaml#/properties/port
>          description: HDMI output port

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: display: bridge: sil,sii9022: Add data-lines

[Wadim Egorov]

Am 03.10.24 um 12:03 schrieb Krzysztof Kozlowski:
> On 03/10/2024 10:20, Wadim Egorov wrote:
>> The SI9022 HDMI transmitter can be configured with 16, 18, or 24 input
>> data lines. This commit introduces the data-lines property to the input
> 
> lines? lanes? What are lines? like pins?

Yes, "lines" in this context refers to the number of pins used for the 
input pixel data bus, which can support 16, 18, or 24-bit wide data 
buses. These are parallel data lines (or pins) that carry uncompressed 
digital video to the HDMI transmitter.

> 
>> endpoint, specifying the number of parallel RGB input pins connected
>> to the transmitter.
>>
>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>> ---
>>   .../bindings/display/bridge/sil,sii9022.yaml        | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>> index 5a69547ad3d7..24306f8eb107 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>> +++ b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>> @@ -81,9 +81,20 @@ properties:
>>   
>>       properties:
>>         port@0:
>> -        $ref: /schemas/graph.yaml#/properties/port
>> +        unevaluatedProperties: false
>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>           description: Parallel RGB input port
>>   
>> +        properties:
>> +          endpoint:
>> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
>> +            unevaluatedProperties: false
>> +
>> +            properties:
>> +              data-lines:
> 
> No, this will confuse everyone. Considering lack of description how
> anyone would figure out what this means?

I guess from working with the hardware/reference manual and using this chip?

I don't think it is overly confusing, especially since the port is 
already described as the "Parallel RGB input port" which clearly implies 
the use of pins for data transmission.

I am open to other suggestions if you believe a different name would 
improve clarity.

Btw, bridge/toshiba,tc358768.yaml, which performs a similar function, 
also uses the term data-lines.

Regards,
Wadim

> 
> I don't know much about RGB parallel port to advice how this should be
> called. Can you describe the hardware more?
> 
>> +                $ref: /schemas/types.yaml#/definitions/uint32
>> +                enum: [ 16, 18, 24 ]
>> +
>>         port@1:
>>           $ref: /schemas/graph.yaml#/properties/port
>>           description: HDMI output port
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/2] dt-bindings: display: bridge: sil,sii9022: Add data-lines

[Krzysztof Kozlowski]

On 03/10/2024 13:56, Wadim Egorov wrote:
> 
> 
> Am 03.10.24 um 12:03 schrieb Krzysztof Kozlowski:
>> On 03/10/2024 10:20, Wadim Egorov wrote:
>>> The SI9022 HDMI transmitter can be configured with 16, 18, or 24 input
>>> data lines. This commit introduces the data-lines property to the input
>>
>> lines? lanes? What are lines? like pins?
> 
> Yes, "lines" in this context refers to the number of pins used for the 
> input pixel data bus, which can support 16, 18, or 24-bit wide data 
> buses. These are parallel data lines (or pins) that carry uncompressed 
> digital video to the HDMI transmitter.
> 
>>
>>> endpoint, specifying the number of parallel RGB input pins connected
>>> to the transmitter.
>>>
>>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>>> ---
>>>   .../bindings/display/bridge/sil,sii9022.yaml        | 13 ++++++++++++-
>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>> index 5a69547ad3d7..24306f8eb107 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>> +++ b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>> @@ -81,9 +81,20 @@ properties:
>>>   
>>>       properties:
>>>         port@0:
>>> -        $ref: /schemas/graph.yaml#/properties/port
>>> +        unevaluatedProperties: false
>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>>           description: Parallel RGB input port
>>>   
>>> +        properties:
>>> +          endpoint:
>>> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
>>> +            unevaluatedProperties: false
>>> +
>>> +            properties:
>>> +              data-lines:
>>
>> No, this will confuse everyone. Considering lack of description how
>> anyone would figure out what this means?
> 
> I guess from working with the hardware/reference manual and using this chip?
> 
> I don't think it is overly confusing, especially since the port is 
> already described as the "Parallel RGB input port" which clearly implies 
> the use of pins for data transmission.


I am surprised you do not find data-lanes and data-lines confusing. For
non-native English speakers this even might sound the same.

You used earlier pins and bits, so maybe it's the same as bus-width,
which is already used all over the bindings, including one of the bridges.

Anyway a generic property should go to video-interfaces.

> 
> I am open to other suggestions if you believe a different name would 
> improve clarity.
> 
> Btw, bridge/toshiba,tc358768.yaml, which performs a similar function, 
> also uses the term data-lines.

Then this has to go to common schema.

Oh, wait, video-interfaces already have it!

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: display: bridge: sil,sii9022: Add data-lines

[Wadim Egorov]

Am 03.10.24 um 15:26 schrieb Krzysztof Kozlowski:
> On 03/10/2024 13:56, Wadim Egorov wrote:
>>
>>
>> Am 03.10.24 um 12:03 schrieb Krzysztof Kozlowski:
>>> On 03/10/2024 10:20, Wadim Egorov wrote:
>>>> The SI9022 HDMI transmitter can be configured with 16, 18, or 24 input
>>>> data lines. This commit introduces the data-lines property to the input
>>>
>>> lines? lanes? What are lines? like pins?
>>
>> Yes, "lines" in this context refers to the number of pins used for the
>> input pixel data bus, which can support 16, 18, or 24-bit wide data
>> buses. These are parallel data lines (or pins) that carry uncompressed
>> digital video to the HDMI transmitter.
>>
>>>
>>>> endpoint, specifying the number of parallel RGB input pins connected
>>>> to the transmitter.
>>>>
>>>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>>>> ---
>>>>    .../bindings/display/bridge/sil,sii9022.yaml        | 13 ++++++++++++-
>>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>>> index 5a69547ad3d7..24306f8eb107 100644
>>>> --- a/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/bridge/sil,sii9022.yaml
>>>> @@ -81,9 +81,20 @@ properties:
>>>>    
>>>>        properties:
>>>>          port@0:
>>>> -        $ref: /schemas/graph.yaml#/properties/port
>>>> +        unevaluatedProperties: false
>>>> +        $ref: /schemas/graph.yaml#/$defs/port-base
>>>>            description: Parallel RGB input port
>>>>    
>>>> +        properties:
>>>> +          endpoint:
>>>> +            $ref: /schemas/graph.yaml#/$defs/endpoint-base
>>>> +            unevaluatedProperties: false
>>>> +
>>>> +            properties:
>>>> +              data-lines:
>>>
>>> No, this will confuse everyone. Considering lack of description how
>>> anyone would figure out what this means?
>>
>> I guess from working with the hardware/reference manual and using this chip?
>>
>> I don't think it is overly confusing, especially since the port is
>> already described as the "Parallel RGB input port" which clearly implies
>> the use of pins for data transmission.
> 
> 
> I am surprised you do not find data-lanes and data-lines confusing. For
> non-native English speakers this even might sound the same.
> 
> You used earlier pins and bits, so maybe it's the same as bus-width,
> which is already used all over the bindings, including one of the bridges.

Thanks, the bus-width property seems to be a better fit here. I'll 
rework my patches.

> 
> Anyway a generic property should go to video-interfaces.
> 
>>
>> I am open to other suggestions if you believe a different name would
>> improve clarity.
>>
>> Btw, bridge/toshiba,tc358768.yaml, which performs a similar function,
>> also uses the term data-lines.
> 
> Then this has to go to common schema.
> 
> Oh, wait, video-interfaces already have it!
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] drm/bridge: sii902x: Provide data-lines property to input endpoint

[Aradhya Bhatia]

Hi Wadim,

Thanks for the patch.

Probably a nit, but the dt-binding patch should come before the driver
patch.

On 03-10-2024 13:50, Wadim Egorov wrote:
> Introduce a data-lines property to define the number of parallel RGB
> input pins connected to the transmitter. The input bus formats are updated
> accordingly. If the property is not specified, default to 24 data lines.
> 
> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 7f91b0db161e..3565c3533597 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -180,6 +180,8 @@ struct sii902x {
>  	struct gpio_desc *reset_gpio;
>  	struct i2c_mux_core *i2cmux;
>  	bool sink_is_hdmi;
> +	u32 pd_lines; /* number of Parallel Port Input Data Lines */
> +
>  	/*
>  	 * Mutex protects audio and video functions from interfering
>  	 * each other, by keeping their i2c command sequences atomic.
> @@ -477,6 +479,8 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>  						     u32 output_fmt,
>  						     unsigned int *num_input_fmts)
>  {
> +
> +	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>  	u32 *input_fmts;
>  
>  	*num_input_fmts = 0;
> @@ -485,7 +489,19 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>  	if (!input_fmts)
>  		return NULL;
>  
> -	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +	switch (sii902x->pd_lines) {
> +	case 16:
> +		input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16;
> +		break;
> +	case 18:
> +		input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18;
> +		break;
> +	default:

For backward compatibility - in cases where the property is absent - you
have already defaulted sii902x->pd_lines to 24 below, which I think is
the right way.

So, the default case should be kept separately, as an error case -
which should then return back NULL / num_input_fmts = 0.

> +	case 24:
> +		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +		break;
> +	}
> +
>  	*num_input_fmts = 1;
>  
>  	return input_fmts;
> @@ -1167,6 +1183,15 @@ static int sii902x_probe(struct i2c_client *client)
>  		return PTR_ERR(sii902x->reset_gpio);
>  	}
>
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> +	if (endpoint) {
> +		ret = of_property_read_u32(endpoint, "data-lines", &sii902x->pd_lines);
> +		if (ret) {
> +			dev_dbg(dev, "Could not get data-lines, fallback to 24 data-lines\n");
> +			sii902x->pd_lines = 24;
> +		}
> +	}
> +
>  	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
>  	if (endpoint) {
>  		struct device_node *remote = of_graph_get_remote_port_parent(endpoint);

--
Regards
Aradhya

Re: [PATCH 1/2] drm/bridge: sii902x: Provide data-lines property to input endpoint

[Aradhya Bhatia]

Hi,

On 03-10-2024 15:13, Aradhya Bhatia wrote:
> Hi Wadim,
> 
> Thanks for the patch.
> 
> Probably a nit, but the dt-binding patch should come before the driver
> patch.
> 
> On 03-10-2024 13:50, Wadim Egorov wrote:
>> Introduce a data-lines property to define the number of parallel RGB
>> input pins connected to the transmitter. The input bus formats are updated
>> accordingly. If the property is not specified, default to 24 data lines.

One more thing. This driver also supports the older way of
encoder-bridge connections - that is, with no
DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.

In the sii902x_bridge_attach, you will see that the bus_format is being
statically assigned to MEDIA_BUS_FMT_RGB888_1X24. When the
ATTACH_NO_CONNECTOR flag is set, it doesn't matter. But when it is not,
the RGB888 bus format will get trickled back to the encoder even if the
dt property says differently.

>>
>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>> ---
>>  drivers/gpu/drm/bridge/sii902x.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index 7f91b0db161e..3565c3533597 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -180,6 +180,8 @@ struct sii902x {
>>  	struct gpio_desc *reset_gpio;
>>  	struct i2c_mux_core *i2cmux;
>>  	bool sink_is_hdmi;
>> +	u32 pd_lines; /* number of Parallel Port Input Data Lines */
>> +
>>  	/*
>>  	 * Mutex protects audio and video functions from interfering
>>  	 * each other, by keeping their i2c command sequences atomic.
>> @@ -477,6 +479,8 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>  						     u32 output_fmt,
>>  						     unsigned int *num_input_fmts)
>>  {
>> +

And this is probably a stray.

>> +	struct sii902x *sii902x = bridge_to_sii902x(bridge);
>>  	u32 *input_fmts;
>>  
>>  	*num_input_fmts = 0; 
>> @@ -485,7 +489,19 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
>>  	if (!input_fmts)
>>  		return NULL;
>>  
>> -	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>> +	switch (sii902x->pd_lines) {
>> +	case 16:
>> +		input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16;
>> +		break;
>> +	case 18:
>> +		input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18;
>> +		break;
>> +	default:
> 
> For backward compatibility - in cases where the property is absent - you
> have already defaulted sii902x->pd_lines to 24 below, which I think is
> the right way.
> 
> So, the default case should be kept separately, as an error case -
> which should then return back NULL / num_input_fmts = 0.
> 
>> +	case 24:
>> +		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
>> +		break;
>> +	}
>> +
>>  	*num_input_fmts = 1;
>>  
>>  	return input_fmts;
>> @@ -1167,6 +1183,15 @@ static int sii902x_probe(struct i2c_client *client)
>>  		return PTR_ERR(sii902x->reset_gpio);
>>  	}
>>
>> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
>> +	if (endpoint) {
>> +		ret = of_property_read_u32(endpoint, "data-lines", &sii902x->pd_lines);
>> +		if (ret) {
>> +			dev_dbg(dev, "Could not get data-lines, fallback to 24 data-lines\n");
>> +			sii902x->pd_lines = 24;
>> +		}
>> +	}
>> +
>>  	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
>>  	if (endpoint) {
>>  		struct device_node *remote = of_graph_get_remote_port_parent(endpoint);
> 
> --
> Regards
> Aradhya
> 

--
Regards
Aradhya