linux-kernel - Re: [PATCH v3 2/2] drm: bridge: Add TI SN65DSI83/84/85 DSI to LVDS bridge

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 Mar 2021 22:51:43 +0100
From:   Marek Vasut <marex@...x.de>
To:     Jagan Teki <jagan@...rulasolutions.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Sam Ravnborg <sam@...nborg.org>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-amarula@...rulasolutions.com
Subject: Re: [PATCH v3 2/2] drm: bridge: Add TI SN65DSI83/84/85 DSI to LVDS
 bridge

On 2/14/21 6:44 PM, Jagan Teki wrote:

[...]

> +static const struct regmap_config sn65dsi_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = SN65DSI_CHA_ERR,
> +	.name = "sn65dsi",
> +	.cache_type = REGCACHE_RBTREE,
> +};

You might want to look at the driver I posted one more time, it defines 
the regmap precisely and limits each register access, see:
[PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
that way it can be dumped via debugfs and the regmap does not cache 
registers which do not exist, like it does here.

[...]

> +static int sn65dsi_get_clk_range(int min, int max, unsigned long clock,
> +				 unsigned long start, unsigned long diff)
> +{
> +	unsigned long next;
> +	int i;
> +
> +	for (i = min; i <= max; i++) {
> +		next = start + diff;
> +		if (start <= clock && clock < next)
> +			return i;
> +
> +		start += diff;
> +	}
> +
> +	return -EINVAL;
> +}

The clock rates can be calculated in linear time, see the driver above, 
it is implemented there.

> +static void sn65dsi_enable(struct drm_bridge *bridge)
> +{
> +	struct sn65dsi *sn = bridge_to_sn65dsi(bridge);
> +	struct drm_display_mode *mode = bridge_to_mode(bridge);
> +	int bpp = mipi_dsi_pixel_format_to_bpp(sn->dsi->format);
> +	unsigned int lanes = sn->dsi->lanes;
> +	unsigned int pixel_clk = mode->clock * 1000;
> +	unsigned int dsi_clk = pixel_clk * bpp / (lanes * 2);
> +	unsigned int val;
> +
> +	/* reset SOFT_RESET bit */
> +	regmap_write(sn->regmap, SN65DSI_SOFT_RESET, 0x0);
> +
> +	msleep(10);

Why is there msleep(10) all over the place ?
I don't see such a requirement listed anywhere in the DSI83 datasheet.

> +	/* reset PLL_EN bit */
> +	regmap_write(sn->regmap, SN65DSI_CLK_PLL, 0x0);
> +
> +	msleep(10);

Here too.

[...]

You also want to check the feedback on the driver I posted, it deals 
with polling for the PLL to be ready, which seems to be missing here. 
That should remove most of the msleep() calls.