[PATCH 1/4] [media] ad5820: Define entity function

[PATCH 1/4] [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>
---
 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.18.0

[PATCH 2/4] [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>
---
 drivers/media/i2c/Kconfig  |  2 +-
 drivers/media/i2c/ad5820.c | 20 ++++++++++++++++++++
 2 files changed, 21 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..20931217e3b1 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,9 @@ static int ad5820_power_off(struct ad5820_device *coil, bool standby)
 		ret = ad5820_update_hw(coil);
 	}
 
+	if (coil->enable_gpio)
+		gpiod_set_value_cansleep(coil->enable_gpio, 0);
+
 	ret2 = regulator_disable(coil->vana);
 	if (ret)
 		return ret;
@@ -136,6 +142,9 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
 	if (ret < 0)
 		return ret;
 
+	if (coil->enable_gpio)
+		gpiod_set_value_cansleep(coil->enable_gpio, 1);
+
 	if (restore) {
 		/* Restore the hardware settings. */
 		coil->standby = false;
@@ -146,6 +155,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
 	return 0;
 
 fail:
+	if (coil->enable_gpio)
+		gpiod_set_value_cansleep(coil->enable_gpio, 0);
 	coil->standby = true;
 	regulator_disable(coil->vana);
 
@@ -312,6 +323,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.18.0

Re: [PATCH 2/4] [media] ad5820: Add support for enable pin

[Sakari Ailus]

Hi Ricardo,

Thanks for the set! A few comments below...

On Thu, Sep 20, 2018 at 06:19:10PM +0200, Ricardo Ribalda Delgado wrote:
> 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

Missing period at the end of the sentence.

> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/media/i2c/Kconfig  |  2 +-
>  drivers/media/i2c/ad5820.c | 20 ++++++++++++++++++++
>  2 files changed, 21 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..20931217e3b1 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,9 @@ static int ad5820_power_off(struct ad5820_device *coil, bool standby)
>  		ret = ad5820_update_hw(coil);
>  	}
>  
> +	if (coil->enable_gpio)
> +		gpiod_set_value_cansleep(coil->enable_gpio, 0);

gpiod_set_value_cansleep(), as I think most (or all?) similar functions,
are happy with NULL gpio descriptor. You can thus drop the NULL check here
and below.

> +
>  	ret2 = regulator_disable(coil->vana);
>  	if (ret)
>  		return ret;
> @@ -136,6 +142,9 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (coil->enable_gpio)
> +		gpiod_set_value_cansleep(coil->enable_gpio, 1);
> +
>  	if (restore) {
>  		/* Restore the hardware settings. */
>  		coil->standby = false;
> @@ -146,6 +155,8 @@ static int ad5820_power_on(struct ad5820_device *coil, bool restore)
>  	return 0;
>  
>  fail:
> +	if (coil->enable_gpio)
> +		gpiod_set_value_cansleep(coil->enable_gpio, 0);
>  	coil->standby = true;
>  	regulator_disable(coil->vana);
>  
> @@ -312,6 +323,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);

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 2/4] [media] ad5820: Add support for enable pin

[Ricardo Ribalda Delgado]

Hi Sakari

On Thu, Sep 20, 2018 at 8:40 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> Thanks for the set! A few comments below...
>
> On Thu, Sep 20, 2018 at 06:19:10PM +0200, Ricardo Ribalda Delgado wrote:
> > 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
>
> Missing period at the end of the sentence.
>

Thanks for the superfast response.

I have just sent a patch with your comments and also resend 4/4. I was
missing a comma.

Thanks!
> >

