[PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap

[PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap

[Alexandru Ardelean]

On some HW designs, it's easier for the layout if the P/N pins are swapped.
In those cases, we need to adjust (for this) by configuring the MIPI analog
registers differently. Specifically, register 0x3e needs to be 0xf6
(instead of 0xd6).

This change adds a 'lontium,pn-swap' device-tree property to configure the
MIPI analog registers for P/N swap.

Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
---
 drivers/gpu/drm/bridge/lontium-lt8912b.c | 25 +++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c
index 4b2ae27f0a57f..154126bb922b4 100644
--- a/drivers/gpu/drm/bridge/lontium-lt8912b.c
+++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c
@@ -47,6 +47,7 @@ struct lt8912 {
 
 	u8 data_lanes;
 	bool is_power_on;
+	bool do_pn_swap;
 };
 
 static int lt8912_write_init_config(struct lt8912 *lt)
@@ -78,15 +79,31 @@ static int lt8912_write_init_config(struct lt8912 *lt)
 		{0x55, 0x44},
 		{0x57, 0x01},
 		{0x5a, 0x02},
-
-		/*MIPI Analog*/
+	};
+	const struct reg_sequence mipi_analog_seq[] = {
 		{0x3e, 0xd6},
 		{0x3f, 0xd4},
 		{0x41, 0x3c},
 		{0xB2, 0x00},
 	};
+	const struct reg_sequence mipi_analog_pn_swap_seq[] = {
+		{0x3e, 0xf6},
+		{0x3f, 0xd4},
+		{0x41, 0x3c},
+		{0xB2, 0x00},
+	};
+	int ret;
 
-	return regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
+	ret = regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
+	if (ret < 0)
+		return ret;
+
+	if (!lt->do_pn_swap)
+		return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_seq,
+					      ARRAY_SIZE(mipi_analog_seq));
+
+	return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_pn_swap_seq,
+				      ARRAY_SIZE(mipi_analog_pn_swap_seq));
 }
 
 static int lt8912_write_mipi_basic_config(struct lt8912 *lt)
@@ -702,6 +719,8 @@ static int lt8912_parse_dt(struct lt8912 *lt)
 	}
 	lt->gp_reset = gp_reset;
 
+	lt->do_pn_swap = device_property_read_bool(dev, "lontium,pn-swap");
+
 	data_lanes = drm_of_get_data_lanes_count_ep(dev->of_node, 0, -1, 1, 4);
 	if (data_lanes < 0) {
 		dev_err(lt->dev, "%s: Bad data-lanes property\n", __func__);
-- 
2.44.0

[PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium, pn-swap' property

[Alexandru Ardelean]

On some HW designs, it's easier for the layout if the P/N pins are swapped.
The driver currently has a DT property to do that.

This change documents the 'lontium,pn-swap' property.

Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
---
 .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
index 2cef252157985..3a804926b288a 100644
--- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
@@ -24,6 +24,12 @@ properties:
     maxItems: 1
     description: GPIO connected to active high RESET pin.
 
+  lontium,pn-swap:
+    description: Swap the polarities of the P/N pins in software.
+      On some HW designs, the layout is simplified if the P/N pins
+      are inverted.
+    type: boolean
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
-- 
2.44.0

Re: [PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap

[Francesco Dolcini]

Hello Alexandru, thanks for your patch.

On Tue, Apr 02, 2024 at 01:59:24PM +0300, Alexandru Ardelean wrote:
> On some HW designs, it's easier for the layout if the P/N pins are swapped.
> In those cases, we need to adjust (for this) by configuring the MIPI analog
> registers differently. Specifically, register 0x3e needs to be 0xf6
> (instead of 0xd6).
> 
> This change adds a 'lontium,pn-swap' device-tree property to configure the
> MIPI analog registers for P/N swap.
> 
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
>  drivers/gpu/drm/bridge/lontium-lt8912b.c | 25 +++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> index 4b2ae27f0a57f..154126bb922b4 100644
> --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c
> +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> @@ -47,6 +47,7 @@ struct lt8912 {
>  
>  	u8 data_lanes;
>  	bool is_power_on;
> +	bool do_pn_swap;
>  };
>  
>  static int lt8912_write_init_config(struct lt8912 *lt)
> @@ -78,15 +79,31 @@ static int lt8912_write_init_config(struct lt8912 *lt)
>  		{0x55, 0x44},
>  		{0x57, 0x01},
>  		{0x5a, 0x02},
> -
> -		/*MIPI Analog*/
> +	};
> +	const struct reg_sequence mipi_analog_seq[] = {
>  		{0x3e, 0xd6},
>  		{0x3f, 0xd4},
>  		{0x41, 0x3c},
>  		{0xB2, 0x00},
>  	};
> +	const struct reg_sequence mipi_analog_pn_swap_seq[] = {
> +		{0x3e, 0xf6},
> +		{0x3f, 0xd4},
> +		{0x41, 0x3c},
> +		{0xB2, 0x00},
> +	};
> +	int ret;
>  
> -	return regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> +	ret = regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!lt->do_pn_swap)
> +		return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_seq,
> +					      ARRAY_SIZE(mipi_analog_seq));
> +
> +	return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_pn_swap_seq,
> +				      ARRAY_SIZE(mipi_analog_pn_swap_seq));

