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]>
Acked-by: Pavel Machek <[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);
--
2.18.0
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]>
Acked-by: Pavel Machek <[email protected]>
---
drivers/media/i2c/ad5820.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 625867472929..e461d36201a4 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -372,12 +372,21 @@ static const struct i2c_device_id ad5820_id_table[] = {
};
MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
+#ifdef CONFIG_OF
+static const struct of_device_id ad5820_of_table[] = {
+ { .compatible = "adi,ad5820" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad5820_of_table);
+#endif
+
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 = of_match_ptr(ad5820_of_table),
},
.probe = ad5820_probe,
.remove = ad5820_remove,
--
2.18.0
Document new compatible devices.
Cc: [email protected]
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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.18.0
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 <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/media/i2c/ad5820.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index 5d1185e7f78d..c52af302d516 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -34,8 +34,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
@@ -368,7 +366,9 @@ 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);
@@ -376,6 +376,8 @@ MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
#ifdef CONFIG_OF
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);
@@ -384,6 +386,8 @@ MODULE_DEVICE_TABLE(of, ad5820_of_table);
#ifdef CONFIG_ACPI
static const struct acpi_device_id ad5820_acpi_ids[] = {
{ "AD5820" },
+ { "AD5821" },
+ { "AD5823" },
{ }
};
@@ -394,7 +398,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 = of_match_ptr(ad5820_of_table),
.acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
--
2.18.0
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]>
Acked-by: Pavel Machek <[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
Allow module autoloading of ad5820 ACPI devices.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/media/i2c/ad5820.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index e461d36201a4..5d1185e7f78d 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -22,6 +22,7 @@
* General Public License for more details.
*/
+#include <linux/acpi.h>
#include <linux/errno.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
@@ -380,6 +381,15 @@ static const struct of_device_id ad5820_of_table[] = {
MODULE_DEVICE_TABLE(of, ad5820_of_table);
#endif
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id ad5820_acpi_ids[] = {
+ { "AD5820" },
+ { }
+};
+
+MODULE_DEVICE_TABLE(acpi, ad5820_acpi_ids);
+#endif
+
static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
static struct i2c_driver ad5820_i2c_driver = {
@@ -387,6 +397,7 @@ static struct i2c_driver ad5820_i2c_driver = {
.name = AD5820_NAME,
.pm = &ad5820_pm,
.of_match_table = of_match_ptr(ad5820_of_table),
+ .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
},
.probe = ad5820_probe,
.remove = ad5820_remove,
--
2.18.0
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]>
Acked-by: Pavel Machek <[email protected]>
---
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.18.0
On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote:
> Document new enable-gpio field. It can be used to disable the part
enable-gpios
> without turning down its regulator.
>
> Cc: [email protected]
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> ---
> 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).
shutdown-gpios is also standard and seems like it would make more sense
here. Yes, it is a bit redundant to have both, but things just evolved
that way and we don't want to totally abandon the hardware names (just
all the variants).
Rob
On Thu, 20 Sep 2018 22:47:51 +0200, Ricardo Ribalda Delgado wrote:
> Document new compatible devices.
>
> Cc: [email protected]
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
Reviewed-by: Rob Herring <[email protected]>
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:48PM +0200, 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]>
> Acked-by: Pavel Machek <[email protected]>
> ---
> drivers/media/i2c/ad5820.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index 625867472929..e461d36201a4 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -372,12 +372,21 @@ static const struct i2c_device_id ad5820_id_table[] = {
> };
> MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id ad5820_of_table[] = {
> + { .compatible = "adi,ad5820" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad5820_of_table);
> +#endif
> +
> 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 = of_match_ptr(ad5820_of_table),
No need to use of_match_ptr() or #ifdef above --- not doing so makes this
work on ACPI, too.
> },
> .probe = ad5820_probe,
> .remove = ad5820_remove,
--
Regards,
Sakari Ailus
e-mail: [email protected]
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:49PM +0200, Ricardo Ribalda Delgado wrote:
> Allow module autoloading of ad5820 ACPI devices.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/media/i2c/ad5820.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index e461d36201a4..5d1185e7f78d 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -22,6 +22,7 @@
> * General Public License for more details.
> */
>
> +#include <linux/acpi.h>
> #include <linux/errno.h>
> #include <linux/i2c.h>
> #include <linux/kernel.h>
> @@ -380,6 +381,15 @@ static const struct of_device_id ad5820_of_table[] = {
> MODULE_DEVICE_TABLE(of, ad5820_of_table);
> #endif
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ad5820_acpi_ids[] = {
> + { "AD5820" },
This is not a valid ACPI _HID. Is there a need to add ACPI support for the
chip this way?
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, ad5820_acpi_ids);
> +#endif
> +
> static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
>
> static struct i2c_driver ad5820_i2c_driver = {
> @@ -387,6 +397,7 @@ static struct i2c_driver ad5820_i2c_driver = {
> .name = AD5820_NAME,
> .pm = &ad5820_pm,
> .of_match_table = of_match_ptr(ad5820_of_table),
> + .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
> },
> .probe = ad5820_probe,
> .remove = ad5820_remove,
--
Regards,
Sakari Ailus
e-mail: [email protected]
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:50PM +0200, 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
A silly question --- the maximum current of these devices differs from each
other. Is the control value range still the same?
>
> Suggested-by: Pavel Machek <[email protected]>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/media/i2c/ad5820.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index 5d1185e7f78d..c52af302d516 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -34,8 +34,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
> @@ -368,7 +366,9 @@ 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);
> @@ -376,6 +376,8 @@ MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
> #ifdef CONFIG_OF
> static const struct of_device_id ad5820_of_table[] = {
> { .compatible = "adi,ad5820" },
> + { .compatible = "adi,ad5821" },
> + { .compatible = "adi,ad5823" },
You could set the subdev name accordingly as well.
> { }
> };
> MODULE_DEVICE_TABLE(of, ad5820_of_table);
> @@ -384,6 +386,8 @@ MODULE_DEVICE_TABLE(of, ad5820_of_table);
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id ad5820_acpi_ids[] = {
> { "AD5820" },
> + { "AD5821" },
> + { "AD5823" },
> { }
> };
>
> @@ -394,7 +398,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 = of_match_ptr(ad5820_of_table),
> .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
--
Regards,
Sakari Ailus
e-mail: [email protected]
Hi Sakari
On Thu, Sep 27, 2018 at 9:35 PM Sakari Ailus <[email protected]> wrote:
>
> Hi Ricardo,
>
> On Thu, Sep 20, 2018 at 10:47:50PM +0200, 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
>
> A silly question --- the maximum current of these devices differs from each
> other. Is the control value range still the same?
AFAIK yes, and fortuntately/unfortunatelly the control interface is in
a value, not in Amp, so there is nothing to convert on the driver.
Regards!
>
> >
> > Suggested-by: Pavel Machek <[email protected]>
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > drivers/media/i2c/ad5820.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> > index 5d1185e7f78d..c52af302d516 100644
> > --- a/drivers/media/i2c/ad5820.c
> > +++ b/drivers/media/i2c/ad5820.c
> > @@ -34,8 +34,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
> > @@ -368,7 +366,9 @@ 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);
> > @@ -376,6 +376,8 @@ MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
> > #ifdef CONFIG_OF
> > static const struct of_device_id ad5820_of_table[] = {
> > { .compatible = "adi,ad5820" },
> > + { .compatible = "adi,ad5821" },
> > + { .compatible = "adi,ad5823" },
>
> You could set the subdev name accordingly as well.
>
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, ad5820_of_table);
> > @@ -384,6 +386,8 @@ MODULE_DEVICE_TABLE(of, ad5820_of_table);
> > #ifdef CONFIG_ACPI
> > static const struct acpi_device_id ad5820_acpi_ids[] = {
> > { "AD5820" },
> > + { "AD5821" },
> > + { "AD5823" },
> > { }
> > };
> >
> > @@ -394,7 +398,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 = of_match_ptr(ad5820_of_table),
> > .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
>
> --
> Regards,
>
> Sakari Ailus
> e-mail: [email protected]
--
Ricardo Ribalda
Hi Rob
On Thu, Sep 27, 2018 at 8:23 PM Rob Herring <[email protected]> wrote:
>
> On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote:
> > Document new enable-gpio field. It can be used to disable the part
>
> enable-gpios
>
> > without turning down its regulator.
> >
> > Cc: [email protected]
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > Acked-by: Pavel Machek <[email protected]>
> > ---
> > 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).
>
> shutdown-gpios is also standard and seems like it would make more sense
> here. Yes, it is a bit redundant to have both, but things just evolved
> that way and we don't want to totally abandon the hardware names (just
> all the variants).
>
Sorry to insist
The pin is called xshutdown, not shutdown and is inverse logic,
Wouldnt it make more sense to use the name
enable-gpios?
Regards
> Rob
--
Ricardo Ribalda
On Mon, Oct 1, 2018 at 3:20 AM Ricardo Ribalda Delgado
<[email protected]> wrote:
>
> Hi Rob
> On Thu, Sep 27, 2018 at 8:23 PM Rob Herring <[email protected]> wrote:
> >
> > On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote:
> > > Document new enable-gpio field. It can be used to disable the part
> >
> > enable-gpios
> >
> > > without turning down its regulator.
> > >
> > > Cc: [email protected]
> > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > > Acked-by: Pavel Machek <[email protected]>
> > > ---
> > > 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).
> >
> > shutdown-gpios is also standard and seems like it would make more sense
> > here. Yes, it is a bit redundant to have both, but things just evolved
> > that way and we don't want to totally abandon the hardware names (just
> > all the variants).
> >
>
> Sorry to insist
>
> The pin is called xshutdown, not shutdown and is inverse logic,
> Wouldnt it make more sense to use the name
> enable-gpios?
Inverse of what? shutdown-gpios is the inverse of enable-gpios. By
using shutdown-gpios you can just get rid of "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)."
This looks to me like a case of just standardizing the name so for
example we just have "reset" instead of many flavors like rst, RSTb,
RESETb, RESETn, nRESET, etc.
Rob
Hi
On Mon, Oct 1, 2018 at 2:36 PM Rob Herring <[email protected]> wrote:
>
> On Mon, Oct 1, 2018 at 3:20 AM Ricardo Ribalda Delgado
> <[email protected]> wrote:
> >
> > Hi Rob
> > On Thu, Sep 27, 2018 at 8:23 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote:
> > > > Document new enable-gpio field. It can be used to disable the part
> > >
> > > enable-gpios
> > >
> > > > without turning down its regulator.
> > > >
> > > > Cc: [email protected]
> > > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > > > Acked-by: Pavel Machek <[email protected]>
> > > > ---
> > > > 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).
> > >
> > > shutdown-gpios is also standard and seems like it would make more sense
> > > here. Yes, it is a bit redundant to have both, but things just evolved
> > > that way and we don't want to totally abandon the hardware names (just
> > > all the variants).
> > >
> >
> > Sorry to insist
> >
> > The pin is called xshutdown, not shutdown and is inverse logic,
> > Wouldnt it make more sense to use the name
> > enable-gpios?
>
> Inverse of what? shutdown-gpios is the inverse of enable-gpios. By
> using shutdown-gpios you can just get rid of "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)."
The pin is called XSHUTDOWN
0V means shutdown
3.3V means enable
This is why I think is more clear to use enable as name in the device tree.
>
> This looks to me like a case of just standardizing the name so for
> example we just have "reset" instead of many flavors like rst, RSTb,
> RESETb, RESETn, nRESET, etc.
>
> Rob
--
Ricardo Ribalda
On Mon, Oct 1, 2018 at 7:40 AM Ricardo Ribalda Delgado
<[email protected]> wrote:
>
> Hi
> On Mon, Oct 1, 2018 at 2:36 PM Rob Herring <[email protected]> wrote:
> >
> > On Mon, Oct 1, 2018 at 3:20 AM Ricardo Ribalda Delgado
> > <[email protected]> wrote:
> > >
> > > Hi Rob
> > > On Thu, Sep 27, 2018 at 8:23 PM Rob Herring <[email protected]> wrote:
> > > >
> > > > On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote:
> > > > > Document new enable-gpio field. It can be used to disable the part
> > > >
> > > > enable-gpios
> > > >
> > > > > without turning down its regulator.
> > > > >
> > > > > Cc: [email protected]
> > > > > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > > > > Acked-by: Pavel Machek <[email protected]>
> > > > > ---
> > > > > 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).
> > > >
> > > > shutdown-gpios is also standard and seems like it would make more sense
> > > > here. Yes, it is a bit redundant to have both, but things just evolved
> > > > that way and we don't want to totally abandon the hardware names (just
> > > > all the variants).
> > > >
> > >
> > > Sorry to insist
> > >
> > > The pin is called xshutdown, not shutdown and is inverse logic,
> > > Wouldnt it make more sense to use the name
> > > enable-gpios?
> >
> > Inverse of what? shutdown-gpios is the inverse of enable-gpios. By
> > using shutdown-gpios you can just get rid of "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)."
>
> The pin is called XSHUTDOWN
>
> 0V means shutdown
>
> 3.3V means enable
>
> This is why I think is more clear to use enable as name in the device tree.
Neither enable-gpios nor shutdown-gpios have a defined polarity. The
polarity is part of the flags cell in the specifier.
Rob
Hello,
On Monday, 1 October 2018 18:01:42 EEST Rob Herring wrote:
> On Mon, Oct 1, 2018 at 7:40 AM Ricardo Ribalda Delgado wrote:
> > On Mon, Oct 1, 2018 at 2:36 PM Rob Herring wrote:
> >> On Mon, Oct 1, 2018 at 3:20 AM Ricardo Ribalda Delgado wrote:
> >>> On Thu, Sep 27, 2018 at 8:23 PM Rob Herring wrote:
> >>>> On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado
wrote:
> >>>>> Document new enable-gpio field. It can be used to disable the part
> >>>>> enable-gpios without turning down its regulator.
> >>>>>
> >>>>> Cc: [email protected]
> >>>>> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> >>>>> Acked-by: Pavel Machek <[email protected]>
> >>>>> ---
> >>>>>
> >>>>> 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).
> >>>>
> >>>> shutdown-gpios is also standard and seems like it would make more
> >>>> sense here. Yes, it is a bit redundant to have both, but things just
> >>>> evolved that way and we don't want to totally abandon the hardware
> >>>> names (just all the variants).
> >>>
> >>> Sorry to insist
> >>>
> >>> The pin is called xshutdown, not shutdown and is inverse logic,
> >>> Wouldnt it make more sense to use the name enable-gpios?
> >>
> >> Inverse of what? shutdown-gpios is the inverse of enable-gpios. By
> >> using shutdown-gpios you can just get rid of "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)."
> >
> > The pin is called XSHUTDOWN
> >
> > 0V means shutdown
> >
> > 3.3V means enable
> >
> > This is why I think is more clear to use enable as name in the device
> > tree.
>
> Neither enable-gpios nor shutdown-gpios have a defined polarity. The
> polarity is part of the flags cell in the specifier.
To be precise, the polarity is the relationship between the logical level (low
or high) *on the GPIO side* and the logical state (asserted or deasserted) of
the signal *on the device side*. This is important in order to take all
components on the signal path into account, such as inverters on the board.
The polarity does tell what level to output on the GPIO in order to achieve a
given effect.
The polarity, however, doesn't dictate what effect is expected. This is
defined by the DT bindings (together with the documentation of the device).
For instance an enable-gpios property in DT implies that an asserted logical
state will enable the device. The GPIO polarity flags thus take into account a
possible inverter at the device input (as in the difference between the ENABLE
and nENABLE signals), but stops there.
In this case, we have an XSHUTDOWN pin that will shut the device down when
driven to 0V. If we call the related DT property shutdown, its logical level
will be the inverse of XSHUTDOWN: the signal will need to be driven low to
assert the shutdown effect. The GPIO flags will thus need to take this into
account, and documenting it in DT could be useful to avoid errors.
On the other hand, if we call the related DT property enable its logical level
will the the same as XSHUTDOWN: the signal will need to be driven high to
assert the enable effect.
On the driver side we would have to deassert shutdown or assert enable to
enable the device.
I don't mind which option is selected, as long as the DT bindings are clear
(without any inverter in the signal path beside the one inside the ad5820, the
polarity would need to be high for the enable case and low for the shutdown
case).
--
Regards,
Laurent Pinchart
Hi Laurent
On Mon, Oct 1, 2018 at 5:55 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hello,
>
> On Monday, 1 October 2018 18:01:42 EEST Rob Herring wrote:
> > On Mon, Oct 1, 2018 at 7:40 AM Ricardo Ribalda Delgado wrote:
> > > On Mon, Oct 1, 2018 at 2:36 PM Rob Herring wrote:
> > >> On Mon, Oct 1, 2018 at 3:20 AM Ricardo Ribalda Delgado wrote:
> > >>> On Thu, Sep 27, 2018 at 8:23 PM Rob Herring wrote:
> > >>>> On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado
> wrote:
> > >>>>> Document new enable-gpio field. It can be used to disable the part
> > >>>>> enable-gpios without turning down its regulator.
> > >>>>>
> > >>>>> Cc: [email protected]
> > >>>>> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > >>>>> Acked-by: Pavel Machek <[email protected]>
> > >>>>> ---
> > >>>>>
> > >>>>> 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).
> > >>>>
> > >>>> shutdown-gpios is also standard and seems like it would make more
> > >>>> sense here. Yes, it is a bit redundant to have both, but things just
> > >>>> evolved that way and we don't want to totally abandon the hardware
> > >>>> names (just all the variants).
> > >>>
> > >>> Sorry to insist
> > >>>
> > >>> The pin is called xshutdown, not shutdown and is inverse logic,
> > >>> Wouldnt it make more sense to use the name enable-gpios?
> > >>
> > >> Inverse of what? shutdown-gpios is the inverse of enable-gpios. By
> > >> using shutdown-gpios you can just get rid of "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)."
> > >
> > > The pin is called XSHUTDOWN
> > >
> > > 0V means shutdown
> > >
> > > 3.3V means enable
> > >
> > > This is why I think is more clear to use enable as name in the device
> > > tree.
> >
> > Neither enable-gpios nor shutdown-gpios have a defined polarity. The
> > polarity is part of the flags cell in the specifier.
>
> To be precise, the polarity is the relationship between the logical level (low
> or high) *on the GPIO side* and the logical state (asserted or deasserted) of
> the signal *on the device side*. This is important in order to take all
> components on the signal path into account, such as inverters on the board.
> The polarity does tell what level to output on the GPIO in order to achieve a
> given effect.
>
> The polarity, however, doesn't dictate what effect is expected. This is
> defined by the DT bindings (together with the documentation of the device).
> For instance an enable-gpios property in DT implies that an asserted logical
> state will enable the device. The GPIO polarity flags thus take into account a
> possible inverter at the device input (as in the difference between the ENABLE
> and nENABLE signals), but stops there.
>
> In this case, we have an XSHUTDOWN pin that will shut the device down when
> driven to 0V. If we call the related DT property shutdown, its logical level
> will be the inverse of XSHUTDOWN: the signal will need to be driven low to
> assert the shutdown effect. The GPIO flags will thus need to take this into
> account, and documenting it in DT could be useful to avoid errors.
>
> On the other hand, if we call the related DT property enable its logical level
> will the the same as XSHUTDOWN: the signal will need to be driven high to
> assert the enable effect.
>
> On the driver side we would have to deassert shutdown or assert enable to
> enable the device.
>
> I don't mind which option is selected, as long as the DT bindings are clear
> (without any inverter in the signal path beside the one inside the ad5820, the
> polarity would need to be high for the enable case and low for the shutdown
> case).
Thanks for the clarification. I definitely prefer the name enable, so
if there is no strong opposition against it I will
send it with that name.
Best regards!
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
--
Ricardo Ribalda