Re: [PATCH v5 1/8] usb: misc: onboard_hub: 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 v5 1/8] usb: misc: onboard_hub: use device supply names
Date: Wed, 28 Feb 2024 16:13:31 +0000	[thread overview]
Message-ID: <Zd9bq5hLj3Pv5ZHa@google.com> (raw)
In-Reply-To: <2bbd5eae-f963-40d6-a827-1c273ef5b211@wolfvision.net>

On Wed, Feb 28, 2024 at 05:02:10PM +0100, Javier Carrasco wrote:
> On 28.02.24 16:37, Matthias Kaehlcke wrote:
> > Hi Javier,
> > 
> > Thanks for moving this patch to the front of the series!
> > 
> > A few more comments inline.
> > 
> > On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
> >> The current implementation 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 for existing devices to keep backward compatibility.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> >> ---
> >>  drivers/usb/misc/onboard_usb_hub.c | 49 ++++++++++++++++++++------------------
> >>  drivers/usb/misc/onboard_usb_hub.h | 12 ++++++++++
> >>  2 files changed, 38 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> >> index 0dd2b032c90b..3755f6cc1eda 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.c
> >> +++ b/drivers/usb/misc/onboard_usb_hub.c
> >> @@ -29,17 +29,6 @@
> >>  
> >>  #include "onboard_usb_hub.h"
> >>  
> >> -/*
> >> - * Use generic names, as the actual names might differ between hubs. If a new
> >> - * hub 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_hub_attach_usb_driver(struct work_struct *work);
> >>  
> >>  static struct usb_device_driver onboard_hub_usbdev_driver;
> >> @@ -65,6 +54,30 @@ struct onboard_hub {
> >>  	struct clk *clk;
> >>  };
> >>  
> >> +static int onboard_hub_get_regulator_bulk(struct device *dev,
> >> +					  struct onboard_hub *onboard_hub)
> > 
> > Let's call this onboard_hub_get_regulators(), it's an implementation detail
> > that regulator_bulk_get() is used for getting them.
> > 
> > no need to pass 'dev', there is onboard_hub->dev
> > 
> 
> Not at this point, though. The hub->dev = dev assignment happens a few
> lines below, but there is no reason not to move the line up. I will
> modify this for v6.
> 
> >>  static int onboard_hub_power_on(struct onboard_hub *hub)
> >>  {
> >>  	int err;
> >> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
> >>  {
> >>  	struct device *dev = &pdev->dev;
> >>  	struct onboard_hub *hub;
> >> -	unsigned int i;
> >>  	int err;
> >>  
> >>  	hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> >> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device *pdev)
> >>  	if (!hub->pdata)
> >>  		return -EINVAL;
> >>  
> >> -	if (hub->pdata->num_supplies > MAX_SUPPLIES)
> >> -		return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
> >> -				     MAX_SUPPLIES);
> >> -
> >> -	for (i = 0; i < hub->pdata->num_supplies; i++)
> >> -		hub->supplies[i].supply = supply_names[i];
> >> -
> >> -	err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, hub->supplies);
> >> -	if (err) {
> >> -		dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err));
> >> +	err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
> > 
> > The local variable is called 'hub', not 'onboard_hub'.
> > 
> 
> Good catch! Actually this patch alone would have not compiled, but once
> the renaming is done, everything is ok again. I will fix this for v6.
> 
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> >> index f360d5cf8d8a..ea24bd6790f0 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.h
> >> +++ b/drivers/usb/misc/onboard_usb_hub.h
> >> @@ -6,54 +6,66 @@
> >>  #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> >>  #define _USB_MISC_ONBOARD_USB_HUB_H
> >>  
> >> +#define MAX_SUPPLIES 2
> >> +
> >>  struct onboard_hub_pdata {
> >>  	unsigned long reset_us;		/* reset pulse width in us */
> >>  	unsigned int num_supplies;	/* number of supplies */
> >> +	const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
> > 
> > The comment isn't particularly useful or accurate. Not in all cases
> > real names are used and outside of the context of this change the
> > comment is hard to understand.
> > 
> > I'd say just omit it, the name of the field is self-documenting enough,
> > there is no need to repeat the same in a comment (as for 'num_supplies'
> > ...)
> 
> I added tthe comment because I can foresee what is going to happen:
> people will copy the names from existing devices, we will have to ask if
> the supplies are actually called vdd and vdd2 in the datasheet, and then
> the real names will be sent in v2. Especially at the beginning, when the
> supported devices are using vdd and vdd2.

It will probably happen, but I don't expect the comment to prevent that
in most cases. First people need to read the comment and then interpret
it correctly, which isn't a given.

> But if you think the field name is self-documenting, I am fine with it
> too. I will remove the comment for v6.
> 
> Thanks again and best regards,
> Javier Carrasco