| Message ID | 20240402105925.905144-1-alex@shruggie.ro (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/bridge: lt8912b: add support for P/N pin swap | expand |
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
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 >
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
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__);
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(-)