[PATCH 0/4] Add simple-pm ops

[PATCH 0/4] Add simple-pm ops

[Marco Felsch]

Hi,

this series adds the simple-pm ops to reinitialize the hub after a
suspend. For this reason I added the vdd-supply to ensure that vdd is
ready.

Regards,
  Marco

Marco Felsch (4):
  dt-bindings: usb: usb251xb: add documentation for voltage supply
  usb: usb251xb: add vdd supply support
  usb: usb251xb: simplify reset helper
  usb: usb251xb: add pm_ops

 .../devicetree/bindings/usb/usb251xb.txt      |  1 +
 drivers/usb/misc/usb251xb.c                   | 49 ++++++++++++++++---
 2 files changed, 42 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/4] dt-bindings: usb: usb251xb: add documentation for voltage supply

[Marco Felsch]

Add the optional voltage supply documentation. If not specified the
dummy-regulator is used.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 17915f64b8ee..4d5808b1cee0 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -12,6 +12,7 @@ Required properties :
 
 Optional properties :
  - reset-gpios : Should specify the gpio for hub reset
+ - vdd-supply : Should specify the phandle to the regulator supplying vdd
  - skip-config : Skip Hub configuration, but only send the USB-Attach command
  - vendor-id : Set USB Vendor ID of the hub (16 bit, default is 0x0424)
  - product-id : Set USB Product ID of the hub (16 bit, default depends on type)
-- 
2.20.1

Re: [PATCH 1/4] dt-bindings: usb: usb251xb: add documentation for voltage supply

[Richard Leitner]

On 17/09/2019 16:44, Marco Felsch wrote:
> Add the optional voltage supply documentation. If not specified the
> dummy-regulator is used.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Hi,
please feel free to add:

Acked-by: Richard Leitner <richard.leitner@skidata.com>

regards;Richard.L

> ---
>   Documentation/devicetree/bindings/usb/usb251xb.txt | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> index 17915f64b8ee..4d5808b1cee0 100644
> --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> @@ -12,6 +12,7 @@ Required properties :
>   
>   Optional properties :
>    - reset-gpios : Should specify the gpio for hub reset
> + - vdd-supply : Should specify the phandle to the regulator supplying vdd
>    - skip-config : Skip Hub configuration, but only send the USB-Attach command
>    - vendor-id : Set USB Vendor ID of the hub (16 bit, default is 0x0424)
>    - product-id : Set USB Product ID of the hub (16 bit, default depends on type)
> 

Re: [PATCH 1/4] dt-bindings: usb: usb251xb: add documentation for voltage supply

[Rob Herring]

On Tue, 17 Sep 2019 16:44:46 +0200, Marco Felsch wrote:
> Add the optional voltage supply documentation. If not specified the
> dummy-regulator is used.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 2/4] usb: usb251xb: add vdd supply support

[Marco Felsch]

Currently we don't handle the supply. We need to add the supply support
to be able to switch the supply off e.g. during a suspend-to-ram
operation. So we can guarantee a correct (re-)initialization.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/misc/usb251xb.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 6ca9111d150a..05819167604d 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/nls.h>
 #include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
@@ -116,6 +117,7 @@
 struct usb251xb {
 	struct device *dev;
 	struct i2c_client *i2c;
+	struct regulator *vdd;
 	u8 skip_config;
 	struct gpio_desc *gpio_reset;
 	u16 vendor_id;
@@ -420,6 +422,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		return err;
 	}
 
+	hub->vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(hub->vdd))
+		return PTR_ERR(hub->vdd);
+
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
 		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
 
@@ -665,6 +671,10 @@ static int usb251xb_probe(struct usb251xb *hub)
 	if (err)
 		return err;
 
