Hello Andy,
Thanks for the review. Please see the replies below.
On Mon, 2019-09-05 at 12:26 +0200, Andy Shevchenko wrote:
> On Thu, Sep 05, 2019 at 03:21:42PM +0800, dongchun.zhu@xxxxxxxxxxxx wrote:
> > From: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx>
> >
> > This patch adds a V4L2 sub-device driver for DW9768 lens voice coil,
> > and provides control to set the desired focus.
> >
> > The DW9768 is a 10 bit DAC with 100mA output current sink capability
> > from Dongwoon, designed for linear control of voice coil motor, and
> > controlled via I2C serial interface.
>
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@xxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
>
> This should go to the same patch, where you introduce a record in the MAINTAINERS database.
>
> > +#define DW9768_SET_POSITION_ADDR 0x03
>
> Indentation issue.
>
Fixed in next release.
> > +static struct regval_list dw9768_init_regs[] = {
> > +{0x02, 0x02},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> > +{0x06, 0x41},
> > +{0x07, 0x39},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY}, };
> > +
> > +static struct regval_list dw9768_release_regs[] = {
> > +{0x02, 0x00},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> > +{0x01, 0x00},
> > +{DW9768_CMD_DELAY, DW9768_CMD_DELAY}, };
> > +
> > +static int dw9768_write_smbus(struct dw9768 *dw9768, unsigned char reg,
> > + unsigned char value)
> > +{
> > +struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> > +int ret;
> > +
>
> > +if (reg == DW9768_CMD_DELAY && value == DW9768_CMD_DELAY)
>
> Indentation issue.
> But see other's comments.
>
> > +usleep_range(DW9768_CTRL_DELAY_US,
> > + DW9768_CTRL_DELAY_US + 100);
>
> > +else
>
> This needs an explanation.
>
Fixed in next release.
> > +ret = i2c_smbus_write_byte_data(client, reg, value);
> > +return ret;
> > +}
>
> I'm wondering if we can benefit from regmap I²C API in this driver.
>
> > +static int __maybe_unused dw9768_vcm_suspend(struct device *dev) {
>
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
>
> isn't is simple dev_get_drvdata() ?
>
As discussed in another group mail, even dev_set_drvdata() is set when
probe, the pointer that point to the private device struct changes when
using dev_get_drvdata() API.
Then resume/power on would result in kernel exception in
regulator_enable.
This is so weird.
We filed one Google issue to trace this behavior.
https://partnerissuetracker.corp.google.com/issues/147957975
I am wondering whether there is any condition of the use of
dev_get_drvdata().
I checked the other drivers and found that most devices that use
dev_get_drvdata() are platform devices or non i2c client devices.
So maybe dw9768 or other media/i2c devices cannot support
dev_get_drvdata()?
> > +struct dw9768 *dw9768 = sd_to_dw9768_vcm(sd);
> > +
> > +return dw9768_power_off(dw9768);
> > +}
> > +
> > +static int __maybe_unused dw9768_vcm_resume(struct device *dev) {
>
> > +struct i2c_client *client = to_i2c_client(dev);
> > +struct v4l2_subdev *sd = i2c_get_clientdata(client);
>
> Ditto.
>
> > +struct dw9768 *dw9768 = sd_to_dw9768_vcm(sd);
> > +
> > +return dw9768_power_on(dw9768);
> > +}
>
> > +static const struct i2c_device_id dw9768_id_table[] = {
> > +{ DW9768_NAME, 0 },
>
> > +{ },
>
> No comma.
>
This struct would be removed in next release.
> > +};
>
> > +static const struct of_device_id dw9768_of_table[] = {
> > +{ .compatible = "dongwoon,dw9768" },
>
> > +{ },
>
> Ditto.
>
Fixed in next release.
> > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
> *********************MEDIATEK Confidential/Internal Use*********************
