Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/2] drm/bridge: Add tc358768 driver
To:     Andrzej Hajda <a.hajda@samsung.com>, <airlied@linux.ie>,
        <daniel@ffwll.ch>, <robh+dt@kernel.org>, <mark.rutland@arm.com>,
        <narmstrong@baylibre.com>
CC:     <tomi.valkeinen@ti.com>, <dri-devel@lists.freedesktop.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <Laurent.pinchart@ideasonboard.com>, <jonas@kwiboo.se>,
        <jernej.skrabec@siol.net>
References: <20191217101506.18910-1-peter.ujfalusi@ti.com>
 <CGME20191217101520epcas1p4a2bdee0cab0c11670b74fbe9e9397835@epcas1p4.samsung.com>
 <20191217101506.18910-3-peter.ujfalusi@ti.com>
 <35d664fe-8091-2744-abf2-69828ebf1148@samsung.com>
 <14306079-500d-09ca-df94-4cf72c43f858@ti.com>
 <cdc941c5-681f-962b-7f99-ebfda6aaaa91@samsung.com>
From:   Peter Ujfalusi <peter.ujfalusi@ti.com>
Message-ID: <e8774c16-c34c-adf8-fee5-17323dcd95bf@ti.com>
Date:   Wed, 22 Jan 2020 11:16:00 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <cdc941c5-681f-962b-7f99-ebfda6aaaa91@samsung.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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