| Message ID | 20241210091901.83028-3-andrej.picej@norik.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SN65DSI83/4 lvds_vod_swing properties | expand |
On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: > Add a optional properties to change LVDS output voltage. This should not > be static as this depends mainly on the connected display voltage > requirement. We have three properties: > - "ti,lvds-termination-ohms", which sets near end termination, > - "ti,lvds-vod-swing-data-microvolt" and > - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential > output voltage for data and clock lanes. They are defined as an array > with min and max values. The appropriate bitfield will be set if > selected constraints can be met. > > If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near > end termination will be used. Selecting only one: > "ti,lvds-vod-swing-data-microvolt" or > "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage > constraint for only data/clock lanes will be met. Setting both is > recommended. > > Signed-off-by: Andrej Picej <andrej.picej@norik.com> > --- > Changes in v5: > - specify default values in sn65dsi83_parse_lvds_endpoint, > - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, > Changes in v4: > - fix typo in commit message bitfiled -> bitfield > - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead > of separate variables for channel A/B > - add more checks on return value of "of_property_read_u32_array" > Changes in v3: > - use microvolts for default array values 1000 mV -> 1000000 uV. > Changes in v2: > - use datasheet tables to get the proper configuration > - since major change was done change the authorship to myself > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- > 1 file changed, 139 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 57a7ed13f996..f9578b38da28 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -132,6 +132,16 @@ > #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) > #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) > > +enum sn65dsi83_channel { > + CHANNEL_A, > + CHANNEL_B > +}; > + > +enum sn65dsi83_lvds_term { > + OHM_100, > + OHM_200 > +}; > + > enum sn65dsi83_model { > MODEL_SN65DSI83, > MODEL_SN65DSI84, > @@ -147,6 +157,8 @@ struct sn65dsi83 { > struct regulator *vcc; > bool lvds_dual_link; > bool lvds_dual_link_even_odd_swap; > + int lvds_vod_swing_conf[2]; > + int lvds_term_conf[2]; > }; > > static const struct regmap_range sn65dsi83_readable_ranges[] = { > @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { > .max_register = REG_IRQ_STAT, > }; > > +static const int lvds_vod_swing_data_table[2][4][2] = { > + { /* 100 Ohm */ > + { 180000, 313000 }, > + { 215000, 372000 }, > + { 250000, 430000 }, > + { 290000, 488000 }, > + }, > + { /* 200 Ohm */ > + { 150000, 261000 }, > + { 200000, 346000 }, > + { 250000, 428000 }, > + { 300000, 511000 }, > + }, > +}; > + > +static const int lvds_vod_swing_clock_table[2][4][2] = { > + { /* 100 Ohm */ > + { 140000, 244000 }, > + { 168000, 290000 }, > + { 195000, 335000 }, > + { 226000, 381000 }, > + }, > + { /* 200 Ohm */ > + { 117000, 204000 }, > + { 156000, 270000 }, > + { 195000, 334000 }, > + { 234000, 399000 }, > + }, > +}; > + > static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) > { > return container_of(bridge, struct sn65dsi83, bridge); > @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > val |= REG_LVDS_FMT_LVDS_LINK_CFG; > > regmap_write(ctx->regmap, REG_LVDS_FMT, val); > - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); > + regmap_write(ctx->regmap, REG_LVDS_VCOM, > + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | > + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); > regmap_write(ctx->regmap, REG_LVDS_LANE, > (ctx->lvds_dual_link_even_odd_swap ? > REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | > - REG_LVDS_LANE_CHA_LVDS_TERM | > - REG_LVDS_LANE_CHB_LVDS_TERM); > + (ctx->lvds_term_conf[CHANNEL_A] ? > + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | > + (ctx->lvds_term_conf[CHANNEL_B] ? > + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); > regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); > > le16val = cpu_to_le16(mode->hdisplay); > @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { > .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, > }; > > +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, > + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) > +{ > + int i; > + > + for (i = 0; i <= 3; i++) { > + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && > + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && > + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && > + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) > + return i; > + } > + > + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); > + return -EINVAL; > +} > + > +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) > +{ > + struct device *dev = ctx->dev; > + struct device_node *endpoint; > + int endpoint_reg; > + /* Set so the property can be freely selected if not defined */ > + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; > + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; > + u32 lvds_term; > + u8 lvds_term_conf = 0x1; > + int lvds_vod_swing_conf = 0x1; Magic values > + int ret = 0; > + int ret_data; > + int ret_clock; > + > + if (channel == CHANNEL_A) > + endpoint_reg = 2; > + else > + endpoint_reg = 3; > + > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1); > + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) { The code has been better before: provide default for lvds_term, read the property (keeping the default in case of an error), then use the lvds_term to set up lvds_term_conf, as expected. > + if (lvds_term == 100) > + lvds_term_conf = OHM_100; > + else > + lvds_term_conf = OHM_200; > + } > + > + ctx->lvds_term_conf[channel] = lvds_term_conf; > + > + ret_data = of_property_read_u32_array(endpoint, > + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data, > + ARRAY_SIZE(lvds_vod_swing_data)); > + if (ret_data != 0 && ret_data != -EINVAL) { > + ret = ret_data; > + goto exit; > + } > + > + ret_clock = of_property_read_u32_array(endpoint, > + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk, > + ARRAY_SIZE(lvds_vod_swing_clk)); > + if (ret_clock != 0 && ret_clock != -EINVAL) { > + ret = ret_clock; > + goto exit; > + } > + > + /* If any of the two properties is defined. */ > + if (!ret_data || !ret_clock) { > + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, > + lvds_vod_swing_data, lvds_vod_swing_clk, > + lvds_term_conf); > + if (lvds_vod_swing_conf < 0) { > + ret = lvds_vod_swing_conf; > + goto exit; > + } > + } > + > + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; > + ret = 0; > +exit: > + of_node_put(endpoint); > + return ret; > +} > + > static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > { > struct drm_bridge *panel_bridge; > struct device *dev = ctx->dev; > + int ret; > + > + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A); > + if (ret < 0) > + return ret; > + > + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B); > + if (ret < 0) > + return ret; > > ctx->lvds_dual_link = false; > ctx->lvds_dual_link_even_odd_swap = false; > -- > 2.34.1 >
On 10. 12. 24 12:43, Dmitry Baryshkov wrote: > On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: >> Add a optional properties to change LVDS output voltage. This should not >> be static as this depends mainly on the connected display voltage >> requirement. We have three properties: >> - "ti,lvds-termination-ohms", which sets near end termination, >> - "ti,lvds-vod-swing-data-microvolt" and >> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential >> output voltage for data and clock lanes. They are defined as an array >> with min and max values. The appropriate bitfield will be set if >> selected constraints can be met. >> >> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near >> end termination will be used. Selecting only one: >> "ti,lvds-vod-swing-data-microvolt" or >> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage >> constraint for only data/clock lanes will be met. Setting both is >> recommended. >> >> Signed-off-by: Andrej Picej <andrej.picej@norik.com> >> --- >> Changes in v5: >> - specify default values in sn65dsi83_parse_lvds_endpoint, >> - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, >> Changes in v4: >> - fix typo in commit message bitfiled -> bitfield >> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead >> of separate variables for channel A/B >> - add more checks on return value of "of_property_read_u32_array" >> Changes in v3: >> - use microvolts for default array values 1000 mV -> 1000000 uV. >> Changes in v2: >> - use datasheet tables to get the proper configuration >> - since major change was done change the authorship to myself >> --- >> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- >> 1 file changed, 139 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >> index 57a7ed13f996..f9578b38da28 100644 >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >> @@ -132,6 +132,16 @@ >> #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) >> #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) >> >> +enum sn65dsi83_channel { >> + CHANNEL_A, >> + CHANNEL_B >> +}; >> + >> +enum sn65dsi83_lvds_term { >> + OHM_100, >> + OHM_200 >> +}; >> + >> enum sn65dsi83_model { >> MODEL_SN65DSI83, >> MODEL_SN65DSI84, >> @@ -147,6 +157,8 @@ struct sn65dsi83 { >> struct regulator *vcc; >> bool lvds_dual_link; >> bool lvds_dual_link_even_odd_swap; >> + int lvds_vod_swing_conf[2]; >> + int lvds_term_conf[2]; >> }; >> >> static const struct regmap_range sn65dsi83_readable_ranges[] = { >> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { >> .max_register = REG_IRQ_STAT, >> }; >> >> +static const int lvds_vod_swing_data_table[2][4][2] = { >> + { /* 100 Ohm */ >> + { 180000, 313000 }, >> + { 215000, 372000 }, >> + { 250000, 430000 }, >> + { 290000, 488000 }, >> + }, >> + { /* 200 Ohm */ >> + { 150000, 261000 }, >> + { 200000, 346000 }, >> + { 250000, 428000 }, >> + { 300000, 511000 }, >> + }, >> +}; >> + >> +static const int lvds_vod_swing_clock_table[2][4][2] = { >> + { /* 100 Ohm */ >> + { 140000, 244000 }, >> + { 168000, 290000 }, >> + { 195000, 335000 }, >> + { 226000, 381000 }, >> + }, >> + { /* 200 Ohm */ >> + { 117000, 204000 }, >> + { 156000, 270000 }, >> + { 195000, 334000 }, >> + { 234000, 399000 }, >> + }, >> +}; >> + >> static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) >> { >> return container_of(bridge, struct sn65dsi83, bridge); >> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, >> val |= REG_LVDS_FMT_LVDS_LINK_CFG; >> >> regmap_write(ctx->regmap, REG_LVDS_FMT, val); >> - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); >> + regmap_write(ctx->regmap, REG_LVDS_VCOM, >> + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | >> + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); >> regmap_write(ctx->regmap, REG_LVDS_LANE, >> (ctx->lvds_dual_link_even_odd_swap ? >> REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | >> - REG_LVDS_LANE_CHA_LVDS_TERM | >> - REG_LVDS_LANE_CHB_LVDS_TERM); >> + (ctx->lvds_term_conf[CHANNEL_A] ? >> + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | >> + (ctx->lvds_term_conf[CHANNEL_B] ? >> + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); >> regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); >> >> le16val = cpu_to_le16(mode->hdisplay); >> @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { >> .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, >> }; >> >> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, >> + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) >> +{ >> + int i; >> + >> + for (i = 0; i <= 3; i++) { >> + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && >> + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && >> + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && >> + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) >> + return i; >> + } >> + >> + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); >> + return -EINVAL; >> +} >> + >> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) >> +{ >> + struct device *dev = ctx->dev; >> + struct device_node *endpoint; >> + int endpoint_reg; >> + /* Set so the property can be freely selected if not defined */ >> + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; >> + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; >> + u32 lvds_term; >> + u8 lvds_term_conf = 0x1; >> + int lvds_vod_swing_conf = 0x1; > > Magic values Can you please elaborate. I can use: u8 lvds_term_conf = OHM_200; What about lvds_vod_swing_conf? Should I create additional define for it? But this doesn't solve a hidden meaning? Maybe additional comment above? Would like to avoid using voltages for it, since then we are reverse engineering the table in datasheet to match the default reg value. > >> + int ret = 0; >> + int ret_data; >> + int ret_clock; >> + >> + if (channel == CHANNEL_A) >> + endpoint_reg = 2; >> + else >> + endpoint_reg = 3; >> + >> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1); >> + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) { > > The code has been better before: > provide default for lvds_term, read the property (keeping the default in > case of an error), then use the lvds_term to set up lvds_term_conf, as > expected. Ok, will revert back. > >> + if (lvds_term == 100) >> + lvds_term_conf = OHM_100; >> + else >> + lvds_term_conf = OHM_200; >> + } >> + >> + ctx->lvds_term_conf[channel] = lvds_term_conf; >> + >> + ret_data = of_property_read_u32_array(endpoint, >> + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data, >> + ARRAY_SIZE(lvds_vod_swing_data)); >> + if (ret_data != 0 && ret_data != -EINVAL) { >> + ret = ret_data; >> + goto exit; >> + } >> + >> + ret_clock = of_property_read_u32_array(endpoint, >> + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk, >> + ARRAY_SIZE(lvds_vod_swing_clk)); >> + if (ret_clock != 0 && ret_clock != -EINVAL) { >> + ret = ret_clock; >> + goto exit; >> + } >> + >> + /* If any of the two properties is defined. */ >> + if (!ret_data || !ret_clock) { >> + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, >> + lvds_vod_swing_data, lvds_vod_swing_clk, >> + lvds_term_conf); >> + if (lvds_vod_swing_conf < 0) { >> + ret = lvds_vod_swing_conf; >> + goto exit; >> + } >> + } >> + >> + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; >> + ret = 0; >> +exit: >> + of_node_put(endpoint); >> + return ret; >> +} >> + >> static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) >> { >> struct drm_bridge *panel_bridge; >> struct device *dev = ctx->dev; >> + int ret; >> + >> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A); >> + if (ret < 0) >> + return ret; >> + >> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B); >> + if (ret < 0) >> + return ret; >> >> ctx->lvds_dual_link = false; >> ctx->lvds_dual_link_even_odd_swap = false; >> -- >> 2.34.1 >> >
On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: > > > On 10. 12. 24 12:43, Dmitry Baryshkov wrote: > > On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: > > > Add a optional properties to change LVDS output voltage. This should not > > > be static as this depends mainly on the connected display voltage > > > requirement. We have three properties: > > > - "ti,lvds-termination-ohms", which sets near end termination, > > > - "ti,lvds-vod-swing-data-microvolt" and > > > - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential > > > output voltage for data and clock lanes. They are defined as an array > > > with min and max values. The appropriate bitfield will be set if > > > selected constraints can be met. > > > > > > If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near > > > end termination will be used. Selecting only one: > > > "ti,lvds-vod-swing-data-microvolt" or > > > "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage > > > constraint for only data/clock lanes will be met. Setting both is > > > recommended. > > > > > > Signed-off-by: Andrej Picej <andrej.picej@norik.com> > > > --- > > > Changes in v5: > > > - specify default values in sn65dsi83_parse_lvds_endpoint, > > > - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, > > > Changes in v4: > > > - fix typo in commit message bitfiled -> bitfield > > > - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead > > > of separate variables for channel A/B > > > - add more checks on return value of "of_property_read_u32_array" > > > Changes in v3: > > > - use microvolts for default array values 1000 mV -> 1000000 uV. > > > Changes in v2: > > > - use datasheet tables to get the proper configuration > > > - since major change was done change the authorship to myself > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- > > > 1 file changed, 139 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > index 57a7ed13f996..f9578b38da28 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > @@ -132,6 +132,16 @@ > > > #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) > > > #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) > > > +enum sn65dsi83_channel { > > > + CHANNEL_A, > > > + CHANNEL_B > > > +}; > > > + > > > +enum sn65dsi83_lvds_term { > > > + OHM_100, > > > + OHM_200 > > > +}; > > > + > > > enum sn65dsi83_model { > > > MODEL_SN65DSI83, > > > MODEL_SN65DSI84, > > > @@ -147,6 +157,8 @@ struct sn65dsi83 { > > > struct regulator *vcc; > > > bool lvds_dual_link; > > > bool lvds_dual_link_even_odd_swap; > > > + int lvds_vod_swing_conf[2]; > > > + int lvds_term_conf[2]; > > > }; > > > static const struct regmap_range sn65dsi83_readable_ranges[] = { > > > @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { > > > .max_register = REG_IRQ_STAT, > > > }; > > > +static const int lvds_vod_swing_data_table[2][4][2] = { > > > + { /* 100 Ohm */ > > > + { 180000, 313000 }, > > > + { 215000, 372000 }, > > > + { 250000, 430000 }, > > > + { 290000, 488000 }, > > > + }, > > > + { /* 200 Ohm */ > > > + { 150000, 261000 }, > > > + { 200000, 346000 }, > > > + { 250000, 428000 }, > > > + { 300000, 511000 }, > > > + }, > > > +}; > > > + > > > +static const int lvds_vod_swing_clock_table[2][4][2] = { > > > + { /* 100 Ohm */ > > > + { 140000, 244000 }, > > > + { 168000, 290000 }, > > > + { 195000, 335000 }, > > > + { 226000, 381000 }, > > > + }, > > > + { /* 200 Ohm */ > > > + { 117000, 204000 }, > > > + { 156000, 270000 }, > > > + { 195000, 334000 }, > > > + { 234000, 399000 }, > > > + }, > > > +}; > > > + > > > static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) > > > { > > > return container_of(bridge, struct sn65dsi83, bridge); > > > @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > > val |= REG_LVDS_FMT_LVDS_LINK_CFG; > > > regmap_write(ctx->regmap, REG_LVDS_FMT, val); > > > - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); > > > + regmap_write(ctx->regmap, REG_LVDS_VCOM, > > > + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | > > > + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); > > > regmap_write(ctx->regmap, REG_LVDS_LANE, > > > (ctx->lvds_dual_link_even_odd_swap ? > > > REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | > > > - REG_LVDS_LANE_CHA_LVDS_TERM | > > > - REG_LVDS_LANE_CHB_LVDS_TERM); > > > + (ctx->lvds_term_conf[CHANNEL_A] ? > > > + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | > > > + (ctx->lvds_term_conf[CHANNEL_B] ? > > > + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); > > > regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); > > > le16val = cpu_to_le16(mode->hdisplay); > > > @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { > > > .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, > > > }; > > > +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, > > > + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i <= 3; i++) { > > > + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && > > > + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && > > > + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && > > > + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) > > > + return i; > > > + } > > > + > > > + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); > > > + return -EINVAL; > > > +} > > > + > > > +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) > > > +{ > > > + struct device *dev = ctx->dev; > > > + struct device_node *endpoint; > > > + int endpoint_reg; > > > + /* Set so the property can be freely selected if not defined */ > > > + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; > > > + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; > > > + u32 lvds_term; > > > + u8 lvds_term_conf = 0x1; > > > + int lvds_vod_swing_conf = 0x1; > > > > Magic values > > Can you please elaborate. > > I can use: > u8 lvds_term_conf = OHM_200; > > What about lvds_vod_swing_conf? Should I create additional define for it? > But this doesn't solve a hidden meaning? Maybe additional comment above? > Would like to avoid using voltages for it, since then we are reverse > engineering the table in datasheet to match the default reg value. I think the following example solves both problems: lvds_term = 200; of_property_read_u32(..., &lvds_term); if (lvds_term == 100) ctx->lvds_term_conf[channel] = OHM_100; else if (lvds_term == 200) ctx->lvds_term_conf[channel] = OHM_200; else return -EINVAL; The same approach can be applied to lvds_vod_swing_conf, resulting in removal of magic values. > > > > > > + int ret = 0; > > > + int ret_data; > > > + int ret_clock; > > > + > > > + if (channel == CHANNEL_A) > > > + endpoint_reg = 2; > > > + else > > > + endpoint_reg = 3; > > > + > > > + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1); > > > + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) { > > > > The code has been better before: > > provide default for lvds_term, read the property (keeping the default in > > case of an error), then use the lvds_term to set up lvds_term_conf, as > > expected. > > Ok, will revert back. > > > > > > + if (lvds_term == 100) > > > + lvds_term_conf = OHM_100; > > > + else > > > + lvds_term_conf = OHM_200; > > > + } > > > + > > > + ctx->lvds_term_conf[channel] = lvds_term_conf; > > > + > > > + ret_data = of_property_read_u32_array(endpoint, > > > + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data, > > > + ARRAY_SIZE(lvds_vod_swing_data)); > > > + if (ret_data != 0 && ret_data != -EINVAL) { > > > + ret = ret_data; > > > + goto exit; > > > + } > > > + > > > + ret_clock = of_property_read_u32_array(endpoint, > > > + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk, > > > + ARRAY_SIZE(lvds_vod_swing_clk)); > > > + if (ret_clock != 0 && ret_clock != -EINVAL) { > > > + ret = ret_clock; > > > + goto exit; > > > + } > > > + > > > + /* If any of the two properties is defined. */ > > > + if (!ret_data || !ret_clock) { > > > + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, > > > + lvds_vod_swing_data, lvds_vod_swing_clk, > > > + lvds_term_conf); > > > + if (lvds_vod_swing_conf < 0) { > > > + ret = lvds_vod_swing_conf; > > > + goto exit; > > > + } > > > + } > > > + > > > + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; > > > + ret = 0; > > > +exit: > > > + of_node_put(endpoint); > > > + return ret; > > > +} > > > + > > > static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > > { > > > struct drm_bridge *panel_bridge; > > > struct device *dev = ctx->dev; > > > + int ret; > > > + > > > + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A); > > > + if (ret < 0) > > > + return ret; > > > + > > > + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B); > > > + if (ret < 0) > > > + return ret; > > > ctx->lvds_dual_link = false; > > > ctx->lvds_dual_link_even_odd_swap = false; > > > -- > > > 2.34.1 > > > > >
On 10. 12. 24 14:59, Dmitry Baryshkov wrote: > On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: >> >> >> On 10. 12. 24 12:43, Dmitry Baryshkov wrote: >>> On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: >>>> Add a optional properties to change LVDS output voltage. This should not >>>> be static as this depends mainly on the connected display voltage >>>> requirement. We have three properties: >>>> - "ti,lvds-termination-ohms", which sets near end termination, >>>> - "ti,lvds-vod-swing-data-microvolt" and >>>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential >>>> output voltage for data and clock lanes. They are defined as an array >>>> with min and max values. The appropriate bitfield will be set if >>>> selected constraints can be met. >>>> >>>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near >>>> end termination will be used. Selecting only one: >>>> "ti,lvds-vod-swing-data-microvolt" or >>>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage >>>> constraint for only data/clock lanes will be met. Setting both is >>>> recommended. >>>> >>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com> >>>> --- >>>> Changes in v5: >>>> - specify default values in sn65dsi83_parse_lvds_endpoint, >>>> - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, >>>> Changes in v4: >>>> - fix typo in commit message bitfiled -> bitfield >>>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead >>>> of separate variables for channel A/B >>>> - add more checks on return value of "of_property_read_u32_array" >>>> Changes in v3: >>>> - use microvolts for default array values 1000 mV -> 1000000 uV. >>>> Changes in v2: >>>> - use datasheet tables to get the proper configuration >>>> - since major change was done change the authorship to myself >>>> --- >>>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- >>>> 1 file changed, 139 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>> index 57a7ed13f996..f9578b38da28 100644 >>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>> @@ -132,6 +132,16 @@ >>>> #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) >>>> #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) >>>> +enum sn65dsi83_channel { >>>> + CHANNEL_A, >>>> + CHANNEL_B >>>> +}; >>>> + >>>> +enum sn65dsi83_lvds_term { >>>> + OHM_100, >>>> + OHM_200 >>>> +}; >>>> + >>>> enum sn65dsi83_model { >>>> MODEL_SN65DSI83, >>>> MODEL_SN65DSI84, >>>> @@ -147,6 +157,8 @@ struct sn65dsi83 { >>>> struct regulator *vcc; >>>> bool lvds_dual_link; >>>> bool lvds_dual_link_even_odd_swap; >>>> + int lvds_vod_swing_conf[2]; >>>> + int lvds_term_conf[2]; >>>> }; >>>> static const struct regmap_range sn65dsi83_readable_ranges[] = { >>>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { >>>> .max_register = REG_IRQ_STAT, >>>> }; >>>> +static const int lvds_vod_swing_data_table[2][4][2] = { >>>> + { /* 100 Ohm */ >>>> + { 180000, 313000 }, >>>> + { 215000, 372000 }, >>>> + { 250000, 430000 }, >>>> + { 290000, 488000 }, >>>> + }, >>>> + { /* 200 Ohm */ >>>> + { 150000, 261000 }, >>>> + { 200000, 346000 }, >>>> + { 250000, 428000 }, >>>> + { 300000, 511000 }, >>>> + }, >>>> +}; >>>> + >>>> +static const int lvds_vod_swing_clock_table[2][4][2] = { >>>> + { /* 100 Ohm */ >>>> + { 140000, 244000 }, >>>> + { 168000, 290000 }, >>>> + { 195000, 335000 }, >>>> + { 226000, 381000 }, >>>> + }, >>>> + { /* 200 Ohm */ >>>> + { 117000, 204000 }, >>>> + { 156000, 270000 }, >>>> + { 195000, 334000 }, >>>> + { 234000, 399000 }, >>>> + }, >>>> +}; >>>> + >>>> static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) >>>> { >>>> return container_of(bridge, struct sn65dsi83, bridge); >>>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, >>>> val |= REG_LVDS_FMT_LVDS_LINK_CFG; >>>> regmap_write(ctx->regmap, REG_LVDS_FMT, val); >>>> - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); >>>> + regmap_write(ctx->regmap, REG_LVDS_VCOM, >>>> + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | >>>> + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); >>>> regmap_write(ctx->regmap, REG_LVDS_LANE, >>>> (ctx->lvds_dual_link_even_odd_swap ? >>>> REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | >>>> - REG_LVDS_LANE_CHA_LVDS_TERM | >>>> - REG_LVDS_LANE_CHB_LVDS_TERM); >>>> + (ctx->lvds_term_conf[CHANNEL_A] ? >>>> + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | >>>> + (ctx->lvds_term_conf[CHANNEL_B] ? >>>> + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); >>>> regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); >>>> le16val = cpu_to_le16(mode->hdisplay); >>>> @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { >>>> .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, >>>> }; >>>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, >>>> + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i <= 3; i++) { >>>> + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && >>>> + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && >>>> + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && >>>> + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) >>>> + return i; >>>> + } >>>> + >>>> + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); >>>> + return -EINVAL; >>>> +} >>>> + >>>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) >>>> +{ >>>> + struct device *dev = ctx->dev; >>>> + struct device_node *endpoint; >>>> + int endpoint_reg; >>>> + /* Set so the property can be freely selected if not defined */ >>>> + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; >>>> + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; >>>> + u32 lvds_term; >>>> + u8 lvds_term_conf = 0x1; >>>> + int lvds_vod_swing_conf = 0x1; >>> >>> Magic values >> >> Can you please elaborate. >> >> I can use: >> u8 lvds_term_conf = OHM_200; >> >> What about lvds_vod_swing_conf? Should I create additional define for it? >> But this doesn't solve a hidden meaning? Maybe additional comment above? >> Would like to avoid using voltages for it, since then we are reverse >> engineering the table in datasheet to match the default reg value. > > I think the following example solves both problems: > > lvds_term = 200; > of_property_read_u32(..., &lvds_term); > > if (lvds_term == 100) > ctx->lvds_term_conf[channel] = OHM_100; > else if (lvds_term == 200) > ctx->lvds_term_conf[channel] = OHM_200; > else > return -EINVAL; > > The same approach can be applied to lvds_vod_swing_conf, resulting in > removal of magic values. Sorry, but I think it is not that easy when it comes to the lvds_vod_swing_conf. We should assign default value if "ti,lvds-vod-swing-data-microvolt" and "ti,lvds-vod-swing-clock-microvolt" are not defined. Default value of the lvds_vod_swing_conf is 0x1, but this doesn't have any straight forward meaning like OHM_200 for example. What we can do in that case is that we copy the values from defined datasheet tables to the "lvds_vod_swing_data[2]" and "lvds_vod_swing_clk[2]" arrays and then run the sn65dsi83_select_lvds_vod_swing with it, which will return the default value (0x1). /* If both properties are not defined assign default limits */ if (ret_data && ret_clock) { memcpy(lvds_vod_swing_data, lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1], sizeof(lvds_vod_swing_data)); memcpy(lvds_vod_swing_clk, lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1], sizeof(lvds_vod_swing_clk)); } lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, lvds_vod_swing_data, lvds_vod_swing_clk, ctx->lvds_term_conf[channel]); if (lvds_vod_swing_conf < 0) { ret = lvds_vod_swing_conf; goto exit; } ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; I'm not sure if using this approach gets rid of the problem with magic values. Or maybe I'm not seeing the obvious solution so please bear with me. > >> >>> >>>> + int ret = 0; >>>> + int ret_data; >>>> + int ret_clock; >>>> + >>>> + if (channel == CHANNEL_A) >>>> + endpoint_reg = 2; >>>> + else >>>> + endpoint_reg = 3; >>>> + >>>> + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1); >>>> + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) { >>> >>> The code has been better before: >>> provide default for lvds_term, read the property (keeping the default in >>> case of an error), then use the lvds_term to set up lvds_term_conf, as >>> expected. >> >> Ok, will revert back. >> >>> >>>> + if (lvds_term == 100) >>>> + lvds_term_conf = OHM_100; >>>> + else >>>> + lvds_term_conf = OHM_200; >>>> + } >>>> + >>>> + ctx->lvds_term_conf[channel] = lvds_term_conf; >>>> + >>>> + ret_data = of_property_read_u32_array(endpoint, >>>> + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data, >>>> + ARRAY_SIZE(lvds_vod_swing_data)); >>>> + if (ret_data != 0 && ret_data != -EINVAL) { >>>> + ret = ret_data; >>>> + goto exit; >>>> + } >>>> + >>>> + ret_clock = of_property_read_u32_array(endpoint, >>>> + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk, >>>> + ARRAY_SIZE(lvds_vod_swing_clk)); >>>> + if (ret_clock != 0 && ret_clock != -EINVAL) { >>>> + ret = ret_clock; >>>> + goto exit; >>>> + } >>>> + >>>> + /* If any of the two properties is defined. */ >>>> + if (!ret_data || !ret_clock) { >>>> + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, >>>> + lvds_vod_swing_data, lvds_vod_swing_clk, >>>> + lvds_term_conf); >>>> + if (lvds_vod_swing_conf < 0) { >>>> + ret = lvds_vod_swing_conf; >>>> + goto exit; >>>> + } >>>> + } >>>> + >>>> + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; >>>> + ret = 0; >>>> +exit: >>>> + of_node_put(endpoint); >>>> + return ret; >>>> +} >>>> + >>>> static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) >>>> { >>>> struct drm_bridge *panel_bridge; >>>> struct device *dev = ctx->dev; >>>> + int ret; >>>> + >>>> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B); >>>> + if (ret < 0) >>>> + return ret; >>>> ctx->lvds_dual_link = false; >>>> ctx->lvds_dual_link_even_odd_swap = false; >>>> -- >>>> 2.34.1 >>>> >>> >
On Wed, Dec 11, 2024 at 08:57:17AM +0100, Andrej Picej wrote: > > > On 10. 12. 24 14:59, Dmitry Baryshkov wrote: > > On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: > > > > > > > > > On 10. 12. 24 12:43, Dmitry Baryshkov wrote: > > > > On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: > > > > > Add a optional properties to change LVDS output voltage. This should not > > > > > be static as this depends mainly on the connected display voltage > > > > > requirement. We have three properties: > > > > > - "ti,lvds-termination-ohms", which sets near end termination, > > > > > - "ti,lvds-vod-swing-data-microvolt" and > > > > > - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential > > > > > output voltage for data and clock lanes. They are defined as an array > > > > > with min and max values. The appropriate bitfield will be set if > > > > > selected constraints can be met. > > > > > > > > > > If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near > > > > > end termination will be used. Selecting only one: > > > > > "ti,lvds-vod-swing-data-microvolt" or > > > > > "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage > > > > > constraint for only data/clock lanes will be met. Setting both is > > > > > recommended. > > > > > > > > > > Signed-off-by: Andrej Picej <andrej.picej@norik.com> > > > > > --- > > > > > Changes in v5: > > > > > - specify default values in sn65dsi83_parse_lvds_endpoint, > > > > > - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, > > > > > Changes in v4: > > > > > - fix typo in commit message bitfiled -> bitfield > > > > > - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead > > > > > of separate variables for channel A/B > > > > > - add more checks on return value of "of_property_read_u32_array" > > > > > Changes in v3: > > > > > - use microvolts for default array values 1000 mV -> 1000000 uV. > > > > > Changes in v2: > > > > > - use datasheet tables to get the proper configuration > > > > > - since major change was done change the authorship to myself > > > > > --- > > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- > > > > > 1 file changed, 139 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > index 57a7ed13f996..f9578b38da28 100644 > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > @@ -132,6 +132,16 @@ > > > > > #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) > > > > > #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) > > > > > +enum sn65dsi83_channel { > > > > > + CHANNEL_A, > > > > > + CHANNEL_B > > > > > +}; > > > > > + > > > > > +enum sn65dsi83_lvds_term { > > > > > + OHM_100, > > > > > + OHM_200 > > > > > +}; > > > > > + > > > > > enum sn65dsi83_model { > > > > > MODEL_SN65DSI83, > > > > > MODEL_SN65DSI84, > > > > > @@ -147,6 +157,8 @@ struct sn65dsi83 { > > > > > struct regulator *vcc; > > > > > bool lvds_dual_link; > > > > > bool lvds_dual_link_even_odd_swap; > > > > > + int lvds_vod_swing_conf[2]; > > > > > + int lvds_term_conf[2]; > > > > > }; > > > > > static const struct regmap_range sn65dsi83_readable_ranges[] = { > > > > > @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { > > > > > .max_register = REG_IRQ_STAT, > > > > > }; > > > > > +static const int lvds_vod_swing_data_table[2][4][2] = { > > > > > + { /* 100 Ohm */ > > > > > + { 180000, 313000 }, > > > > > + { 215000, 372000 }, > > > > > + { 250000, 430000 }, > > > > > + { 290000, 488000 }, > > > > > + }, > > > > > + { /* 200 Ohm */ > > > > > + { 150000, 261000 }, > > > > > + { 200000, 346000 }, > > > > > + { 250000, 428000 }, > > > > > + { 300000, 511000 }, > > > > > + }, > > > > > +}; > > > > > + > > > > > +static const int lvds_vod_swing_clock_table[2][4][2] = { > > > > > + { /* 100 Ohm */ > > > > > + { 140000, 244000 }, > > > > > + { 168000, 290000 }, > > > > > + { 195000, 335000 }, > > > > > + { 226000, 381000 }, > > > > > + }, > > > > > + { /* 200 Ohm */ > > > > > + { 117000, 204000 }, > > > > > + { 156000, 270000 }, > > > > > + { 195000, 334000 }, > > > > > + { 234000, 399000 }, > > > > > + }, > > > > > +}; > > > > > + > > > > > static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) > > > > > { > > > > > return container_of(bridge, struct sn65dsi83, bridge); > > > > > @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > > > > val |= REG_LVDS_FMT_LVDS_LINK_CFG; > > > > > regmap_write(ctx->regmap, REG_LVDS_FMT, val); > > > > > - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); > > > > > + regmap_write(ctx->regmap, REG_LVDS_VCOM, > > > > > + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | > > > > > + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); > > > > > regmap_write(ctx->regmap, REG_LVDS_LANE, > > > > > (ctx->lvds_dual_link_even_odd_swap ? > > > > > REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | > > > > > - REG_LVDS_LANE_CHA_LVDS_TERM | > > > > > - REG_LVDS_LANE_CHB_LVDS_TERM); > > > > > + (ctx->lvds_term_conf[CHANNEL_A] ? > > > > > + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | > > > > > + (ctx->lvds_term_conf[CHANNEL_B] ? > > > > > + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); > > > > > regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); > > > > > le16val = cpu_to_le16(mode->hdisplay); > > > > > @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { > > > > > .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, > > > > > }; > > > > > +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, > > > > > + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = 0; i <= 3; i++) { > > > > > + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && > > > > > + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && > > > > > + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && > > > > > + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) > > > > > + return i; > > > > > + } > > > > > + > > > > > + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); > > > > > + return -EINVAL; > > > > > +} > > > > > + > > > > > +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) > > > > > +{ > > > > > + struct device *dev = ctx->dev; > > > > > + struct device_node *endpoint; > > > > > + int endpoint_reg; > > > > > + /* Set so the property can be freely selected if not defined */ > > > > > + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; > > > > > + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; > > > > > + u32 lvds_term; > > > > > + u8 lvds_term_conf = 0x1; > > > > > + int lvds_vod_swing_conf = 0x1; > > > > > > > > Magic values > > > > > > Can you please elaborate. > > > > > > I can use: > > > u8 lvds_term_conf = OHM_200; > > > > > > What about lvds_vod_swing_conf? Should I create additional define for it? > > > But this doesn't solve a hidden meaning? Maybe additional comment above? > > > Would like to avoid using voltages for it, since then we are reverse > > > engineering the table in datasheet to match the default reg value. > > > > I think the following example solves both problems: > > > > lvds_term = 200; > > of_property_read_u32(..., &lvds_term); > > > > if (lvds_term == 100) > > ctx->lvds_term_conf[channel] = OHM_100; > > else if (lvds_term == 200) > > ctx->lvds_term_conf[channel] = OHM_200; > > else > > return -EINVAL; > > > > The same approach can be applied to lvds_vod_swing_conf, resulting in > > removal of magic values. > > Sorry, but I think it is not that easy when it comes to the > lvds_vod_swing_conf. We should assign default value if > "ti,lvds-vod-swing-data-microvolt" and "ti,lvds-vod-swing-clock-microvolt" > are not defined. Default value of the lvds_vod_swing_conf is 0x1, but this > doesn't have any straight forward meaning like OHM_200 for example. > > What we can do in that case is that we copy the values from defined > datasheet tables to the "lvds_vod_swing_data[2]" and "lvds_vod_swing_clk[2]" > arrays and then run the > sn65dsi83_select_lvds_vod_swing with it, which will return the default value > (0x1). > > /* If both properties are not defined assign default limits */ > if (ret_data && ret_clock) { > memcpy(lvds_vod_swing_data, > lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1], > sizeof(lvds_vod_swing_data)); > memcpy(lvds_vod_swing_clk, > lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1], > sizeof(lvds_vod_swing_clk)); > } > lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, > lvds_vod_swing_data, lvds_vod_swing_clk, > ctx->lvds_term_conf[channel]); > if (lvds_vod_swing_conf < 0) { > ret = lvds_vod_swing_conf; > goto exit; > } > > ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; > > I'm not sure if using this approach gets rid of the problem with magic > values. > Or maybe I'm not seeing the obvious solution so please bear with me. Yes, the defaults (0..1000000) should be fixed to result in the same value (0x01) as if the property wasn't specified at all. I think the following should work: /* artifical values to select the defaults in both cases */ u32 lvds_vod_swing_data[2] = { 190000, 330000 }; u32 lvds_vod_swing_clk[2] = { 150000, 250000 }; Yes, they are artificial, as stated in the comment. Yes, I think it's better than special-casing in the property handling.
On 12. 12. 24 00:04, Dmitry Baryshkov wrote: > On Wed, Dec 11, 2024 at 08:57:17AM +0100, Andrej Picej wrote: >> >> >> On 10. 12. 24 14:59, Dmitry Baryshkov wrote: >>> On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: >>>> >>>> >>>> On 10. 12. 24 12:43, Dmitry Baryshkov wrote: >>>>> On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: >>>>>> Add a optional properties to change LVDS output voltage. This should not >>>>>> be static as this depends mainly on the connected display voltage >>>>>> requirement. We have three properties: >>>>>> - "ti,lvds-termination-ohms", which sets near end termination, >>>>>> - "ti,lvds-vod-swing-data-microvolt" and >>>>>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential >>>>>> output voltage for data and clock lanes. They are defined as an array >>>>>> with min and max values. The appropriate bitfield will be set if >>>>>> selected constraints can be met. >>>>>> >>>>>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near >>>>>> end termination will be used. Selecting only one: >>>>>> "ti,lvds-vod-swing-data-microvolt" or >>>>>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage >>>>>> constraint for only data/clock lanes will be met. Setting both is >>>>>> recommended. >>>>>> >>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com> >>>>>> --- >>>>>> Changes in v5: >>>>>> - specify default values in sn65dsi83_parse_lvds_endpoint, >>>>>> - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, >>>>>> Changes in v4: >>>>>> - fix typo in commit message bitfiled -> bitfield >>>>>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead >>>>>> of separate variables for channel A/B >>>>>> - add more checks on return value of "of_property_read_u32_array" >>>>>> Changes in v3: >>>>>> - use microvolts for default array values 1000 mV -> 1000000 uV. >>>>>> Changes in v2: >>>>>> - use datasheet tables to get the proper configuration >>>>>> - since major change was done change the authorship to myself >>>>>> --- >>>>>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- >>>>>> 1 file changed, 139 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> index 57a7ed13f996..f9578b38da28 100644 >>>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>> @@ -132,6 +132,16 @@ >>>>>> #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) >>>>>> #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) >>>>>> +enum sn65dsi83_channel { >>>>>> + CHANNEL_A, >>>>>> + CHANNEL_B >>>>>> +}; >>>>>> + >>>>>> +enum sn65dsi83_lvds_term { >>>>>> + OHM_100, >>>>>> + OHM_200 >>>>>> +}; >>>>>> + >>>>>> enum sn65dsi83_model { >>>>>> MODEL_SN65DSI83, >>>>>> MODEL_SN65DSI84, >>>>>> @@ -147,6 +157,8 @@ struct sn65dsi83 { >>>>>> struct regulator *vcc; >>>>>> bool lvds_dual_link; >>>>>> bool lvds_dual_link_even_odd_swap; >>>>>> + int lvds_vod_swing_conf[2]; >>>>>> + int lvds_term_conf[2]; >>>>>> }; >>>>>> static const struct regmap_range sn65dsi83_readable_ranges[] = { >>>>>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { >>>>>> .max_register = REG_IRQ_STAT, >>>>>> }; >>>>>> +static const int lvds_vod_swing_data_table[2][4][2] = { >>>>>> + { /* 100 Ohm */ >>>>>> + { 180000, 313000 }, >>>>>> + { 215000, 372000 }, >>>>>> + { 250000, 430000 }, >>>>>> + { 290000, 488000 }, >>>>>> + }, >>>>>> + { /* 200 Ohm */ >>>>>> + { 150000, 261000 }, >>>>>> + { 200000, 346000 }, >>>>>> + { 250000, 428000 }, >>>>>> + { 300000, 511000 }, >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> +static const int lvds_vod_swing_clock_table[2][4][2] = { >>>>>> + { /* 100 Ohm */ >>>>>> + { 140000, 244000 }, >>>>>> + { 168000, 290000 }, >>>>>> + { 195000, 335000 }, >>>>>> + { 226000, 381000 }, >>>>>> + }, >>>>>> + { /* 200 Ohm */ >>>>>> + { 117000, 204000 }, >>>>>> + { 156000, 270000 }, >>>>>> + { 195000, 334000 }, >>>>>> + { 234000, 399000 }, >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) >>>>>> { >>>>>> return container_of(bridge, struct sn65dsi83, bridge); >>>>>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, >>>>>> val |= REG_LVDS_FMT_LVDS_LINK_CFG; >>>>>> regmap_write(ctx->regmap, REG_LVDS_FMT, val); >>>>>> - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); >>>>>> + regmap_write(ctx->regmap, REG_LVDS_VCOM, >>>>>> + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | >>>>>> + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); >>>>>> regmap_write(ctx->regmap, REG_LVDS_LANE, >>>>>> (ctx->lvds_dual_link_even_odd_swap ? >>>>>> REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | >>>>>> - REG_LVDS_LANE_CHA_LVDS_TERM | >>>>>> - REG_LVDS_LANE_CHB_LVDS_TERM); >>>>>> + (ctx->lvds_term_conf[CHANNEL_A] ? >>>>>> + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | >>>>>> + (ctx->lvds_term_conf[CHANNEL_B] ? >>>>>> + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); >>>>>> regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); >>>>>> le16val = cpu_to_le16(mode->hdisplay); >>>>>> @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { >>>>>> .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, >>>>>> }; >>>>>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, >>>>>> + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) >>>>>> +{ >>>>>> + int i; >>>>>> + >>>>>> + for (i = 0; i <= 3; i++) { >>>>>> + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && >>>>>> + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && >>>>>> + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && >>>>>> + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) >>>>>> + return i; >>>>>> + } >>>>>> + >>>>>> + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); >>>>>> + return -EINVAL; >>>>>> +} >>>>>> + >>>>>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) >>>>>> +{ >>>>>> + struct device *dev = ctx->dev; >>>>>> + struct device_node *endpoint; >>>>>> + int endpoint_reg; >>>>>> + /* Set so the property can be freely selected if not defined */ >>>>>> + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; >>>>>> + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; >>>>>> + u32 lvds_term; >>>>>> + u8 lvds_term_conf = 0x1; >>>>>> + int lvds_vod_swing_conf = 0x1; >>>>> >>>>> Magic values >>>> >>>> Can you please elaborate. >>>> >>>> I can use: >>>> u8 lvds_term_conf = OHM_200; >>>> >>>> What about lvds_vod_swing_conf? Should I create additional define for it? >>>> But this doesn't solve a hidden meaning? Maybe additional comment above? >>>> Would like to avoid using voltages for it, since then we are reverse >>>> engineering the table in datasheet to match the default reg value. >>> >>> I think the following example solves both problems: >>> >>> lvds_term = 200; >>> of_property_read_u32(..., &lvds_term); >>> >>> if (lvds_term == 100) >>> ctx->lvds_term_conf[channel] = OHM_100; >>> else if (lvds_term == 200) >>> ctx->lvds_term_conf[channel] = OHM_200; >>> else >>> return -EINVAL; >>> >>> The same approach can be applied to lvds_vod_swing_conf, resulting in >>> removal of magic values. >> >> Sorry, but I think it is not that easy when it comes to the >> lvds_vod_swing_conf. We should assign default value if >> "ti,lvds-vod-swing-data-microvolt" and "ti,lvds-vod-swing-clock-microvolt" >> are not defined. Default value of the lvds_vod_swing_conf is 0x1, but this >> doesn't have any straight forward meaning like OHM_200 for example. >> >> What we can do in that case is that we copy the values from defined >> datasheet tables to the "lvds_vod_swing_data[2]" and "lvds_vod_swing_clk[2]" >> arrays and then run the >> sn65dsi83_select_lvds_vod_swing with it, which will return the default value >> (0x1). >> >> /* If both properties are not defined assign default limits */ >> if (ret_data && ret_clock) { >> memcpy(lvds_vod_swing_data, >> lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1], >> sizeof(lvds_vod_swing_data)); >> memcpy(lvds_vod_swing_clk, >> lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1], >> sizeof(lvds_vod_swing_clk)); >> } >> lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, >> lvds_vod_swing_data, lvds_vod_swing_clk, >> ctx->lvds_term_conf[channel]); >> if (lvds_vod_swing_conf < 0) { >> ret = lvds_vod_swing_conf; >> goto exit; >> } >> >> ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; >> >> I'm not sure if using this approach gets rid of the problem with magic >> values. >> Or maybe I'm not seeing the obvious solution so please bear with me. > > Yes, the defaults (0..1000000) should be fixed to result in the same > value (0x01) as if the property wasn't specified at all The defaults (0..1000000) is selected because in case if only one property is defined in dts (ti,lvds-vod-swing-data-microvolt or ti,lvds-vod-swing-clock-microvolt) the other array values don't effect the decision which "lvds_vod_swing_conf" is selected. That's why we initialized the array to be out off bounds of the datasheet tables, all values in the table match the not defined property, so lvds_vod_swing_conf is selected purely on the basis of the defined property. Example: DTS ti,lvds-vod-swing-data-microvolt = <250000 428000>; //ti,lvds-vod-swing-clock-microvolt NOT DEFINED; After parsing the devicetree we will get: lvds_vod_swing_data = [ 250000, 428000 ] lvds_vod_swing_clk = [ 0, 1000000 ] In sn65dsi83_select_lvds_vod_swing lvds_vod_swing_clk[] values don't effect the decision making since lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1] is always true. > > I think the following should work: > > /* artifical values to select the defaults in both cases */ > u32 lvds_vod_swing_data[2] = { 190000, 330000 }; > u32 lvds_vod_swing_clk[2] = { 150000, 250000 }; This sets the default to 0x0. It should be: u32 lvds_vod_swing_data[2] = { 200000, 372000 }; u32 lvds_vod_swing_clk[2] = { 156000, 290000 }; This selects the default 0x1 in both cases, if termination is 100 or 200 Ohms. Nevertheless I think I got your point. But I would still like to give the user the freedom to only specify one property if maybe connected panel only has limits on data lanes/clock lane. So maybe set the arrays lvds_vod_swing_data/clk to [0, 1000000] if of_property_read_u32_array returns -EINVAL (property does not exist). What do you say? > > Yes, they are artificial, as stated in the comment. Yes, I think it's > better than special-casing in the property handling. >
On Thu, Dec 12, 2024 at 09:08:03AM +0100, Andrej Picej wrote: > > > On 12. 12. 24 00:04, Dmitry Baryshkov wrote: > > On Wed, Dec 11, 2024 at 08:57:17AM +0100, Andrej Picej wrote: > > > > > > > > > On 10. 12. 24 14:59, Dmitry Baryshkov wrote: > > > > On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: > > > > > > > > > > > > > > > On 10. 12. 24 12:43, Dmitry Baryshkov wrote: > > > > > > On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: > > > > > > > Add a optional properties to change LVDS output voltage. This should not > > > > > > > be static as this depends mainly on the connected display voltage > > > > > > > requirement. We have three properties: > > > > > > > - "ti,lvds-termination-ohms", which sets near end termination, > > > > > > > - "ti,lvds-vod-swing-data-microvolt" and > > > > > > > - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential > > > > > > > output voltage for data and clock lanes. They are defined as an array > > > > > > > with min and max values. The appropriate bitfield will be set if > > > > > > > selected constraints can be met. > > > > > > > > > > > > > > If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near > > > > > > > end termination will be used. Selecting only one: > > > > > > > "ti,lvds-vod-swing-data-microvolt" or > > > > > > > "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage > > > > > > > constraint for only data/clock lanes will be met. Setting both is > > > > > > > recommended. > > > > > > > > > > > > > > Signed-off-by: Andrej Picej <andrej.picej@norik.com> > > > > > > > --- > > > > > > > Changes in v5: > > > > > > > - specify default values in sn65dsi83_parse_lvds_endpoint, > > > > > > > - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, > > > > > > > Changes in v4: > > > > > > > - fix typo in commit message bitfiled -> bitfield > > > > > > > - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead > > > > > > > of separate variables for channel A/B > > > > > > > - add more checks on return value of "of_property_read_u32_array" > > > > > > > Changes in v3: > > > > > > > - use microvolts for default array values 1000 mV -> 1000000 uV. > > > > > > > Changes in v2: > > > > > > > - use datasheet tables to get the proper configuration > > > > > > > - since major change was done change the authorship to myself > > > > > > > --- > > > > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- > > > > > > > 1 file changed, 139 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > > > index 57a7ed13f996..f9578b38da28 100644 > > > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > > > @@ -132,6 +132,16 @@ > > > > > > > #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) > > > > > > > #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) > > > > > > > +enum sn65dsi83_channel { > > > > > > > + CHANNEL_A, > > > > > > > + CHANNEL_B > > > > > > > +}; > > > > > > > + > > > > > > > +enum sn65dsi83_lvds_term { > > > > > > > + OHM_100, > > > > > > > + OHM_200 > > > > > > > +}; > > > > > > > + > > > > > > > enum sn65dsi83_model { > > > > > > > MODEL_SN65DSI83, > > > > > > > MODEL_SN65DSI84, > > > > > > > @@ -147,6 +157,8 @@ struct sn65dsi83 { > > > > > > > struct regulator *vcc; > > > > > > > bool lvds_dual_link; > > > > > > > bool lvds_dual_link_even_odd_swap; > > > > > > > + int lvds_vod_swing_conf[2]; > > > > > > > + int lvds_term_conf[2]; > > > > > > > }; > > > > > > > static const struct regmap_range sn65dsi83_readable_ranges[] = { > > > > > > > @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { > > > > > > > .max_register = REG_IRQ_STAT, > > > > > > > }; > > > > > > > +static const int lvds_vod_swing_data_table[2][4][2] = { > > > > > > > + { /* 100 Ohm */ > > > > > > > + { 180000, 313000 }, > > > > > > > + { 215000, 372000 }, > > > > > > > + { 250000, 430000 }, > > > > > > > + { 290000, 488000 }, > > > > > > > + }, > > > > > > > + { /* 200 Ohm */ > > > > > > > + { 150000, 261000 }, > > > > > > > + { 200000, 346000 }, > > > > > > > + { 250000, 428000 }, > > > > > > > + { 300000, 511000 }, > > > > > > > + }, > > > > > > > +}; > > > > > > > + > > > > > > > +static const int lvds_vod_swing_clock_table[2][4][2] = { > > > > > > > + { /* 100 Ohm */ > > > > > > > + { 140000, 244000 }, > > > > > > > + { 168000, 290000 }, > > > > > > > + { 195000, 335000 }, > > > > > > > + { 226000, 381000 }, > > > > > > > + }, > > > > > > > + { /* 200 Ohm */ > > > > > > > + { 117000, 204000 }, > > > > > > > + { 156000, 270000 }, > > > > > > > + { 195000, 334000 }, > > > > > > > + { 234000, 399000 }, > > > > > > > + }, > > > > > > > +}; > > > > > > > + > > > > > > > static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) > > > > > > > { > > > > > > > return container_of(bridge, struct sn65dsi83, bridge); > > > > > > > @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > > > > > > val |= REG_LVDS_FMT_LVDS_LINK_CFG; > > > > > > > regmap_write(ctx->regmap, REG_LVDS_FMT, val); > > > > > > > - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); > > > > > > > + regmap_write(ctx->regmap, REG_LVDS_VCOM, > > > > > > > + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | > > > > > > > + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); > > > > > > > regmap_write(ctx->regmap, REG_LVDS_LANE, > > > > > > > (ctx->lvds_dual_link_even_odd_swap ? > > > > > > > REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | > > > > > > > - REG_LVDS_LANE_CHA_LVDS_TERM | > > > > > > > - REG_LVDS_LANE_CHB_LVDS_TERM); > > > > > > > + (ctx->lvds_term_conf[CHANNEL_A] ? > > > > > > > + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | > > > > > > > + (ctx->lvds_term_conf[CHANNEL_B] ? > > > > > > > + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); > > > > > > > regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); > > > > > > > le16val = cpu_to_le16(mode->hdisplay); > > > > > > > @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { > > > > > > > .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, > > > > > > > }; > > > > > > > +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, > > > > > > > + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) > > > > > > > +{ > > > > > > > + int i; > > > > > > > + > > > > > > > + for (i = 0; i <= 3; i++) { > > > > > > > + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && > > > > > > > + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && > > > > > > > + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && > > > > > > > + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) > > > > > > > + return i; > > > > > > > + } > > > > > > > + > > > > > > > + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); > > > > > > > + return -EINVAL; > > > > > > > +} > > > > > > > + > > > > > > > +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) > > > > > > > +{ > > > > > > > + struct device *dev = ctx->dev; > > > > > > > + struct device_node *endpoint; > > > > > > > + int endpoint_reg; > > > > > > > + /* Set so the property can be freely selected if not defined */ > > > > > > > + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; > > > > > > > + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; > > > > > > > + u32 lvds_term; > > > > > > > + u8 lvds_term_conf = 0x1; > > > > > > > + int lvds_vod_swing_conf = 0x1; > > > > > > > > > > > > Magic values > > > > > > > > > > Can you please elaborate. > > > > > > > > > > I can use: > > > > > u8 lvds_term_conf = OHM_200; > > > > > > > > > > What about lvds_vod_swing_conf? Should I create additional define for it? > > > > > But this doesn't solve a hidden meaning? Maybe additional comment above? > > > > > Would like to avoid using voltages for it, since then we are reverse > > > > > engineering the table in datasheet to match the default reg value. > > > > > > > > I think the following example solves both problems: > > > > > > > > lvds_term = 200; > > > > of_property_read_u32(..., &lvds_term); > > > > > > > > if (lvds_term == 100) > > > > ctx->lvds_term_conf[channel] = OHM_100; > > > > else if (lvds_term == 200) > > > > ctx->lvds_term_conf[channel] = OHM_200; > > > > else > > > > return -EINVAL; > > > > > > > > The same approach can be applied to lvds_vod_swing_conf, resulting in > > > > removal of magic values. > > > > > > Sorry, but I think it is not that easy when it comes to the > > > lvds_vod_swing_conf. We should assign default value if > > > "ti,lvds-vod-swing-data-microvolt" and "ti,lvds-vod-swing-clock-microvolt" > > > are not defined. Default value of the lvds_vod_swing_conf is 0x1, but this > > > doesn't have any straight forward meaning like OHM_200 for example. > > > > > > What we can do in that case is that we copy the values from defined > > > datasheet tables to the "lvds_vod_swing_data[2]" and "lvds_vod_swing_clk[2]" > > > arrays and then run the > > > sn65dsi83_select_lvds_vod_swing with it, which will return the default value > > > (0x1). > > > > > > /* If both properties are not defined assign default limits */ > > > if (ret_data && ret_clock) { > > > memcpy(lvds_vod_swing_data, > > > lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1], > > > sizeof(lvds_vod_swing_data)); > > > memcpy(lvds_vod_swing_clk, > > > lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1], > > > sizeof(lvds_vod_swing_clk)); > > > } > > > lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, > > > lvds_vod_swing_data, lvds_vod_swing_clk, > > > ctx->lvds_term_conf[channel]); > > > if (lvds_vod_swing_conf < 0) { > > > ret = lvds_vod_swing_conf; > > > goto exit; > > > } > > > > > > ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; > > > > > > I'm not sure if using this approach gets rid of the problem with magic > > > values. > > > Or maybe I'm not seeing the obvious solution so please bear with me. > > > > Yes, the defaults (0..1000000) should be fixed to result in the same > > value (0x01) as if the property wasn't specified at all > > The defaults (0..1000000) is selected because in case if only one property > is defined in dts (ti,lvds-vod-swing-data-microvolt or > ti,lvds-vod-swing-clock-microvolt) the other array values don't effect the > decision which "lvds_vod_swing_conf" is selected. That's why we initialized > the array to be out off bounds of the datasheet tables, all values in the > table match the not defined property, so lvds_vod_swing_conf is selected > purely on the basis of the defined property. I see, thanks for the explanation. > > Example: > DTS > ti,lvds-vod-swing-data-microvolt = <250000 428000>; > //ti,lvds-vod-swing-clock-microvolt NOT DEFINED; > > After parsing the devicetree we will get: > lvds_vod_swing_data = [ 250000, 428000 ] > lvds_vod_swing_clk = [ 0, 1000000 ] > > In sn65dsi83_select_lvds_vod_swing lvds_vod_swing_clk[] values don't effect > the decision making since > > lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && > lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1] > > is always true. > > > > > I think the following should work: > > > > /* artifical values to select the defaults in both cases */ > > u32 lvds_vod_swing_data[2] = { 190000, 330000 }; > > u32 lvds_vod_swing_clk[2] = { 150000, 250000 }; > > This sets the default to 0x0. It should be: > u32 lvds_vod_swing_data[2] = { 200000, 372000 }; > u32 lvds_vod_swing_clk[2] = { 156000, 290000 }; > This selects the default 0x1 in both cases, if termination is 100 or 200 > Ohms. > > Nevertheless I think I got your point. But I would still like to give the > user the freedom to only specify one property if maybe connected panel only > has limits on data lanes/clock lane. > So maybe set the arrays lvds_vod_swing_data/clk to [0, 1000000] if > of_property_read_u32_array returns -EINVAL (property does not exist). > What do you say? After your explanation, I think it might be better to explicitly set the value to 0x1, but not at the top of the function, but next to a check that both properties are (not) set. > > > > > Yes, they are artificial, as stated in the comment. Yes, I think it's > > better than special-casing in the property handling. > >
On 12. 12. 24 10:28, Dmitry Baryshkov wrote: > On Thu, Dec 12, 2024 at 09:08:03AM +0100, Andrej Picej wrote: >> >> >> On 12. 12. 24 00:04, Dmitry Baryshkov wrote: >>> On Wed, Dec 11, 2024 at 08:57:17AM +0100, Andrej Picej wrote: >>>> >>>> >>>> On 10. 12. 24 14:59, Dmitry Baryshkov wrote: >>>>> On Tue, Dec 10, 2024 at 02:41:01PM +0100, Andrej Picej wrote: >>>>>> >>>>>> >>>>>> On 10. 12. 24 12:43, Dmitry Baryshkov wrote: >>>>>>> On Tue, Dec 10, 2024 at 10:19:00AM +0100, Andrej Picej wrote: >>>>>>>> Add a optional properties to change LVDS output voltage. This should not >>>>>>>> be static as this depends mainly on the connected display voltage >>>>>>>> requirement. We have three properties: >>>>>>>> - "ti,lvds-termination-ohms", which sets near end termination, >>>>>>>> - "ti,lvds-vod-swing-data-microvolt" and >>>>>>>> - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential >>>>>>>> output voltage for data and clock lanes. They are defined as an array >>>>>>>> with min and max values. The appropriate bitfield will be set if >>>>>>>> selected constraints can be met. >>>>>>>> >>>>>>>> If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near >>>>>>>> end termination will be used. Selecting only one: >>>>>>>> "ti,lvds-vod-swing-data-microvolt" or >>>>>>>> "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage >>>>>>>> constraint for only data/clock lanes will be met. Setting both is >>>>>>>> recommended. >>>>>>>> >>>>>>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com> >>>>>>>> --- >>>>>>>> Changes in v5: >>>>>>>> - specify default values in sn65dsi83_parse_lvds_endpoint, >>>>>>>> - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, >>>>>>>> Changes in v4: >>>>>>>> - fix typo in commit message bitfiled -> bitfield >>>>>>>> - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead >>>>>>>> of separate variables for channel A/B >>>>>>>> - add more checks on return value of "of_property_read_u32_array" >>>>>>>> Changes in v3: >>>>>>>> - use microvolts for default array values 1000 mV -> 1000000 uV. >>>>>>>> Changes in v2: >>>>>>>> - use datasheet tables to get the proper configuration >>>>>>>> - since major change was done change the authorship to myself >>>>>>>> --- >>>>>>>> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- >>>>>>>> 1 file changed, 139 insertions(+), 3 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>>>> index 57a7ed13f996..f9578b38da28 100644 >>>>>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c >>>>>>>> @@ -132,6 +132,16 @@ >>>>>>>> #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) >>>>>>>> #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) >>>>>>>> +enum sn65dsi83_channel { >>>>>>>> + CHANNEL_A, >>>>>>>> + CHANNEL_B >>>>>>>> +}; >>>>>>>> + >>>>>>>> +enum sn65dsi83_lvds_term { >>>>>>>> + OHM_100, >>>>>>>> + OHM_200 >>>>>>>> +}; >>>>>>>> + >>>>>>>> enum sn65dsi83_model { >>>>>>>> MODEL_SN65DSI83, >>>>>>>> MODEL_SN65DSI84, >>>>>>>> @@ -147,6 +157,8 @@ struct sn65dsi83 { >>>>>>>> struct regulator *vcc; >>>>>>>> bool lvds_dual_link; >>>>>>>> bool lvds_dual_link_even_odd_swap; >>>>>>>> + int lvds_vod_swing_conf[2]; >>>>>>>> + int lvds_term_conf[2]; >>>>>>>> }; >>>>>>>> static const struct regmap_range sn65dsi83_readable_ranges[] = { >>>>>>>> @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { >>>>>>>> .max_register = REG_IRQ_STAT, >>>>>>>> }; >>>>>>>> +static const int lvds_vod_swing_data_table[2][4][2] = { >>>>>>>> + { /* 100 Ohm */ >>>>>>>> + { 180000, 313000 }, >>>>>>>> + { 215000, 372000 }, >>>>>>>> + { 250000, 430000 }, >>>>>>>> + { 290000, 488000 }, >>>>>>>> + }, >>>>>>>> + { /* 200 Ohm */ >>>>>>>> + { 150000, 261000 }, >>>>>>>> + { 200000, 346000 }, >>>>>>>> + { 250000, 428000 }, >>>>>>>> + { 300000, 511000 }, >>>>>>>> + }, >>>>>>>> +}; >>>>>>>> + >>>>>>>> +static const int lvds_vod_swing_clock_table[2][4][2] = { >>>>>>>> + { /* 100 Ohm */ >>>>>>>> + { 140000, 244000 }, >>>>>>>> + { 168000, 290000 }, >>>>>>>> + { 195000, 335000 }, >>>>>>>> + { 226000, 381000 }, >>>>>>>> + }, >>>>>>>> + { /* 200 Ohm */ >>>>>>>> + { 117000, 204000 }, >>>>>>>> + { 156000, 270000 }, >>>>>>>> + { 195000, 334000 }, >>>>>>>> + { 234000, 399000 }, >>>>>>>> + }, >>>>>>>> +}; >>>>>>>> + >>>>>>>> static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) >>>>>>>> { >>>>>>>> return container_of(bridge, struct sn65dsi83, bridge); >>>>>>>> @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, >>>>>>>> val |= REG_LVDS_FMT_LVDS_LINK_CFG; >>>>>>>> regmap_write(ctx->regmap, REG_LVDS_FMT, val); >>>>>>>> - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); >>>>>>>> + regmap_write(ctx->regmap, REG_LVDS_VCOM, >>>>>>>> + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | >>>>>>>> + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); >>>>>>>> regmap_write(ctx->regmap, REG_LVDS_LANE, >>>>>>>> (ctx->lvds_dual_link_even_odd_swap ? >>>>>>>> REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | >>>>>>>> - REG_LVDS_LANE_CHA_LVDS_TERM | >>>>>>>> - REG_LVDS_LANE_CHB_LVDS_TERM); >>>>>>>> + (ctx->lvds_term_conf[CHANNEL_A] ? >>>>>>>> + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | >>>>>>>> + (ctx->lvds_term_conf[CHANNEL_B] ? >>>>>>>> + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); >>>>>>>> regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); >>>>>>>> le16val = cpu_to_le16(mode->hdisplay); >>>>>>>> @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { >>>>>>>> .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, >>>>>>>> }; >>>>>>>> +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, >>>>>>>> + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) >>>>>>>> +{ >>>>>>>> + int i; >>>>>>>> + >>>>>>>> + for (i = 0; i <= 3; i++) { >>>>>>>> + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && >>>>>>>> + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && >>>>>>>> + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && >>>>>>>> + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) >>>>>>>> + return i; >>>>>>>> + } >>>>>>>> + >>>>>>>> + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); >>>>>>>> + return -EINVAL; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) >>>>>>>> +{ >>>>>>>> + struct device *dev = ctx->dev; >>>>>>>> + struct device_node *endpoint; >>>>>>>> + int endpoint_reg; >>>>>>>> + /* Set so the property can be freely selected if not defined */ >>>>>>>> + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; >>>>>>>> + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; >>>>>>>> + u32 lvds_term; >>>>>>>> + u8 lvds_term_conf = 0x1; >>>>>>>> + int lvds_vod_swing_conf = 0x1; >>>>>>> >>>>>>> Magic values >>>>>> >>>>>> Can you please elaborate. >>>>>> >>>>>> I can use: >>>>>> u8 lvds_term_conf = OHM_200; >>>>>> >>>>>> What about lvds_vod_swing_conf? Should I create additional define for it? >>>>>> But this doesn't solve a hidden meaning? Maybe additional comment above? >>>>>> Would like to avoid using voltages for it, since then we are reverse >>>>>> engineering the table in datasheet to match the default reg value. >>>>> >>>>> I think the following example solves both problems: >>>>> >>>>> lvds_term = 200; >>>>> of_property_read_u32(..., &lvds_term); >>>>> >>>>> if (lvds_term == 100) >>>>> ctx->lvds_term_conf[channel] = OHM_100; >>>>> else if (lvds_term == 200) >>>>> ctx->lvds_term_conf[channel] = OHM_200; >>>>> else >>>>> return -EINVAL; >>>>> >>>>> The same approach can be applied to lvds_vod_swing_conf, resulting in >>>>> removal of magic values. >>>> >>>> Sorry, but I think it is not that easy when it comes to the >>>> lvds_vod_swing_conf. We should assign default value if >>>> "ti,lvds-vod-swing-data-microvolt" and "ti,lvds-vod-swing-clock-microvolt" >>>> are not defined. Default value of the lvds_vod_swing_conf is 0x1, but this >>>> doesn't have any straight forward meaning like OHM_200 for example. >>>> >>>> What we can do in that case is that we copy the values from defined >>>> datasheet tables to the "lvds_vod_swing_data[2]" and "lvds_vod_swing_clk[2]" >>>> arrays and then run the >>>> sn65dsi83_select_lvds_vod_swing with it, which will return the default value >>>> (0x1). >>>> >>>> /* If both properties are not defined assign default limits */ >>>> if (ret_data && ret_clock) { >>>> memcpy(lvds_vod_swing_data, >>>> lvds_vod_swing_data_table[ctx->lvds_term_conf[channel]][1], >>>> sizeof(lvds_vod_swing_data)); >>>> memcpy(lvds_vod_swing_clk, >>>> lvds_vod_swing_clock_table[ctx->lvds_term_conf[channel]][1], >>>> sizeof(lvds_vod_swing_clk)); >>>> } >>>> lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, >>>> lvds_vod_swing_data, lvds_vod_swing_clk, >>>> ctx->lvds_term_conf[channel]); >>>> if (lvds_vod_swing_conf < 0) { >>>> ret = lvds_vod_swing_conf; >>>> goto exit; >>>> } >>>> >>>> ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; >>>> >>>> I'm not sure if using this approach gets rid of the problem with magic >>>> values. >>>> Or maybe I'm not seeing the obvious solution so please bear with me. >>> >>> Yes, the defaults (0..1000000) should be fixed to result in the same >>> value (0x01) as if the property wasn't specified at all >> >> The defaults (0..1000000) is selected because in case if only one property >> is defined in dts (ti,lvds-vod-swing-data-microvolt or >> ti,lvds-vod-swing-clock-microvolt) the other array values don't effect the >> decision which "lvds_vod_swing_conf" is selected. That's why we initialized >> the array to be out off bounds of the datasheet tables, all values in the >> table match the not defined property, so lvds_vod_swing_conf is selected >> purely on the basis of the defined property. > > I see, thanks for the explanation. > >> >> Example: >> DTS >> ti,lvds-vod-swing-data-microvolt = <250000 428000>; >> //ti,lvds-vod-swing-clock-microvolt NOT DEFINED; >> >> After parsing the devicetree we will get: >> lvds_vod_swing_data = [ 250000, 428000 ] >> lvds_vod_swing_clk = [ 0, 1000000 ] >> >> In sn65dsi83_select_lvds_vod_swing lvds_vod_swing_clk[] values don't effect >> the decision making since >> >> lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && >> lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1] >> >> is always true. >> >>> >>> I think the following should work: >>> >>> /* artifical values to select the defaults in both cases */ >>> u32 lvds_vod_swing_data[2] = { 190000, 330000 }; >>> u32 lvds_vod_swing_clk[2] = { 150000, 250000 }; >> >> This sets the default to 0x0. It should be: >> u32 lvds_vod_swing_data[2] = { 200000, 372000 }; >> u32 lvds_vod_swing_clk[2] = { 156000, 290000 }; >> This selects the default 0x1 in both cases, if termination is 100 or 200 >> Ohms. >> >> Nevertheless I think I got your point. But I would still like to give the >> user the freedom to only specify one property if maybe connected panel only >> has limits on data lanes/clock lane. >> So maybe set the arrays lvds_vod_swing_data/clk to [0, 1000000] if >> of_property_read_u32_array returns -EINVAL (property does not exist). >> What do you say? > > After your explanation, I think it might be better to explicitly set the > value to 0x1, but not at the top of the function, but next to a check > that both properties are (not) set. Ok. Will apply this in the next version. Thanks. > >> >>> >>> Yes, they are artificial, as stated in the comment. Yes, I think it's >>> better than special-casing in the property handling. >>> >
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 57a7ed13f996..f9578b38da28 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -132,6 +132,16 @@ #define REG_IRQ_STAT_CHA_SOT_BIT_ERR BIT(2) #define REG_IRQ_STAT_CHA_PLL_UNLOCK BIT(0) +enum sn65dsi83_channel { + CHANNEL_A, + CHANNEL_B +}; + +enum sn65dsi83_lvds_term { + OHM_100, + OHM_200 +}; + enum sn65dsi83_model { MODEL_SN65DSI83, MODEL_SN65DSI84, @@ -147,6 +157,8 @@ struct sn65dsi83 { struct regulator *vcc; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap; + int lvds_vod_swing_conf[2]; + int lvds_term_conf[2]; }; static const struct regmap_range sn65dsi83_readable_ranges[] = { @@ -237,6 +249,36 @@ static const struct regmap_config sn65dsi83_regmap_config = { .max_register = REG_IRQ_STAT, }; +static const int lvds_vod_swing_data_table[2][4][2] = { + { /* 100 Ohm */ + { 180000, 313000 }, + { 215000, 372000 }, + { 250000, 430000 }, + { 290000, 488000 }, + }, + { /* 200 Ohm */ + { 150000, 261000 }, + { 200000, 346000 }, + { 250000, 428000 }, + { 300000, 511000 }, + }, +}; + +static const int lvds_vod_swing_clock_table[2][4][2] = { + { /* 100 Ohm */ + { 140000, 244000 }, + { 168000, 290000 }, + { 195000, 335000 }, + { 226000, 381000 }, + }, + { /* 200 Ohm */ + { 117000, 204000 }, + { 156000, 270000 }, + { 195000, 334000 }, + { 234000, 399000 }, + }, +}; + static struct sn65dsi83 *bridge_to_sn65dsi83(struct drm_bridge *bridge) { return container_of(bridge, struct sn65dsi83, bridge); @@ -435,12 +477,16 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, val |= REG_LVDS_FMT_LVDS_LINK_CFG; regmap_write(ctx->regmap, REG_LVDS_FMT, val); - regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05); + regmap_write(ctx->regmap, REG_LVDS_VCOM, + REG_LVDS_VCOM_CHA_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_A]) | + REG_LVDS_VCOM_CHB_LVDS_VOD_SWING(ctx->lvds_vod_swing_conf[CHANNEL_B])); regmap_write(ctx->regmap, REG_LVDS_LANE, (ctx->lvds_dual_link_even_odd_swap ? REG_LVDS_LANE_EVEN_ODD_SWAP : 0) | - REG_LVDS_LANE_CHA_LVDS_TERM | - REG_LVDS_LANE_CHB_LVDS_TERM); + (ctx->lvds_term_conf[CHANNEL_A] ? + REG_LVDS_LANE_CHA_LVDS_TERM : 0) | + (ctx->lvds_term_conf[CHANNEL_B] ? + REG_LVDS_LANE_CHB_LVDS_TERM : 0)); regmap_write(ctx->regmap, REG_LVDS_CM, 0x00); le16val = cpu_to_le16(mode->hdisplay); @@ -576,10 +622,100 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = { .atomic_get_input_bus_fmts = sn65dsi83_atomic_get_input_bus_fmts, }; +static int sn65dsi83_select_lvds_vod_swing(struct device *dev, + u32 lvds_vod_swing_data[2], u32 lvds_vod_swing_clk[2], u8 lvds_term) +{ + int i; + + for (i = 0; i <= 3; i++) { + if (lvds_vod_swing_data_table[lvds_term][i][0] >= lvds_vod_swing_data[0] && + lvds_vod_swing_data_table[lvds_term][i][1] <= lvds_vod_swing_data[1] && + lvds_vod_swing_clock_table[lvds_term][i][0] >= lvds_vod_swing_clk[0] && + lvds_vod_swing_clock_table[lvds_term][i][1] <= lvds_vod_swing_clk[1]) + return i; + } + + dev_err(dev, "failed to find appropriate LVDS_VOD_SWING configuration\n"); + return -EINVAL; +} + +static int sn65dsi83_parse_lvds_endpoint(struct sn65dsi83 *ctx, int channel) +{ + struct device *dev = ctx->dev; + struct device_node *endpoint; + int endpoint_reg; + /* Set so the property can be freely selected if not defined */ + u32 lvds_vod_swing_data[2] = { 0, 1000000 }; + u32 lvds_vod_swing_clk[2] = { 0, 1000000 }; + u32 lvds_term; + u8 lvds_term_conf = 0x1; + int lvds_vod_swing_conf = 0x1; + int ret = 0; + int ret_data; + int ret_clock; + + if (channel == CHANNEL_A) + endpoint_reg = 2; + else + endpoint_reg = 3; + + endpoint = of_graph_get_endpoint_by_regs(dev->of_node, endpoint_reg, -1); + if (!of_property_read_u32(endpoint, "ti,lvds-termination-ohms", &lvds_term)) { + if (lvds_term == 100) + lvds_term_conf = OHM_100; + else + lvds_term_conf = OHM_200; + } + + ctx->lvds_term_conf[channel] = lvds_term_conf; + + ret_data = of_property_read_u32_array(endpoint, + "ti,lvds-vod-swing-data-microvolt", lvds_vod_swing_data, + ARRAY_SIZE(lvds_vod_swing_data)); + if (ret_data != 0 && ret_data != -EINVAL) { + ret = ret_data; + goto exit; + } + + ret_clock = of_property_read_u32_array(endpoint, + "ti,lvds-vod-swing-clock-microvolt", lvds_vod_swing_clk, + ARRAY_SIZE(lvds_vod_swing_clk)); + if (ret_clock != 0 && ret_clock != -EINVAL) { + ret = ret_clock; + goto exit; + } + + /* If any of the two properties is defined. */ + if (!ret_data || !ret_clock) { + lvds_vod_swing_conf = sn65dsi83_select_lvds_vod_swing(dev, + lvds_vod_swing_data, lvds_vod_swing_clk, + lvds_term_conf); + if (lvds_vod_swing_conf < 0) { + ret = lvds_vod_swing_conf; + goto exit; + } + } + + ctx->lvds_vod_swing_conf[channel] = lvds_vod_swing_conf; + ret = 0; +exit: + of_node_put(endpoint); + return ret; +} + static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) { struct drm_bridge *panel_bridge; struct device *dev = ctx->dev; + int ret; + + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_A); + if (ret < 0) + return ret; + + ret = sn65dsi83_parse_lvds_endpoint(ctx, CHANNEL_B); + if (ret < 0) + return ret; ctx->lvds_dual_link = false; ctx->lvds_dual_link_even_odd_swap = false;
Add a optional properties to change LVDS output voltage. This should not be static as this depends mainly on the connected display voltage requirement. We have three properties: - "ti,lvds-termination-ohms", which sets near end termination, - "ti,lvds-vod-swing-data-microvolt" and - "ti,lvds-vod-swing-clock-microvolt" which both set LVDS differential output voltage for data and clock lanes. They are defined as an array with min and max values. The appropriate bitfield will be set if selected constraints can be met. If "ti,lvds-termination-ohms" is not defined the default of 200 Ohm near end termination will be used. Selecting only one: "ti,lvds-vod-swing-data-microvolt" or "ti,lvds-vod-swing-clock-microvolt" can be done, but the output voltage constraint for only data/clock lanes will be met. Setting both is recommended. Signed-off-by: Andrej Picej <andrej.picej@norik.com> --- Changes in v5: - specify default values in sn65dsi83_parse_lvds_endpoint, - move sn65dsi83_parse_lvds_endpoint for channel B up, outside if, Changes in v4: - fix typo in commit message bitfiled -> bitfield - use arrays (lvds_vod_swing_conf and lvds_term_conf) in private data, instead of separate variables for channel A/B - add more checks on return value of "of_property_read_u32_array" Changes in v3: - use microvolts for default array values 1000 mV -> 1000000 uV. Changes in v2: - use datasheet tables to get the proper configuration - since major change was done change the authorship to myself --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 142 +++++++++++++++++++++++++- 1 file changed, 139 insertions(+), 3 deletions(-)