+	err = regulator_enable(hub->vdd);
+	if (err)
+		return err;
+
 	err = usb251xb_connect(hub);
 	if (err) {
 		dev_err(dev, "Failed to connect hub (%d)\n", err);
-- 
2.20.1

Re: [PATCH 2/4] usb: usb251xb: add vdd supply support

[Richard Leitner]

On 17/09/2019 16:44, Marco Felsch wrote:
> Currently we don't handle the supply. We need to add the supply support
> to be able to switch the supply off e.g. during a suspend-to-ram
> operation. So we can guarantee a correct (re-)initialization.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Hi,
tested it on a i.MX6 based platform with an usb2512.
Therefore please feel free to add:

Reviewed-by: Richard Leitner <richard.leitner@skidata.com>

and/or

Tested-by: Richard Leitner <richard.leitner@skidata.com>

regards;Richard.L

> ---
>   drivers/usb/misc/usb251xb.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 6ca9111d150a..05819167604d 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -17,6 +17,7 @@
>   #include <linux/module.h>
>   #include <linux/nls.h>
>   #include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/slab.h>
>   
>   /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
> @@ -116,6 +117,7 @@
>   struct usb251xb {
>   	struct device *dev;
>   	struct i2c_client *i2c;
> +	struct regulator *vdd;
>   	u8 skip_config;
>   	struct gpio_desc *gpio_reset;
>   	u16 vendor_id;
> @@ -420,6 +422,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		return err;
>   	}
>   
> +	hub->vdd = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(hub->vdd))
> +		return PTR_ERR(hub->vdd);
> +
>   	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
>   		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
>   
> @@ -665,6 +671,10 @@ static int usb251xb_probe(struct usb251xb *hub)
>   	if (err)
>   		return err;
>   
> +	err = regulator_enable(hub->vdd);
> +	if (err)
> +		return err;
> +
>   	err = usb251xb_connect(hub);
>   	if (err) {
>   		dev_err(dev, "Failed to connect hub (%d)\n", err);
> 

[PATCH 3/4] usb: usb251xb: simplify reset helper

[Marco Felsch]

Currently the reset handler was always called to deassert the reset
line because assert the line was done during probe. Now if we want to
support pm by turn of the supply we need to call this routine twice and
the i2c_lock_bus is done twice too. To simplify that we can drop the
state and just do a reset in one go. So a future pm operation don't need
to lock the i2c bus twice.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/misc/usb251xb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 05819167604d..bc031d33f433 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -263,20 +263,19 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub)
 }
 #endif
 
-static void usb251xb_reset(struct usb251xb *hub, int state)
+static void usb251xb_reset(struct usb251xb *hub)
 {
 	if (!hub->gpio_reset)
 		return;
 
 	i2c_lock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
 
-	gpiod_set_value_cansleep(hub->gpio_reset, state);
+	gpiod_set_value_cansleep(hub->gpio_reset, 1);
+	usleep_range(1, 10);	/* >=1us RESET_N asserted */
+	gpiod_set_value_cansleep(hub->gpio_reset, 0);
 
 	/* wait for hub recovery/stabilization */
-	if (!state)
-		usleep_range(500, 750);	/* >=500us at power on */
-	else
-		usleep_range(1, 10);	/* >=1us at power down */
+	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
 
 	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
 }
@@ -294,7 +293,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 		i2c_wb[0] = 0x01;
 		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
 
