| Message ID | 20230620-hx3-v2-1-76a53434c713@skidata.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_usb_hub: add support for Cypress HX3 USB 3.0 family | expand |
Hi, On Wed, Jun 21, 2023 at 04:26:27PM +0200, Benjamin Bara wrote: > From: Benjamin Bara <benjamin.bara@skidata.com> > > As some of the onboard hubs require multiple power supplies, provide the > environment to support them. > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com> Overall this looks good to me, a few nits inside. > --- > v2: > - replace (err != 0) with (err) > --- > drivers/usb/misc/onboard_usb_hub.c | 36 ++++++++++++++++++++++++++++-------- > drivers/usb/misc/onboard_usb_hub.h | 1 + > 2 files changed, 29 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > index 12fc6eb67c3b..3de30356a684 100644 > --- a/drivers/usb/misc/onboard_usb_hub.c > +++ b/drivers/usb/misc/onboard_usb_hub.c > @@ -27,6 +27,12 @@ > > #include "onboard_usb_hub.h" > > +#define SUPPLIES_NUM_MAX 2 MAX_SUPPLIES? add empty line > +static const char * const supply_names[] = { > + "vdd", > + "vdd2", > +}; > + > static void onboard_hub_attach_usb_driver(struct work_struct *work); > > static struct usb_device_driver onboard_hub_usbdev_driver; > @@ -40,7 +46,8 @@ struct usbdev_node { > }; > > struct onboard_hub { > - struct regulator *vdd; > + struct regulator_bulk_data supplies[SUPPLIES_NUM_MAX]; > + unsigned int supplies_num; num_supplies? > struct device *dev; > const struct onboard_hub_pdata *pdata; > struct gpio_desc *reset_gpio; > @@ -55,9 +62,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) > { > int err; > > - err = regulator_enable(hub->vdd); > + err = regulator_bulk_enable(hub->supplies_num, hub->supplies); > if (err) { > - dev_err(hub->dev, "failed to enable regulator: %d\n", err); > + dev_err(hub->dev, "failed to enable supplies: %d\n", err); > return err; > } > > @@ -75,9 +82,9 @@ static int onboard_hub_power_off(struct onboard_hub *hub) > > gpiod_set_value_cansleep(hub->reset_gpio, 1); > > - err = regulator_disable(hub->vdd); > + err = regulator_bulk_disable(hub->supplies_num, hub->supplies); > if (err) { > - dev_err(hub->dev, "failed to disable regulator: %d\n", err); > + dev_err(hub->dev, "failed to disable supplies: %d\n", err); > return err; > } > > @@ -232,6 +239,7 @@ static int onboard_hub_probe(struct platform_device *pdev) > const struct of_device_id *of_id; > struct device *dev = &pdev->dev; > struct onboard_hub *hub; > + unsigned int i; > int err; > > hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > @@ -246,9 +254,21 @@ static int onboard_hub_probe(struct platform_device *pdev) > if (!hub->pdata) > return -EINVAL; > > - hub->vdd = devm_regulator_get(dev, "vdd"); > - if (IS_ERR(hub->vdd)) > - return PTR_ERR(hub->vdd); > + if (hub->pdata->supplies_num > SUPPLIES_NUM_MAX) > + return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n", > + SUPPLIES_NUM_MAX); > + hub->supplies_num = 1; > + if (hub->pdata->supplies_num > 1) > + hub->supplies_num = hub->pdata->supplies_num; Please change the above to: if (hub->pdata->supplies_num != 0) hub->supplies_num = hub->pdata->supplies_num; else hub->supplies_num = 1; > + > + for (i = 0; i < SUPPLIES_NUM_MAX; i++) > + hub->supplies[i].supply = supply_names[i]; > + > + err = devm_regulator_bulk_get(dev, hub->supplies_num, hub->supplies); > + if (err) { > + dev_err(dev, "Failed to get regulator supplies: %d\n", err); > + return err; > + } > > hub->reset_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_HIGH); > diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h > index aca5f50eb0da..657190bf1799 100644 > --- a/drivers/usb/misc/onboard_usb_hub.h > +++ b/drivers/usb/misc/onboard_usb_hub.h > @@ -8,6 +8,7 @@ > > struct onboard_hub_pdata { > unsigned long reset_us; /* reset pulse width in us */ > + unsigned int supplies_num; /* num of supplies: 0 considered as 1 */ num_supplies? s/num of/number of/ > };
Hi, Am Mittwoch, 21. Juni 2023, 16:57:03 CEST schrieb Matthias Kaehlcke: > ******************** > Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, > dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie > die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter. > Attention external email: Open attachments and links only if you know that > they are from a secure source and are safe. In doubt forward the email to > the IT-Helpdesk to check it. ******************** > > Hi, > > On Wed, Jun 21, 2023 at 04:26:27PM +0200, Benjamin Bara wrote: > > From: Benjamin Bara <benjamin.bara@skidata.com> > > > > As some of the onboard hubs require multiple power supplies, provide the > > environment to support them. > > > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com> > > Overall this looks good to me, a few nits inside. > > > --- > > v2: > > - replace (err != 0) with (err) > > --- > > > > drivers/usb/misc/onboard_usb_hub.c | 36 > > ++++++++++++++++++++++++++++-------- drivers/usb/misc/onboard_usb_hub.h > > | 1 + > > 2 files changed, 29 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.c > > b/drivers/usb/misc/onboard_usb_hub.c index 12fc6eb67c3b..3de30356a684 > > 100644 > > --- a/drivers/usb/misc/onboard_usb_hub.c > > +++ b/drivers/usb/misc/onboard_usb_hub.c > > @@ -27,6 +27,12 @@ > > > > #include "onboard_usb_hub.h" > > > > +#define SUPPLIES_NUM_MAX 2 > > MAX_SUPPLIES? Do we need this MAX_SUPPLIES macro at all? This essentially is ARRAY_SIZE(supply_names), no? At least it should be > #define MAX_SUPPLIES ARRAY_SIZE(supply_names) > > add empty line > > > +static const char * const supply_names[] = { > > + "vdd", > > + "vdd2", > > +}; > > + > > > > static void onboard_hub_attach_usb_driver(struct work_struct *work); > > > > static struct usb_device_driver onboard_hub_usbdev_driver; > > > > @@ -40,7 +46,8 @@ struct usbdev_node { > > > > }; > > > > struct onboard_hub { > > > > - struct regulator *vdd; > > + struct regulator_bulk_data supplies[SUPPLIES_NUM_MAX]; > > + unsigned int supplies_num; > > num_supplies? > > > struct device *dev; > > const struct onboard_hub_pdata *pdata; > > struct gpio_desc *reset_gpio; > > > > @@ -55,9 +62,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) > > > > { > > > > int err; > > > > - err = regulator_enable(hub->vdd); > > + err = regulator_bulk_enable(hub->supplies_num, hub->supplies); > > > > if (err) { > > > > - dev_err(hub->dev, "failed to enable regulator: %d\n", err); > > + dev_err(hub->dev, "failed to enable supplies: %d\n", err); > > > > return err; > > > > } > > > > @@ -75,9 +82,9 @@ static int onboard_hub_power_off(struct onboard_hub > > *hub) > > > > gpiod_set_value_cansleep(hub->reset_gpio, 1); > > > > - err = regulator_disable(hub->vdd); > > + err = regulator_bulk_disable(hub->supplies_num, hub->supplies); > > > > if (err) { > > > > - dev_err(hub->dev, "failed to disable regulator: %d\n", err); > > + dev_err(hub->dev, "failed to disable supplies: %d\n", err); > > > > return err; > > > > } > > > > @@ -232,6 +239,7 @@ static int onboard_hub_probe(struct platform_device > > *pdev)> > > const struct of_device_id *of_id; > > struct device *dev = &pdev->dev; > > struct onboard_hub *hub; > > > > + unsigned int i; > > > > int err; > > > > hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > > > > @@ -246,9 +254,21 @@ static int onboard_hub_probe(struct platform_device > > *pdev)> > > if (!hub->pdata) > > > > return -EINVAL; > > > > - hub->vdd = devm_regulator_get(dev, "vdd"); > > - if (IS_ERR(hub->vdd)) > > - return PTR_ERR(hub->vdd); > > + if (hub->pdata->supplies_num > SUPPLIES_NUM_MAX) > > + return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n", > > + SUPPLIES_NUM_MAX); > > + hub->supplies_num = 1; > > + if (hub->pdata->supplies_num > 1) > > + hub->supplies_num = hub->pdata->supplies_num; > > Please change the above to: > > if (hub->pdata->supplies_num != 0) > hub->supplies_num = hub->pdata->supplies_num; > else > hub->supplies_num = 1; > In the kernel there is also this style: > hub->supplies_num = hub->pdata->supplies_num? : 1; But it's up to you which one you prefer. Best regards, Alexander > > + > > + for (i = 0; i < SUPPLIES_NUM_MAX; i++) > > + hub->supplies[i].supply = supply_names[i]; > > + > > + err = devm_regulator_bulk_get(dev, hub->supplies_num, hub- >supplies); > > + if (err) { > > + dev_err(dev, "Failed to get regulator supplies: %d\n", err); > > + return err; > > + } > > > > hub->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > > > GPIOD_OUT_HIGH); > > > > diff --git a/drivers/usb/misc/onboard_usb_hub.h > > b/drivers/usb/misc/onboard_usb_hub.h index aca5f50eb0da..657190bf1799 > > 100644 > > --- a/drivers/usb/misc/onboard_usb_hub.h > > +++ b/drivers/usb/misc/onboard_usb_hub.h > > @@ -8,6 +8,7 @@ > > > > struct onboard_hub_pdata { > > > > unsigned long reset_us; /* reset pulse width in us */ > > > > + unsigned int supplies_num; /* num of supplies: 0 considered as 1 */ > > num_supplies? > > s/num of/number of/ > > > };
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c index 12fc6eb67c3b..3de30356a684 100644 --- a/drivers/usb/misc/onboard_usb_hub.c +++ b/drivers/usb/misc/onboard_usb_hub.c @@ -27,6 +27,12 @@ #include "onboard_usb_hub.h" +#define SUPPLIES_NUM_MAX 2 +static const char * const supply_names[] = { + "vdd", + "vdd2", +}; + static void onboard_hub_attach_usb_driver(struct work_struct *work); static struct usb_device_driver onboard_hub_usbdev_driver; @@ -40,7 +46,8 @@ struct usbdev_node { }; struct onboard_hub { - struct regulator *vdd; + struct regulator_bulk_data supplies[SUPPLIES_NUM_MAX]; + unsigned int supplies_num; struct device *dev; const struct onboard_hub_pdata *pdata; struct gpio_desc *reset_gpio; @@ -55,9 +62,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub) { int err; - err = regulator_enable(hub->vdd); + err = regulator_bulk_enable(hub->supplies_num, hub->supplies); if (err) { - dev_err(hub->dev, "failed to enable regulator: %d\n", err); + dev_err(hub->dev, "failed to enable supplies: %d\n", err); return err; } @@ -75,9 +82,9 @@ static int onboard_hub_power_off(struct onboard_hub *hub) gpiod_set_value_cansleep(hub->reset_gpio, 1); - err = regulator_disable(hub->vdd); + err = regulator_bulk_disable(hub->supplies_num, hub->supplies); if (err) { - dev_err(hub->dev, "failed to disable regulator: %d\n", err); + dev_err(hub->dev, "failed to disable supplies: %d\n", err); return err; } @@ -232,6 +239,7 @@ static int onboard_hub_probe(struct platform_device *pdev) const struct of_device_id *of_id; struct device *dev = &pdev->dev; struct onboard_hub *hub; + unsigned int i; int err; hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); @@ -246,9 +254,21 @@ static int onboard_hub_probe(struct platform_device *pdev) if (!hub->pdata) return -EINVAL; - hub->vdd = devm_regulator_get(dev, "vdd"); - if (IS_ERR(hub->vdd)) - return PTR_ERR(hub->vdd); + if (hub->pdata->supplies_num > SUPPLIES_NUM_MAX) + return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n", + SUPPLIES_NUM_MAX); + hub->supplies_num = 1; + if (hub->pdata->supplies_num > 1) + hub->supplies_num = hub->pdata->supplies_num; + + for (i = 0; i < SUPPLIES_NUM_MAX; i++) + hub->supplies[i].supply = supply_names[i]; + + err = devm_regulator_bulk_get(dev, hub->supplies_num, hub->supplies); + if (err) { + dev_err(dev, "Failed to get regulator supplies: %d\n", err); + return err; + } hub->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h index aca5f50eb0da..657190bf1799 100644 --- a/drivers/usb/misc/onboard_usb_hub.h +++ b/drivers/usb/misc/onboard_usb_hub.h @@ -8,6 +8,7 @@ struct onboard_hub_pdata { unsigned long reset_us; /* reset pulse width in us */ + unsigned int supplies_num; /* num of supplies: 0 considered as 1 */ }; static const struct onboard_hub_pdata microchip_usb424_data = {