| Message ID | 20231123134728.709533-1-frieder@fris.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] usb: misc: onboard_usb_hub: Add support for clock input | expand |
On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote: > From: Frieder Schrempf <frieder.schrempf@kontron.de> > > Most onboard USB hubs have a dedicated crystal oscillator but on some > boards the clock signal for the hub is provided by the SoC. > > In order to support this, we add the possibility of specifying a > clock in the devicetree that gets enabled/disabled when the hub > is powered up/down. > > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de> > --- > drivers/usb/misc/onboard_usb_hub.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c > index a341b2fbb7b44..e710e3c82ba9b 100644 > --- a/drivers/usb/misc/onboard_usb_hub.c > +++ b/drivers/usb/misc/onboard_usb_hub.c > @@ -5,6 +5,7 @@ > * Copyright (c) 2022, Google LLC > */ > > +#include <linux/clk.h> > #include <linux/device.h> > #include <linux/export.h> > #include <linux/gpio/consumer.h> > @@ -60,12 +61,19 @@ struct onboard_hub { > bool going_away; > struct list_head udev_list; > struct mutex lock; > + struct clk *clk; > }; > > static int onboard_hub_power_on(struct onboard_hub *hub) > { > int err; > > + err = clk_prepare_enable(hub->clk); > + if (err) { > + dev_err(hub->dev, "failed to enable clock: %d\n", err); > + return err; > + } But what happens if clk is not set here? And doesn't clk_prepare_enable() print out a message if it fails? thanks, greg k-h
Hello, On Thu, Nov 23, 2023 at 01:55:57PM +0000, Greg Kroah-Hartman wrote: > On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote: > > + err = clk_prepare_enable(hub->clk); > > + if (err) { > > + dev_err(hub->dev, "failed to enable clock: %d\n", err); > > + return err; I suggest to use %pe (and ERR_PTR(err)) here. > > + } > > But what happens if clk is not set here? clk_prepare_enable() just does "return 0" if the clk argument is NULL. > And doesn't clk_prepare_enable() print out a message if it fails? clk_prepare_enable is silent on errors. Best regards Uwe
Hi Greg, hi Uwe, thanks for reviewing! On 23.11.23 18:36, Uwe Kleine-König wrote: > Hello, > > On Thu, Nov 23, 2023 at 01:55:57PM +0000, Greg Kroah-Hartman wrote: >> On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote: >>> + err = clk_prepare_enable(hub->clk); >>> + if (err) { >>> + dev_err(hub->dev, "failed to enable clock: %d\n", err); >>> + return err; > > I suggest to use %pe (and ERR_PTR(err)) here. Ok, I added this in v2. I also added a patch to convert the other error logs to be consistent within the driver. > >>> + } >> >> But what happens if clk is not set here? > > clk_prepare_enable() just does "return 0" if the clk argument is NULL. Exactly! > >> And doesn't clk_prepare_enable() print out a message if it fails? > > clk_prepare_enable is silent on errors. Right! Thanks Frieder
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c index a341b2fbb7b44..e710e3c82ba9b 100644 --- a/drivers/usb/misc/onboard_usb_hub.c +++ b/drivers/usb/misc/onboard_usb_hub.c @@ -5,6 +5,7 @@ * Copyright (c) 2022, Google LLC */ +#include <linux/clk.h> #include <linux/device.h> #include <linux/export.h> #include <linux/gpio/consumer.h> @@ -60,12 +61,19 @@ struct onboard_hub { bool going_away; struct list_head udev_list; struct mutex lock; + struct clk *clk; }; static int onboard_hub_power_on(struct onboard_hub *hub) { int err; + err = clk_prepare_enable(hub->clk); + if (err) { + dev_err(hub->dev, "failed to enable clock: %d\n", err); + return err; + } + err = regulator_bulk_enable(hub->pdata->num_supplies, hub->supplies); if (err) { dev_err(hub->dev, "failed to enable supplies: %d\n", err); @@ -92,6 +100,8 @@ static int onboard_hub_power_off(struct onboard_hub *hub) return err; } + clk_disable_unprepare(hub->clk); + hub->is_powered_on = false; return 0; @@ -266,6 +276,10 @@ static int onboard_hub_probe(struct platform_device *pdev) return err; } + hub->clk = devm_clk_get_optional(dev, NULL); + if (IS_ERR(hub->clk)) + return dev_err_probe(dev, PTR_ERR(hub->clk), "failed to get clock\n"); + hub->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); if (IS_ERR(hub->reset_gpio))