[PATCH v2 2/4] [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>
---
 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..625867472929 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.18.0

Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin

[Pavel Machek]

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

On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> 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>

Do we really want to do that?

Would it make more sense to add gpio-regulator and connect ad5820 to
it in such case?

								Pavel
								
-- 
(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 v2 2/4] [media] ad5820: Add support for enable pin

[Ricardo Ribalda Delgado]

Hi Pavel

On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > 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>
>
> Do we really want to do that?
>
> Would it make more sense to add gpio-regulator and connect ad5820 to
> it in such case?
>

My board (based on db820c)  has both:

ad5820: dac@0c {
   compatible = "adi,ad5820";
   reg = <0x0c>;

   VANA-supply = <&pm8994_l23>;
   enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
};



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



-- 
Ricardo Ribalda

Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin

[Pavel Machek]

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

On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> Hi Pavel
> 
> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > > 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>
> >
> > Do we really want to do that?
> >
> > Would it make more sense to add gpio-regulator and connect ad5820 to
> > it in such case?
> >
> 
> My board (based on db820c)  has both:
> 
> ad5820: dac@0c {
>    compatible = "adi,ad5820";
>    reg = <0x0c>;
> 
>    VANA-supply = <&pm8994_l23>;
>    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> };

Well, I'm sure you could have gpio-based regulator powered from
pm8994_l23, and outputting to ad5820.

Does ad5820 chip have a gpio input for enable?
									Pavel
-- 
(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 v2 2/4] [media] ad5820: Add support for enable pin

[Ricardo Ribalda Delgado]

On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> > Hi Pavel
> >
> > On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > > > 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>
> > >
> > > Do we really want to do that?
> > >
> > > Would it make more sense to add gpio-regulator and connect ad5820 to
> > > it in such case?
> > >
> >
> > My board (based on db820c)  has both:
> >
> > ad5820: dac@0c {
> >    compatible = "adi,ad5820";
> >    reg = <0x0c>;
> >
> >    VANA-supply = <&pm8994_l23>;
> >    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > };
>
> Well, I'm sure you could have gpio-based regulator powered from
> pm8994_l23, and outputting to ad5820.
>
> Does ad5820 chip have a gpio input for enable?

xshutdown pin:
http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf

(AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
the module manufacturer says :)


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



--
Ricardo Ribalda

Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin

[Laurent Pinchart]

Hi Ricardo,

On Thursday, 20 September 2018 22:12:44 EEST Ricardo Ribalda Delgado wrote:
> On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <pavel@ucw.cz> wrote:
> > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
> >>> On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> >>>> 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>
> >>> 
> >>> Do we really want to do that?
> >>> 
> >>> Would it make more sense to add gpio-regulator and connect ad5820 to
> >>> it in such case?
> >> 
> >> My board (based on db820c)  has both:
> >> 
> >> ad5820: dac@0c {
> >>    compatible = "adi,ad5820";
> >>    reg = <0x0c>;
> >>    
> >>    VANA-supply = <&pm8994_l23>;
> >>    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> >> };
> > 
> > Well, I'm sure you could have gpio-based regulator powered from
> > pm8994_l23, and outputting to ad5820.
> > 
> > Does ad5820 chip have a gpio input for enable?
> 
> xshutdown pin:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pd
> f
> 
> (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> the module manufacturer says :)

Is that the pin that msmgpio 26 is connected to on your board ?

I'd argue that the GPIO should be called xshutdown in that case, as DT usually 
uses the pin name, but there's precedent of using well-known names for pins 
with the same functionality. In any case you should update the DT bindings to 
document the new property, and clearly explain that it describes the GPIO 
connected to the xshutdown pin. Please CC the devicetree@vger.kernel.org 
mailing list on the bindings change (they usually like bindings changes to be 
split to a separate patch).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin

[Laurent Pinchart]

On Thursday, 20 September 2018 23:04:21 EEST Laurent Pinchart wrote:
> Hi Ricardo,
> 
> On Thursday, 20 September 2018 22:12:44 EEST Ricardo Ribalda Delgado wrote:
> > On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> > >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >>> On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > >>>> 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>
> > >>> 
> > >>> Do we really want to do that?
> > >>> 
> > >>> Would it make more sense to add gpio-regulator and connect ad5820 to
> > >>> it in such case?
> > >> 
> > >> My board (based on db820c)  has both:
> > >> 
> > >> ad5820: dac@0c {
> > >> 
> > >>    compatible = "adi,ad5820";
> > >>    reg = <0x0c>;
> > >>    
> > >>    VANA-supply = <&pm8994_l23>;
> > >>    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > >> 
> > >> };
> > > 
> > > Well, I'm sure you could have gpio-based regulator powered from
> > > pm8994_l23, and outputting to ad5820.
> > > 
> > > Does ad5820 chip have a gpio input for enable?
> > 
> > xshutdown pin:
> > http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.
> > pd f
> > 
> > (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> > the module manufacturer says :)
> 
> Is that the pin that msmgpio 26 is connected to on your board ?
> 
> I'd argue that the GPIO should be called xshutdown in that case, as DT
> usually uses the pin name, but there's precedent of using well-known names
> for pins with the same functionality. In any case you should update the DT
> bindings to document the new property, and clearly explain that it
> describes the GPIO connected to the xshutdown pin. Please CC the
> devicetree@vger.kernel.org mailing list on the bindings change (they
> usually like bindings changes to be split to a separate patch).

And now I've noticed patch 3/4 :-/ Please scratch this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] [media] ad5820: Add support for enable pin

[Pavel Machek]

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

On Thu 2018-09-20 21:12:44, Ricardo Ribalda Delgado wrote:
> On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> > > Hi Pavel
> > >
> > > On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > >
> > > > On Thu 2018-09-20 20:45:52, Ricardo Ribalda Delgado wrote:
> > > > > 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>
> > > >
> > > > Do we really want to do that?
> > > >
> > > > Would it make more sense to add gpio-regulator and connect ad5820 to
> > > > it in such case?
> > > >
> > >
> > > My board (based on db820c)  has both:
> > >
> > > ad5820: dac@0c {
> > >    compatible = "adi,ad5820";
> > >    reg = <0x0c>;
> > >
> > >    VANA-supply = <&pm8994_l23>;
> > >    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > > };
> >
> > Well, I'm sure you could have gpio-based regulator powered from
> > pm8994_l23, and outputting to ad5820.
> >
> > Does ad5820 chip have a gpio input for enable?
> 
> xshutdown pin:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
> 
> (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> the module manufacturer says :)

Aha, sorry for the noise.

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

Thanks,
								Pavel

-- 
(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 v2 2/4] [media] ad5820: Add support for enable pin

[Pavel Machek]

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

Hi!

> > > > ad5820: dac@0c {
> > > >    compatible = "adi,ad5820";
> > > >    reg = <0x0c>;
> > > >
> > > >    VANA-supply = <&pm8994_l23>;
> > > >    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > > > };
> > >
> > > Well, I'm sure you could have gpio-based regulator powered from
> > > pm8994_l23, and outputting to ad5820.
> > >
> > > Does ad5820 chip have a gpio input for enable?
> > 
> > xshutdown pin:
> > http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
> > 
> > (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> > the module manufacturer says :)
> 
> Aha, sorry for the noise.
> 
> 2,3: Acked-by: Pavel Machek <pavel@ucw.cz>

And I forgot to mention. If ad5821 and ad5823 are compatible, it would
be good to mention it somewhere where it is easy to find... That can
save quite a bit of work to someone.

Thanks,
								Pavel


-- 
(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 v2 2/4] [media] ad5820: Add support for enable pin

[Ricardo Ribalda Delgado]

Hi
On Thu, Sep 20, 2018 at 10:21 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > > ad5820: dac@0c {
> > > > >    compatible = "adi,ad5820";
> > > > >    reg = <0x0c>;
> > > > >
> > > > >    VANA-supply = <&pm8994_l23>;
> > > > >    enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > > > > };
> > > >
> > > > Well, I'm sure you could have gpio-based regulator powered from
> > > > pm8994_l23, and outputting to ad5820.
> > > >
> > > > Does ad5820 chip have a gpio input for enable?
> > >
> > > xshutdown pin:
> > > http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
> > >
> > > (AD5820,AD5821, and AD5823 are compatibles, or at least that is waht
> > > the module manufacturer says :)
> >
> > Aha, sorry for the noise.
> >
> > 2,3: Acked-by: Pavel Machek <pavel@ucw.cz>
>
> And I forgot to mention. If ad5821 and ad5823 are compatible, it would
> be good to mention it somewhere where it is easy to find... That can
> save quite a bit of work to someone.
>

For the ad5821 I have the datasheet and I would not mind adding it
For the ad5823 I have no datasheet, just a schematic from a camera
module saying: you can replace ad5823 with ad5821.

I think I will add this as an extra patch

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



-- 
Ricardo Ribalda

[PATCH 3/4] [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>
---
 Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
index 5940ca11c021..07d577bb37f7 100644
--- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
+++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
@@ -8,6 +8,11 @@ Required Properties:
 
   - VANA-supply: supply of voltage for VANA pin
 
+Optional properties:
+
+   - enable-gpios : GPIO spec for the XSHUTDOWN pin. If specified, it will be
+     asserted when VANA-supply is enabled.
+
 Example:
 
        ad5820: coil@c {
@@ -15,5 +20,6 @@ Example:
                reg = <0x0c>;
 
                VANA-supply = <&vaux4>;
+               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
        };
 
-- 
2.18.0

Re: [PATCH 3/4] [media] ad5820: DT new optional field enable-gpios

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thursday, 20 September 2018 19:19:11 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>
> ---
>  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> 5940ca11c021..07d577bb37f7 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> @@ -8,6 +8,11 @@ Required Properties:
> 
>    - VANA-supply: supply of voltage for VANA pin
> 
> +Optional properties:
> +
> +   - enable-gpios : GPIO spec for the XSHUTDOWN pin.

xshutdown is active-low, so enable is active-high. Should this be documented 
explicitly, to avoid polarity errors ? Maybe something along the lines of

- 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).

> If specified, it will be
> +     asserted when VANA-supply is enabled.

That documents a driver behaviour, is it needed in DT ?


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

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] [media] ad5820: DT new optional field enable-gpios

