Received-SPF: pass (google.com: domain of linux-kernel+bounces-141561-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d7ec1064-b82c-4721-853a-06b5710fdffa@wolfvision.net>
Date: Thu, 11 Apr 2024 22:13:08 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RESEND 2/2] usb: typec: tipd: fix event checking for
 tps6598x
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Abdel Alkuor <abdelalkuor@geotab.com>, linux-usb@vger.kernel.org,
 linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20240328-tps6598x_fix_event_handling-v1-0-502721ff705b@wolfvision.net>
 <20240328-tps6598x_fix_event_handling-v1-2-502721ff705b@wolfvision.net>
 <ZgveG5Ly3mw0O0eo@kuha.fi.intel.com>
 <b6bf7f8e-7d46-4b70-930c-9483f13fd80a@wolfvision.net>
 <Zg+fD6w1MykCsEe6@kuha.fi.intel.com>
Content-Language: en-US, de-AT
From: Javier Carrasco <javier.carrasco@wolfvision.net>
In-Reply-To: <Zg+fD6w1MykCsEe6@kuha.fi.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RWI3VmIwcy9LejdhTFpONUJ0Y3lSS2hyY3Q4dFpMY2Rwbnl5QnV2am50SUMx?=
 =?utf-8?B?cEoyc0RRMklCdDJRQzNJU2kvMExYZzFnWVJGenkvZjJPUjg3cExJN094L1VP?=
 =?utf-8?B?eHBneFVEMVVVOXlXbGhPZ2xiMG1zTStQUDNOcjdrYjRVZ2Q0RUQ3RDdTd29Z?=
 =?utf-8?B?SUNPa0FCRElvL1dRYVhYTlRGT3BqSzFjcFZOb1hsTFF5MG42UE1hcnB0OVIx?=
 =?utf-8?B?RWRnWFlkaStLc0dSQzhtOWJkY0tqRlQwcWNTQnliRk5GK2d4dlFtYnhDRUJJ?=
 =?utf-8?B?SVcvZk5BdFl4SlRZQnAvdE1MV1NPSVp0VkMveXR1bGxUdUlLcS81WFp4aS9P?=
 =?utf-8?B?UXh2ckFkL3Jzak96b3RCQVpHTzdrV2lVQVFobFZ2cEw0Vzc4MjZDelplMUdm?=
 =?utf-8?B?TkVOUStHZTdFZm9seUVTU1VVYlNPNGFoaHlnbTVJR0pURUFmMU1LaXBzbkRh?=
 =?utf-8?B?RlB1bUNmM1VuMzhHdmJDbWNPMDhjTHJISUtyei8zSlR5b29SaFV3TWdqSUYy?=
 =?utf-8?B?TU1BTmdkQWtocGdzZllJS0hIaTNnVk5Cd0VqSjJiQVpKT0x4NTNRaU5FQnJv?=
 =?utf-8?B?dG5vaTZJN1kraUlnWVY1ZTRjemxXMUlVeGR6U00yckt0aUtwM2U1c1RrN1lT?=
 =?utf-8?B?dldQQVhWVTJSMzhsVndSSjZhdTJQWk5CS2NQejk2aCsvbHp3NGxhT3hndnlM?=
 =?utf-8?B?Rk1GSEYyYlZ5bFNyMWZteUkrRy9mRUpId3ptNnhockdrZ25GWjE1VythRWpy?=
 =?utf-8?B?cERLM0t3eWJpVW5JS3VaOS9RSnNVcjNRRlgrekduMGdmQmkwenl4dmw0a1Ra?=
 =?utf-8?B?Yi9BSFVZNHBOTTBxZjdtS1VKNEMyaXh4NURYaHlvYUEyMmVDOS92N2VIOTJH?=
 =?utf-8?B?dUZtVTdKT0ppK0pJSzk2NUhKbVlmbkxJQUpUaDNkeEN2UmovbXUxamt0NExa?=
 =?utf-8?B?NFk3MXd6ZkY5UWFESzVVdXBPTnNVQ2M3NTNzOWVQVU9TVStVYUtRdkpuOTJP?=
 =?utf-8?B?Mk1CbGVzVmdDbzFwbEljdW44eXVoUCthTTJ0eVRBVHVjTzRuOVB1WXpzTnlC?=
 =?utf-8?B?VFNNSms2ek1LZVJVQmVFV08xTVpORWU0NVdtSFZGcE5iZzVGSkd4eXZMOTdS?=
 =?utf-8?B?NGNWK0JrZ2RRZXBVZUVUN1IrUFp4REZVcWZJQjZFWmtFZ0JHQXZGUEdDT2o1?=
 =?utf-8?B?ZDRiUjB4dTZRY2RCa0Q1V1NwNVkzTGMxSWZkcy9DcEVDU1BFcURvcG5Zait5?=
 =?utf-8?B?R1pNSHRTNWphdmVrZmxTZVRzOGlIeEt2OGI2Z3R1L2gzb0JZUUtQZTZtQjRB?=
 =?utf-8?B?bDdBRzk1WjI1QVJHVGJBdjZDTmhHT3Z3RXpFdlJEMk9uYUtPenFrZmpYUFVZ?=
 =?utf-8?B?Vlg3L1VlNkVWN3VYRnJkYVFqVUpxWTRUbUVRR2hsVHM0WFovNFB0NlZQSmVt?=
 =?utf-8?B?M3kzSUoyVm1QN2s4S3g1MXVjMnFwRWZtNnFtcjhzajRrTG1VZVQyNVNLeHZm?=
 =?utf-8?B?RmhEczV3TFhpeDdPWTg0d2VhWldFV3hybEtHUHVsemFWbWVYdm1wc0t6YU80?=
 =?utf-8?B?RlYrRGxaSE84MlQrWWhFVFQvTHNscVZwcFU4YkJmeHRjTmxCYzVlcm95RXI2?=
 =?utf-8?B?dmtsd1A3RHVCOUJOdzdYVjFGVm1sRFhsS1NHc09BRlNnTUp0VlRuV0hOVjRt?=
 =?utf-8?B?cHF4ZXVlc2VuTGc3Szd0MGtIQ1JpeGl0d0J2RjBVWWIrOXFYeEp3YkhuNVJL?=
 =?utf-8?B?amJnckdyUmx0aUxyanE0T29QV2tQbGpOZWt4OVB5NTFVUGRHd0pvUkJTSm9j?=
 =?utf-8?B?Y2IzcnNyQ0VZUUw4dGRJUEoxcHJlanBsZ29qNVUxSU5jY0pTRllHLzNUNmVJ?=
 =?utf-8?B?ZXh4aFNQdmVIaVF2OTNpQlVVUlUvcGl2R09GSm1ROUJnSDAwa29oTTZhZnkz?=
 =?utf-8?B?YUN5dUNGNVpML24ycnF6ZlRjRGlCQzJuY0RKSXU2bks0NlRKR0E3ZHdzNHBB?=
 =?utf-8?B?dVN4T1VPcno4VFJsQndMeGkzOHJSNXo1TUVlQzY2SmdsUzB6UmtCRTNkMzcy?=
 =?utf-8?B?WWZ6Um5HK3lzL0V4Q3kvb0Z1VVBWYU8wczdqVjNxOUt5NmFWb3BEVWUxQTJM?=
 =?utf-8?B?bldHT3owQnFRditiQkI2dXp6OG13bjF1a1hmQmNleFozOGtyVWVhb3Y1OVFR?=
 =?utf-8?B?WUE9PQ==?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: e0bf64eb-64c9-4963-21b8-08dc5a63cf15
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4974.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2024 20:13:10.3755
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: e94ec9da-9183-471e-83b3-51baa8eb804f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LyczT6U79Yn5a2m+7QXR/0khM3bEcjZkQlRXSmwIzk9+gcDmzaDP1ZKvoMPCmucGth4bdtN4PWN4KnnLEsTyr9d5CafCnW1+3W3uUu+U2cU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR08MB5307

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