[PATCH v5 1/6] [media] ad5820: Define entity function

[PATCH v5 1/6] [media] ad5820: Define entity function

[Ricardo Ribalda Delgado]

Without this patch, media_device_register_entity throws a warning:

dev_warn(mdev->dev,
	 "Entity type for entity %s was not initialized!\n",
	 entity->name);

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ad5820.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 907323f0ca3b..22759aaa2dba 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -317,6 +317,7 @@ static int ad5820_probe(struct i2c_client *client,
 	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
 	coil->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	coil->subdev.internal_ops = &ad5820_internal_ops;
+	coil->subdev.entity.function = MEDIA_ENT_F_LENS;
 	strscpy(coil->subdev.name, "ad5820 focus", sizeof(coil->subdev.name));
 
 	ret = media_entity_pads_init(&coil->subdev.entity, 0, NULL);
-- 
2.19.0

[PATCH v5 2/6] [media] ad5820: DT new optional field enable-gpios

[Ricardo Ribalda Delgado]

Document new enable-gpio field. It can be used to disable the part
without turning down its regulator.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 Documentation/devicetree/bindings/media/i2c/ad5820.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 5940ca11c021..9ccd96d3d5f0 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -8,6 +8,12 @@ Required Properties:
 
   - VANA-supply: supply of voltage for VANA pin
 
+Optional properties:
+
+   - enable-gpios : GPIO spec for the XSHUTDOWN pin. Note that the polarity of
+the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the enable
+GPIO deasserts the XSHUTDOWN signal and vice versa).
+
 Example:
 
        ad5820: coil@c {
@@ -15,5 +21,6 @@ Example:
                reg = <0x0c>;
 
                VANA-supply = <&vaux4>;
+               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
        };
 
-- 
2.19.0

[PATCH v5 3/6] [media] ad5820: Add support for enable pin

[Ricardo Ribalda Delgado]

This patch adds support for a programmable enable pin. It can be used in
situations where the ANA-vcc is not configurable (dummy-regulator), or
just to have a more fine control of the power saving.

The use of the enable pin is optional.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ad5820.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index bfdb494686bf..1ba6eaaf58fb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -321,7 +321,7 @@ config VIDEO_ML86V7667
 
 config VIDEO_AD5820
 	tristate "AD5820 lens voice coil support"
-	depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER
+	depends on GPIOLIB && 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).
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 22759aaa2dba..97eb05e65833 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -27,6 +27,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
+#include <linux/gpio/consumer.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -55,6 +56,8 @@ struct ad5820_device {
 	u32 focus_ramp_time;
 	u32 focus_ramp_mode;
 
+	struct gpio_desc *enable_gpio;
+
 	struct mutex power_lock;
 	int power_count;
 
@@ -122,6 +125,8 @@ static int ad5820_power_off(struct ad5820_device *coil, bool standby)
 		ret = ad5820_update_hw(coil);
 	}
 
+	gpiod_set_value_cansleep(coil->enable_gpio, 0);
+
 	ret2 = regulator_disable(coil->vana);
 	if (ret)
 		return ret;
@@ -136,6 +141,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
 	if (ret < 0)
 		return ret;
 
+	gpiod_set_value_cansleep(coil->enable_gpio, 1);
+
 	if (restore) {
 		/* Restore the hardware settings. */
 		coil->standby = false;
@@ -146,6 +153,7 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
 	return 0;
 
 fail:
+	gpiod_set_value_cansleep(coil->enable_gpio, 0);
 	coil->standby = true;
 	regulator_disable(coil->vana);
 
@@ -312,6 +320,15 @@ static int ad5820_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	coil->enable_gpio = devm_gpiod_get_optional(&client->dev, "enable",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(coil->enable_gpio)) {
+		ret = PTR_ERR(coil->enable_gpio);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&client->dev, "could not get enable gpio\n");
+		return ret;
+	}
+
 	mutex_init(&coil->power_lock);
 
 	v4l2_i2c_subdev_init(&coil->subdev, client, &ad5820_ops);
-- 
2.19.0

[PATCH v5 4/6] [media] ad5820: Add support for of-autoload

[Ricardo Ribalda Delgado]

Since kernel 4.16, i2c devices with DT compatible tag are modprobed
using their DT modalias.
Without this patch, if this driver is build as module it would never
be autoprobed.

