| Message ID | 1527242135-22866-2-git-send-email-bingbu.cao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bingbu, A few comments below. On Fri, May 25, 2018 at 05:55:35PM +0800, bingbu.cao@intel.com wrote: > From: Bingbu Cao <bingbu.cao@intel.com> > > Add a V4L2 sub-device driver for the ak7375 lens voice coil. > This is a voice coil module using the I2C bus to control the > focus position. > > Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> > Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> > --- > MAINTAINERS | 8 ++ > drivers/media/i2c/Kconfig | 10 ++ > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ak7375.c | 278 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 297 insertions(+) > create mode 100644 drivers/media/i2c/ak7375.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ea362219c4aa..20379a7baca0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -625,6 +625,14 @@ T: git git://linuxtv.org/anttip/media_tree.git > S: Maintained > F: drivers/media/usb/airspy/ > > +AKM AK7375 LENS VOICE COIL DRIVER > +M: Tianshu Qiu <tian.shu.qiu@intel.com> > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/ak7375.c > +F: Documentation/devicetree/bindings/media/i2c/akm,ak7375.txt The name of the file also needs to match. Currently it doesn't. How about "asahi-kasei,ak7375.txt"? Could you also move the MAINTAINERS entry to the patch adding the DT bindings? > + > ALACRITECH GIGABIT ETHERNET DRIVER > M: Lino Sanfilippo <LinoSanfilippo@gmx.de> > S: Maintained > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 341452fe98df..ff3cb5afb0e1 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -326,6 +326,16 @@ config VIDEO_AD5820 > This is a driver for the AD5820 camera lens voice coil. > It is used for example in Nokia N900 (RX-51). > > +config VIDEO_AK7375 > + tristate "AK7375 lens voice coil support" > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER > + depends on VIDEO_V4L2_SUBDEV_API > + help > + This is a driver for the AK7375 camera lens voice coil. > + AK7375 is a 12 bit DAC with 120mA output current sink > + capability. This is designed for linear control of > + voice coil motors, controlled via I2C serial interface. > + > config VIDEO_DW9714 > tristate "DW9714 lens voice coil support" > depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index d679d57cd3b3..05b97e319ea9 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -23,6 +23,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o > obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o > obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o > obj-$(CONFIG_VIDEO_AD5820) += ad5820.o > +obj-$(CONFIG_VIDEO_AK7375) += ak7375.o > obj-$(CONFIG_VIDEO_DW9714) += dw9714.o > obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o > obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o > diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c > new file mode 100644 > index 000000000000..012e673e9ced > --- /dev/null > +++ b/drivers/media/i2c/ak7375.c ... > +static const struct of_device_id ak7375_of_table[] = { > + { .compatible = "akm,ak7375" }, "asahi-kasei,ak7375"
On Fri, May 25, 2018 at 05:55:35PM +0800, bingbu.cao@intel.com wrote: > +static int ak7375_i2c_write(struct ak7375_device *ak7375, > + u8 addr, u16 data, int size) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ak7375->sd); > + int ret; > + u8 buf[3]; > + > + if (size != 1 && size != 2) > + return -EINVAL; > + buf[0] = addr; > + buf[2] = data & 0xff; > + if (size == 2) > + buf[1] = data >> 8; > + ret = i2c_master_send(client, (const char *)buf, size + 1); I don't have a data datasheet for this thing, but it looks like buf[1] will be undefined for writes the size of which is 1. And this what appears to be written to the device as well... > + if (ret < 0) > + return ret; > + if (ret != size + 1) > + return -EIO; > + return 0; > +}
On 2018年06月01日 17:34, Sakari Ailus wrote: > Hi Bingbu, > > A few comments below. > > On Fri, May 25, 2018 at 05:55:35PM +0800, bingbu.cao@intel.com wrote: >> From: Bingbu Cao <bingbu.cao@intel.com> >> >> Add a V4L2 sub-device driver for the ak7375 lens voice coil. >> This is a voice coil module using the I2C bus to control the >> focus position. >> >> Signed-off-by: Tianshu Qiu <tian.shu.qiu@intel.com> >> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com> >> --- >> MAINTAINERS | 8 ++ >> drivers/media/i2c/Kconfig | 10 ++ >> drivers/media/i2c/Makefile | 1 + >> drivers/media/i2c/ak7375.c | 278 +++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 297 insertions(+) >> create mode 100644 drivers/media/i2c/ak7375.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index ea362219c4aa..20379a7baca0 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -625,6 +625,14 @@ T: git git://linuxtv.org/anttip/media_tree.git >> S: Maintained >> F: drivers/media/usb/airspy/ >> >> +AKM AK7375 LENS VOICE COIL DRIVER >> +M: Tianshu Qiu <tian.shu.qiu@intel.com> >> +L: linux-media@vger.kernel.org >> +T: git git://linuxtv.org/media_tree.git >> +S: Maintained >> +F: drivers/media/i2c/ak7375.c >> +F: Documentation/devicetree/bindings/media/i2c/akm,ak7375.txt > The name of the file also needs to match. Currently it doesn't. How about > "asahi-kasei,ak7375.txt"? Ack. Does it make sense if just change the compatible string to "asahi-kasei,ak7375" and keep the file name unchanged? I have no clear idea about the DT binding rules. > > Could you also move the MAINTAINERS entry to the patch adding the DT > bindings? Ack. > >> + >> ALACRITECH GIGABIT ETHERNET DRIVER >> M: Lino Sanfilippo <LinoSanfilippo@gmx.de> >> S: Maintained >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index 341452fe98df..ff3cb5afb0e1 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -326,6 +326,16 @@ config VIDEO_AD5820 >> This is a driver for the AD5820 camera lens voice coil. >> It is used for example in Nokia N900 (RX-51). >> >> +config VIDEO_AK7375 >> + tristate "AK7375 lens voice coil support" >> + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER >> + depends on VIDEO_V4L2_SUBDEV_API >> + help >> + This is a driver for the AK7375 camera lens voice coil. >> + AK7375 is a 12 bit DAC with 120mA output current sink >> + capability. This is designed for linear control of >> + voice coil motors, controlled via I2C serial interface. >> + >> config VIDEO_DW9714 >> tristate "DW9714 lens voice coil support" >> depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index d679d57cd3b3..05b97e319ea9 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -23,6 +23,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o >> obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o >> obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o >> obj-$(CONFIG_VIDEO_AD5820) += ad5820.o >> +obj-$(CONFIG_VIDEO_AK7375) += ak7375.o >> obj-$(CONFIG_VIDEO_DW9714) += dw9714.o >> obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o >> obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o >> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c >> new file mode 100644 >> index 000000000000..012e673e9ced >> --- /dev/null >> +++ b/drivers/media/i2c/ak7375.c > ... > >> +static const struct of_device_id ak7375_of_table[] = { >> + { .compatible = "akm,ak7375" }, > "asahi-kasei,ak7375" >
On Fri, Jun 01, 2018 at 05:54:30PM +0800, Bing Bu Cao wrote: > > > +AKM AK7375 LENS VOICE COIL DRIVER > > > +M: Tianshu Qiu <tian.shu.qiu@intel.com> > > > +L: linux-media@vger.kernel.org > > > +T: git git://linuxtv.org/media_tree.git > > > +S: Maintained > > > +F: drivers/media/i2c/ak7375.c > > > +F: Documentation/devicetree/bindings/media/i2c/akm,ak7375.txt > > The name of the file also needs to match. Currently it doesn't. How about > > "asahi-kasei,ak7375.txt"? > Ack. > Does it make sense if just change the compatible string to > "asahi-kasei,ak7375" and keep the file name unchanged? Most binding files in the directory seem to use the chip name rather than the full compatible string (including the vendor). I guess both are fine.
On 2018年06月01日 17:42, Sakari Ailus wrote: > On Fri, May 25, 2018 at 05:55:35PM +0800, bingbu.cao@intel.com wrote: >> +static int ak7375_i2c_write(struct ak7375_device *ak7375, >> + u8 addr, u16 data, int size) >> +{ >> + struct i2c_client *client = v4l2_get_subdevdata(&ak7375->sd); >> + int ret; >> + u8 buf[3]; >> + >> + if (size != 1 && size != 2) >> + return -EINVAL; >> + buf[0] = addr; >> + buf[2] = data & 0xff; >> + if (size == 2) >> + buf[1] = data >> 8; >> + ret = i2c_master_send(client, (const char *)buf, size + 1); > I don't have a data datasheet for this thing, but it looks like buf[1] will > be undefined for writes the size of which is 1. And this what appears to be > written to the device as well... I check the datasheet once again and find out that the logic here for size==1 is not correct, I will change it in next version. Thanks. Here is the write section in datasheet: ------ After receiving the second byte (register address), AK7375 generates an acknowledge then receives the third byte. The third and the following bytes represent control data. Control data consists of 8 bits and is based on the MSB-first configuration. AK7375 can write multiple bytes of data at a time. After reception of the third byte (control data), AK7375 generates an acknowledge then receives the next data. If additional data is received instead of the stop condition after receiving one byte of data, the address counter inside the LSI chip is automatically incremented and the data is written at the next address. ------ > >> + if (ret < 0) >> + return ret; >> + if (ret != size + 1) >> + return -EIO; >> + return 0; >> +}
diff --git a/MAINTAINERS b/MAINTAINERS index ea362219c4aa..20379a7baca0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -625,6 +625,14 @@ T: git git://linuxtv.org/anttip/media_tree.git S: Maintained F: drivers/media/usb/airspy/ +AKM AK7375 LENS VOICE COIL DRIVER +M: Tianshu Qiu <tian.shu.qiu@intel.com> +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +S: Maintained +F: drivers/media/i2c/ak7375.c +F: Documentation/devicetree/bindings/media/i2c/akm,ak7375.txt + ALACRITECH GIGABIT ETHERNET DRIVER M: Lino Sanfilippo <LinoSanfilippo@gmx.de> S: Maintained diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 341452fe98df..ff3cb5afb0e1 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -326,6 +326,16 @@ config VIDEO_AD5820 This is a driver for the AD5820 camera lens voice coil. It is used for example in Nokia N900 (RX-51). +config VIDEO_AK7375 + tristate "AK7375 lens voice coil support" + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER + depends on VIDEO_V4L2_SUBDEV_API + help + This is a driver for the AK7375 camera lens voice coil. + AK7375 is a 12 bit DAC with 120mA output current sink + capability. This is designed for linear control of + voice coil motors, controlled via I2C serial interface. + config VIDEO_DW9714 tristate "DW9714 lens voice coil support" depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index d679d57cd3b3..05b97e319ea9 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -23,6 +23,7 @@ obj-$(CONFIG_VIDEO_SAA7127) += saa7127.o obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o obj-$(CONFIG_VIDEO_AD5820) += ad5820.o +obj-$(CONFIG_VIDEO_AK7375) += ak7375.o obj-$(CONFIG_VIDEO_DW9714) += dw9714.o obj-$(CONFIG_VIDEO_ADV7170) += adv7170.o obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c new file mode 100644 index 000000000000..012e673e9ced --- /dev/null +++ b/drivers/media/i2c/ak7375.c @@ -0,0 +1,278 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Intel Corporation + +#include <linux/acpi.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> + +#define AK7375_MAX_FOCUS_POS 4095 +/* + * This sets the minimum granularity for the focus positions. + * A value of 1 gives maximum accuracy for a desired focus position + */ +#define AK7375_FOCUS_STEPS 1 +/* + * This acts as the minimum granularity of lens movement. + * Keep this value power of 2, so the control steps can be + * uniformly adjusted for gradual lens movement, with desired + * number of control steps. + */ +#define AK7375_CTRL_STEPS 64 +#define AK7375_CTRL_DELAY_US 1000 + +#define AK7375_REG_POSITION 0x0 +#define AK7375_REG_CONT 0x2 +#define AK7375_MODE_ACTIVE 0x0 +#define AK7375_MODE_STANDBY 0x40 + +/* ak7375 device structure */ +struct ak7375_device { + struct v4l2_ctrl_handler ctrls_vcm; + struct v4l2_subdev sd; + struct v4l2_ctrl *focus; +}; + +static inline struct ak7375_device *to_ak7375_vcm(struct v4l2_ctrl *ctrl) +{ + return container_of(ctrl->handler, struct ak7375_device, ctrls_vcm); +} + +static inline struct ak7375_device *sd_to_ak7375_vcm(struct v4l2_subdev *subdev) +{ + return container_of(subdev, struct ak7375_device, sd); +} + +static int ak7375_i2c_write(struct ak7375_device *ak7375, + u8 addr, u16 data, int size) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ak7375->sd); + int ret; + u8 buf[3]; + + if (size != 1 && size != 2) + return -EINVAL; + buf[0] = addr; + buf[2] = data & 0xff; + if (size == 2) + buf[1] = data >> 8; + ret = i2c_master_send(client, (const char *)buf, size + 1); + if (ret < 0) + return ret; + if (ret != size + 1) + return -EIO; + return 0; +} + +static int ak7375_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct ak7375_device *dev_vcm = to_ak7375_vcm(ctrl); + + if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE) + return ak7375_i2c_write(dev_vcm, AK7375_REG_POSITION, + ctrl->val << 4, 2); + + return -EINVAL; +} + +static const struct v4l2_ctrl_ops ak7375_vcm_ctrl_ops = { + .s_ctrl = ak7375_set_ctrl, +}; + +static int ak7375_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + int rval; + + rval = pm_runtime_get_sync(sd->dev); + if (rval < 0) { + pm_runtime_put_noidle(sd->dev); + return rval; + } + + return 0; +} + +static int ak7375_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + pm_runtime_put(sd->dev); + + return 0; +} + +static const struct v4l2_subdev_internal_ops ak7375_int_ops = { + .open = ak7375_open, + .close = ak7375_close, +}; + +static const struct v4l2_subdev_ops ak7375_ops = { }; + +static void ak7375_subdev_cleanup(struct ak7375_device *ak7375_dev) +{ + v4l2_async_unregister_subdev(&ak7375_dev->sd); + v4l2_ctrl_handler_free(&ak7375_dev->ctrls_vcm); + media_entity_cleanup(&ak7375_dev->sd.entity); +} + +static int ak7375_init_controls(struct ak7375_device *dev_vcm) +{ + struct v4l2_ctrl_handler *hdl = &dev_vcm->ctrls_vcm; + const struct v4l2_ctrl_ops *ops = &ak7375_vcm_ctrl_ops; + + v4l2_ctrl_handler_init(hdl, 1); + + dev_vcm->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, + 0, AK7375_MAX_FOCUS_POS, AK7375_FOCUS_STEPS, 0); + + if (hdl->error) + dev_err(dev_vcm->sd.dev, "%s fail error: 0x%x\n", + __func__, hdl->error); + dev_vcm->sd.ctrl_handler = hdl; + return hdl->error; +} + +static int ak7375_probe(struct i2c_client *client) +{ + struct ak7375_device *ak7375_dev; + int val; + + ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev), + GFP_KERNEL); + if (!ak7375_dev) + return -ENOMEM; + + v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops); + ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + ak7375_dev->sd.internal_ops = &ak7375_int_ops; + ak7375_dev->sd.entity.function = MEDIA_ENT_F_LENS; + + val = ak7375_init_controls(ak7375_dev); + if (val) + goto err_cleanup; + + val = media_entity_pads_init(&ak7375_dev->sd.entity, 0, NULL); + if (val < 0) + goto err_cleanup; + + val = v4l2_async_register_subdev(&ak7375_dev->sd); + if (val < 0) + goto err_cleanup; + + pm_runtime_set_active(&client->dev); + pm_runtime_enable(&client->dev); + pm_runtime_idle(&client->dev); + + return 0; + +err_cleanup: + v4l2_ctrl_handler_free(&ak7375_dev->ctrls_vcm); + media_entity_cleanup(&ak7375_dev->sd.entity); + dev_err(&client->dev, "Probe failed: %d\n", val); + return val; +} + +static int ak7375_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd); + + ak7375_subdev_cleanup(ak7375_dev); + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + + return 0; +} + +/* + * This function sets the vcm position, so it consumes least current + * The lens position is gradually moved in units of AK7375_CTRL_STEPS, + * to make the movements smoothly. + */ +static int __maybe_unused ak7375_vcm_suspend(struct device *dev) +{ + + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd); + int ret, val; + + for (val = ak7375_dev->focus->val & ~(AK7375_CTRL_STEPS - 1); + val >= 0; val -= AK7375_CTRL_STEPS) { + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_POSITION, + val << 4, 2); + if (ret) + dev_err_once(dev, "%s I2C failure: %d\n", + __func__, ret); + usleep_range(AK7375_CTRL_DELAY_US, AK7375_CTRL_DELAY_US + 10); + } + + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, + AK7375_MODE_STANDBY, 1); + if (ret) { + dev_err(dev, "%s I2C failure: %d\n", __func__, ret); + return ret; + } + 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 AK7375_CTRL_STEPS, + * to make the movements smoothly. + */ +static int __maybe_unused ak7375_vcm_resume(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ak7375_device *ak7375_dev = sd_to_ak7375_vcm(sd); + int ret, val; + + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, + AK7375_MODE_ACTIVE, 1); + if (ret) { + dev_err(dev, "%s I2C failure: %d\n", __func__, ret); + return ret; + } + + for (val = ak7375_dev->focus->val % AK7375_CTRL_STEPS; + val <= ak7375_dev->focus->val; + val += AK7375_CTRL_STEPS) { + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_POSITION, + val << 4, 2); + if (ret) + dev_err_ratelimited(dev, "%s I2C failure: %d\n", + __func__, ret); + usleep_range(AK7375_CTRL_DELAY_US, AK7375_CTRL_DELAY_US + 10); + } + return 0; +} + +static const struct of_device_id ak7375_of_table[] = { + { .compatible = "akm,ak7375" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, ak7375_of_table); + +static const struct dev_pm_ops ak7375_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(ak7375_vcm_suspend, ak7375_vcm_resume) + SET_RUNTIME_PM_OPS(ak7375_vcm_suspend, ak7375_vcm_resume, NULL) +}; + +static struct i2c_driver ak7375_i2c_driver = { + .driver = { + .name = "ak7375", + .pm = &ak7375_pm_ops, + .of_match_table = ak7375_of_table, + }, + .probe_new = ak7375_probe, + .remove = ak7375_remove, +}; +module_i2c_driver(ak7375_i2c_driver); + +MODULE_AUTHOR("Tianshu Qiu <tian.shu.qiu@intel.com>"); +MODULE_AUTHOR("Bingbu Cao <bingbu.cao@intel.com>"); +MODULE_DESCRIPTION("AK7375 VCM driver"); +MODULE_LICENSE("GPL v2");