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@xxxxxxxxxxxxxx>
> >> ---
> >> 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
