Hi Tomasz, Thanks for the reviews. I have consolidated my responses to comments from all of you below and are addressed in v5 of this patch. > -----Original Message----- > From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > Sent: Wednesday, May 10, 2017 11:31 PM > To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx; Hans Verkuil > <hverkuil@xxxxxxxxx>; Sakari Ailus <sakari.ailus@xxxxxx> > Subject: Re: [PATCH v4] dw9714: Initial driver for dw9714 VCM > > Hi Raj, > > Thanks for re-spin. Still a bit more comments inline. (I missed few more before, > sorry.) > > On Thu, May 11, 2017 at 1:00 PM, Rajmohan Mani > <rajmohan.mani@xxxxxxxxx> wrote: > > DW9714 is a 10 bit DAC, designed for linear control of voice coil > > motor. > [snip] > > +static int dw9714_i2c_write(struct i2c_client *client, u16 data) { > > + int ret; > > + u16 val = cpu_to_be16(data); > > + const int num_bytes = sizeof(val); > > + > > + ret = i2c_master_send(client, (const char *) &val, > > + sizeof(val)); > > nit: No need for space between cast and casted value. > Ack > > + > > + /*One retry */ > > + if (ret != num_bytes) > > + ret = i2c_master_send(client, (const char *) &val, > > + sizeof(val)); > > Why do we need this retry? > This was found to be useful in the early bring up days of this vcm. I think this can be removed now. > > + > > + if (ret != num_bytes) { > > + dev_err(&client->dev, "I2C write fail\n"); > > + return -EIO; > > + } > > + return 0; > > +} > > + > > +static int dw9714_t_focus_vcm(struct dw9714_device *dw9714_dev, u16 > > +val) { > > + struct i2c_client *client = dw9714_dev->client; > > + > > + dw9714_dev->current_val = val; > > + > > + return dw9714_i2c_write(client, DW9714_VAL(val, > > + DW9714_DEFAULT_S)); > > This still doesn't seem to apply the control gradually as suspend and resume do. > Ack As was discussed with Sakari on the follow up threads, we can leave it as it is here, so the user space can control this as needed. > > +} > [snip] > > +static int dw9714_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > > +*fh) { > > + struct dw9714_device *dw9714_dev = container_of(sd, > > + struct dw9714_device, > > + sd); > > + struct device *dev = &dw9714_dev->client->dev; > > + int rval; > > + > > + rval = pm_runtime_get_sync(dev); > > + if (rval >= 0) > > + return 0; > > + > > + pm_runtime_put(dev); > > + return rval; > > nit: The typical coding style is to return early in case of a special case and keep > the common path linear, i.e. > Ack > rval = pm_runtime_get_sync(dev); > if (rval < 0) { > pm_runtime_put(dev); > return rval; > } > > return 0; > > > +} > > + > [snip] > > +static int dw9714_remove(struct i2c_client *client) { > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct dw9714_device *dw9714_dev = container_of(sd, > > + struct dw9714_device, > > + sd); > > + > > + pm_runtime_disable(&client->dev); > > + dw9714_subdev_cleanup(dw9714_dev); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > #if defined(CONFIG_PM) || defined(CONFIG_PM_SLEEP) > Ack. __maybe_unused is added for vcm_*suspend/resume functions > > + > > +/* > > + * This function sets the vcm position, so it consumes least current > > + * The lens position is gradually moved in units of > > +DW9714_CTRL_STEPS, > > + * to make the movements smoothly. > > + */ > > +static int dw9714_vcm_suspend(struct device *dev) { > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct dw9714_device *dw9714_dev = container_of(sd, > > + struct dw9714_device, > > + sd); > > + int ret, val; > > + > > + for (val = dw9714_dev->current_val & ~(DW9714_CTRL_STEPS - 1); > > + val >= 0; val -= DW9714_CTRL_STEPS) { > > + ret = dw9714_i2c_write(client, > > + DW9714_VAL((u16) val, > > + DW9714_DEFAULT_S)); > > DW9714_VAL() already contains such cast. Anyway, I still think they don't really > give us anything and should be removed. > Ack > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, > > + ret); > > I think we should just return an error code here and fail the suspend. > As discussed, we are going to ignore this error and not let it affect system suspend. > > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US > + 10); > > + } > > + return 0; > > +} > > + > > +/* > > + * This function sets the vcm position to the value set by the user > > + * through v4l2_ctrl_ops s_ctrl handler > > + * The lens position is gradually moved in units of > > +DW9714_CTRL_STEPS, > > + * to make the movements smoothly. > > + */ > > +static int dw9714_vcm_resume(struct device *dev) { > > + struct i2c_client *client = to_i2c_client(dev); > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct dw9714_device *dw9714_dev = container_of(sd, > > + struct dw9714_device, > > + sd); > > + int ret, val; > > + > > + for (val = dw9714_dev->current_val % DW9714_CTRL_STEPS; > > + val < dw9714_dev->current_val + DW9714_CTRL_STEPS - 1; > > + val += DW9714_CTRL_STEPS) { > > + ret = dw9714_i2c_write(client, > > + DW9714_VAL((u16) val, > > + DW9714_DEFAULT_S)); > > Ditto. > > > + if (ret) > > + dev_err(dev, "%s I2C failure: %d", __func__, > > + ret); > > Ditto. > > > + usleep_range(DW9714_CTRL_DELAY_US, DW9714_CTRL_DELAY_US > + 10); > > + } > > + > > + /* restore v4l2 control values */ > > + ret = v4l2_ctrl_handler_setup(&dw9714_dev->ctrls_vcm); > > + return ret; > > Hmm, actually I believe v4l2_ctrl_handler_setup() will call .s_ctrl() here and set > the motor value again. If we just make .s_ctrl() do the adjustment in steps > properly, we can simplify the resume to simply call v4l2_ctrl_handler_setup() > alone. > As Sakari suggested, v4l2_ctrl_handler_setup() code is removed, as the preceding code should restore the vcm position prior to suspend > > +} > > #endif > > #ifdef CONFIG_PM > > > + > > +static int dw9714_runtime_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); } > > + > > +static int dw9714_runtime_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); } > > #endif > > #ifdef CONFIG_PM_SLEEP > > > + > > +static int dw9714_suspend(struct device *dev) { > > + return dw9714_vcm_suspend(dev); } > > + > > +static int dw9714_resume(struct device *dev) { > > + return dw9714_vcm_resume(dev); } > > #endif > > Or you could actually just use dw9714_vcm_{suspend,resume}() directly for the > callbacks and avoid the duplicates above. > Ack > > + > > +#else > > + > > +#define dw9714_vcm_suspend NULL > > +#define dw9714_vcm_resume NULL > > This #else block is not needed. > Ack > Best regards, > Tomasz
