diff mbox series

[2/2] usb: typec: tipd: Fix initialization sequence for cd321x

Message ID 20211117151450.207168-3-marcan@marcan.st (mailing list archive)
State Superseded
Headers show
Series usb: typec: tipd: Fixes for Apple M1 (CD321X) support | expand

Commit Message

Hector Martin Nov. 17, 2021, 3:14 p.m. UTC 
The power state switch needs to happen first, as that
kickstarts the firmware into normal mode.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Greg KH Nov. 17, 2021, 4:04 p.m. UTC | #1 
On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
> The power state switch needs to happen first, as that
> kickstarts the firmware into normal mode.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)

Same question here, what commit id does this fix?

thanks,

greg k-h
Hector Martin Nov. 17, 2021, 4:11 p.m. UTC | #2 
On 18/11/2021 01.04, Greg Kroah-Hartman wrote:
> On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
>> The power state switch needs to happen first, as that
>> kickstarts the firmware into normal mode.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> Same question here, what commit id does this fix?
> 
> thanks,
> 
> greg k-h
> 

Logically speaking, this fixes the same commit too (though it does it by
moving everything around the hunk it introduced instead), so:

Fixes: c9c14be664cf ("usb: typec: tipd: Switch CD321X power state to S0")

Thanks,
Heikki Krogerus Nov. 18, 2021, 1:12 p.m. UTC | #3 
Hi,

On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
> The power state switch needs to happen first, as that
> kickstarts the firmware into normal mode.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Please resend these with the appropriate Fixes tag included.

> ---
>  drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 4da5a0b2aed2..6d27a5b5e3ca 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -707,6 +707,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	u32 conf;
>  	u32 vid;
>  	int ret;
> +	u64 mask1;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>  	if (!tps)
> @@ -730,11 +731,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		tps->i2c_protocol = true;
>  
> -	/* Make sure the controller has application firmware running */
> -	ret = tps6598x_check_mode(tps);
> -	if (ret)
> -		return ret;
> -
>  	if (np && of_device_is_compatible(np, "apple,cd321x")) {
>  		/* Switch CD321X chips to the correct system power state */
>  		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
> @@ -742,24 +738,27 @@ static int tps6598x_probe(struct i2c_client *client)
>  			return ret;
>  
>  		/* CD321X chips have all interrupts masked initially */
> -		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> -					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> -					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> -					APPLE_CD_REG_INT_PLUG_EVENT);
> -		if (ret)
> -			return ret;
> +		mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +			APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +			APPLE_CD_REG_INT_PLUG_EVENT;
>  
>  		irq_handler = cd321x_interrupt;
>  	} else {
>  		/* Enable power status, data status and plug event interrupts */
> -		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> -				       TPS_REG_INT_POWER_STATUS_UPDATE |
> -				       TPS_REG_INT_DATA_STATUS_UPDATE |
> -				       TPS_REG_INT_PLUG_EVENT);
> -		if (ret)
> -			return ret;
> +		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> +			TPS_REG_INT_DATA_STATUS_UPDATE |
> +			TPS_REG_INT_PLUG_EVENT;
>  	}
>  
> +	/* Make sure the controller has application firmware running */
> +	ret = tps6598x_check_mode(tps);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
> +	if (ret)
> +		return ret;
> +
>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.33.0

thanks,
Hector Martin Nov. 20, 2021, 3:08 a.m. UTC | #4 
On 18/11/2021 22.12, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
>> The power state switch needs to happen first, as that
>> kickstarts the firmware into normal mode.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Please resend these with the appropriate Fixes tag included.

Done, thanks for the review!
diff mbox series

Patch

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 4da5a0b2aed2..6d27a5b5e3ca 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -707,6 +707,7 @@  static int tps6598x_probe(struct i2c_client *client)
 	u32 conf;
 	u32 vid;
 	int ret;
+	u64 mask1;
 
 	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
 	if (!tps)
@@ -730,11 +731,6 @@  static int tps6598x_probe(struct i2c_client *client)
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		tps->i2c_protocol = true;
 
-	/* Make sure the controller has application firmware running */
-	ret = tps6598x_check_mode(tps);
-	if (ret)
-		return ret;
-
 	if (np && of_device_is_compatible(np, "apple,cd321x")) {
 		/* Switch CD321X chips to the correct system power state */
 		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
@@ -742,24 +738,27 @@  static int tps6598x_probe(struct i2c_client *client)
 			return ret;
 
 		/* CD321X chips have all interrupts masked initially */
-		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
-					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
-					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
-					APPLE_CD_REG_INT_PLUG_EVENT);
-		if (ret)
-			return ret;
+		mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
+			APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
+			APPLE_CD_REG_INT_PLUG_EVENT;
 
 		irq_handler = cd321x_interrupt;
 	} else {
 		/* Enable power status, data status and plug event interrupts */
-		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
-				       TPS_REG_INT_POWER_STATUS_UPDATE |
-				       TPS_REG_INT_DATA_STATUS_UPDATE |
-				       TPS_REG_INT_PLUG_EVENT);
-		if (ret)
-			return ret;
+		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
+			TPS_REG_INT_DATA_STATUS_UPDATE |
+			TPS_REG_INT_PLUG_EVENT;
 	}
 
+	/* Make sure the controller has application firmware running */
+	ret = tps6598x_check_mode(tps);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
+	if (ret)
+		return ret;
+
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
 		return ret;