LinuxLists.cc - [PATCH v2] mfd: as3722: disable auto power on when AC OK

2018-07-03 15:16:12

Subject: [PATCH v2] mfd: as3722: disable auto power on when AC OK

From: Marcel Ziswiler <[email protected]>

On ams AS3722, power on when AC OK is enabled by default.
Making this option as disable by default and enable only
when platform need this explicitly.

Signed-off-by: Laxman Dewangan <[email protected]>
Reviewed-by: Bibek Basu <[email protected]>
Tested-by: Bibek Basu <[email protected]>
Signed-off-by: Marcel Ziswiler <[email protected]>

---

Changes in v2:
- Document device tree property as suggested by Stefan.
- Rename SEQ1 to SEQU1 as per datasheet as suggested by Stefan.
- Drop reference to downstream commit as suggested by Lee.

Documentation/devicetree/bindings/mfd/as3722.txt | 2 ++
drivers/mfd/as3722.c | 12 ++++++++++++
include/linux/mfd/as3722.h | 3 +++
3 files changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/as3722.txt b/Documentation/devicetree/bindings/mfd/as3722.txt
index 5297b2210704..2a665741d7fe 100644
--- a/Documentation/devicetree/bindings/mfd/as3722.txt
+++ b/Documentation/devicetree/bindings/mfd/as3722.txt
@@ -20,6 +20,8 @@ Optional properties:
- ams,enable-internal-i2c-pullup: Boolean property, to enable internal pullup on
i2c scl/sda pins. Missing this will disable internal pullup on i2c
scl/sda lines.
+- ams,enable-ac-ok-power-on: Boolean property, to enable exit out of power off
+ mode with AC_OK pin (pin enabled in power off mode).