Can you just remove {0x3e, 0xd6} from the register/value array and write
it afterward depending on `do_pn_swap` value? Or keep it with the
current value and only overwrite it when do_pn_swap is true?

If you do it this way is a 4 line change.


>  static int lt8912_write_mipi_basic_config(struct lt8912 *lt)
> @@ -702,6 +719,8 @@ static int lt8912_parse_dt(struct lt8912 *lt)
>  	}
>  	lt->gp_reset = gp_reset;
>  
> +	lt->do_pn_swap = device_property_read_bool(dev, "lontium,pn-swap");

I would call this variable the same that is called in the lontium
documentation, mipirx_diff_swap

Francesco

Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium,pn-swap' property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

On Tue, Apr 02, 2024 at 01:59:25PM +0300, Alexandru Ardelean wrote:
> On some HW designs, it's easier for the layout if the P/N pins are swapped.
> The driver currently has a DT property to do that.

"currently", because 1/2 adds it. bindings patches should precede the
driver patches in the series, so please swap the patches and remove this
portion of the description.

> 
> This change documents the 'lontium,pn-swap' property.
> 
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
>  .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> index 2cef252157985..3a804926b288a 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> @@ -24,6 +24,12 @@ properties:
>      maxItems: 1
>      description: GPIO connected to active high RESET pin.
>  
> +  lontium,pn-swap:
> +    description: Swap the polarities of the P/N pins in software.
> +      On some HW designs, the layout is simplified if the P/N pins
> +      are inverted.

Please explain what configuration of a board would cause these to be
swapped, rather than why someone might want to configure the board this
way. I've got no idea what this hardware is actually doing, so this is
being pulled out of a hat, but I'd expect something like "Some boards
swap the polarity of the P/N pins, use this property to indicate this to
software". 

> +    type: boolean

The type here should be flag.

Cheers,
Conor.

> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  
> -- 
> 2.44.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium,pn-swap' property

[Alexandru Ardelean]

On Tue, Apr 2, 2024 at 9:06 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Apr 02, 2024 at 01:59:25PM +0300, Alexandru Ardelean wrote:
> > On some HW designs, it's easier for the layout if the P/N pins are swapped.
> > The driver currently has a DT property to do that.
>
> "currently", because 1/2 adds it. bindings patches should precede the
> driver patches in the series, so please swap the patches and remove this
> portion of the description.

ack;
i'll invert the order and remove this;

>
> >
> > This change documents the 'lontium,pn-swap' property.
> >
> > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> > ---
> >  .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > index 2cef252157985..3a804926b288a 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > @@ -24,6 +24,12 @@ properties:
> >      maxItems: 1
> >      description: GPIO connected to active high RESET pin.
> >
> > +  lontium,pn-swap:
> > +    description: Swap the polarities of the P/N pins in software.
> > +      On some HW designs, the layout is simplified if the P/N pins
> > +      are inverted.
>
> Please explain what configuration of a board would cause these to be
> swapped, rather than why someone might want to configure the board this
> way. I've got no idea what this hardware is actually doing, so this is
> being pulled out of a hat, but I'd expect something like "Some boards
> swap the polarity of the P/N pins, use this property to indicate this to
> software".