[Laurent Pinchart]

On Thursday, 20 September 2018 23:21:28 EEST Laurent Pinchart wrote:
> Hi Ricardo,
> 
> Thank you for the patch.
> 
> On Thursday, 20 September 2018 19:19:11 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>
> > ---
> > 
> >  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> > 5940ca11c021..07d577bb37f7 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > 
> > @@ -8,6 +8,11 @@ Required Properties:
> >    - VANA-supply: supply of voltage for VANA pin
> > 
> > +Optional properties:
> > +
> > +   - enable-gpios : GPIO spec for the XSHUTDOWN pin.
> 
> xshutdown is active-low, so enable is active-high. Should this be documented
> explicitly, to avoid polarity errors ? Maybe something along the lines of
> 
> - 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).

Or alternatively you could name the property xshutdown-gpios, as explained in 
my (incorrect) review of 2/4.

> > If specified, it will be
> > +     asserted when VANA-supply is enabled.
> 
> That documents a driver behaviour, is it needed in DT ?
> 
> >  Example:
> >         ad5820: coil@c {
> > 
> > @@ -15,5 +20,6 @@ Example:
> >                 reg = <0x0c>;
> >                 VANA-supply = <&vaux4>;
> > 
> > +               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> >         };


-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] [media] ad5820: DT new optional field enable-gpios

[Ricardo Ribalda Delgado]

Hi
On Thu, Sep 20, 2018 at 10:23 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thursday, 20 September 2018 23:21:28 EEST Laurent Pinchart wrote:
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Thursday, 20 September 2018 19:19:11 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>
> > > ---
> > >
> > >  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> > > 5940ca11c021..07d577bb37f7 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > >
> > > @@ -8,6 +8,11 @@ Required Properties:
> > >    - VANA-supply: supply of voltage for VANA pin
> > >
> > > +Optional properties:
> > > +
> > > +   - enable-gpios : GPIO spec for the XSHUTDOWN pin.
> >
> > xshutdown is active-low, so enable is active-high. Should this be documented
> > explicitly, to avoid polarity errors ? Maybe something along the lines of
> >
> > - 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).

Agreed

>
> Or alternatively you could name the property xshutdown-gpios, as explained in
> my (incorrect) review of 2/4.

I have double negatives :). If there is no other option I will rename
it xshutdown, but I want to give it a try to enable.

>
> > > If specified, it will be
> > > +     asserted when VANA-supply is enabled.
> >
> > That documents a driver behaviour, is it needed in DT ?
> >
> > >  Example:
> > >         ad5820: coil@c {
> > >
> > > @@ -15,5 +20,6 @@ Example:
> > >                 reg = <0x0c>;
> > >                 VANA-supply = <&vaux4>;
> > >
> > > +               enable-gpios = <&msmgpio 26 GPIO_ACTIVE_HIGH>;
> > >         };
>
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


