| Message ID | 20180920184552.4919-1-ricardo.ribalda@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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
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
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
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).
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.
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
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
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
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);
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(-)