ack
if it's fine with you, i'll use your suggested description;

for a broader context, we were using a DSI-HDMI converter [1] from
SomLabs on a different (than SomLabs) board;
and we were not seeing anything on the HDMI-connected display;
as I understand it, some DSI-HDMI bridges support P/N auto-inversion;
this one doesn't AFAICT;
on this DSI-HDMI converter [1], we've noticed that the P/N pins were
inverted from the DSI to the chip (vs what we expected to see)
after changing the register value (for the P/N swap), it worked;
our conclusion was that, the design of the converter (board) was done
as-such, because it made the layout easier

[1] https://wiki.somlabs.com/index.php/SL-MIPI-LVDS-HDMI-CNV-11_Datasheet_and_Pinout

>
> > +    type: boolean
>
> The type here should be flag.

ack; i'll change the type

>
> Cheers,
> Conor.
>
> > +
> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> >
> > --
> > 2.44.0
> >

Re: [PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap

[Alexandru Ardelean]

On Tue, Apr 2, 2024 at 7:53 PM Francesco Dolcini <francesco@dolcini.it> wrote:
>
> Hello Alexandru, thanks for your patch.
>
> On Tue, Apr 02, 2024 at 01:59:24PM +0300, Alexandru Ardelean wrote:
> > On some HW designs, it's easier for the layout if the P/N pins are swapped.
> > In those cases, we need to adjust (for this) by configuring the MIPI analog
> > registers differently. Specifically, register 0x3e needs to be 0xf6
> > (instead of 0xd6).
> >
> > This change adds a 'lontium,pn-swap' device-tree property to configure the
> > MIPI analog registers for P/N swap.
> >
> > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> > ---
> >  drivers/gpu/drm/bridge/lontium-lt8912b.c | 25 +++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/lontium-lt8912b.c b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> > index 4b2ae27f0a57f..154126bb922b4 100644
> > --- a/drivers/gpu/drm/bridge/lontium-lt8912b.c
> > +++ b/drivers/gpu/drm/bridge/lontium-lt8912b.c
> > @@ -47,6 +47,7 @@ struct lt8912 {
> >
> >       u8 data_lanes;
> >       bool is_power_on;
> > +     bool do_pn_swap;
> >  };
> >
> >  static int lt8912_write_init_config(struct lt8912 *lt)
> > @@ -78,15 +79,31 @@ static int lt8912_write_init_config(struct lt8912 *lt)
> >               {0x55, 0x44},
> >               {0x57, 0x01},
> >               {0x5a, 0x02},
> > -
> > -             /*MIPI Analog*/
> > +     };
> > +     const struct reg_sequence mipi_analog_seq[] = {
> >               {0x3e, 0xd6},
> >               {0x3f, 0xd4},
> >               {0x41, 0x3c},
> >               {0xB2, 0x00},
> >       };
> > +     const struct reg_sequence mipi_analog_pn_swap_seq[] = {
> > +             {0x3e, 0xf6},
> > +             {0x3f, 0xd4},
> > +             {0x41, 0x3c},
> > +             {0xB2, 0x00},
> > +     };
> > +     int ret;
> >
> > -     return regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> > +     ret = regmap_multi_reg_write(lt->regmap[I2C_MAIN], seq, ARRAY_SIZE(seq));
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (!lt->do_pn_swap)
> > +             return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_seq,
> > +                                           ARRAY_SIZE(mipi_analog_seq));
> > +
> > +     return regmap_multi_reg_write(lt->regmap[I2C_MAIN], mipi_analog_pn_swap_seq,
> > +                                   ARRAY_SIZE(mipi_analog_pn_swap_seq));
>
> Can you just remove {0x3e, 0xd6} from the register/value array and write
> it afterward depending on `do_pn_swap` value? Or keep it with the
> current value and only overwrite it when do_pn_swap is true?
>
> If you do it this way is a 4 line change.

Hmm, good point.
I did it like this, because I don't have a board with the P/N in the
0xd6 configuration, to test.
But, if I leave it like this, and just overwrite 0x3e when
`do_pn_swap` is true, I can test that; and I don't need to test the
original case.

I'm actually not 100% sure here if the order of registers (being
written) matters for the initialization.


