Re: [PATCH v6 2/3] drm/bridge: ti-sn65dsi83: Add ti,lvds-vod-swing optional properties

[Andrej Picej <andrej.picej@norik.com>]

Hi Christophe,

On 12. 12. 24 13:33, Christophe JAILLET wrote:
> Le 12/12/2024 à 13:17, Andrej Picej a écrit :
>> 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.
> 
> ...
> 
>> +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])
> 
> Adding a few spaces to align things would help reading.

Ok, that makes sense, will fix.

> 
>> +            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 };
>> +    /* Set default near end terminataion to 200 Ohm */
>> +    u32 lvds_term = 200;
>> +    int lvds_vod_swing_conf;
>> +    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);
>> +
>> +    of_property_read_u32(endpoint, "ti,lvds-termination-ohms", 
>> &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;
> 
> Should it be:
>      else {
>          ret = -EINVAL;
>          goto exit;
>      }
> ?

Of course, good catch. Thanks.

Best regards,
Andrej.

> 
>> +
>> +    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;
>> +    }
>> +
>> +    /* Use default value if both properties are NOT defined. */
>> +    if (ret_data == -EINVAL && ret_clock == -EINVAL)
>> +        lvds_vod_swing_conf = 0x1;
>> +
>> +    /* Use lookup table 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, 
>> 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;
>> +    ret = 0;
>> +exit:
>> +    of_node_put(endpoint);
>> +    return ret;
>> +}
> 
> ...
> 
> CJ