mbox series

[V10,0/2] media: i2c: Add support for DW9768 VCM

Message ID 20200703080404.29770-1-dongchun.zhu@mediatek.com
Headers show
Series media: i2c: Add support for DW9768 VCM | expand

Message

Dongchun Zhu July 3, 2020, 8:04 a.m. UTC 
Hello,

This series adds DT binding and V4L2 sub-device driver for Dongwoon's DW9768.
DW9768 is a 10-bit DAC with 100mA output current sink capability, designed
for linear control of voice coil motor(VCM).

This driver creates a V4L2 sub-device and provides control to set the desired
focus via II2C serial interface.

It controls the position with 10-bit DAC data D[9:0] and seperates two 8-bit
registers to control the VCM position as belows.
DAC_MSB: D[9:8](ADDR: 0x03):
     +---+---+---+---+---+---+---+---+
     |---|---|---|---|---|---|D09|D08|
     +---+---+---+---+---+---+---+---+
DAC_LSB: D[7:0](ADDR: 0x04):
     +---+---+---+---+---+---+---+---+
     |D07|D06|D05|D04|D03|D02|D01|D00|
     +---+---+---+---+---+---+---+---+

This driver supports:
 - set DW9768 to standby mode once suspend and turn it back to active if resume
 - set the desired focus via V4L2_CID_FOCUS_ABSOLUTE ctrl

Previous versions of this patch-set can be found here:
 v9: https://lore.kernel.org/linux-media/20200630062211.22988-1-dongchun.zhu@mediatek.com/
 v8: https://lore.kernel.org/linux-media/20200616125531.31671-1-dongchun.zhu@mediatek.com/
 v7: https://lore.kernel.org/linux-media/20200605105412.18813-1-dongchun.zhu@mediatek.com/
 v6: https://lore.kernel.org/linux-media/20200518132731.20855-1-dongchun.zhu@mediatek.com/
 v5: https://lore.kernel.org/linux-media/20200502161727.30463-1-dongchun.zhu@mediatek.com/
 v4: https://lore.kernel.org/linux-media/20200330123634.363-1-dongchun.zhu@mediatek.com/
 v3: https://lore.kernel.org/linux-media/20200228155958.20657-1-dongchun.zhu@mediatek.com/
 v2: https://lore.kernel.org/linux-media/20190905072142.14606-1-dongchun.zhu@mediatek.com/
 v1: https://lore.kernel.org/linux-media/20190708100641.2702-1-dongchun.zhu@mediatek.com/

Mainly changes of v10 address comments from Sakari, Tomasz, Bingbu.
Compared to v9:
 - Add reviewed-by for VCM driver
 - Re-send plain patch to test if there are still weird trailing whitespaces

Please review.
Thanks.

Dongchun Zhu (2):
  media: dt-bindings: media: i2c: Document DW9768 bindings
  media: i2c: dw9768: Add DW9768 VCM driver

 .../bindings/media/i2c/dongwoon,dw9768.yaml        | 100 ++++
 MAINTAINERS                                        |   8 +
 drivers/media/i2c/Kconfig                          |  12 +
 drivers/media/i2c/Makefile                         |   1 +
 drivers/media/i2c/dw9768.c                         | 554 +++++++++++++++++++++
 5 files changed, 675 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
 create mode 100644 drivers/media/i2c/dw9768.c

Comments

Sergey Senozhatsky Sept. 2, 2020, 5:35 a.m. UTC | #1 
On (20/07/03 16:04), Dongchun Zhu wrote:
[..]
> +static int dw9768_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct dw9768 *dw9768;
> +	unsigned int i;
> +	int ret;
> +
> +	dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> +	if (!dw9768)
> +		return -ENOMEM;
> +
> +	/* Initialize subdev */
> +	v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> +
[..]
> +	dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	pm_runtime_enable(dev);
> +	if (!pm_runtime_enabled(dev)) {
> +		ret = dw9768_runtime_resume(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to power on: %d\n", ret);
> +			goto err_clean_entity;
> +		}
> +	}
> +
> +	ret = v4l2_async_register_subdev(&dw9768->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> +		goto err_power_off;
> +	}

I would expect to see a slightly different order here: first set
everything up, then expose the device to PM subsystem.

[..]
> +static int dw9768_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9768 *dw9768 = sd_to_dw9768(sd);
> +
> +	v4l2_async_unregister_subdev(&dw9768->sd);
> +	v4l2_ctrl_handler_free(&dw9768->ctrls);
> +	media_entity_cleanup(&dw9768->sd.entity);
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		dw9768_runtime_suspend(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);

Ditto. Shall we first disable PM (so that we won't get any unexpected
PM callbacks) and then destroy the device?

	-ss
Tomasz Figa Sept. 2, 2020, 12:38 p.m. UTC | #2 
On Wed, Sep 2, 2020 at 7:35 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (20/07/03 16:04), Dongchun Zhu wrote:
> [..]
> > +static int dw9768_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct dw9768 *dw9768;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > +     if (!dw9768)
> > +             return -ENOMEM;
> > +
> > +     /* Initialize subdev */
> > +     v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > +
> [..]
> > +     dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > +     pm_runtime_enable(dev);
> > +     if (!pm_runtime_enabled(dev)) {
> > +             ret = dw9768_runtime_resume(dev);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to power on: %d\n", ret);
> > +                     goto err_clean_entity;
> > +             }
> > +     }
> > +
> > +     ret = v4l2_async_register_subdev(&dw9768->sd);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > +             goto err_power_off;
> > +     }
>
> I would expect to see a slightly different order here: first set
> everything up, then expose the device to PM subsystem.
>

Thanks for chiming in. However, the code above is correct. Runtime PM
callbacks don't usually rely on the availability of any
userspace-facing components, while the opposite is true - when the
device is registered to the userspace, it must have all the
initialization aspects completed and that includes initialization of
runtime PM.

> [..]
> > +static int dw9768_remove(struct i2c_client *client)
> > +{
> > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +     struct dw9768 *dw9768 = sd_to_dw9768(sd);
> > +
> > +     v4l2_async_unregister_subdev(&dw9768->sd);
> > +     v4l2_ctrl_handler_free(&dw9768->ctrls);
> > +     media_entity_cleanup(&dw9768->sd.entity);
> > +     pm_runtime_disable(&client->dev);
> > +     if (!pm_runtime_status_suspended(&client->dev))
> > +             dw9768_runtime_suspend(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
>
> Ditto. Shall we first disable PM (so that we won't get any unexpected
> PM callbacks) and then destroy the device?

Ditto. +/- the opposite order

Best regards,
Tomasz