Re: Nokia N900 - audio TPA6130A2 problems

[Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>]

On 21.03.2016 21:34, Ivaylo Dimitrov wrote:
>
>
> On 21.03.2016 16:53, Sebastian Reichel wrote:
>> Hi Mark,
>>
>> On Mon, Mar 21, 2016 at 01:45:15PM +0000, Mark Brown wrote:
>>> On Mon, Mar 21, 2016 at 03:39:15PM +0200, Ivaylo Dimitrov wrote:
>>>> On 21.03.2016 13:45, Mark Brown wrote:
>>>
>>>>> No, if the voltage is variable we can't tell what the current
>>>>> constraints are without something telling us so we just don't vary the
>>>>> voltage until we're told to do this.  If we immediately lower the
>>>>> voltage to the minimum supported voltage that's going to break things.
>>>
>>>> There are constraints set by the board DTS. Isn't it reasonable the
>>>> framework to set the voltage to minimum voltage from the dts if the
>>>> current
>>>> set one is bellow it?
>>>
>>> Yes, if it's out of bounds for the constraints we should bring it
>>> up/down to the minimum/maximum (when copying people into a thread it's a
>>> good idea to explain what the problem you are trying to solve is,
>>> especially if you're throwing around bodges).
>>
>> We have this regulator definition in omap3-n900.dts:
>>
>> &vmmc2 {
>>     regulator-name = "V28_A";
>>     regulator-min-microvolt = <2800000>;
>>     regulator-max-microvolt = <3000000>;
>>     regulator-always-on; /* due VIO leak to AIC34 VDDs */
>> };
>>
>> The regulator is enabled during probe, but the voltage is not
>> configured. The default reset voltage of the regulator is 2.6V.
>> So basically when the regulator is enabled, it uses a voltage,
>> which is out of the DT specified range.
>>
>> We also have a second problem: If the system has been rebooted from
>> Nokia's stock kernel the regulator is left in STANDBY mode. Since
>> the mode is not configured during probe, it results in different
>> problems. According to my understanding it can be fixed trivially
>> by adding
>>
>> &vmmc2 {
>>      regulator-initial-mode = <2>;
>> };
>>
>
> doesn't work:
>
> "regulator-vmmc2: mapping for mode 2 not defined"
>
> twl-regulator is missing .of_map_mode function.
>
> Also, if we go that route, we should set the initial modes for all the
> regulators, not only vmmc2 (and not only for N900), as we don't really
> know what is the status of regulators at startup. I think a better
> approach is if regulator framework sets all always-on regulators to
> enabled, unless stated otherwise (which it already does iiuc).
>
> I think there is a bug in twl-regulator twl4030reg_enable() and/or
> twl4030reg_is_enabled() - the latter only checks if DEV_GRP is P1, but
> not for the actual state of the regulator (bits 3:0). Also, what looks
> suspicious to me is that all the regulators are put in P1 device group.
> Legacy board code spreads the regulators all over the groups, so maybe
> this is simply a regression compared to legacy boot.
>

This is what seems to work, I would like some comments from those who 
are more experienced with twl4030 than me before posting a formal patch. 
I borrowed the code from stock Nokia kernel.

diff --git a/drivers/regulator/twl-regulator.c 
b/drivers/regulator/twl-regulator.c
index 955a6fb..3740df4 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -21,7 +21,7 @@
  #include <linux/regulator/machine.h>
  #include <linux/regulator/of_regulator.h>
  #include <linux/i2c/twl.h>
-
+#include <linux/delay.h>

  /*
   * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power 
management, a
@@ -165,7 +165,7 @@ static int twl4030reg_is_enabled(struct 
regulator_dev *rdev)
  	if (state < 0)
  		return state;

-	return state & P1_GRP_4030;
+	return (state & 0x0f) != 0;
  }

  static int twl6030reg_is_enabled(struct regulator_dev *rdev)
@@ -188,11 +188,75 @@ static int twl6030reg_is_enabled(struct 
regulator_dev *rdev)
  	return grp && (val == TWL6030_CFG_STATE_ON);
  }

+static int twl4030_wait_pb_ready(void)
+{
+
+	int ret, timeout = 10;
+	u8 pb_state;
+
+	do {
+		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &pb_state,
+				      TWL4030_PM_MASTER_PB_CFG);
+		if (ret < 0)
+			return ret;
+
+		if (!(pb_state & 1))
+			return 0;
+
+		mdelay(1);
+		timeout--;
+
+	} while (timeout);
+
+	return -ETIMEDOUT;
+}
+
+static int twl4030_send_pb_msg(unsigned msg)
+{
+	u8 pb_state;
+	int ret;
+
+	/* save powerbus configuration */
+	ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &pb_state,
+			      TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	/* Enable I2C access to powerbus */
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, pb_state | BIT(1),
+			       TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg >> 8,
+			       TWL4030_PM_MASTER_PB_WORD_MSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg & 0xff,
+			       TWL4030_PM_MASTER_PB_WORD_LSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	/* Restore powerbus configuration */
+	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, pb_state,
+				TWL_MODULE_PM_MASTER);
+}
+
  static int twl4030reg_enable(struct regulator_dev *rdev)
  {
  	struct twlreg_info	*info = rdev_get_drvdata(rdev);
  	int			grp;
  	int			ret;
+	unsigned 		message;

  	grp = twlreg_grp(rdev);
  	if (grp < 0)
@@ -201,8 +265,12 @@ static int twl4030reg_enable(struct regulator_dev 
*rdev)
  	grp |= P1_GRP_4030;

  	ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_GRP, grp);
+	if (ret < 0)
+		return ret;

-	return ret;
+	message = MSG_SINGULAR(DEV_GRP_P1, info->id, RES_STATE_ACTIVE);
+
+	return twl4030_send_pb_msg(message);
  }

  static int twl6030reg_enable(struct regulator_dev *rdev)
@@ -324,13 +392,7 @@ static int twl4030reg_set_mode(struct regulator_dev 
*rdev, unsigned mode)
  	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
  		return -EACCES;

-	status = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message >> 8, TWL4030_PM_MASTER_PB_WORD_MSB);
-	if (status < 0)
-		return status;
-
-	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message & 0xff, TWL4030_PM_MASTER_PB_WORD_LSB);
+	return twl4030_send_pb_msg(message);
  }

  static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)

Regards,
Ivo