mbox series

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

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

Message

Dongchun Zhu June 16, 2020, 12:55 p.m. UTC 
Hello,

This series adds DT bindings and V4L2 sub-device driver for Dongwoon's DW9768,
which is a 10-bit DAC with 100mA output current sink capability.

The driver is designed for linear control of voice coil motor(VCM),
and controlled via IIC serial interface to set the desired focus.

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:
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 v8 are addressing comments from Rob, Sakari, Andy, Tomasz.
Compared to v7:
 - Rebase onto 5.8-rc1
 - Calculate move delay time once in probe() and restore it in private struct
 - Use the fwnode property API to read the properties into the filed directly
 - Rename error_async_register to err_power_off and refine it
 - Fix other review comments in v7

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                         | 553 +++++++++++++++++++++
 5 files changed, 674 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
 create mode 100644 drivers/media/i2c/dw9768.c

Comments

Tomasz Figa June 18, 2020, 6:45 p.m. UTC | #1 
On Tue, Jun 16, 2020 at 08:55:31PM +0800, Dongchun Zhu wrote:
> Add a V4L2 sub-device driver for DW9768 voice coil motor,
> providing control to set the desired focus via IIC serial interface.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
>  MAINTAINERS                |   1 +
>  drivers/media/i2c/Kconfig  |  12 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9768.c | 553 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 567 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9768.c
[snip]
> +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->aac_mode = DW9768_AAC_MODE_DEFAULT;
> +	dw9768->aac_timing = DW9768_AAC_TIME_DEFAULT;
> +	dw9768->clock_presc = DW9768_CLOCK_PRE_SCALE_DEFAULT;
> +
> +	/* Optional indication of AAC mode select */
> +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> +				 &dw9768->aac_mode);
> +
> +	/* Optional indication of clock pre-scale select */
> +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,clock-presc",
> +				 &dw9768->clock_presc);
> +
> +	/* Optional indication of AAC Timing */
> +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> +				 &dw9768->aac_timing);
> +
> +	dw9768->move_delay_us = dw9768_cal_move_delay(dw9768->aac_mode,
> +						      dw9768->clock_presc,
> +						      dw9768->aac_timing) / 100;

nit: Could we make the function return the value in us already? One would
expect the function to return the value in a standard unit, so this
division by 100 here is confusing.

> +
> +	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> +		dw9768->supplies[i].supply = dw9768_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> +				      dw9768->supplies);
> +	if (ret) {
> +		dev_err(dev, "failed to get regulators\n");
> +		return ret;
> +	}
> +
> +	/* Initialize controls */
> +	ret = dw9768_init_controls(dw9768);
> +	if (ret)
> +		goto err_free_handler;
> +
> +	/* Initialize subdev */
> +	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	dw9768->sd.internal_ops = &dw9768_int_ops;
> +
> +	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> +	if (ret < 0)
> +		goto err_free_handler;
> +
> +	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;
> +	}
> +
> +	return 0;
> +
> +err_power_off:
> +	pm_runtime_disable(dev);
> +	if (!pm_runtime_enabled(dev))

We just disabled runtime PM in the line above, so this check would be
always true. Need to call pm_runtime_disable() after this if.

> +		dw9768_runtime_suspend(dev);
> +err_clean_entity:
> +	media_entity_cleanup(&dw9768->sd.entity);
> +err_free_handler:
> +	v4l2_ctrl_handler_free(&dw9768->ctrls);
> +
> +	return ret;
> +}
> +
> +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_suspended(&client->dev))

Oops, I just realized that my suggestion about the function to use here
was incorrect. pm_runtime_status_suspended() should be the correct function
here. Sorry for the confusion.

This is because we only have 2 cases here:
 - runtime PM compiled out - the stubs function is used, which returns
   false, so the condition is true,
 - runtime PM compiled in - we enabled runtime PM in probe, so here we
   don't need to consider the enable state.

