Received-SPF: pass (google.com: domain of linux-kernel+bounces-63296-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: <2a23817d-b797-4659-a4e6-5c8a75864c90@wolfvision.net>
Date: Tue, 13 Feb 2024 11:00:43 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/7] 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>,
 linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
References: <20240206-onboard_xvf3500-v3-0-f85b04116688@wolfvision.net>
 <20240206-onboard_xvf3500-v3-2-f85b04116688@wolfvision.net>
 <ZcJ9OnYOtUVMu2Yk@google.com>
Content-Language: en-US
From: Javier Carrasco <javier.carrasco@wolfvision.net>
In-Reply-To: <ZcJ9OnYOtUVMu2Yk@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?Y2dhdkRkY0RSM2Q4R3gzdWhxTW8ydXM5OHJnbWI5YzhGemo4UE9RRDhCRFAv?=
 =?utf-8?B?Y3kwY3UwWms2MmFCdjd6bmpWQW5XdWlJVGx5NGVPazFlbzZXSXlEY2R2Ry9W?=
 =?utf-8?B?WDk5cUdaUjUyZWxoUFQ0N3lwZmQ0K0RkOEFrNnArczlpYkQ3d2N0U0tKd0Y2?=
 =?utf-8?B?cEFVeERmdnZsMEl6SjJKWE41eVBaVWtKZjd6blRwTWlQNlFYTVRKTkwyQ1Ay?=
 =?utf-8?B?QjR5SWthWU1rVHhUZTNKUG51dUxHSk5lbmlGN1RqM2NidkVXM0UrUWt1MDRx?=
 =?utf-8?B?NGI3UDdkSFhyTGo0UHFSZTNDMjBUa1Z0SERDMlFsM0xDQTdORUVkaVpwWGY1?=
 =?utf-8?B?VjlUN2RPSE1TeW51YndVRld0cHdSc0M5cEN3RGZRUko2V2RkOVBqL3hValV1?=
 =?utf-8?B?QkNRZXJsS1V5bkhacXNVcnNqQnZBYy9JVDNnWGRYMDdlU0dEZVNzLzlLZFpP?=
 =?utf-8?B?aUpGalN3M2w4NXJJcklGTTZhdzEzTCtyemVCTmVVUUNnWFBTR0tpOUdPVGxM?=
 =?utf-8?B?eDN3cUp5cjNIZ2pYdWl1SkptdXVvYXNhcHZHbkEzVHZIUjZ6SE5tV0wwMVUr?=
 =?utf-8?B?YWFWRDNaa3BidEFyQVVXMVorcWVCd2VyNGtuUkF3Sk1Rd1lVdUZOYTh2YjJ2?=
 =?utf-8?B?Umdyc1c2THorVUd2aEtoL0dOa08vdWM5VUozMFd0SDkySWVaUDdkT29SSi9F?=
 =?utf-8?B?UmpiNWlpTWdwU2YrYUhHV2ZUSlNYMFJIVmk4QmYzcHlmV3lZMzJRdjQzZEtP?=
 =?utf-8?B?SGo0SGN6dEY3UVlTYzdVdkQxdFZVdEYwOTlNVjY3VHVCSVBMOS9FdXZ5cm5X?=
 =?utf-8?B?bkd2eSsvQnNtRlRyUkVxS0JCc0w0RGxCMFNHeEk0U2VaRURBRExUSGhXYWxL?=
 =?utf-8?B?eXA2NjNQYUROTjdqdXdrWnlrUEVKS1pOUlN1Z3ZINDRsU3JhTytELzhUY29x?=
 =?utf-8?B?WStiY1lLTWx4K2RSSkw5dytaNUJWWGlDVEh1RHlJVSs4UURUNWV3Y2JJbEFS?=
 =?utf-8?B?VGM3NzlkOUpOTlI1NXpxSEtramlaSjJxVG16NXM3dVJJbzB2Y0RmUUFvNlU3?=
 =?utf-8?B?MVBrYXNKNHAxRmR3djJjMnVMejMzR1hYSU9OU3lzMlEyVUw4emRIUy9RUmlS?=
 =?utf-8?B?MXk3TzczamNBMG5udWRGZTk2SVFjd1VjQStqbi9SYTZrNE1ReXNuUG9MS0po?=
 =?utf-8?B?NWJaalk0c043Q25GSTlKUEUzNyt6aWhxN2p4eGRxWStBRjFuUEFaWnovLzVt?=
 =?utf-8?B?WE1VY3dVRm9NVlpsVUFuQXJreEVpbW9NUlBnTVZCbG9RWFlhR3pZOXlGV0Z5?=
 =?utf-8?B?SnBTM0YyMDFwYkxsN3ZORE5JT2NnMlVkZGxMWHpFYU9DOXRUYWQ4UElQTkNq?=
 =?utf-8?B?Yzd3Ykk2STlhNmR2Q3F5c2UrRHNJTC9qMllEMno4VUVXNlpER3Z6bjRob2ND?=
 =?utf-8?B?eDFaZW1HQnVIZkZtVmNVVnJuU0M3SnFVV25sa05Za2ZySkNkRjg0a3ZLUXFy?=
 =?utf-8?B?d2NSNit2NVcvbUlETVBJRi9abFdHU1NzMjltRGlNbERVS3hzMTU3YVk0RDVw?=
 =?utf-8?B?ZndZUDVEaXlCS3RoZ0UreEdEeHprcUpWNHVvejVLbzBRTkFHOUYzMXMwL3o5?=
 =?utf-8?B?ODUvbjJKbzUvSmZyUjZ6NHZSbGJ1aTFIT1ZjN0NQeGlTV0MwQTRaSFk5VmFJ?=
 =?utf-8?B?R2p0QU54U0dmUTZtUGZjcFpaUUYrc0x0a05CaVhnRmZqUUgwK2k5VktNUWxo?=
 =?utf-8?B?d0R3UTlId2Q4NktlZVVFQUMwVlYyYTd2OHJzbm9IRFBRWlZtdU85a0RlQUhi?=
 =?utf-8?B?RnFwMWFJU21ycHNVNjRlVWRuRDNhOU0vQ1M0R2xXalF3T2taZHRSZjF4ZVVS?=
 =?utf-8?B?WWVoSEcyMlZwWmFkcCt4MUhldXcrQlE1WWwwb3g5cXV1dzVKUWdWZlBLY0Rm?=
 =?utf-8?B?QmR6NThzRU1tYzBNZFJXSTVRcVRKVmRLa0w1NjRyNnluRUFoRkQwQnQyZGly?=
 =?utf-8?B?OXNsbkwrNWsvYnpwTWV2L1FHekM3ZGdTdUFuR3F2YnM2WGlaOU9DK3daRGll?=
 =?utf-8?B?ZE5nNm9NTDhkTEgwcVMyK0pCb0JYV0VXWlBkdEJXdjRuZUJpUHpaUUVTcXRw?=
 =?utf-8?B?V1BNUTZrY2dNbnZ3Uk1SUXUwbXdVRG5LMkNFQjlIRm1RZU9MUFdzWmNob0RS?=
 =?utf-8?B?VkE9PQ==?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 940d5c64-0078-46d5-79b5-08dc2c7aa565
