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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 22 Jan 2020 11:16:00 +0200
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Andrzej Hajda <a.hajda@...sung.com>, <airlied@...ux.ie>,
        <daniel@...ll.ch>, <robh+dt@...nel.org>, <mark.rutland@....com>,
        <narmstrong@...libre.com>
CC:     <tomi.valkeinen@...com>, <dri-devel@...ts.freedesktop.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <Laurent.pinchart@...asonboard.com>, <jonas@...boo.se>,
        <jernej.skrabec@...l.net>
Subject: Re: [PATCH 2/2] drm/bridge: Add tc358768 driver

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