Re: [PATCH v2 0/4] usb: typec: tipd: add patch update support for tps6598x

[Roger Quadros <rogerq@kernel.org>]

On 04/01/2024 18:36, Javier Carrasco wrote:
> On 04.01.24 17:15, Roger Quadros wrote:
>>
>>
>> On 04/01/2024 17:47, Jai Luthra wrote:
>>> Hi Javier,
>>> The following change seems to fix boot on SK-AM62A without reverting 
>>> this whole series:
>>>
>>> ------------------
>>>
>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>> index a956eb976906a5..8ba2aa05db519b 100644
>>> --- a/drivers/usb/typec/tipd/core.c
>>> +++ b/drivers/usb/typec/tipd/core.c
>>> @@ -1223,11 +1223,16 @@ static int cd321x_reset(struct tps6598x *tps)
>>>  	return 0;
>>>  }
>>>  
>>> -static int tps6598x_reset(struct tps6598x *tps)
>>> +static int tps25750_reset(struct tps6598x *tps)
>>>  {
>>>  	return tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
>>>  }
>>>  
>>> +static int tps6598x_reset(struct tps6598x *tps)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>>  static int
>>>  tps25750_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>>>  {
>>> @@ -1545,7 +1550,7 @@ static const struct tipd_data tps25750_data = {
>>>  	.trace_status = trace_tps25750_status,
>>>  	.apply_patch = tps25750_apply_patch,
>>>  	.init = tps25750_init,
>>> -	.reset = tps6598x_reset,
>>> +	.reset = tps25750_reset,
>>>  };
>>>  
>>>  static const struct of_device_id tps6598x_of_match[] = {
>>>
>>> ------------------
>>>
>>> I am not an expert on this, will let you/others decide on what should be 
>>> the correct way to reset TPS6598x for patching without breaking this SK.
>>>
>>>
>>
>> This looks like a correct fix to me.
>> Could you please send a proper PATCH with Fixes tag? Thanks!
>>
> Hi Roger,
> 
> that fix only removes the reset function and does nothing instead, but
> the reset call is identical for both devices (hence why there was a
> single function for both devices). As I mentioned in my reply to Jai

This is incorrect.

Look at the original code, the GAID command is issued only if it is a
tps25750 device.

Below is your part of the code that replaces it with reset() ops.

> @@ -1340,8 +1360,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  	tps6598x_write64(tps, TPS_REG_INT_MASK1, 0);
>  err_reset_controller:
>  	/* Reset PD controller to remove any applied patch */
> -	if (is_tps25750)
> -		tps6598x_exec_cmd_tmo(tps, "GAID", 0, NULL, 0, NULL, 2000, 0);
> +	tps->data->reset(tps);
> +
>  	return ret;
>  }

which means the GAID command will be executed for both tps25750 and tps6598x
as you have a single reset function for both.
This is a problem for boards using the tps6598x.

-- 
cheers,
-roger