X-MS-Exchange-CrossTenant-AuthSource: VE1PR08MB4974.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2024 10:00:45.3922
 (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: RlJmBj4ibowwyCLS89kK0Mf+sGrD0FPQzBVwpMKzqj/eN8uvp6qaSJ/hHQk2RMO9ATVTAOKCtS4yfMNXS4MAxViw07cE7lzc1NY/z/cbt2U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR08MB5300

On 06.02.24 19:40, Matthias Kaehlcke wrote:
> On Tue, Feb 06, 2024 at 02:59:30PM +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.
>>
>> 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, 13 insertions(+)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>> index e2e1e1e30c1e..3ac21ec38ac0 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -123,6 +123,9 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>>  	if (onboard_dev->always_powered_in_suspend)
>>  		return 0;
>>  
>> +	if (!onboard_dev->pdata->is_hub)
>> +		return onboard_dev_power_off(onboard_dev);
> 
> Why turn the device always off when it isn't a hub? It could be a device
> with wakeup support.
> 
> I really regret making 'off in suspend' the default :(
> 


The power management seems to be a critical point to consider.

Maybe we keep the current implementation and add support to non-hub
devices by simply adding a check to set power_off to false:

static int __maybe_unused onboard_dev_suspend(struct device *dev)

{

        struct onboard_dev *onboard_dev = dev_get_drvdata(dev);

        struct usbdev_node *node;

        bool power_off = true;


        if (onboard_dev->always_powered_in_suspend)

                return 0;


        mutex_lock(&onboard_dev->lock);


        list_for_each_entry(node, &onboard_dev->udev_list, list) {

                if (!device_may_wakeup(node->udev->bus->controller))

                        continue;


~                if (usb_wakeup_enabled_descendants(node->udev) ||

+                    !onboard_dev->pdata->is_hub) {

                        power_off = false;

                        break;

                }

        }


        mutex_unlock(&onboard_dev->lock);


        if (!power_off)

                return 0;


        return onboard_dev_power_off(onboard_dev);

}


Best regards,
Javier Carrasco