Best regards,
Tomasz
Dongchun Zhu June 20, 2020, 6:58 a.m. UTC | #2 
Hello Tomasz,

Thanks for the review.

On Thu, 2020-06-18 at 18:45 +0000, Tomasz Figa wrote:
> On Tue, Jun 16, 2020 at 08:55:31PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for DW9768 voice coil motor,
> > providing control to set the desired focus via IIC serial interface.
> > 
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> >  MAINTAINERS                |   1 +
> >  drivers/media/i2c/Kconfig  |  12 +
> >  drivers/media/i2c/Makefile |   1 +
> >  drivers/media/i2c/dw9768.c | 553 +++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 567 insertions(+)
> >  create mode 100644 drivers/media/i2c/dw9768.c
> [snip]
> > +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->aac_mode = DW9768_AAC_MODE_DEFAULT;
> > +	dw9768->aac_timing = DW9768_AAC_TIME_DEFAULT;
> > +	dw9768->clock_presc = DW9768_CLOCK_PRE_SCALE_DEFAULT;
> > +
> > +	/* Optional indication of AAC mode select */
> > +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-mode",
> > +				 &dw9768->aac_mode);
> > +
> > +	/* Optional indication of clock pre-scale select */
> > +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,clock-presc",
> > +				 &dw9768->clock_presc);
> > +
> > +	/* Optional indication of AAC Timing */
> > +	fwnode_property_read_u32(dev_fwnode(dev), "dongwoon,aac-timing",
> > +				 &dw9768->aac_timing);
> > +
> > +	dw9768->move_delay_us = dw9768_cal_move_delay(dw9768->aac_mode,
> > +						      dw9768->clock_presc,
> > +						      dw9768->aac_timing) / 100;
> 
> nit: Could we make the function return the value in us already? One would
> expect the function to return the value in a standard unit, so this
> division by 100 here is confusing.
> 

Good idea.

> > +
> > +	for (i = 0; i < ARRAY_SIZE(dw9768_supply_names); i++)
> > +		dw9768->supplies[i].supply = dw9768_supply_names[i];
> > +
> > +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dw9768_supply_names),
> > +				      dw9768->supplies);
> > +	if (ret) {
> > +		dev_err(dev, "failed to get regulators\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Initialize controls */
> > +	ret = dw9768_init_controls(dw9768);
> > +	if (ret)
> > +		goto err_free_handler;
> > +
> > +	/* Initialize subdev */
> > +	dw9768->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +	dw9768->sd.internal_ops = &dw9768_int_ops;
> > +
> > +	ret = media_entity_pads_init(&dw9768->sd.entity, 0, NULL);
> > +	if (ret < 0)
> > +		goto err_free_handler;
> > +
> > +	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;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_power_off:
> > +	pm_runtime_disable(dev);
> > +	if (!pm_runtime_enabled(dev))
> 
> We just disabled runtime PM in the line above, so this check would be
> always true. Need to call pm_runtime_disable() after this if.
> 

Sorry to make such a mistake.

> > +		dw9768_runtime_suspend(dev);
> > +err_clean_entity:
> > +	media_entity_cleanup(&dw9768->sd.entity);
> > +err_free_handler:
> > +	v4l2_ctrl_handler_free(&dw9768->ctrls);
> > +
> > +	return ret;
> > +}
> > +
> > +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_suspended(&client->dev))
> 
> Oops, I just realized that my suggestion about the function to use here
> was incorrect. pm_runtime_status_suspended() should be the correct function
> here. Sorry for the confusion.
> 
> This is because we only have 2 cases here:
>  - runtime PM compiled out - the stubs function is used, which returns
>    false, so the condition is true,
>  - runtime PM compiled in - we enabled runtime PM in probe, so here we
>    don't need to consider the enable state.
> 

Uh-huh...
Thanks for the explaining.
It seems OV8856, OV5695 and OV2685 also use the API
'pm_runtime_status_suspended'.

> Best regards,
> Tomasz