Received-SPF: pass (google.com: domain of linux-kernel+bounces-76719-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <174ce57a-3197-4251-831f-205ec5cfeae9@wolfvision.net>
Date: Thu, 22 Feb 2024 15:42:26 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 2/8] usb: misc: onboard_dev: add support for non-hub
 devices
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Helen Koike <helen.koike@collabora.com>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>,
 Russell King <linux@armlinux.org.uk>, linux-sound@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-usb@vger.kernel.org, dri-devel@lists.freedesktop.org,
 linux-arm-kernel@lists.infradead.org
References: <20240220-onboard_xvf3500-v4-0-dc1617cc5dd4@wolfvision.net>
 <20240220-onboard_xvf3500-v4-2-dc1617cc5dd4@wolfvision.net>
 <ZdZN3FIS4zcKe4Kw@google.com>
Content-Language: en-US
From: Javier Carrasco <javier.carrasco@wolfvision.net>
In-Reply-To: <ZdZN3FIS4zcKe4Kw@google.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?VjVDMDZoTEx2dWxLWHNBeDQ2Vkh0aGZPWFVxQTlQd01nWHVTZmpaN043RWtR?=
 =?utf-8?B?Tkx3aElxbkM2R3RjUXhhTW1zOFBCUzA0V1J1WFF6NTMrSnR4a0JqSDNtWVB3?=
 =?utf-8?B?UklOS3VxMU9LT0FUK2FUYm1TZ2RrVHBteG5UaWsvMTZyanN5Y3BGeUZDakIr?=
 =?utf-8?B?aUtTdnd3VTFvYmNNeTA0M0k3eVhFU3pMRjFGcEdPT1NoUlVuM3k2QS93bWc5?=
 =?utf-8?B?MkFJbndvZkxOd20wamwxaVNpQ2lPamdqK1V1L3VZMnhXQlQ0OUdYaHN1NWcy?=
 =?utf-8?B?K1BXYW9LbitzRENxVDlhZk03UFZWbFVXZXZQNkJXVit4MUx4M3dJQ0k3L2Ex?=
 =?utf-8?B?bkxSbUxIS3ZGUzM4R0NUdHZ2MFUxYVpONkFMbFBXUHBuU0RKQlRUdDRXNDlW?=
 =?utf-8?B?cW1oSmZLZksyYmJGYnJWN3JseGdZK3lNNzJCUTB1dVdualFzMWQ5S0k2V0NX?=
 =?utf-8?B?WVB0b2VjZ1VHcWpZNWFiUit5eW4vbHlsbDhLMWdPbWtuS2tCNXNQOFZBL0FS?=
 =?utf-8?B?Zi82ei9GT0JuajBuc3A5R2NCRUhTMlo0aWtrdlFIbmNFM2w3dFRvc21Ld3FL?=
 =?utf-8?B?OEZHR3NaN3hIbEp5TGFlQWNEbTdxdTA5SUsvR0grclZuNmUvbGpXRjZ3aVM4?=
 =?utf-8?B?RjFBYlcwQis2Y3AwRjU1Q2hWaUoreUE3b2tnbTBOUHpYeEVuRWhMMGNMbHJ2?=
 =?utf-8?B?azg0SDBkeXFVL0VGNnlwazE4TENOWnB0a2NYejFaNDZVem9ackNBeSs2NTc2?=
 =?utf-8?B?WEduYk1KTDhIWGtuem43MWlFaXVLNWc2a2JYWEdQN2d3WkpxVnpMaGFrQi9j?=
 =?utf-8?B?NTZIcXM4SDExMDNzREFSR0JRSlhyRU4vcjZ0T1QrOHFTNGFkbjh3V29kTThM?=
 =?utf-8?B?cXZQTUxUWmVKZURTL3I3Q2NhcHMyL2ZYZktMVC9LeUdPWGJzVGZYbzU2YjdN?=
 =?utf-8?B?aDhDZkRwMXJzM1NQOHVCbEJEVWhGSmxGRDFlRHpUdkJDTGV5LzhzYnk4M1Qr?=
 =?utf-8?B?Ulg1MnIyaFN2SmRERndDdDQ2VjZac3oyMGlVYURSUjVwN1p5MzczcHdtUWZW?=
 =?utf-8?B?L2U1UHdvb3hZOXc5aDBTZjFTcnd4WlVkdkFyN0g2YWkzYk1IVmE5VUVUdTVr?=
 =?utf-8?B?bmQzQklzSkVQYnlDaktUL2ZEL2w0YWR0ZlFzMnNGR1d1QkxibzhoRy9VSVJX?=
 =?utf-8?B?a1VCMUVvUkZ6NkxQR2dxUkF3dytjZ1hUcjZIandLVWZ3VzNlWVB3V3BZUlFh?=
 =?utf-8?B?bVVuRVh6NjdxbDZ4TzFmZXg1WGFVYyszaDhZRERRL0NocmNVNnZESFhEUUVL?=
 =?utf-8?B?Rmd4RCsvZTRrcklHUUQ2L1dueG44NklqdXZNby8yZmZMNVZLaEpsWlJNOFp6?=
 =?utf-8?B?d3FZeDJnNWdFZHplUUJNcEF6UkhpYmhxbWcyelJHeDFkVFh6WFBGWG52ZEhz?=
 =?utf-8?B?aEFmbndTaVZ6UHVvTzhZSTJ3UzZ5ZW0rS1NMbzF1aVcwelZ6aENxNUpqUmY1?=
 =?utf-8?B?NXpNMklPTUd6Vkt1UTBsTlVFemFtY2poUzUwYlZpbHJlSEdTRXoySUhQV25Z?=
 =?utf-8?B?a3hJUTBYSUNvbnBJTnQwd29ramZhcTd4S0Z3N0lEUW9zQ2t2TEdCQ09DMWVX?=
 =?utf-8?B?T2g3TTgwVjVmWW94YXpLN3BOajA5Tlk3eEJPQ21ISEh3bHBCcmJsNzBRT2Vt?=
 =?utf-8?B?NVovaWpvSXplZ1FsdU9HWWh4V28yZHZxdUdyMjBtVUdRUHJKanBCRlpVd2tV?=
 =?utf-8?B?Z29hUnRYME9FWWk3SzROS2h6TkF0NTJ4RFZBN2JIN1N1enFFZG5jclRGSll4?=
 =?utf-8?B?Y1FXQnJaNDY0dndhL3pLdENVeGZxREpRN2prMFpqbGhEbUQvYVhtY3cxek1n?=
 =?utf-8?B?dW9OSHpGZUVhUnhvSDhnbUZoQUQxWVBOVlFDbzVrU0w3QXdyR2JEckJrdWQw?=
 =?utf-8?B?U1FsTXFYZDM2QU1HRnJtcEJkcTlwZmdBcklFc0pySjE2cE5XdWZJOXpyT1dN?=
 =?utf-8?B?V0hpb01BaWlXbG4yVEFKY0NXYWV2amF3TmkxZit1TWxyWmFNRTdTVC9CZEdl?=
 =?utf-8?B?anRCZjNwMUZZTzlTSWFjTzE3RHBEWU5YRHZoRmdPMnRta3pmZFVUSGVsaEtj?=
 =?utf-8?B?YkNDQ1hjNjk1RHoyZHA1VjlnUU01aW9acTFocWNNc1lXRUdDenBNVzJMS3dy?=
 =?utf-8?B?WlE9PQ==?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: b937cc37-f9e5-4b56-743f-08dc33b47ef5
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4974.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 14:42:29.8744
 (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: 5jA18zFhKZ4/Oze986zyMwG3S2pnYtHkPIf3MsXL0EO7gLQBRTq6ybQcYo9ZFfEQ6pj6k0FXLmaXZI7Ne04D4dEW/oWGda1L0iAbRdGot9U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR08MB6093

On 21.02.24 20:24, Matthias Kaehlcke wrote:
> On Tue, Feb 20, 2024 at 03:05:46PM +0100, Javier Carrasco wrote:
>> Most of the functionality this driver provides can be used by non-hub
>> devices as well.
>>
>> To account for the hub-specific code, add a flag to the device data
>> structure and check its value for hub-specific code.
> 
> Please mention that the driver doesn't power off non-hub devices
> during system suspend.
> 
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>>  drivers/usb/misc/onboard_usb_dev.c |  3 ++-
>>  drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>> index 2103af2cb2a6..f43130a6786f 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>>  		if (!device_may_wakeup(node->udev->bus->controller))
>>  			continue;
>>  
>> -		if (usb_wakeup_enabled_descendants(node->udev)) {
>> +		if (usb_wakeup_enabled_descendants(node->udev) ||
>> +		    !onboard_dev->pdata->is_hub) {
> 
> 
> This check isn't dependent on characteristics of the USB devices processed
> in this loop, therefore it can be performed at function entry. Please combine
> it with the check of 'always_powered_in_suspend'. It's also an option to
> omit the check completely, 'always_powered_in_suspend' will never be set for
> non-hub devices (assuming the sysfs attribute isn't added).
> 

The attribute will not be available for non-hub devices in v5. However,
if the check is completely removed, will power_off not stay true at the
end of the function, always leading to a device power off? As you said,
'always_powered_in_suspend' will not be set for non-hub devices.

>>  			power_off = false;
>>  			break;
>>  		}
> 
> Without code context: please omit the creation of the 'always_powered_in_suspend'
> attribute for non-hub devices. As per above we don't plan to hone it, so it
> shouldn't exist.

Best regards,
Javier Carrasco