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.pi...@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
> > >
> >
--
With best wishes
Dmitry
