Re: [PATCH RESEND 2/2] usb: typec: tipd: fix event checking for tps6598x

[Javier Carrasco <javier.carrasco@wolfvision.net>]

On 4/5/24 08:49, Heikki Krogerus wrote:
> On Wed, Apr 03, 2024 at 10:55:29AM +0200, Javier Carrasco wrote:
>>>> -	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1);
>>>> -	ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2);
>>>> +	ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event1, 11);
>>>
>>> This is not going to work with the older TI PD controllers.
>>>
>>> The lenght of these registers is 8 bytes on the older TI PD
>>> controllers (TPS65981, TPS65982, etc.). I think we need to split this
>>> function.
>>>
>>
>> That is a good point. I had a look at the older TI PD controllers and I
>> agree with you that we should split the function to cover both register
>> lengths separately.
>>
>> I was thinking about adding a new compatible for the newer PD
>> controllers (tps65987 and tps65988), keeping the current tps6598x for
>> the older ones as well as backwards compatibility. But backwards
>> compatibility would also mean that flags beyond the first 8 bytes would
>> be ignored.
>>
>> On the other hand, the upper flags are only relevant for firmware
>> updates, so we could check those (i.e. read 11 bytes) if a firmware was
>> provided via "firmware-name", and ignore them (i.e. read 8 bytes) otherwise.
>>
>> Other ideas or improvements to mine are more than welcome.
> 
> I don't have any good ideas. On ACPI platforms the same device ID may
> be used with all of these, so we should actually try to figure out the
> version from registers like VID, DID and Version (if they are
> available).
> 
> thanks,
> 

VID and DID can be modified by the application firmware, but there is a
byte in the Version register we can use for this. According to TI[1], it
is guaranteed that the older TI PD controllers (TPS65981/2/6) will
always deliver AB = 0x00 when reading from the Version register, which
is 4 bytes long formatted like this: ABXX.YY.ZZ. The newer PD
controllers (TPS65987/8) will return either AB = 0xF7 (DH parts) or AB =
0xF9 (DK parts).

We can add some simple logic to read 8 bytes if AB is 0x00, which could
be the default as well, and 11 bytes otherwise.

Link:
https://e2e.ti.com/support/power-management-group/power-management/f/power-management-forum/1346521/tps65987d-register-command-to-distinguish-between-tps6591-2-6-and-tps65987-8
[1]

Thanks for your feedback and best regards,
Javier Carrasco