[PATCH 1/2] ACPI / PMIC: xpower: Fix power_table addresses

[PATCH 1/2] ACPI / PMIC: xpower: Fix power_table addresses

[Hans de Goede]

The power table addresses should be contiguous, but there was a hole
where 0x34 was missing. On most devices this is not a problem as
addresses above 0x34 are used for the BUC# convertors which are not
used in the DSDTs I've access to but after the BUC# convertors
there is a field named GPI1 in the DSTDs, which does get used in some
cases and ended up turning BUC6 on and off due to the wrong addresses,
resulting in turning the entire device off (or causing it to reboot).

Removing the hole in the addresses fixes this, fixing one of my
Bay Trail tablets turning off while booting the mainline kernel.

While at it add comments with the field names used in the DSDTs to
make it easier to compare the register and bits used at each address
with the datasheet.

Cc: russianneuromancer@ya.ru
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic_xpower.c | 50 +++++++++++++++++------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 55f5111..1a76c78 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -27,97 +27,97 @@ static struct pmic_table power_table[] = {
 		.address = 0x00,
 		.reg = 0x13,
 		.bit = 0x05,
-	},
+	}, /* ALD1 */
 	{
 		.address = 0x04,
 		.reg = 0x13,
 		.bit = 0x06,
-	},
+	}, /* ALD2 */
 	{
 		.address = 0x08,
 		.reg = 0x13,
 		.bit = 0x07,
-	},
+	}, /* ALD3 */
 	{
 		.address = 0x0c,
 		.reg = 0x12,
 		.bit = 0x03,
-	},
+	}, /* DLD1 */
 	{
 		.address = 0x10,
 		.reg = 0x12,
 		.bit = 0x04,
-	},
+	}, /* DLD2 */
 	{
 		.address = 0x14,
 		.reg = 0x12,
 		.bit = 0x05,
-	},
+	}, /* DLD3 */
 	{
 		.address = 0x18,
 		.reg = 0x12,
 		.bit = 0x06,
-	},
+	}, /* DLD4 */
 	{
 		.address = 0x1c,
 		.reg = 0x12,
 		.bit = 0x00,
-	},
+	}, /* ELD1 */
 	{
 		.address = 0x20,
 		.reg = 0x12,
 		.bit = 0x01,
-	},
+	}, /* ELD2 */
 	{
 		.address = 0x24,
 		.reg = 0x12,
 		.bit = 0x02,
-	},
+	}, /* ELD3 */
 	{
 		.address = 0x28,
 		.reg = 0x13,
 		.bit = 0x02,
-	},
+	}, /* FLD1 */
 	{
 		.address = 0x2c,
 		.reg = 0x13,
 		.bit = 0x03,
-	},
+	}, /* FLD2 */
 	{
 		.address = 0x30,
 		.reg = 0x13,
 		.bit = 0x04,
-	},
+	}, /* FLD3 */
 	{
-		.address = 0x38,
+		.address = 0x34,
 		.reg = 0x10,
 		.bit = 0x03,
-	},
+	}, /* BUC1 */
 	{
-		.address = 0x3c,
+		.address = 0x38,
 		.reg = 0x10,
 		.bit = 0x06,
-	},
+	}, /* BUC2 */
 	{
-		.address = 0x40,
+		.address = 0x3c,
 		.reg = 0x10,
 		.bit = 0x05,
-	},
+	}, /* BUC3 */
 	{
-		.address = 0x44,
+		.address = 0x40,
 		.reg = 0x10,
 		.bit = 0x04,
-	},
+	}, /* BUC4 */
 	{
-		.address = 0x48,
+		.address = 0x44,
 		.reg = 0x10,
 		.bit = 0x01,
-	},
+	}, /* BUC5 */
 	{
-		.address = 0x4c,
+		.address = 0x48,
 		.reg = 0x10,
 		.bit = 0x00
-	},
+	}, /* BUC6 */
 };
 
 /* TMP0 - TMP5 are the same, all from GPADC */
-- 
2.9.3

[PATCH 2/2] ACPI / PMIC: xpower: Add support for the GPI1 regulator to the OpRegion handler

[Hans de Goede]

Some Bay Trail devices use a GPI1 regulator field (address 0x4c) in
their 0x8d power OpRegion, add support for this.

This fixes AE_BAD_PARAMETER errors getting thrown on these devices and
fixes these errors causing these devices to not suspend.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic_xpower.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 1a76c78..b8af87c 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -118,6 +118,10 @@ static struct pmic_table power_table[] = {
 		.reg = 0x10,
 		.bit = 0x00
 	}, /* BUC6 */
+	{
+		.address = 0x4c,
+		.reg = 0x92,
+	}, /* GPI1 */
 };
 
 /* TMP0 - TMP5 are the same, all from GPADC */
