linux-kernel - Re: [PATCH v8 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge 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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Apr 2020 09:03:24 +0200
From:   Sam Ravnborg <sam@...nborg.org>
To:     Xin Ji <xji@...logixsemi.com>
Cc:     devel@...verdev.osuosl.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        Pi-Hsun Shih <pihsun@...omium.org>,
        Sheng Pan <span@...logixsemi.com>
Subject: Re: [PATCH v8 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to
 DP bridge driver

Hi Xin Ji.

> > > +static void anx7625_power_on_init(struct anx7625_data *ctx)
> > > +{
> > > +	int retry_count, i;
> > > +	int ret;
> > > +	struct device *dev = &ctx->client->dev;
> > > +
> > > +	for (retry_count = 0; retry_count < 3; retry_count++) {
> > > +		anx7625_power_on(ctx);
> > > +		anx7625_config(ctx);
> > > +
> > > +		for (i = 0; i < OCM_LOADING_TIME; i++) {
> > Code in this for loop is a candidate for a helper function.
> I didn't find any helper function can be used, so I'll keep it.
I was not very clear in my way to express this, sorry.

> > 
> > > +			/* check interface workable */
> > > +			ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
> > > +					       FLASH_LOAD_STA);
> > > +			if (ret < 0) {
> > > +				DRM_ERROR("IO error : access flash load.\n");
> > > +				return;
> > > +			}
> > > +			if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
> > > +				anx7625_disable_pd_protocol(ctx);
> > > +				DRM_DEV_DEBUG_DRIVER(dev,
> > > +						     "Firmware ver %02x%02x,",
> > > +					anx7625_reg_read(ctx,
> > > +							 ctx->i2c.rx_p0_client,
> > > +							 OCM_FW_VERSION),
> > > +					anx7625_reg_read(ctx,
> > > +							 ctx->i2c.rx_p0_client,
> > > +							 OCM_FW_REVERSION));
> > > +				DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
> > > +						     ANX7625_DRV_VERSION);
> > > +
> > > +				return;
> > > +			}
> > > +			usleep_range(1000, 1100);
> > > +		}
What I wanted to express is that the for loop is heavily indented.
So create a small function like:

anx7625_power_on_interface(ctx)
{
	/* check interface workable */
	ret = anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, FLASH_LOAD_STA);
	if (ret < 0) {
	        DRM_ERROR("IO error : access flash load.\n");
	        return;
	}
	if ((ret & FLASH_LOAD_STA_CHK) == FLASH_LOAD_STA_CHK) {
	        anx7625_disable_pd_protocol(ctx);
	        DRM_DEV_DEBUG_DRIVER(dev, "Firmware ver %02x%02x,",
	                anx7625_reg_read(ctx, ctx->i2c.rx_p0_client,
                                         OCM_FW_VERSION), anx7625_reg_read(ctx,
	                                 ctx->i2c.rx_p0_client, OCM_FW_REVERSION));
	        DRM_DEV_DEBUG_DRIVER(dev, "Driver version %s\n",
	                             ANX7625_DRV_VERSION);
		retunrn 1;
	}
	return 0;
}

and then

	for (i = 0; i < OCM_LOADING_TIME; i++) {
		if (anx7625_power_on_interface(ctx))
			return;
		else
			usleep_range(1000, 1100);
	}

Or something like that. To make it more readable.
I think you get the idea now.


> > > +		container_of(work, struct anx7625_data, extcon_wq);
> > > +	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_DP);
> > > +
> > > +	mutex_lock(&ctx->lock);
> > > +	anx7625_chip_control(ctx, state);
> > > +	mutex_unlock(&ctx->lock);
> > I tried to follow the locking - but failed.
> > Could you check that locking seems correct.
> > 
> > A standard bridge driver do not need locking,
> > but this is no small bridge driver so I do not imply that
> > locking is not needed. Only that I would like you
> > to check it again as I could not follow it.
> OK, it seems lock is not necessary, I'll remove itA
It has a worker, so please be careful in you analysis.

> > 
> > > +
> > > +	if (pdata->panel_flags == 1)
> > > +		pdata->internal_panel = 1;
> > > +	else if (pdata->panel_flags == 2)
> > > +		pdata->extcon_supported = 1;
> > > +	DRM_DEV_DEBUG_DRIVER(dev, "%s support extcon, %s internal panel\n",
> > > +			     pdata->extcon_supported ? "" : "not",
> > > +			     pdata->internal_panel ? "has" : "no");
> > > +
> > The way the internal panel - versus external connector is modelled
> > looks like it could use some of the abstractions used by other bridge
> > drivers.
> > 
> > The connector_type shall for example for internal panels come
> > form the panel.
> > And use the panel bridge driver - see examples in patches I referenced
> > before.
> > 
> > And external connectors may beneft from using the
> > display-connector bridge driver.
> I'm not familiar with it, the extcon interface is Google engineer give
> to me, I just follow their sample driver. If you think it is not good,
> I'll remove the extcon support.
It would be better to start without, and then add it later
so we end up with a clean design.

I for one would have an easier time reviewing.

So please go ahead and remove it for now.


	Sam