-		usb251xb_reset(hub, 0);
+		usb251xb_reset(hub);
 
 		err = i2c_smbus_write_i2c_block_data(hub->i2c,
 				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
@@ -344,7 +343,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 	i2c_wb[USB251XB_ADDR_PORT_MAP_7]        = hub->port_map7;
 	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
 
-	usb251xb_reset(hub, 0);
+	usb251xb_reset(hub);
 
 	/* write registers */
 	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
-- 
2.20.1

Re: [PATCH 3/4] usb: usb251xb: simplify reset helper

[Richard Leitner]

On 17/09/2019 16:44, Marco Felsch wrote:
> Currently the reset handler was always called to deassert the reset
> line because assert the line was done during probe. Now if we want to
> support pm by turn of the supply we need to call this routine twice and
> the i2c_lock_bus is done twice too. To simplify that we can drop the
> state and just do a reset in one go. So a future pm operation don't need
> to lock the i2c bus twice.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Hi,
tested it on a i.MX6 based platform with an usb2512.
Therefore please feel free to add:

Reviewed-by: Richard Leitner <richard.leitner@skidata.com>

and/or

Tested-by: Richard Leitner <richard.leitner@skidata.com>

regards;Richard.L

> ---
>   drivers/usb/misc/usb251xb.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 05819167604d..bc031d33f433 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -263,20 +263,19 @@ static int usb251x_check_gpio_chip(struct usb251xb *hub)
>   }
>   #endif
>   
> -static void usb251xb_reset(struct usb251xb *hub, int state)
> +static void usb251xb_reset(struct usb251xb *hub)
>   {
>   	if (!hub->gpio_reset)
>   		return;
>   
>   	i2c_lock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
>   
> -	gpiod_set_value_cansleep(hub->gpio_reset, state);
> +	gpiod_set_value_cansleep(hub->gpio_reset, 1);
> +	usleep_range(1, 10);	/* >=1us RESET_N asserted */
> +	gpiod_set_value_cansleep(hub->gpio_reset, 0);
>   
>   	/* wait for hub recovery/stabilization */
> -	if (!state)
> -		usleep_range(500, 750);	/* >=500us at power on */
> -	else
> -		usleep_range(1, 10);	/* >=1us at power down */
> +	usleep_range(500, 750);	/* >=500us after RESET_N deasserted */
>   
>   	i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
>   }
> @@ -294,7 +293,7 @@ static int usb251xb_connect(struct usb251xb *hub)
>   		i2c_wb[0] = 0x01;
>   		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
>   
> -		usb251xb_reset(hub, 0);
> +		usb251xb_reset(hub);
>   
>   		err = i2c_smbus_write_i2c_block_data(hub->i2c,
>   				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
> @@ -344,7 +343,7 @@ static int usb251xb_connect(struct usb251xb *hub)
>   	i2c_wb[USB251XB_ADDR_PORT_MAP_7]        = hub->port_map7;
>   	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
>   
> -	usb251xb_reset(hub, 0);
> +	usb251xb_reset(hub);
>   
>   	/* write registers */
>   	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
> 

[PATCH 4/4] usb: usb251xb: add pm_ops

[Marco Felsch]

Currently the driver don't support pm_ops. These ops are not necessary
if the supply isn't switchable (always on). This assumptions seems to be
wrong because no one needs a powered hub during suspend-to-ram/disk.

So adding simple_dev_pm_ops to be able to switch off the hub during
suspend and to restore the config after a resume operation.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/misc/usb251xb.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index bc031d33f433..5bba19937da1 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -701,6 +701,29 @@ static int usb251xb_i2c_probe(struct i2c_client *i2c,
 	return usb251xb_probe(hub);
 }
 
+static int __maybe_unused usb251xb_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct usb251xb *hub = i2c_get_clientdata(client);
+
+	return regulator_disable(hub->vdd);
+}
+
+static int __maybe_unused usb251xb_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct usb251xb *hub = i2c_get_clientdata(client);
+	int err;
+
+	err = regulator_enable(hub->vdd);
+	if (err)
+		return err;
+
+	return usb251xb_connect(hub);
+}
+
+static SIMPLE_DEV_PM_OPS(usb251xb_pm_ops, usb251xb_suspend, usb251xb_resume);
+
 static const struct i2c_device_id usb251xb_id[] = {
 	{ "usb2512b", 0 },
 	{ "usb2512bi", 0 },
@@ -718,6 +741,7 @@ static struct i2c_driver usb251xb_i2c_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
 		.of_match_table = of_match_ptr(usb251xb_of_match),
+		.pm = &usb251xb_pm_ops,
 	},
 	.probe    = usb251xb_i2c_probe,
 	.id_table = usb251xb_id,
-- 
2.20.1

Re: [PATCH 4/4] usb: usb251xb: add pm_ops

[Richard Leitner]

On 17/09/2019 16:44, Marco Felsch wrote:
> Currently the driver don't support pm_ops. These ops are not necessary
> if the supply isn't switchable (always on). This assumptions seems to be
> wrong because no one needs a powered hub during suspend-to-ram/disk.
> 
> So adding simple_dev_pm_ops to be able to switch off the hub during
> suspend and to restore the config after a resume operation.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Hi,
please feel free to add:

Acked-by: Richard Leitner <richard.leitner@skidata.com>

regards;Richard.L

> ---
>   drivers/usb/misc/usb251xb.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index bc031d33f433..5bba19937da1 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -701,6 +701,29 @@ static int usb251xb_i2c_probe(struct i2c_client *i2c,
>   	return usb251xb_probe(hub);
>   }
>   
> +static int __maybe_unused usb251xb_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct usb251xb *hub = i2c_get_clientdata(client);
> +
> +	return regulator_disable(hub->vdd);
> +}
> +
> +static int __maybe_unused usb251xb_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct usb251xb *hub = i2c_get_clientdata(client);
> +	int err;
> +
> +	err = regulator_enable(hub->vdd);
> +	if (err)
> +		return err;
> +
> +	return usb251xb_connect(hub);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(usb251xb_pm_ops, usb251xb_suspend, usb251xb_resume);
> +
>   static const struct i2c_device_id usb251xb_id[] = {
>   	{ "usb2512b", 0 },
>   	{ "usb2512bi", 0 },
> @@ -718,6 +741,7 @@ static struct i2c_driver usb251xb_i2c_driver = {
>   	.driver = {
>   		.name = DRIVER_NAME,
>   		.of_match_table = of_match_ptr(usb251xb_of_match),
> +		.pm = &usb251xb_pm_ops,
>   	},
>   	.probe    = usb251xb_i2c_probe,
>   	.id_table = usb251xb_id,
>