>
>
> >  static int lt8912_write_mipi_basic_config(struct lt8912 *lt)
> > @@ -702,6 +719,8 @@ static int lt8912_parse_dt(struct lt8912 *lt)
> >       }
> >       lt->gp_reset = gp_reset;
> >
> > +     lt->do_pn_swap = device_property_read_bool(dev, "lontium,pn-swap");
>
> I would call this variable the same that is called in the lontium
> documentation, mipirx_diff_swap

Oh.
I actually based this change on a reference software for the LT8912B.
I didn't get a chance to see/find a documentation for the registers.
I compared with the Linux driver, to see what was missing to get
output on the HDMI display (for our setup).

>
> Francesco
>

Re: [PATCH 1/2] drm/bridge: lt8912b: add support for P/N pin swap

[Francesco Dolcini]

On Wed, Apr 03, 2024 at 09:32:41AM +0300, Alexandru Ardelean wrote:
> I did it like this, because I don't have a board with the P/N in the

You use this 'P/N' both here and in the binding document, to me this is
just too generic and confusing.

Just use some wording that people familiar with the topic can easily undestand,
the Lontium datasheet uses MIPI RX DP/DN, MIPI DSI DP/DN would also work fine
for me, or at least DP/DN that is the working used on some MIPI documentation.

This comment applies to both the changes in the driver and the binding.

Thanks,
Francesco

Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium,pn-swap' property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

On Wed, Apr 03, 2024 at 09:16:31AM +0300, Alexandru Ardelean wrote:

> >
> > > +    type: boolean
> >
> > The type here should be flag.
> 
> ack; i'll change the type

I prob shoulda said, its "$ref: /schemas/types.yaml#/definitions/flag"
instead of "type: boolean".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium, pn-swap' property

[Dmitry Baryshkov]

On Tue, Apr 02, 2024 at 01:59:25PM +0300, Alexandru Ardelean wrote:
> On some HW designs, it's easier for the layout if the P/N pins are swapped.
> The driver currently has a DT property to do that.
> 
> This change documents the 'lontium,pn-swap' property.
> 
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
>  .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> index 2cef252157985..3a804926b288a 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> @@ -24,6 +24,12 @@ properties:
>      maxItems: 1
>      description: GPIO connected to active high RESET pin.
>  
> +  lontium,pn-swap:
> +    description: Swap the polarities of the P/N pins in software.
> +      On some HW designs, the layout is simplified if the P/N pins
> +      are inverted.
> +    type: boolean
> +

I'd like to point out the standard `lane-polarities` property defined at
Documentation/devicetree/bindings/media/video-interfaces.yaml. You can
define and use it for the corresponding endpoint in the lt8912b schema.

>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  
> -- 
> 2.44.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] dt-bindings: display: bridge: lt8912b: document 'lontium, pn-swap' property

[Alexandru Ardelean]

On Sun, Apr 7, 2024 at 11:31 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, Apr 02, 2024 at 01:59:25PM +0300, Alexandru Ardelean wrote:
> > On some HW designs, it's easier for the layout if the P/N pins are swapped.
> > The driver currently has a DT property to do that.
> >
> > This change documents the 'lontium,pn-swap' property.
> >
> > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> > ---
> >  .../devicetree/bindings/display/bridge/lontium,lt8912b.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > index 2cef252157985..3a804926b288a 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/lontium,lt8912b.yaml
> > @@ -24,6 +24,12 @@ properties:
> >      maxItems: 1
> >      description: GPIO connected to active high RESET pin.
> >
> > +  lontium,pn-swap:
> > +    description: Swap the polarities of the P/N pins in software.
> > +      On some HW designs, the layout is simplified if the P/N pins
> > +      are inverted.
> > +    type: boolean
> > +
>
> I'd like to point out the standard `lane-polarities` property defined at
> Documentation/devicetree/bindings/media/video-interfaces.yaml. You can
> define and use it for the corresponding endpoint in the lt8912b schema.
>

Ohhh.
Interesting :)
Many thanks for pointing this out.
This will make things much easier.
Will do a V2 with this in a week or two.
I'm traveling now.

Thanks
Alex



> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> >
> > --
> > 2.44.0
> >
>
> --
> With best wishes
> Dmitry