Re: [PATCH 2/2] drm/bridge: Add tc358768 driver

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

Hi Andrzej,

On 22/01/2020 10.46, Andrzej Hajda wrote:
>>>> +struct tc358768_priv {
>>>> +	struct device *dev;
>>>> +	struct regmap *regmap;
>>>> +	struct gpio_desc *reset_gpio;
>>>> +	struct regulator_bulk_data supplies[ARRAY_SIZE(tc358768_supplies)];
>>>> +	struct clk *refclk;
>>>> +
>>>> +	struct mipi_dsi_host dsi_host;
>>>> +	struct drm_bridge bridge;
>>>> +	struct tc358768_dsi_output output;
>>>
>>> Since tc358768_dsi_output is used only here, you can define it here as
>>> well, up to you.
>> I think I have done it like this to avoid thinking about prefixes for
>> what is under the tc358768_dsi_output.
>> I'll take a look if it will look better unpacked here.
> 
> I though rather about in-place anonymous struct definition:
> 
> +    struct tc358768_dsi_output {
> +        struct mipi_dsi_device *dev;
> +        struct drm_panel *panel;
> +        struct drm_bridge *bridge;
> +    } output;
> 
> But, as I said - up to you.

I see. I think I will keep how it was. They are in proximity, so easy to
check.

>>>> +
>>>> +	refclk = clk_get_rate(priv->refclk);
>>>> +
>>>> +	best_diff = UINT_MAX;
>>>> +	best_pll = 0;
>>>> +	best_prd = 0;
>>>> +	best_fbd = 0;
>>>> +
>>>> +	for (prd = 0; prd < 16; ++prd) {
>>>> +		u32 divisor = (prd + 1) * (1 << frs);
>>>> +		u32 fbd;
>>>> +
>>>> +		for (fbd = 0; fbd < 512; ++fbd) {
>>>> +			u32 pll, diff;
>>>> +
>>>> +			pll = (u32)div_u64((u64)refclk * (fbd + 1), divisor);
>>>> +
>>>> +			if (pll >= max_pll || pll < min_pll)
>>>> +				continue;
>>>> +
>>>> +			diff = max(pll, target_pll) - min(pll, target_pll);
>>>> +
>>>> +			if (diff < best_diff) {
>>>> +				best_diff = diff;
>>>> +				best_pll = pll;
>>>> +				best_prd = prd;
>>>> +				best_fbd = fbd;
>>>> +			}
>>>> +
>>>> +			if (best_diff == 0)
>>>> +				break;
>>>> +		}
>>>> +
>>>> +		if (best_diff == 0)
>>>> +			break;
>>> why another check here?
>> To break out from the top for() loop also in case exact match has been
>> found.
> 
> 
> Ahh, OK. So maybe you should put "if (diff == 0) goto found" inside "if
> (diff < best_diff)" block, in such case goto is not considered harmful
> :), and is more readable.

Exactly my thoughts ;)

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki