2018-09-20 16:20:02

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
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

2018-09-20 16:20:05

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
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

2018-09-20 16:20:09

by Ricardo Ribalda Delgado

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

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

Cc: [email protected]
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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

2018-09-20 16:20:26

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
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

2018-09-20 18:33:40

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
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

2018-09-20 18:42:51

by Sakari Ailus

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

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 <[email protected]>
> ---
> 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: [email protected]

2018-09-20 18:46:58

by Ricardo Ribalda Delgado

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

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 <[email protected]>
---
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

2018-09-20 18:48:37

by Ricardo Ribalda Delgado

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

Hi Sakari

On Thu, Sep 20, 2018 at 8:40 PM Sakari Ailus <[email protected]> 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!
> >

2018-09-20 18:54:51

by Pavel Machek

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

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 <[email protected]>

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

Attachments:

2018-09-20 18:56:17

by Pavel Machek

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

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 <[email protected]>

1,4: Acked-by: Pavel Machek <[email protected]>

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

Attachments:

2018-09-20 19:08:37

by Ricardo Ribalda Delgado

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

Hi Pavel

On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <[email protected]> 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 <[email protected]>
>
> 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

2018-09-20 19:09:26

by Pavel Machek

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

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 <[email protected]> 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 <[email protected]>
> >
> > 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

Attachments:

2018-09-20 19:13:24

by Ricardo Ribalda Delgado

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

On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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

2018-09-20 20:06:56

by Laurent Pinchart

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

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 <[email protected]> wrote:
> > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <[email protected]> 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 <[email protected]>
> >>>
> >>> 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 [email protected]
mailing list on the bindings change (they usually like bindings changes to be
split to a separate patch).

--
Regards,

Laurent Pinchart

2018-09-20 20:07:08

by Laurent Pinchart

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

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 <[email protected]> wrote:
> > > On Thu 2018-09-20 21:06:16, Ricardo Ribalda Delgado wrote:
> > >> On Thu, Sep 20, 2018 at 8:54 PM Pavel Machek <[email protected]> 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 <[email protected]>
> > >>>
> > >>> 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
> [email protected] 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

2018-09-20 20:10:43

by Laurent Pinchart

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

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 <[email protected]>
> ---
> 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

2018-09-20 20:16:19

by Pavel Machek

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

On Thu 2018-09-20 21:12:44, Ricardo Ribalda Delgado wrote:
> On Thu, Sep 20, 2018 at 9:08 PM Pavel Machek <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>

Thanks,
Pavel

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

Attachments:

2018-09-20 20:19:14

by Sakari Ailus

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

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: [email protected]

2018-09-20 20:21:46

by Laurent Pinchart

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

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: [email protected]
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> 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

2018-09-20 20:22:40

by Pavel Machek

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

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 <[email protected]>

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

Attachments:

2018-09-20 20:23:27

by Laurent Pinchart

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

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 <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

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

2018-09-20 20:25:38

by Laurent Pinchart

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

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: [email protected]
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> >
> > 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

2018-09-20 20:28:14

by Ricardo Ribalda Delgado

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

Hi
On Thu, Sep 20, 2018 at 10:23 PM Laurent Pinchart
<[email protected]> 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: [email protected]
> > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > > ---
> > >
> > > 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

2018-09-20 20:29:07

by Pavel Machek

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

On Thu 2018-09-20 22:25:54, Ricardo Ribalda Delgado wrote:
> Hi
> On Thu, Sep 20, 2018 at 10:23 PM Laurent Pinchart
> <[email protected]> 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: [email protected]
> > > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > > > ---
> > > >
> > > > 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

Attachments:

2018-09-20 20:33:12

by Ricardo Ribalda Delgado

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

Hi
On Thu, Sep 20, 2018 at 10:21 PM Pavel Machek <[email protected]> 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 <[email protected]>
>
> 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