@@ -165,6 +169,16 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
 {
 	int data;
 
+	/* GPIO1 LDO regulator needs special handling */
+	if (reg == 0x92) {
+		if (on)
+			data = 0x03;
+		else
+			data = 0x04;
+
+		return regmap_update_bits(regmap, reg, 0x07, data);
+	}
+
 	if (regmap_read(regmap, reg, &data))
 		return -EIO;
 
-- 
2.9.3

Re: [PATCH 2/2] ACPI / PMIC: xpower: Add support for the GPI1 regulator to the OpRegion handler

[Andy Shevchenko]

On Fri, 2017-04-21 at 09:34 +0200, Hans de Goede wrote:
> Some Bay Trail devices use a GPI1 regulator field (address 0x4c) in
> their 0x8d power OpRegion, add support for this.
> 
> This fixes AE_BAD_PARAMETER errors getting thrown on these devices and
> fixes these errors causing these devices to not suspend.

>  	int data;
>  
> +	/* GPIO1 LDO regulator needs special handling */
> +	if (reg == 0x92) {
> +		if (on)
> +			data = 0x03;
> +		else
> +			data = 0x04;
> +
> +		return regmap_update_bits(regmap, reg, 0x07, data);
> +	}

I would go with

if (reg == 0x92)
 return regmap_update_bits(regmap, reg, 0x07, on ? 0x03 : 0x04);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/2] ACPI / PMIC: xpower: Fix power_table addresses

[Andy Shevchenko]

On Fri, 2017-04-21 at 09:34 +0200, Hans de Goede wrote:
> The power table addresses should be contiguous, but there was a hole
> where 0x34 was missing. On most devices this is not a problem as
> addresses above 0x34 are used for the BUC# convertors which are not
> used in the DSDTs I've access to but after the BUC# convertors
> there is a field named GPI1 in the DSTDs, which does get used in some
> cases and ended up turning BUC6 on and off due to the wrong addresses,
> resulting in turning the entire device off (or causing it to reboot).
> 
> Removing the hole in the addresses fixes this, fixing one of my
> Bay Trail tablets turning off while booting the mainline kernel.
> 
> While at it add comments with the field names used in the DSDTs to
> make it easier to compare the register and bits used at each address
> with the datasheet.

Makes sense.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Cc: russianneuromancer@ya.ru
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/pmic/intel_pmic_xpower.c | 50 +++++++++++++++++---------
> ---------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c
> b/drivers/acpi/pmic/intel_pmic_xpower.c
> index 55f5111..1a76c78 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -27,97 +27,97 @@ static struct pmic_table power_table[] = {
>  		.address = 0x00,
>  		.reg = 0x13,
>  		.bit = 0x05,
> -	},
> +	}, /* ALD1 */
>  	{
>  		.address = 0x04,
>  		.reg = 0x13,
>  		.bit = 0x06,
> -	},
> +	}, /* ALD2 */
>  	{
>  		.address = 0x08,
>  		.reg = 0x13,
>  		.bit = 0x07,
> -	},
> +	}, /* ALD3 */
>  	{
>  		.address = 0x0c,
>  		.reg = 0x12,
>  		.bit = 0x03,
> -	},
> +	}, /* DLD1 */
>  	{
>  		.address = 0x10,
>  		.reg = 0x12,
>  		.bit = 0x04,
> -	},
> +	}, /* DLD2 */
>  	{
>  		.address = 0x14,
>  		.reg = 0x12,
>  		.bit = 0x05,
> -	},
> +	}, /* DLD3 */
>  	{
>  		.address = 0x18,
>  		.reg = 0x12,
>  		.bit = 0x06,
> -	},
> +	}, /* DLD4 */
>  	{
>  		.address = 0x1c,
>  		.reg = 0x12,
>  		.bit = 0x00,
> -	},
> +	}, /* ELD1 */
>  	{
>  		.address = 0x20,
>  		.reg = 0x12,
>  		.bit = 0x01,
> -	},
> +	}, /* ELD2 */
>  	{
>  		.address = 0x24,
>  		.reg = 0x12,
>  		.bit = 0x02,
> -	},
> +	}, /* ELD3 */
>  	{
>  		.address = 0x28,
>  		.reg = 0x13,
>  		.bit = 0x02,
> -	},
> +	}, /* FLD1 */
>  	{
>  		.address = 0x2c,
>  		.reg = 0x13,
>  		.bit = 0x03,
> -	},
> +	}, /* FLD2 */
>  	{
>  		.address = 0x30,
>  		.reg = 0x13,
>  		.bit = 0x04,
> -	},
> +	}, /* FLD3 */
>  	{
> -		.address = 0x38,
> +		.address = 0x34,
>  		.reg = 0x10,
>  		.bit = 0x03,
> -	},
> +	}, /* BUC1 */
>  	{
> -		.address = 0x3c,
> +		.address = 0x38,
>  		.reg = 0x10,
>  		.bit = 0x06,
> -	},
> +	}, /* BUC2 */
>  	{
> -		.address = 0x40,
> +		.address = 0x3c,
>  		.reg = 0x10,
>  		.bit = 0x05,
> -	},
> +	}, /* BUC3 */
>  	{
> -		.address = 0x44,
> +		.address = 0x40,
>  		.reg = 0x10,
>  		.bit = 0x04,
> -	},
> +	}, /* BUC4 */
>  	{
> -		.address = 0x48,
> +		.address = 0x44,
>  		.reg = 0x10,
>  		.bit = 0x01,
> -	},
> +	}, /* BUC5 */
>  	{
> -		.address = 0x4c,
> +		.address = 0x48,
>  		.reg = 0x10,
>  		.bit = 0x00
> -	},
> +	}, /* BUC6 */
>  };
>  
>  /* TMP0 - TMP5 are the same, all from GPADC */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy