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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
