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

From: Matthias Kaehlcke <mka@chromium.org>
To: Javier Carrasco <javier.carrasco@wolfvision.net>
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:18:09 +0000	[thread overview]
Message-ID: <ZdZokatitviYZ7rg@google.com> (raw)
In-Reply-To: <503d9ea9-9812-498b-a5ee-2579ba8a7ecf@wolfvision.net>

Hi Javier,

On Wed, Feb 21, 2024 at 09:40:38PM +0100, Javier Carrasco wrote:
> 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.

That's perfectly fine and intended. MAX_SUPPLIES is a max, any list
shorther than that is valid. Any longer list will result in probe()
being aborted with a clear error message.

> 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.

As per above it's not necessary to add a new name when MAX_SUPPLIES is
increased to support more non-generic names. It would only be necessary
if more generic names were added, my understanding is that this
should not happen because any newly supported onboard devices are
supposed to use device specific supply names. I don't like to idea of
adding unused pseudo supply names to the list, just for the sake of
using ARRAY_SIZE.

> 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.

Please let me know if you still think the extra names are needed.