| Message ID | 20200227072545.16856-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: usb251xb: check if hub is already attached | expand |
On Thu, Feb 27, 2020 at 08:25:45AM +0100, Marco Felsch wrote: > It is possible that the hub was configured earlier by the bootloader and > we lack of the reset-gpio. In such a case the usb251xb_connect() fails > because the registers are write-protected. Add a check to test if the > hub is already connected and don't try to reconfigure the hub if we > can't toggle the reset pin. Don't change the usb251xb_connect() logic > if we can't read the status. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/usb/misc/usb251xb.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c > index 29fe5771c21b..9f9a64bab059 100644 > --- a/drivers/usb/misc/usb251xb.c > +++ b/drivers/usb/misc/usb251xb.c > @@ -266,6 +266,30 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub) > } > #endif > > +static bool usb251xb_hub_attached(struct usb251xb *hub) > +{ > + char i2c_rb; > + int err; > + > + err = i2c_smbus_read_block_data(hub->i2c, USB251XB_ADDR_STATUS_COMMAND, > + &i2c_rb); > + if (err < 0) { > + /* > + * The device disables the i2c-interface immediately after it > + * received the USB_ATTACH signal. > + */ > + if (err == -ENXIO) > + return true; > + > + dev_warn(hub->dev, > + "Checking hub Status/Command register failed: %d\n", > + err); > + return false; > + } > + > + return !!(i2c_rb & USB251XB_STATUS_COMMAND_ATTACH); > +} > + > static void usb251xb_reset(struct usb251xb *hub) > { > if (!hub->gpio_reset) > @@ -288,6 +312,15 @@ static int usb251xb_connect(struct usb251xb *hub) > struct device *dev = hub->dev; > int err, i; > char i2c_wb[USB251XB_I2C_REG_SZ]; > + bool is_attached; > + > + /* > + * Check if configuration was done earlier by the bootloader. Trust them > + * if it is the case and we are not capable to reset the hub. > + */ > + is_attached = usb251xb_hub_attached(hub); > + if (!hub->gpio_reset && is_attached) > + return 0; If you write this as: if (!hub->gpio_reset && usb251xb_hub_attached(hub)) return 0; you save some i2c transfers in the presence of a reset gpio. Also I wonder if skipping the initialisation is at least worth a pr_info. Best regards Uwe
On Thu, Feb 27, 2020 at 09:14:48AM +0100, Uwe Kleine-König wrote: > On Thu, Feb 27, 2020 at 08:25:45AM +0100, Marco Felsch wrote: > > It is possible that the hub was configured earlier by the bootloader and > > we lack of the reset-gpio. In such a case the usb251xb_connect() fails > > because the registers are write-protected. Add a check to test if the > > hub is already connected and don't try to reconfigure the hub if we > > can't toggle the reset pin. Don't change the usb251xb_connect() logic > > if we can't read the status. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/usb/misc/usb251xb.c | 33 +++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c > > index 29fe5771c21b..9f9a64bab059 100644 > > --- a/drivers/usb/misc/usb251xb.c > > +++ b/drivers/usb/misc/usb251xb.c > > @@ -266,6 +266,30 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub) > > } > > #endif > > > > +static bool usb251xb_hub_attached(struct usb251xb *hub) > > +{ > > + char i2c_rb; > > + int err; > > + > > + err = i2c_smbus_read_block_data(hub->i2c, USB251XB_ADDR_STATUS_COMMAND, > > + &i2c_rb); > > + if (err < 0) { > > + /* > > + * The device disables the i2c-interface immediately after it > > + * received the USB_ATTACH signal. > > + */ > > + if (err == -ENXIO) > > + return true; > > + > > + dev_warn(hub->dev, > > + "Checking hub Status/Command register failed: %d\n", > > + err); > > + return false; > > + } > > + > > + return !!(i2c_rb & USB251XB_STATUS_COMMAND_ATTACH); > > +} > > + > > static void usb251xb_reset(struct usb251xb *hub) > > { > > if (!hub->gpio_reset) > > @@ -288,6 +312,15 @@ static int usb251xb_connect(struct usb251xb *hub) > > struct device *dev = hub->dev; > > int err, i; > > char i2c_wb[USB251XB_I2C_REG_SZ]; > > + bool is_attached; > > + > > + /* > > + * Check if configuration was done earlier by the bootloader. Trust them > > + * if it is the case and we are not capable to reset the hub. > > + */ > > + is_attached = usb251xb_hub_attached(hub); > > + if (!hub->gpio_reset && is_attached) > > + return 0; > > If you write this as: > > if (!hub->gpio_reset && usb251xb_hub_attached(hub)) > return 0; > > you save some i2c transfers in the presence of a reset gpio. I'd also go with this solution, so you only have i2c transfers when a reset gpio is configured/present. > > Also I wonder if skipping the initialisation is at least worth a > pr_info. +1 for pr_info. regards;rl > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello, On Thu, Feb 27, 2020 at 09:30:07AM +0100, Richard Leitner wrote: > On Thu, Feb 27, 2020 at 09:14:48AM +0100, Uwe Kleine-König wrote: > > If you write this as: > > > > if (!hub->gpio_reset && usb251xb_hub_attached(hub)) > > return 0; > > > > you save some i2c transfers in the presence of a reset gpio. > > I'd also go with this solution, so you only have i2c transfers when a > reset gpio is configured/present. Nitpick: you only have additional i2c transfers if *no* reset gpio is configured/present. Best regards Uwe
On Thu, Feb 27, 2020 at 09:41:33AM +0100, Uwe Kleine-König wrote: > Hello, > > On Thu, Feb 27, 2020 at 09:30:07AM +0100, Richard Leitner wrote: > > On Thu, Feb 27, 2020 at 09:14:48AM +0100, Uwe Kleine-König wrote: > > > If you write this as: > > > > > > if (!hub->gpio_reset && usb251xb_hub_attached(hub)) > > > return 0; > > > > > > you save some i2c transfers in the presence of a reset gpio. > > > > I'd also go with this solution, so you only have i2c transfers when a > > reset gpio is configured/present. > > Nitpick: you only have additional i2c transfers if *no* reset gpio is > configured/present. Sure... That "no" got lost somewhere between my brain and the keyboard ;-) regards;rl > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe, Richard, thanks for your feedback :) Will prepare a v2. On 20-02-27 09:14, Uwe Kleine-König wrote: > On Thu, Feb 27, 2020 at 08:25:45AM +0100, Marco Felsch wrote: > > It is possible that the hub was configured earlier by the bootloader and > > we lack of the reset-gpio. In such a case the usb251xb_connect() fails > > because the registers are write-protected. Add a check to test if the > > hub is already connected and don't try to reconfigure the hub if we > > can't toggle the reset pin. Don't change the usb251xb_connect() logic > > if we can't read the status. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/usb/misc/usb251xb.c | 33 +++++++++++++++++++++++++++++++++ > > 1 file changed, 33 insertions(+) > > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c > > index 29fe5771c21b..9f9a64bab059 100644 > > --- a/drivers/usb/misc/usb251xb.c > > +++ b/drivers/usb/misc/usb251xb.c > > @@ -266,6 +266,30 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub) > > } > > #endif > > > > +static bool usb251xb_hub_attached(struct usb251xb *hub) > > +{ > > + char i2c_rb; > > + int err; > > + > > + err = i2c_smbus_read_block_data(hub->i2c, USB251XB_ADDR_STATUS_COMMAND, > > + &i2c_rb); > > + if (err < 0) { > > + /* > > + * The device disables the i2c-interface immediately after it > > + * received the USB_ATTACH signal. > > + */ > > + if (err == -ENXIO) > > + return true; > > + > > + dev_warn(hub->dev, > > + "Checking hub Status/Command register failed: %d\n", > > + err); > > + return false; > > + } > > + > > + return !!(i2c_rb & USB251XB_STATUS_COMMAND_ATTACH); > > +} > > + > > static void usb251xb_reset(struct usb251xb *hub) > > { > > if (!hub->gpio_reset) > > @@ -288,6 +312,15 @@ static int usb251xb_connect(struct usb251xb *hub) > > struct device *dev = hub->dev; > > int err, i; > > char i2c_wb[USB251XB_I2C_REG_SZ]; > > + bool is_attached; > > + > > + /* > > + * Check if configuration was done earlier by the bootloader. Trust them > > + * if it is the case and we are not capable to reset the hub. > > + */ > > + is_attached = usb251xb_hub_attached(hub); > > + if (!hub->gpio_reset && is_attached) > > + return 0; > > If you write this as: > > if (!hub->gpio_reset && usb251xb_hub_attached(hub)) > return 0; Yeah tought about this too. > you save some i2c transfers in the presence of a reset gpio. > > Also I wonder if skipping the initialisation is at least worth a > pr_info. Okay I will add this. Regards, Marco > Best regards > Uwe
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c index 29fe5771c21b..9f9a64bab059 100644 --- a/drivers/usb/misc/usb251xb.c +++ b/drivers/usb/misc/usb251xb.c @@ -266,6 +266,30 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub) } #endif +static bool usb251xb_hub_attached(struct usb251xb *hub) +{ + char i2c_rb; + int err; + + err = i2c_smbus_read_block_data(hub->i2c, USB251XB_ADDR_STATUS_COMMAND, + &i2c_rb); + if (err < 0) { + /* + * The device disables the i2c-interface immediately after it + * received the USB_ATTACH signal. + */ + if (err == -ENXIO) + return true; + + dev_warn(hub->dev, + "Checking hub Status/Command register failed: %d\n", + err); + return false; + } + + return !!(i2c_rb & USB251XB_STATUS_COMMAND_ATTACH); +} + static void usb251xb_reset(struct usb251xb *hub) { if (!hub->gpio_reset) @@ -288,6 +312,15 @@ static int usb251xb_connect(struct usb251xb *hub) struct device *dev = hub->dev; int err, i; char i2c_wb[USB251XB_I2C_REG_SZ]; + bool is_attached; + + /* + * Check if configuration was done earlier by the bootloader. Trust them + * if it is the case and we are not capable to reset the hub. + */ + is_attached = usb251xb_hub_attached(hub); + if (!hub->gpio_reset && is_attached) + return 0; memset(i2c_wb, 0, USB251XB_I2C_REG_SZ);
It is possible that the hub was configured earlier by the bootloader and we lack of the reset-gpio. In such a case the usb251xb_connect() fails because the registers are write-protected. Add a check to test if the hub is already connected and don't try to reconfigure the hub if we can't toggle the reset pin. Don't change the usb251xb_connect() logic if we can't read the status. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/usb/misc/usb251xb.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)