There is no need to mask it with CONFIG_OF to allow ACPI loading, this
also builds find with CONFIG_OF=n.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ad5820.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 97eb05e65833..4ae4e6a776fd 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -372,12 +372,19 @@ static const struct i2c_device_id ad5820_id_table[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
 
+static const struct of_device_id ad5820_of_table[] = {
+	{ .compatible = "adi,ad5820" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad5820_of_table);
+
 static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
 
 static struct i2c_driver ad5820_i2c_driver = {
 	.driver		= {
 		.name	= AD5820_NAME,
 		.pm	= &ad5820_pm,
+		.of_match_table = ad5820_of_table,
 	},
 	.probe		= ad5820_probe,
 	.remove		= ad5820_remove,
-- 
2.19.0

[PATCH v5 5/6] [media] ad5820: DT new compatible devices

[Ricardo Ribalda Delgado]

Document new compatible devices.

Cc: devicetree@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 9ccd96d3d5f0..cc7b10fe0368 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -2,7 +2,10 @@
 
 Required Properties:
 
-  - compatible: Must contain "adi,ad5820"
+  - compatible: Must contain one of:
+		- "adi,ad5820"
+		- "adi,ad5821"
+		- "adi,ad5823"
 
   - reg: I2C slave address
 
-- 
2.19.0

[PATCH v5 6/6] [media] ad5820: Add support for ad5821 and ad5823

[Ricardo Ribalda Delgado]

According to the datasheet, both AD5821 and AD5820 share a compatible
register-set:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf

Some camera modules also refer that AD5823 is a replacement of AD5820:
https://download.kamami.com/p564094-OV8865_DS.pdf

Suggested-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/media/i2c/ad5820.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 4ae4e6a776fd..2c0dd9960268 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -33,8 +33,6 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
 
-#define AD5820_NAME		"ad5820"
-
 /* Register definitions */
 #define AD5820_POWER_DOWN		(1 << 15)
 #define AD5820_DAC_SHIFT		4
@@ -367,13 +365,17 @@ static int ad5820_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ad5820_id_table[] = {
-	{ AD5820_NAME, 0 },
+	{ "ad5820", 0 },
+	{ "ad5821", 0 },
+	{ "ad5823", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
 
 static const struct of_device_id ad5820_of_table[] = {
 	{ .compatible = "adi,ad5820" },
+	{ .compatible = "adi,ad5821" },
+	{ .compatible = "adi,ad5823" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad5820_of_table);
@@ -382,7 +384,7 @@ static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
 
 static struct i2c_driver ad5820_i2c_driver = {
 	.driver		= {
-		.name	= AD5820_NAME,
+		.name	= "ad5820",
 		.pm	= &ad5820_pm,
 		.of_match_table = ad5820_of_table,
 	},
-- 
2.19.0

Re: [PATCH v5 2/6] [media] ad5820: DT new optional field enable-gpios

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Tuesday, 2 October 2018 10:32:18 EEST Ricardo Ribalda Delgado wrote:
> Document new enable-gpio field. It can be used to disable the part
> without turning down its regulator.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
>  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> 5940ca11c021..9ccd96d3d5f0 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> @@ -8,6 +8,12 @@ Required Properties:
> 
>    - VANA-supply: supply of voltage for VANA pin
> 
> +Optional properties:
> +
> +   - enable-gpios : GPIO spec for the XSHUTDOWN pin. Note that the polarity
> of +the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the
> enable +GPIO deasserts the XSHUTDOWN signal and vice versa).

After reading this one more time, I think the text is at the very least 
confusing. The logic level of the enable GPIO is the same as the logic level 
of the XSHUTDOWN pin. The latter being active low, asserting "enable" will 
deassert "shutdown", but talking about "desserting XSHUTDOWN" is confusing.

>  Example:
> 
>         ad5820: coil@c {
> @@ -15,5 +21,6 @@ Example:
>                 reg = <0x0c>;
> 
>                 VANA-supply = <&vaux4>;
> +               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
>         };


-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] [media] ad5820: DT new optional field enable-gpios

[Ricardo Ribalda Delgado]

Hi Laurent
On Tue, Oct 2, 2018 at 12:35 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tuesday, 2 October 2018 10:32:18 EEST Ricardo Ribalda Delgado wrote:
> > Document new enable-gpio field. It can be used to disable the part
> > without turning down its regulator.
> >
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> > 5940ca11c021..9ccd96d3d5f0 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > @@ -8,6 +8,12 @@ Required Properties:
> >
> >    - VANA-supply: supply of voltage for VANA pin
> >
> > +Optional properties:
> > +
> > +   - enable-gpios : GPIO spec for the XSHUTDOWN pin. Note that the polarity
> > of +the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the
> > enable +GPIO deasserts the XSHUTDOWN signal and vice versa).
>
> After reading this one more time, I think the text is at the very least
> confusing. The logic level of the enable GPIO is the same as the logic level
> of the XSHUTDOWN pin. The latter being active low, asserting "enable" will
> deassert "shutdown", but talking about "desserting XSHUTDOWN" is confusing.
>

what about:

- enable-gpios : GPIO spec for the XSHUTDOWN pin. When the XSHUTDOWN pin
is asserted the device is enabled.

> > of +the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the
> > enable +GPIO deasserts the XSHUTDOWN signal and vice versa).

> >  Example:
> >
> >         ad5820: coil@c {
> > @@ -15,5 +21,6 @@ Example:
> >                 reg = <0x0c>;
> >
> >                 VANA-supply = <&vaux4>;
> > +               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> >         };
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


-- 
Ricardo Ribalda

Re: [PATCH v5 2/6] [media] ad5820: DT new optional field enable-gpios

[Laurent Pinchart]

Hi Ricardo,

On Tuesday, 2 October 2018 13:57:22 EEST Ricardo Ribalda Delgado wrote:
> On Tue, Oct 2, 2018 at 12:35 PM Laurent Pinchart wrote:
> > On Tuesday, 2 October 2018 10:32:18 EEST Ricardo Ribalda Delgado wrote:
> > > Document new enable-gpio field. It can be used to disable the part
> > > without turning down its regulator.
> > > 
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> > > 5940ca11c021..9ccd96d3d5f0 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > 
> > > @@ -8,6 +8,12 @@ Required Properties:
> > >    - VANA-supply: supply of voltage for VANA pin
> > > 
> > > +Optional properties:
> > > +
> > > +   - enable-gpios : GPIO spec for the XSHUTDOWN pin. Note that the
> > > polarity of +the enable GPIO is the opposite of the XSHUTDOWN pin
> > > (asserting the enable +GPIO deasserts the XSHUTDOWN signal and vice
> > > versa).
> > 
> > After reading this one more time, I think the text is at the very least
> > confusing. The logic level of the enable GPIO is the same as the logic
> > level of the XSHUTDOWN pin. The latter being active low, asserting
> > "enable" will deassert "shutdown", but talking about "desserting
> > XSHUTDOWN" is confusing.
> 
> what about:
> 
> - enable-gpios : GPIO spec for the XSHUTDOWN pin. When the XSHUTDOWN pin
> is asserted the device is enabled.

For some reason "asserting a pin" doesn't seem right to me. How about

- enable-gpios: GPIO spec for the XSHUTDOWN pin. The XSHUTDOWN signal is 
active low, a high level on the pin enables the device.

> > > of +the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the
> > > enable +GPIO deasserts the XSHUTDOWN signal and vice versa).
> > > 
> > >  Example:
> > >         ad5820: coil@c {
> > > 
> > > @@ -15,5 +21,6 @@ Example:
> > >                 reg = <0x0c>;
> > >                 
> > >                 VANA-supply = <&vaux4>;
> > > 
> > > +               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > > 
> > >         };
> > 
> > --
> > Regards,
> > 
> > Laurent Pinchart


-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 5/6] [media] ad5820: DT new compatible devices

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On Tue 2018-10-02 09:32:21, Ricardo Ribalda Delgado wrote:
> Document new compatible devices.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

> diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> index 9ccd96d3d5f0..cc7b10fe0368 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> @@ -2,7 +2,10 @@
>  
>  Required Properties:
>  
> -  - compatible: Must contain "adi,ad5820"
> +  - compatible: Must contain one of:
> +		- "adi,ad5820"
> +		- "adi,ad5821"
> +		- "adi,ad5823"
>  
>    - reg: I2C slave address
>  

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 6/6] [media] ad5820: Add support for ad5821 and ad5823

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Tue 2018-10-02 09:32:22, Ricardo Ribalda Delgado wrote:
> According to the datasheet, both AD5821 and AD5820 share a compatible
> register-set:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
> 
> Some camera modules also refer that AD5823 is a replacement of AD5820:
> https://download.kamami.com/p564094-OV8865_DS.pdf
> 
> Suggested-by: Pavel Machek <pavel@ucw.cz>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]