On Thu, Apr 30, 2020 at 09:03:24AM +0200, Sam Ravnborg wrote:
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.
OK, got it, thanks.
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.
OK, I'll double check it.
- 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.
OK
Sam