Re: [PATCH v4 6/8] usb: misc: onboard_dev: use device supply names

From: Javier Carrasco <javier.carrasco@wolfvision.net>
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
Subject: Re: [PATCH v4 6/8] usb: misc: onboard_dev: use device supply names
Date: Wed, 21 Feb 2024 21:40:38 +0100	[thread overview]
Message-ID: <503d9ea9-9812-498b-a5ee-2579ba8a7ecf@wolfvision.net> (raw)
In-Reply-To: <ZdZcLOlSc3FScjLK@google.com>

On 21.02.24 21:25, Matthias Kaehlcke wrote:
> On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote:
>> The current mechanism uses generic names for the power supplies, which
>> conflicts with proper name definitions in the device bindings.
>>
>> Add a per-device property to include real supply names and keep generic
>> names as a fallback mechanism for backward compatibility.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
>>  drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------
>>  drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++
>>  2 files changed, 52 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>> index f43130a6786f..e66fcac93006 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -29,18 +29,6 @@
>>  
>>  #include "onboard_usb_dev.h"
>>  
>> -/*
>> - * Use generic names, as the actual names might differ between devices. If a new
>> - * device requires more than the currently supported supplies, add a new one
>> - * here.
>> - */
>> -static const char * const supply_names[] = {
>> -	"vdd",
>> -	"vdd2",
>> -};
>> -
>> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
>> -
>>  static void onboard_dev_attach_usb_driver(struct work_struct *work);
>>  
>>  static struct usb_device_driver onboard_dev_usbdev_driver;
>> @@ -66,6 +54,33 @@ struct onboard_dev {
>>  	struct clk *clk;
>>  };
>>  
>> +static int onboard_dev_get_regulator_bulk(struct device *dev,
>> +					  struct onboard_dev *onboard_dev)
>> +{
>> +	unsigned int i;
>> +	int err;
>> +
>> +	const char * const *supply_names = onboard_dev->pdata->supply_names;
>> +
>> +	if (onboard_dev->pdata->num_supplies > MAX_SUPPLIES)
>> +		return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
>> +				     MAX_SUPPLIES);
>> +
>> +	if (!supply_names[0])
>> +		supply_names = generic_supply_names;
> 
> Please change to 'if (!supply_names)' and omit the initialization of
> .supply_names for devices that use the generic supply names.
> 

That looks much cleaner, I will apply it to the next version.

>> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
>> index ebe83e19d818..59dced6bd339 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.h
>> +++ b/drivers/usb/misc/onboard_usb_dev.h
>> @@ -6,63 +6,86 @@
>>  #ifndef _USB_MISC_ONBOARD_USB_DEV_H
>>  #define _USB_MISC_ONBOARD_USB_DEV_H
>>  
>> +/*
>> + * Fallback supply names for backwards compatibility. If the device requires
>> + * more than the currently supported supplies, add a new one here, and if
>> + * possible, the real name supplies to the device-specific data.
>> + */
>> +static const char * const generic_supply_names[] = {
>> +	"vdd",
>> +	"vdd2",
>> +};
>> +
>> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names)
> 
> This will have to change when support for a device with more than 2 non-generic
> supply names gets added. Please use a literal value for MAX_SUPPLIES instead of
> ARRAY_SIZE. If the literal is 2 it would still need to change for future devices
> with more supplies, but that change would be more straighforward.
> 

I am not completely sure about this. Someone could increase MAX_SUPPLIES
without adding a generic name. Actually two modifications will be
necessary for every addition (name and MAX_SUPPLIES). If ARRAY_SIZE is
used, only new names are required, and MAX_SUPPLIES is automatically
increased.

I understand that the whole point of this is getting rid of the generic
names, but we still have to provide generic names for every extra
supply, at least for code consistency and to avoid size mismatches
between real an generic supply names.

>> +
>>  struct onboard_dev_pdata {
>>  	unsigned long reset_us;		/* reset pulse width in us */
>>  	unsigned int num_supplies;	/* number of supplies */
>>  	bool is_hub;
>> +	const char * const supply_names[MAX_SUPPLIES];
>> +
>>  };
>>  
>>  static const struct onboard_dev_pdata microchip_usb424_data = {
>>  	.reset_us = 1,
>>  	.num_supplies = 1,
>> +	.supply_names = { NULL },
>>  	.is_hub = true,
>>  };
>>
>> ...

Thanks again for your feedback.

Best regards,
Javier Carrasco