Optional submodule and their properties:
=======================================
diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c
index f87342c211bc..4d069ed21ff6 100644
--- a/drivers/mfd/as3722.c
+++ b/drivers/mfd/as3722.c
@@ -349,6 +349,8 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c,
"ams,enable-internal-int-pullup");
as3722->en_intern_i2c_pullup = of_property_read_bool(np,
"ams,enable-internal-i2c-pullup");
+ as3722->en_ac_ok_pwr_on = of_property_read_bool(np,
+ "ams,enable-ac-ok-power-on");
as3722->irq_flags = irqd_get_trigger_type(irq_data);
dev_dbg(&i2c->dev, "IRQ flags are 0x%08lx\n", as3722->irq_flags);
return 0;
@@ -360,6 +362,7 @@ static int as3722_i2c_probe(struct i2c_client *i2c,
struct as3722 *as3722;
unsigned long irq_flags;
int ret;
+ u8 val = 0;

as3722 = devm_kzalloc(&i2c->dev, sizeof(struct as3722), GFP_KERNEL);
if (!as3722)
@@ -398,6 +401,15 @@ static int as3722_i2c_probe(struct i2c_client *i2c,
if (ret < 0)
return ret;

+ if (as3722->en_ac_ok_pwr_on)
+ val = AS3722_CTRL_SEQU1_AC_OK_PWR_ON;
+ ret = as3722_update_bits(as3722, AS3722_CTRL_SEQU1_REG,
+ AS3722_CTRL_SEQU1_AC_OK_PWR_ON, val);
+ if (ret < 0) {
+ dev_err(as3722->dev, "CTRLsequ1 update failed: %d\n", ret);
+ return ret;
+ }
+
ret = devm_mfd_add_devices(&i2c->dev, -1, as3722_devs,
ARRAY_SIZE(as3722_devs), NULL, 0,
regmap_irq_get_domain(as3722->irq_data));
diff --git a/include/linux/mfd/as3722.h b/include/linux/mfd/as3722.h
index 51e6f9414575..b404a5af9bba 100644
--- a/include/linux/mfd/as3722.h
+++ b/include/linux/mfd/as3722.h
@@ -296,6 +296,8 @@
#define AS3722_ADC1_CONV_NOTREADY BIT(7)
#define AS3722_ADC1_SOURCE_SELECT_MASK 0x1F

+#define AS3722_CTRL_SEQU1_AC_OK_PWR_ON BIT(0)
+
/* GPIO modes */
#define AS3722_GPIO_MODE_MASK 0x07
#define AS3722_GPIO_MODE_INPUT 0x00
@@ -391,6 +393,7 @@ struct as3722 {
unsigned long irq_flags;
bool en_intern_int_pullup;
bool en_intern_i2c_pullup;
+ bool en_ac_ok_pwr_on;
struct regmap_irq_chip_data *irq_data;
};

--
2.14.4

2018-07-04 07:52:57

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: as3722: disable auto power on when AC OK

On Tue, 03 Jul 2018, Marcel Ziswiler wrote:

> From: Marcel Ziswiler <[email protected]>
>
> On ams AS3722, power on when AC OK is enabled by default.
> Making this option as disable by default and enable only
> when platform need this explicitly.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
> Reviewed-by: Bibek Basu <[email protected]>
> Tested-by: Bibek Basu <[email protected]>
> Signed-off-by: Marcel Ziswiler <[email protected]>
>
> ---
>
> Changes in v2:
> - Document device tree property as suggested by Stefan.
> - Rename SEQ1 to SEQU1 as per datasheet as suggested by Stefan.
> - Drop reference to downstream commit as suggested by Lee.
>
> Documentation/devicetree/bindings/mfd/as3722.txt | 2 ++
> drivers/mfd/as3722.c | 12 ++++++++++++
> include/linux/mfd/as3722.h | 3 +++
> 3 files changed, 17 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/as3722.txt b/Documentation/devicetree/bindings/mfd/as3722.txt
> index 5297b2210704..2a665741d7fe 100644
> --- a/Documentation/devicetree/bindings/mfd/as3722.txt
> +++ b/Documentation/devicetree/bindings/mfd/as3722.txt
> @@ -20,6 +20,8 @@ Optional properties:
> - ams,enable-internal-i2c-pullup: Boolean property, to enable internal pullup on
> i2c scl/sda pins. Missing this will disable internal pullup on i2c
> scl/sda lines.
> +- ams,enable-ac-ok-power-on: Boolean property, to enable exit out of power off
> + mode with AC_OK pin (pin enabled in power off mode).

This needs a DT Ack.

> Optional submodule and their properties:
> =======================================
> diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c
> index f87342c211bc..4d069ed21ff6 100644
> --- a/drivers/mfd/as3722.c
> +++ b/drivers/mfd/as3722.c
> @@ -349,6 +349,8 @@ static int as3722_i2c_of_probe(struct i2c_client *i2c,
> "ams,enable-internal-int-pullup");
> as3722->en_intern_i2c_pullup = of_property_read_bool(np,
> "ams,enable-internal-i2c-pullup");
> + as3722->en_ac_ok_pwr_on = of_property_read_bool(np,
> + "ams,enable-ac-ok-power-on");
> as3722->irq_flags = irqd_get_trigger_type(irq_data);
> dev_dbg(&i2c->dev, "IRQ flags are 0x%08lx\n", as3722->irq_flags);
> return 0;
> @@ -360,6 +362,7 @@ static int as3722_i2c_probe(struct i2c_client *i2c,
> struct as3722 *as3722;
> unsigned long irq_flags;
> int ret;
> + u8 val = 0;
>
> as3722 = devm_kzalloc(&i2c->dev, sizeof(struct as3722), GFP_KERNEL);
> if (!as3722)
> @@ -398,6 +401,15 @@ static int as3722_i2c_probe(struct i2c_client *i2c,
> if (ret < 0)
> return ret;
>
> + if (as3722->en_ac_ok_pwr_on)
> + val = AS3722_CTRL_SEQU1_AC_OK_PWR_ON;
> + ret = as3722_update_bits(as3722, AS3722_CTRL_SEQU1_REG,
> + AS3722_CTRL_SEQU1_AC_OK_PWR_ON, val);

What is the default value?

If 0, you could place all of this code inside the "if
(as3722->en_ac_ok_pwr_on)" and save on a few unnecessary cycles.

> + if (ret < 0) {
> + dev_err(as3722->dev, "CTRLsequ1 update failed: %d\n", ret);
> + return ret;
> + }
> +
> ret = devm_mfd_add_devices(&i2c->dev, -1, as3722_devs,
> ARRAY_SIZE(as3722_devs), NULL, 0,
> regmap_irq_get_domain(as3722->irq_data));
> diff --git a/include/linux/mfd/as3722.h b/include/linux/mfd/as3722.h
> index 51e6f9414575..b404a5af9bba 100644
> --- a/include/linux/mfd/as3722.h
> +++ b/include/linux/mfd/as3722.h
> @@ -296,6 +296,8 @@
> #define AS3722_ADC1_CONV_NOTREADY BIT(7)
> #define AS3722_ADC1_SOURCE_SELECT_MASK 0x1F
>
> +#define AS3722_CTRL_SEQU1_AC_OK_PWR_ON BIT(0)
> +
> /* GPIO modes */
> #define AS3722_GPIO_MODE_MASK 0x07
> #define AS3722_GPIO_MODE_INPUT 0x00
> @@ -391,6 +393,7 @@ struct as3722 {
> unsigned long irq_flags;
> bool en_intern_int_pullup;
> bool en_intern_i2c_pullup;
> + bool en_ac_ok_pwr_on;
> struct regmap_irq_chip_data *irq_data;
> };
>

--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2018-07-04 12:04:44

by Marcel Ziswiler

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: as3722: disable auto power on when AC OK

On Wed, 2018-07-04 at 08:52 +0100, Lee Jones wrote:
> On Tue, 03 Jul 2018, Marcel Ziswiler wrote:
>
> > From: Marcel Ziswiler <[email protected]>
> >
> > On ams AS3722, power on when AC OK is enabled by default.
> > Making this option as disable by default and enable only
> > when platform need this explicitly.
> >
> > Signed-off-by: Laxman Dewangan <[email protected]>
> > Reviewed-by: Bibek Basu <[email protected]>
> > Tested-by: Bibek Basu <[email protected]>
> > Signed-off-by: Marcel Ziswiler <[email protected]>
> >
> > ---
> >
> > Changes in v2:
> > - Document device tree property as suggested by Stefan.
> > - Rename SEQ1 to SEQU1 as per datasheet as suggested by Stefan.
> > - Drop reference to downstream commit as suggested by Lee.
> >
> > Documentation/devicetree/bindings/mfd/as3722.txt | 2 ++
> > drivers/mfd/as3722.c | 12 ++++++++++++
> > include/linux/mfd/as3722.h | 3 +++
> > 3 files changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/as3722.txt
> > b/Documentation/devicetree/bindings/mfd/as3722.txt
> > index 5297b2210704..2a665741d7fe 100644
> > --- a/Documentation/devicetree/bindings/mfd/as3722.txt
> > +++ b/Documentation/devicetree/bindings/mfd/as3722.txt
> > @@ -20,6 +20,8 @@ Optional properties:
> > - ams,enable-internal-i2c-pullup: Boolean property, to enable
> > internal pullup on
> > i2c scl/sda pins. Missing this will disable internal
> > pullup on i2c
> > scl/sda lines.
> > +- ams,enable-ac-ok-power-on: Boolean property, to enable exit out
> > of power off
> > + mode with AC_OK pin (pin enabled in power off mode).
>
> This needs a DT Ack.

Yes, certainly. That's why I CC'd the device tree mailing list as well
as Mr. Herring himself.

> > Optional submodule and their properties:
> > =======================================
> > diff --git a/drivers/mfd/as3722.c b/drivers/mfd/as3722.c
> > index f87342c211bc..4d069ed21ff6 100644
> > --- a/drivers/mfd/as3722.c
> > +++ b/drivers/mfd/as3722.c
> > @@ -349,6 +349,8 @@ static int as3722_i2c_of_probe(struct
> > i2c_client *i2c,
> > "ams,enable-internal-int-
> > pullup");
> > as3722->en_intern_i2c_pullup = of_property_read_bool(np,
> > "ams,enable-internal-i2c-
> > pullup");
> > + as3722->en_ac_ok_pwr_on = of_property_read_bool(np,
> > + "ams,enable-ac-ok-power-
> > on");
> > as3722->irq_flags = irqd_get_trigger_type(irq_data);
> > dev_dbg(&i2c->dev, "IRQ flags are 0x%08lx\n", as3722-
> > >irq_flags);
> > return 0;
> > @@ -360,6 +362,7 @@ static int as3722_i2c_probe(struct i2c_client
> > *i2c,
> > struct as3722 *as3722;
> > unsigned long irq_flags;
> > int ret;
> > + u8 val = 0;
> >
> > as3722 = devm_kzalloc(&i2c->dev, sizeof(struct as3722),
> > GFP_KERNEL);
> > if (!as3722)
> > @@ -398,6 +401,15 @@ static int as3722_i2c_probe(struct i2c_client
> > *i2c,
> > if (ret < 0)
> > return ret;
> >
> > + if (as3722->en_ac_ok_pwr_on)
> > + val = AS3722_CTRL_SEQU1_AC_OK_PWR_ON;
> > + ret = as3722_update_bits(as3722, AS3722_CTRL_SEQU1_REG,
> > + AS3722_CTRL_SEQU1_AC_OK_PWR_ON, val);
>
> What is the default value?

Yes, sorry. I quess it may not be completely clear from Laxman's
original commit message.

> If 0, you could place all of this code inside the "if
> (as3722->en_ac_ok_pwr_on)" and save on a few unnecessary cycles.

Unfortunately, it is not quite that simple. While the hardware default
would actually be on at least Jetson TK1 as well as Toradex Apalis TK1
do not work that way. Therefore, my (resp. Laxman's) proposed way of
handling this was/is for the default to be off and anybody really
requiring it to be on having to do so via adding this device tree
property. I admit that this may be perceived as a behavioural change
however as mentioned above at least on Jetson TK1 as well as Toradex
Apalis TK1 this really fixes poweroff which otherwise immediately
powered on again. So this is really a bug fix. I may include some more
information in the commit message if requested.

Maybe somebody from NVIDIA could also comment on whether or not this is
really a good idea or how they would expect things to work. Anybody?

> > + if (ret < 0) {
> > + dev_err(as3722->dev, "CTRLsequ1 update failed:
> > %d\n", ret);
> > + return ret;
> > + }
> > +
> > ret = devm_mfd_add_devices(&i2c->dev, -1, as3722_devs,
> > ARRAY_SIZE(as3722_devs), NULL,
> > 0,
> > regmap_irq_get_domain(as3722-
> > >irq_data));
> > diff --git a/include/linux/mfd/as3722.h
> > b/include/linux/mfd/as3722.h
> > index 51e6f9414575..b404a5af9bba 100644
> > --- a/include/linux/mfd/as3722.h
> > +++ b/include/linux/mfd/as3722.h
> > @@ -296,6 +296,8 @@
> > #define AS3722_ADC1_CONV_NOTREADY BIT(7)
> > #define AS3722_ADC1_SOURCE_SELECT_MASK 0x1F
> >
> > +#define AS3722_CTRL_SEQU1_AC_OK_PWR_ON BIT(
> > 0)
> > +
> > /* GPIO modes */
> > #define AS3722_GPIO_MODE_MASK 0x07
> > #define AS3722_GPIO_MODE_INPUT 0x00
> > @@ -391,6 +393,7 @@ struct as3722 {
> > unsigned long irq_flags;
> > bool en_intern_int_pullup;
> > bool en_intern_i2c_pullup;
> > + bool en_ac_ok_pwr_on;
> > struct regmap_irq_chip_data *irq_data;
> > };

2018-07-11 18:35:15

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2] mfd: as3722: disable auto power on when AC OK

On Tue, Jul 03, 2018 at 05:04:11PM +0200, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <[email protected]>
>
> On ams AS3722, power on when AC OK is enabled by default.
> Making this option as disable by default and enable only
> when platform need this explicitly.
>
> Signed-off-by: Laxman Dewangan <[email protected]>
> Reviewed-by: Bibek Basu <[email protected]>
> Tested-by: Bibek Basu <[email protected]>
> Signed-off-by: Marcel Ziswiler <[email protected]>
>
> ---
>
> Changes in v2:
> - Document device tree property as suggested by Stefan.
> - Rename SEQ1 to SEQU1 as per datasheet as suggested by Stefan.
> - Drop reference to downstream commit as suggested by Lee.
>
> Documentation/devicetree/bindings/mfd/as3722.txt | 2 ++

Acked-by: Rob Herring <[email protected]>

> drivers/mfd/as3722.c | 12 ++++++++++++
> include/linux/mfd/as3722.h | 3 +++
> 3 files changed, 17 insertions(+)

2018-07-27 07:17:19

by Lee Jones

[permalink] [raw]