Re: [PATCHv3] support for AD5820 camera auto-focus coil

[Ivaylo Dimitrov]

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(>dev, NULL);
if (IS_ERR(coil->vana)) {
ret = PTR_ERR(coil->vana);
if (ret != -EPROBE_DEFER)
dev_err(>dev, "could not get regulator for 
vana\n");
return ret;
}

mutex_init(>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(>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(>dev, "VANA");
-   if (IS_ERR(coil->vana)) {
-   dev_err(>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(>dev, NULL);
+   if (IS_ERR(coil->vana)) {
+   ret = PTR_ERR(coil->vana);
+   if (ret != -EPROBE_DEFER)
+   dev_err(>dev, "could not get regulator for 
vana\n");
+   return ret;
+   }
+   
mutex_init(>power_lock);

v4l2_i2c_subdev_init(>subdev, client, _ops);
@@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client,

  cleanup:
media_entity_cleanup(>subdev.entity);
-
-free:
-   kfree(coil);
-
return ret;
  }

@@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client)
v4l2_device_unregister_subdev(>subdev);
v4l2_ctrl_handler_free(>ctrls);
media_entity_cleanup(>subdev.entity);
-   if (coil->vana)
-   regulator_put(coil->vana);
-
-   kfree(coil);
-
return 0;
  }

Re: [PATCHv3] support for AD5820 camera auto-focus coil

[Ivaylo Dimitrov]

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(>dev, NULL);
if (IS_ERR(coil->vana)) {
ret = PTR_ERR(coil->vana);
if (ret != -EPROBE_DEFER)
dev_err(>dev, "could not get regulator for 
vana\n");
return ret;
}

mutex_init(>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(>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(>dev, "VANA");
-   if (IS_ERR(coil->vana)) {
-   dev_err(>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(>dev, NULL);
+   if (IS_ERR(coil->vana)) {
+   ret = PTR_ERR(coil->vana);
+   if (ret != -EPROBE_DEFER)
+   dev_err(>dev, "could not get regulator for 
vana\n");
+   return ret;
+   }
+   
mutex_init(>power_lock);

v4l2_i2c_subdev_init(>subdev, client, _ops);
@@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client,

  cleanup:
media_entity_cleanup(>subdev.entity);
-
-free:
-   kfree(coil);
-
return ret;
  }

@@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client)
v4l2_device_unregister_subdev(>subdev);
v4l2_ctrl_handler_free(>ctrls);
media_entity_cleanup(>subdev.entity);
-   if (coil->vana)
-   regulator_put(coil->vana);
-
-   kfree(coil);
-
return 0;
  }

Re: [PATCHv3] support for AD5820 camera auto-focus coil

[Pavel Machek]

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(>dev, NULL);
>   if (IS_ERR(coil->vana)) {
>   ret = PTR_ERR(coil->vana);
>   if (ret != -EPROBE_DEFER)
>   dev_err(>dev, "could not get regulator for 
> vana\n");
>   return ret;
>   }
> 
>   mutex_init(>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(>dev, "VANA");
-   if (IS_ERR(coil->vana)) {
-   dev_err(>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(>dev, NULL);
+   if (IS_ERR(coil->vana)) {
+   ret = PTR_ERR(coil->vana);
+   if (ret != -EPROBE_DEFER)
+   dev_err(>dev, "could not get regulator for 
vana\n");
+   return ret;
+   }
+   
mutex_init(>power_lock);
 
v4l2_i2c_subdev_init(>subdev, client, _ops);
@@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client,
 
 cleanup:
media_entity_cleanup(>subdev.entity);
-
-free:
-   kfree(coil);
-
return ret;
 }
 
@@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client)
v4l2_device_unregister_subdev(>subdev);
v4l2_ctrl_handler_free(>ctrls);
media_entity_cleanup(>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

[Pavel Machek]

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(>dev, NULL);
>   if (IS_ERR(coil->vana)) {
>   ret = PTR_ERR(coil->vana);
>   if (ret != -EPROBE_DEFER)
>   dev_err(>dev, "could not get regulator for 
> vana\n");
>   return ret;
>   }
> 
>   mutex_init(>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(>dev, "VANA");
-   if (IS_ERR(coil->vana)) {
-   dev_err(>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(>dev, NULL);
+   if (IS_ERR(coil->vana)) {
+   ret = PTR_ERR(coil->vana);
+   if (ret != -EPROBE_DEFER)
+   dev_err(>dev, "could not get regulator for 
vana\n");
+   return ret;
+   }
+   
mutex_init(>power_lock);
 
v4l2_i2c_subdev_init(>subdev, client, _ops);
@@ -390,10 +391,6 @@ static int ad5820_probe(struct i2c_client *client,
 
 cleanup:
media_entity_cleanup(>subdev.entity);
-
-free:
-   kfree(coil);
-
return ret;
 }
 
@@ -405,11 +402,6 @@ static int __exit ad5820_remove(struct i2c_client *client)
v4l2_device_unregister_subdev(>subdev);
v4l2_ctrl_handler_free(>ctrls);
media_entity_cleanup(>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

[Ivaylo Dimitrov]

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(>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(>dev, NULL);
if (IS_ERR(coil->vana)) {
ret = PTR_ERR(coil->vana);
if (ret != -EPROBE_DEFER)
dev_err(>dev, "could not get regulator for 
vana\n");
return ret;
}

mutex_init(>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

[Ivaylo Dimitrov]

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(>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(>dev, NULL);
if (IS_ERR(coil->vana)) {
ret = PTR_ERR(coil->vana);
if (ret != -EPROBE_DEFER)
dev_err(>dev, "could not get regulator for 
vana\n");
return ret;
}

mutex_init(>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

[Pavel Machek]

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(>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

[Pavel Machek]

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(>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

[Pali Rohár]

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

[Pali Rohár]

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

[Ivaylo Dimitrov]

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(>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 *)
+
+   r = i2c_transfer(client->adapter, , 1);
+   if (r < 0) {
+   dev_err(>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 

Re: [PATCHv3] support for AD5820 camera auto-focus coil

[Ivaylo Dimitrov]

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(>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 *)
+
+   r = i2c_transfer(client->adapter, , 1);
+   if (r < 0) {
+   dev_err(>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;
+
+  

[PATCHv3] support for AD5820 camera auto-focus coil

[Pavel Machek]

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(>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 *)
+
+   r = i2c_transfer(client->adapter, , 1);
+   if (r < 0) {
+   dev_err(>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 

[PATCHv3] support for AD5820 camera auto-focus coil

[Pavel Machek]

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(>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 *)
+
+   r = i2c_transfer(client->adapter, , 1);
+   if (r < 0) {
+   dev_err(>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;
+
+   if (restore)