| Message ID | 20240228-onboard_xvf3500-v5-1-76b805fd3fe6@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
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 > 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'. > 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' ...)
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. 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
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
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) +{ + unsigned int i; + int err; + + const char * const *supply_names = onboard_hub->pdata->supply_names; + + if (onboard_hub->pdata->num_supplies > MAX_SUPPLIES) + return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n", + MAX_SUPPLIES); + + for (i = 0; i < onboard_hub->pdata->num_supplies; i++) + onboard_hub->supplies[i].supply = supply_names[i]; + + err = devm_regulator_bulk_get(dev, onboard_hub->pdata->num_supplies, + onboard_hub->supplies); + if (err) + dev_err(dev, "Failed to get regulator supplies: %pe\n", + ERR_PTR(err)); + + return err; +} + 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); + if (err) return err; - } hub->clk = devm_clk_get_optional(dev, NULL); if (IS_ERR(hub->clk)) 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 */ }; static const struct onboard_hub_pdata microchip_usb424_data = { .reset_us = 1, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata microchip_usb5744_data = { .reset_us = 0, .num_supplies = 2, + .supply_names = { "vdd", "vdd2" }, }; static const struct onboard_hub_pdata realtek_rts5411_data = { .reset_us = 0, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata ti_tusb8041_data = { .reset_us = 3000, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata cypress_hx3_data = { .reset_us = 10000, .num_supplies = 2, + .supply_names = { "vdd", "vdd2" }, }; static const struct onboard_hub_pdata cypress_hx2vl_data = { .reset_us = 1, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata genesys_gl850g_data = { .reset_us = 3, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata genesys_gl852g_data = { .reset_us = 50, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct onboard_hub_pdata vialab_vl817_data = { .reset_us = 10, .num_supplies = 1, + .supply_names = { "vdd" }, }; static const struct of_device_id onboard_hub_match[] = {
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(-)