Re: [PATCHv3] support for AD5820 camera auto-focus coil
On 24.05.2016 23:20, Pavel Machek wrote: Hi! devm_regulator_get()? I'd rather avoid devm_ here. Driver is simple enough to allow it. Now thinking about it, what would happen here if regulator_get() returns -EPROBE_DEFER? Wouldn't it be better to move regulator_get to the probe() function, something like: Ok, I can do it. Oh, and don't try to complain about newlines before returns. It looks better this way. All I complain about is missing empty line before a final return at the end of a function :). I guess it is a matter of preference though. static int ad5820_probe(struct i2c_client *client, const struct i2c_device_id *devid) { struct ad5820_device *coil; int ret = 0; coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); if (coil == NULL) return -ENOMEM; coil->vana = devm_regulator_get(&client->dev, NULL); if (IS_ERR(coil->vana)) { ret = PTR_ERR(coil->vana); if (ret != -EPROBE_DEFER) dev_err(&client->dev, "could not get regulator for vana\n"); return ret; } mutex_init(&coil->power_lock); ... with the appropriate changes to remove() because of the devm API usage. Something like this? Didn't try to apply it, but yes, looks right. Besides the missing mutex_destroy(&coil->power_lock); pointed out by Sakari. diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c index f956bd3..f871366 100644 --- a/drivers/media/i2c/ad5820.c +++ b/drivers/media/i2c/ad5820.c @@ -8,7 +8,7 @@ * Copyright (C) 2016 Pavel Machek * * Contact: Tuukka Toivonen - * Sakari Ailus + * Sakari Ailus * * Based on af_d88.c by Texas Instruments. * @@ -263,13 +263,6 @@ static int ad5820_init_controls(struct ad5820_device *coil) static int ad5820_registered(struct v4l2_subdev *subdev) { struct ad5820_device *coil = to_ad5820_device(subdev); - struct i2c_client *client = v4l2_get_subdevdata(subdev); - - coil->vana = regulator_get(&client->dev, "VANA"); - if (IS_ERR(coil->vana)) { - dev_err(&client->dev, "could not get regulator for vana\n"); - return -ENODEV; - } return ad5820_init_controls(coil); } @@ -367,10 +360,18 @@ static int ad5820_probe(struct i2c_client *client, struct ad5820_device *coil; int ret = 0; - coil = kzalloc(sizeof(*coil), GFP_KERNEL); + coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); if (!coil) return -ENOMEM; + coil->vana = devm_regulator_get(&client->dev, NULL); + if (IS_ERR(coil->vana)) { + ret = PTR_ERR(coil->vana); + if (ret != -EPROBE_DEFER) + dev_err(&client->dev, "could not get regulator for vana\n"); + return ret; + } + mutex_init(&coil->power_lock); v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops); @@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client, cleanup: media_entity_cleanup(&coil->subdev.entity); - -free: - kfree(coil); - return ret; } @@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client) v4l2_device_unregister_subdev(&coil->subdev); v4l2_ctrl_handler_free(&coil->ctrls); media_entity_cleanup(&coil->subdev.entity); - if (coil->vana) - regulator_put(coil->vana); - - kfree(coil); - return 0; }
Re: [PATCHv3] support for AD5820 camera auto-focus coil
Hi! > >>devm_regulator_get()? > > > >I'd rather avoid devm_ here. Driver is simple enough to allow it. > > > > Now thinking about it, what would happen here if regulator_get() returns > -EPROBE_DEFER? Wouldn't it be better to move regulator_get to the probe() > function, something like: Ok, I can do it. Oh, and don't try to complain about newlines before returns. It looks better this way. > static int ad5820_probe(struct i2c_client *client, > const struct i2c_device_id *devid) > { > struct ad5820_device *coil; > int ret = 0; > > coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); > if (coil == NULL) > return -ENOMEM; > > coil->vana = devm_regulator_get(&client->dev, NULL); > if (IS_ERR(coil->vana)) { > ret = PTR_ERR(coil->vana); > if (ret != -EPROBE_DEFER) > dev_err(&client->dev, "could not get regulator for > vana\n"); > return ret; > } > > mutex_init(&coil->power_lock); > ... > > with the appropriate changes to remove() because of the devm API > usage. Something like this? diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c index f956bd3..f871366 100644 --- a/drivers/media/i2c/ad5820.c +++ b/drivers/media/i2c/ad5820.c @@ -8,7 +8,7 @@ * Copyright (C) 2016 Pavel Machek * * Contact: Tuukka Toivonen - * Sakari Ailus + * Sakari Ailus * * Based on af_d88.c by Texas Instruments. * @@ -263,13 +263,6 @@ static int ad5820_init_controls(struct ad5820_device *coil) static int ad5820_registered(struct v4l2_subdev *subdev) { struct ad5820_device *coil = to_ad5820_device(subdev); - struct i2c_client *client = v4l2_get_subdevdata(subdev); - - coil->vana = regulator_get(&client->dev, "VANA"); - if (IS_ERR(coil->vana)) { - dev_err(&client->dev, "could not get regulator for vana\n"); - return -ENODEV; - } return ad5820_init_controls(coil); } @@ -367,10 +360,18 @@ static int ad5820_probe(struct i2c_client *client, struct ad5820_device *coil; int ret = 0; - coil = kzalloc(sizeof(*coil), GFP_KERNEL); + coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); if (!coil) return -ENOMEM; + coil->vana = devm_regulator_get(&client->dev, NULL); + if (IS_ERR(coil->vana)) { + ret = PTR_ERR(coil->vana); + if (ret != -EPROBE_DEFER) + dev_err(&client->dev, "could not get regulator for vana\n"); + return ret; + } + mutex_init(&coil->power_lock); v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops); @@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client, cleanup: media_entity_cleanup(&coil->subdev.entity); - -free: - kfree(coil); - return ret; } @@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client) v4l2_device_unregister_subdev(&coil->subdev); v4l2_ctrl_handler_free(&coil->ctrls); media_entity_cleanup(&coil->subdev.entity); - if (coil->vana) - regulator_put(coil->vana); - - kfree(coil); - return 0; } -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCHv3] support for AD5820 camera auto-focus coil
On 24.05.2016 12:04, Pavel Machek wrote: Hi! +static int ad5820_registered(struct v4l2_subdev *subdev) +{ + struct ad5820_device *coil = to_ad5820_device(subdev); + struct i2c_client *client = v4l2_get_subdevdata(subdev); + + coil->vana = regulator_get(&client->dev, "VANA"); devm_regulator_get()? I'd rather avoid devm_ here. Driver is simple enough to allow it. Now thinking about it, what would happen here if regulator_get() returns -EPROBE_DEFER? Wouldn't it be better to move regulator_get to the probe() function, something like: static int ad5820_probe(struct i2c_client *client, const struct i2c_device_id *devid) { struct ad5820_device *coil; int ret = 0; coil = devm_kzalloc(sizeof(*coil), GFP_KERNEL); if (coil == NULL) return -ENOMEM; coil->vana = devm_regulator_get(&client->dev, NULL); if (IS_ERR(coil->vana)) { ret = PTR_ERR(coil->vana); if (ret != -EPROBE_DEFER) dev_err(&client->dev, "could not get regulator for vana\n"); return ret; } mutex_init(&coil->power_lock); ... with the appropriate changes to remove() because of the devm API usage. +#define AD5820_RAMP_MODE_LINEAR(0 << 3) +#define AD5820_RAMP_MODE_64_16 (1 << 3) + +struct ad5820_platform_data { + int (*set_xshutdown)(struct v4l2_subdev *subdev, int set); +}; + +#define to_ad5820_device(sd) container_of(sd, struct ad5820_device, subdev) + +struct ad5820_device { + struct v4l2_subdev subdev; + struct ad5820_platform_data *platform_data; + struct regulator *vana; + + struct v4l2_ctrl_handler ctrls; + u32 focus_absolute; + u32 focus_ramp_time; + u32 focus_ramp_mode; + + struct mutex power_lock; + int power_count; + + int standby : 1; +}; + The same for struct ad5820_device, is it really part of the public API? Let me check what can be done with it. Pavel
Re: [PATCHv3] support for AD5820 camera auto-focus coil
Hi! > >+static int ad5820_registered(struct v4l2_subdev *subdev) > >+{ > >+struct ad5820_device *coil = to_ad5820_device(subdev); > >+struct i2c_client *client = v4l2_get_subdevdata(subdev); > >+ > >+coil->vana = regulator_get(&client->dev, "VANA"); > > devm_regulator_get()? I'd rather avoid devm_ here. Driver is simple enough to allow it. > >+#define AD5820_RAMP_MODE_LINEAR (0 << 3) > >+#define AD5820_RAMP_MODE_64_16 (1 << 3) > >+ > >+struct ad5820_platform_data { > >+int (*set_xshutdown)(struct v4l2_subdev *subdev, int set); > >+}; > >+ > >+#define to_ad5820_device(sd)container_of(sd, struct ad5820_device, > >subdev) > >+ > >+struct ad5820_device { > >+struct v4l2_subdev subdev; > >+struct ad5820_platform_data *platform_data; > >+struct regulator *vana; > >+ > >+struct v4l2_ctrl_handler ctrls; > >+u32 focus_absolute; > >+u32 focus_ramp_time; > >+u32 focus_ramp_mode; > >+ > >+struct mutex power_lock; > >+int power_count; > >+ > >+int standby : 1; > >+}; > >+ > > The same for struct ad5820_device, is it really part of the public API? Let me check what can be done with it. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCHv3] support for AD5820 camera auto-focus coil
On Saturday 21 May 2016 14:43:43 Ivaylo Dimitrov wrote: > >diff --git a/include/media/ad5820.h b/include/media/ad5820.h > >new file mode 100644 > >index 000..f5a1565 > >--- /dev/null > >+++ b/include/media/ad5820.h > >@@ -0,0 +1,70 @@ > >+/* > >+ * include/media/ad5820.h > >+ * > >+ * Copyright (C) 2008 Nokia Corporation > >+ * Copyright (C) 2007 Texas Instruments > >+ * > >+ * Contact: Tuukka Toivonen > >+ * Sakari Ailus > >+ * > >+ * Based on af_d88.c by Texas Instruments. > >+ * > >+ * This program is free software; you can redistribute it and/or > >+ * modify it under the terms of the GNU General Public License > >+ * version 2 as published by the Free Software Foundation. > >+ * > >+ * This program is distributed in the hope that it will be useful, but > >+ * WITHOUT ANY WARRANTY; without even the implied warranty of > >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > >+ * General Public License for more details. > >+ * > >+ * You should have received a copy of the GNU General Public License > >+ * along with this program; if not, write to the Free Software > >+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA > >+ * 02110-1301 USA > >+ */ > >+ > >+#ifndef AD5820_H > >+#define AD5820_H > >+ > >+#include > >+#include > >+#include > >+ > >+#include > >+#include > >+ > >+struct regulator; > >+ > >+#define AD5820_NAME "ad5820" > >+#define AD5820_I2C_ADDR (0x18 >> 1) Maybe write I2C address is more readable form? What is reason such bit shift format? > >+/* Register definitions */ > >+#define AD5820_POWER_DOWN (1 << 15) > >+#define AD5820_DAC_SHIFT4 > > Do those defines really belong here? Isn't it better if they are moved to > ad5820.c? For me it looks like this is private for ad5820.c. > >+#define AD5820_RAMP_MODE_LINEAR (0 << 3) > >+#define AD5820_RAMP_MODE_64_16 (1 << 3) > >+ > >+struct ad5820_platform_data { > >+int (*set_xshutdown)(struct v4l2_subdev *subdev, int set); > >+}; This is for legacy board code support right? We need DT support for N900 as legacy board code is going to be deleted. > >+#define to_ad5820_device(sd)container_of(sd, struct ad5820_device, > >subdev) > >+ > >+struct ad5820_device { > >+struct v4l2_subdev subdev; > >+struct ad5820_platform_data *platform_data; > >+struct regulator *vana; > >+ > >+struct v4l2_ctrl_handler ctrls; > >+u32 focus_absolute; > >+u32 focus_ramp_time; > >+u32 focus_ramp_mode; > >+ > >+struct mutex power_lock; > >+int power_count; > >+ > >+int standby : 1; > >+}; > >+ > > The same for struct ad5820_device, is it really part of the public API? Yes, this is also private for ad5820.c > >+#endif /* AD5820_H */ > > > > > > -- Pali Rohár pali.ro...@gmail.com
Re: [PATCHv3] support for AD5820 camera auto-focus coil
Hi, On 21.05.2016 13:56, Pavel Machek wrote: This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone. Signed-off-by: Pavel Machek --- v2: simple cleanups, fix error paths, simplify probe v3: more cleanups, remove printk, add include diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 993dc50..77313a1 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -279,6 +279,13 @@ config VIDEO_ML86V7667 To compile this driver as a module, choose M here: the module will be called ml86v7667. +config VIDEO_AD5820 + tristate "AD5820 lens voice coil support" + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER + ---help--- + This is a driver for the AD5820 camera lens voice coil. + It is used for example in Nokia N900 (RX-51). + config VIDEO_SAA7110 tristate "Philips SAA7110 video decoder" depends on VIDEO_V4L2 && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 94f2c99..34434ae 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -19,6 +20,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o 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_ADV7170) += adv7170.o obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c new file mode 100644 index 000..7725829 --- /dev/null +++ b/drivers/media/i2c/ad5820.c @@ -0,0 +1,416 @@ +/* + * drivers/media/i2c/ad5820.c + * + * AD5820 DAC driver for camera voice coil focus. + * + * Copyright (C) 2008 Nokia Corporation + * Copyright (C) 2007 Texas Instruments + * Copyright (C) 2016 Pavel Machek + * + * Contact: Tuukka Toivonen + * Sakari Ailus + * + * Based on af_d88.c by Texas Instruments. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + At least on my machine checkpatch.pl complains about FSF address. +#include +#include +#include +#include +#include +#include Doesn't seem to get used. +#include +#include +#include + +#include +#include + +#define CODE_TO_RAMP_US(s) ((s) == 0 ? 0 : (1 << ((s) - 1)) * 50) +#define RAMP_US_TO_CODE(c) fls(((c) + ((c)>>1)) / 50) + +/** + * @brief I2C write using i2c_transfer(). + * @param coil - the driver data structure + * @param data - register value to be written + * @returns nonnegative on success, negative if failed + */ +static int ad5820_write(struct ad5820_device *coil, u16 data) +{ + struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev); + struct i2c_msg msg; + int r; + + if (!client->adapter) + return -ENODEV; + + data = cpu_to_be16(data); + msg.addr = client->addr; + msg.flags = 0; + msg.len = 2; + msg.buf = (u8 *)&data; + + r = i2c_transfer(client->adapter, &msg, 1); + if (r < 0) { + dev_err(&client->dev, "write failed, error %d\n", r); + return r; + } + + return 0; +} + +/* + * Calculate status word and write it to the device based on current + * values of V4L2 controls. It is assumed that the stored V4L2 control + * values are properly limited and rounded. + */ +static int ad5820_update_hw(struct ad5820_device *coil) +{ + u16 status; + + status = RAMP_US_TO_CODE(coil->focus_ramp_time); + status |= coil->focus_ramp_mode + ? AD5820_RAMP_MODE_64_16 : AD5820_RAMP_MODE_LINEAR; + status |= coil->focus_absolute << AD5820_DAC_SHIFT; + + if (coil->standby) + status |= AD5820_POWER_DOWN; + + return ad5820_write(coil, status); +} + +/* + * Power handling + */ +static int ad5820_power_off(struct ad5820_device *coil, int standby) +{ + int ret = 0; + + /* +* Go to standby first as real power off my be denied by the hardware +* (single power line control for both coil and sensor). +*/ + if (standby) { + coil->standby = 1; + ret = ad5820_update_hw(coil); + } + + ret |= regulator_disable(coil->vana); + + return ret; +} + +static int ad5820_power_on(struct ad5820_device *coil, int restore) +
[PATCHv3] support for AD5820 camera auto-focus coil
This adds support for AD5820 autofocus coil, found for example in Nokia N900 smartphone. Signed-off-by: Pavel Machek --- v2: simple cleanups, fix error paths, simplify probe v3: more cleanups, remove printk, add include diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 993dc50..77313a1 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -279,6 +279,13 @@ config VIDEO_ML86V7667 To compile this driver as a module, choose M here: the module will be called ml86v7667. +config VIDEO_AD5820 + tristate "AD5820 lens voice coil support" + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER + ---help--- + This is a driver for the AD5820 camera lens voice coil. + It is used for example in Nokia N900 (RX-51). + config VIDEO_SAA7110 tristate "Philips SAA7110 video decoder" depends on VIDEO_V4L2 && I2C diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 94f2c99..34434ae 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -19,6 +20,7 @@ obj-$(CONFIG_VIDEO_SAA717X) += saa717x.o 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_ADV7170) += adv7170.o obj-$(CONFIG_VIDEO_ADV7175) += adv7175.o obj-$(CONFIG_VIDEO_ADV7180) += adv7180.o diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c new file mode 100644 index 000..7725829 --- /dev/null +++ b/drivers/media/i2c/ad5820.c @@ -0,0 +1,416 @@ +/* + * drivers/media/i2c/ad5820.c + * + * AD5820 DAC driver for camera voice coil focus. + * + * Copyright (C) 2008 Nokia Corporation + * Copyright (C) 2007 Texas Instruments + * Copyright (C) 2016 Pavel Machek + * + * Contact: Tuukka Toivonen + * Sakari Ailus + * + * Based on af_d88.c by Texas Instruments. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#define CODE_TO_RAMP_US(s) ((s) == 0 ? 0 : (1 << ((s) - 1)) * 50) +#define RAMP_US_TO_CODE(c) fls(((c) + ((c)>>1)) / 50) + +/** + * @brief I2C write using i2c_transfer(). + * @param coil - the driver data structure + * @param data - register value to be written + * @returns nonnegative on success, negative if failed + */ +static int ad5820_write(struct ad5820_device *coil, u16 data) +{ + struct i2c_client *client = v4l2_get_subdevdata(&coil->subdev); + struct i2c_msg msg; + int r; + + if (!client->adapter) + return -ENODEV; + + data = cpu_to_be16(data); + msg.addr = client->addr; + msg.flags = 0; + msg.len = 2; + msg.buf = (u8 *)&data; + + r = i2c_transfer(client->adapter, &msg, 1); + if (r < 0) { + dev_err(&client->dev, "write failed, error %d\n", r); + return r; + } + + return 0; +} + +/* + * Calculate status word and write it to the device based on current + * values of V4L2 controls. It is assumed that the stored V4L2 control + * values are properly limited and rounded. + */ +static int ad5820_update_hw(struct ad5820_device *coil) +{ + u16 status; + + status = RAMP_US_TO_CODE(coil->focus_ramp_time); + status |= coil->focus_ramp_mode + ? AD5820_RAMP_MODE_64_16 : AD5820_RAMP_MODE_LINEAR; + status |= coil->focus_absolute << AD5820_DAC_SHIFT; + + if (coil->standby) + status |= AD5820_POWER_DOWN; + + return ad5820_write(coil, status); +} + +/* + * Power handling + */ +static int ad5820_power_off(struct ad5820_device *coil, int standby) +{ + int ret = 0; + + /* +* Go to standby first as real power off my be denied by the hardware +* (single power line control for both coil and sensor). +*/ + if (standby) { + coil->standby = 1; + ret = ad5820_update_hw(coil); + } + + ret |= regulator_disable(coil->vana); + + return ret; +} + +static int ad5820_power_on(struct ad5820_device *coil, int restore) +{ + int ret; + + printk("ad5820_power_on: 1\n"); + ret = regulator_enable(coil->vana); + if (ret < 0) + return ret;