-- 
Ricardo Ribalda

Re: [PATCH 3/4] [media] ad5820: DT new optional field enable-gpios

[Pavel Machek]

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

On Thu 2018-09-20 22:25:54, Ricardo Ribalda Delgado wrote:
> Hi
> On Thu, Sep 20, 2018 at 10:23 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Thursday, 20 September 2018 23:21:28 EEST Laurent Pinchart wrote:
> > > Hi Ricardo,
> > >
> > > Thank you for the patch.
> > >
> > > On Thursday, 20 September 2018 19:19:11 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>
> > > > ---
> > > >
> > > >  Documentation/devicetree/bindings/media/i2c/ad5820.txt | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt index
> > > > 5940ca11c021..07d577bb37f7 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt
> > > >
> > > > @@ -8,6 +8,11 @@ Required Properties:
> > > >    - VANA-supply: supply of voltage for VANA pin
> > > >
> > > > +Optional properties:
> > > > +
> > > > +   - enable-gpios : GPIO spec for the XSHUTDOWN pin.
> > >
> > > xshutdown is active-low, so enable is active-high. Should this be documented
> > > explicitly, to avoid polarity errors ? Maybe something along the lines of
> > >
> > > - 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).
> 
> Agreed
> 
> >
> > Or alternatively you could name the property xshutdown-gpios, as explained in
> > my (incorrect) review of 2/4.
> 
> I have double negatives :). If there is no other option I will rename
> it xshutdown, but I want to give it a try to enable.

I think enable is fine.

									Pavel

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

[PATCH 4/4] [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.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.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 20931217e3b1..d352bc6b6adf 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -375,12 +375,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_NAME },
+	{ }
+};
+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.18.0

[PATCH v2 4/4] [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.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.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 20931217e3b1..75b9b8aa5533 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -375,12 +375,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_NAME },
+	{ }
+};
+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.18.0

Re: [PATCH v2 4/4] [media] ad5820: Add support for of-autoload

[Pavel Machek]

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

On Thu 2018-09-20 20:31:51, Ricardo Ribalda Delgado wrote:
> 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.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

1,4: 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 --]

Re: [PATCH v2 4/4] [media] ad5820: Add support for of-autoload

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thursday, 20 September 2018 21:31:51 EEST Ricardo Ribalda Delgado wrote:
> 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.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.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 20931217e3b1..75b9b8aa5533 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -375,12 +375,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_NAME },

I'd spell this out explicitly, to make it easier to grep for the compatible 
string.

> +	{ }
> +};
> +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,

As the driver doesn't depend on CONFIG_OF, would it make sense to use 
of_config_ptr() (and to compile the of table conditionally on CONFIG_OF) ?

>  	},
>  	.probe		= ad5820_probe,
>  	.remove		= ad5820_remove,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] [media] ad5820: Add support for of-autoload

[Sakari Ailus]

Hi Laurent,

On Thu, Sep 20, 2018 at 11:10:23PM +0300, Laurent Pinchart wrote:
> > +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,
> 
> As the driver doesn't depend on CONFIG_OF, would it make sense to use 
> of_config_ptr() (and to compile the of table conditionally on CONFIG_OF) ?

You get ACPI support as a bonus if you don't use of_config_ptr(). :-) Other
changes could be needed but this enables probing the driver for a device.

In this case the probability of anyone using this device on an ACPI system
could be pretty low though.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 1/4] [media] ad5820: Define entity function

[Laurent Pinchart]

Hi Ricardo,

Thank you for the patch.

On Thursday, 20 September 2018 19:19:09 EEST Ricardo Ribalda Delgado wrote:
> 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>

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);


-- 
Regards,

Laurent Pinchart