[PATCH 0/5] usb: usb251xb: Add USB2517i hub support and fix some bugs

[PATCH 0/5] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to perform following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Serge Semin (5):
  usb: usb251xb: Add USB2517/i hub support
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts nodes
  usb: usb251xb: Use GPIO descriptor consumer interface
  usb: usb251xb: Add copyrights

 Documentation/devicetree/bindings/usb/usb251xb.txt |  12 +-
 drivers/usb/misc/usb251xb.c                        | 154 +++++++++++++++------
 2 files changed, 123 insertions(+), 43 deletions(-)

-- 
2.12.0

[PATCH 0/5] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to perform following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Serge Semin (5):
  usb: usb251xb: Add USB2517/i hub support
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts nodes
  usb: usb251xb: Use GPIO descriptor consumer interface
  usb: usb251xb: Add copyrights

 Documentation/devicetree/bindings/usb/usb251xb.txt |  12 +-
 drivers/usb/misc/usb251xb.c                        | 154 +++++++++++++++------
 2 files changed, 123 insertions(+), 43 deletions(-)

-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

USB2517i hubs are very like USB251xb devices series. They have almost
the same configuration registers space except number of ports, led
configurations and lack of battery settings. All these peculiarities
are reflected in this patch.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-
 drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
 2 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3957d4eda..3d84626d3 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -6,7 +6,8 @@ Hi-Speed Controller.
 Required properties :
  - compatible : Should be "microchip,usb251xb" or one of the specific types:
 	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
-	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
+	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
+	"microchip,usb2517", "microchip,usb2517i"
  - reset-gpios : Should specify the gpio for hub reset
  - reg : I2C address on the selected bus (default is <0x2C>)
 
@@ -36,6 +37,7 @@ Optional properties :
 	an invalid value is given, the default is used instead.
  - compound-device : indicate the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
+ - speed-led-mode : led speed indiation mode selection (usb2517 only)
  - string-support : enable string descriptor support (required for manufacturer,
 	product and serial string configuration)
  - non-removable-ports : Should specify the ports which have a non-removable
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 91f66d68b..2ef22758c 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -38,6 +38,7 @@
 #define USB251XB_DEF_PRODUCT_ID_12	0x2512 /* USB2512B/12Bi */
 #define USB251XB_DEF_PRODUCT_ID_13	0x2513 /* USB2513B/13Bi */
 #define USB251XB_DEF_PRODUCT_ID_14	0x2514 /* USB2514B/14Bi */
+#define USB251XB_DEF_PRODUCT_ID_17	0x2517 /* USB2517i */
 
 #define USB251XB_ADDR_DEVICE_ID_LSB	0x04
 #define USB251XB_ADDR_DEVICE_ID_MSB	0x05
@@ -48,7 +49,7 @@
 #define USB251XB_ADDR_CONFIG_DATA_2	0x07
 #define USB251XB_DEF_CONFIG_DATA_2	0x20
 #define USB251XB_ADDR_CONFIG_DATA_3	0x08
-#define USB251XB_DEF_CONFIG_DATA_3	0x02
+#define USB251XB_DEF_CONFIG_DATA_3	0x00
 
 #define USB251XB_ADDR_NON_REMOVABLE_DEVICES	0x09
 #define USB251XB_DEF_NON_REMOVABLE_DEVICES	0x00
@@ -82,7 +83,7 @@
 
 #define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
 #define USB251XB_ADDR_PRODUCT_STRING		0x54
-#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
+#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi/7i"
 
 #define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
 #define USB251XB_ADDR_SERIAL_STRING		0x92
@@ -93,8 +94,10 @@
 
 #define USB251XB_ADDR_BOOST_UP	0xF6
 #define USB251XB_DEF_BOOST_UP	0x00
-#define USB251XB_ADDR_BOOST_X	0xF8
-#define USB251XB_DEF_BOOST_X	0x00
+#define USB251XB_ADDR_BOOST_57	0xF8
+#define USB251XB_DEF_BOOST_57	0x00
+#define USB251XB_ADDR_BOOST_14	0xF8
+#define USB251XB_DEF_BOOST_14	0x00
 
 #define USB251XB_ADDR_PORT_SWAP	0xFA
 #define USB251XB_DEF_PORT_SWAP	0x00
@@ -102,7 +105,11 @@
 #define USB251XB_ADDR_PORT_MAP_12	0xFB
 #define USB251XB_DEF_PORT_MAP_12	0x00
 #define USB251XB_ADDR_PORT_MAP_34	0xFC
-#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB2513B/i & USB2514B/i only */
+#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB251{3B/i,4B/i,7/i} only */
+#define USB251XB_ADDR_PORT_MAP_56	0xFD
+#define USB251XB_DEF_PORT_MAP_56	0x00 /* USB2517/i only */
+#define USB251XB_ADDR_PORT_MAP_7	0xFE
+#define USB251XB_DEF_PORT_MAP_7		0x00 /* USB2517/i only */
 
 #define USB251XB_ADDR_STATUS_COMMAND		0xFF
 #define USB251XB_STATUS_COMMAND_SMBUS_DOWN	0x04
@@ -144,48 +151,88 @@ struct usb251xb {
 	char serial[USB251XB_STRING_BUFSIZE];
 	u8  bat_charge_en;
 	u8  boost_up;
-	u8  boost_x;
+	u8  boost_57;
+	u8  boost_14;
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
+	u8  port_map56;
+	u8  port_map7;
 	u8  status;
 };
 
 struct usb251xb_data {
 	u16 product_id;
+	u8 port_cnt;
+	bool led_support;
+	bool bat_support;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
+	.led_support = false,
+	.bat_support = true,
 	.product_str = "USB2514Bi",
 };
 
+static const struct usb251xb_data usb2517_data = {
+	.product_id = 0x2517,
+	.port_cnt = 7,
+	.led_support = true,
+	.bat_support = false,
+	.product_str = "USB2517",
+};
+
+static const struct usb251xb_data usb2517i_data = {
+	.product_id = 0x2517,
+	.port_cnt = 7,
+	.led_support = true,
+	.bat_support = false,
+	.product_str = "USB2517i",
+};
+
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
 	if (!gpio_is_valid(hub->gpio_reset))
@@ -254,10 +301,13 @@ static int usb251xb_connect(struct usb251xb *hub)
 	       USB251XB_STRING_BUFSIZE);
 	i2c_wb[USB251XB_ADDR_BATTERY_CHARGING_ENABLE] = hub->bat_charge_en;
 	i2c_wb[USB251XB_ADDR_BOOST_UP]          = hub->boost_up;
-	i2c_wb[USB251XB_ADDR_BOOST_X]           = hub->boost_x;
+	i2c_wb[USB251XB_ADDR_BOOST_57]          = hub->boost_57;
+	i2c_wb[USB251XB_ADDR_BOOST_14]          = hub->boost_14;
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_56]       = hub->port_map56;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_7]        = hub->port_map7;
 	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
 
 	usb251xb_reset(hub, 1);
@@ -402,6 +452,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "port-mapping-mode", NULL))
 		hub->conf_data3 |= BIT(3);
 
+	if (data->led_support && of_get_property(np, "speed-led-mode", NULL))
+		hub->conf_data3 |= BIT(1);
+
 	if (of_get_property(np, "string-support", NULL))
 		hub->conf_data3 |= BIT(0);
 
@@ -411,8 +464,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->non_rem_dev |= BIT(port);
+			else
+				dev_warn(dev, "port %u doesn't exist\n", port);
 		}
 	}
 
@@ -483,10 +538,13 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
-	hub->boost_x = USB251XB_DEF_BOOST_X;
+	hub->boost_57 = USB251XB_DEF_BOOST_57;
+	hub->boost_14 = USB251XB_DEF_BOOST_14;
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
+	hub->port_map56 = USB251XB_DEF_PORT_MAP_56;
+	hub->port_map7  = USB251XB_DEF_PORT_MAP_7;
 
 	return 0;
 }
@@ -511,6 +569,12 @@ static const struct of_device_id usb251xb_of_match[] = {
 		.compatible = "microchip,usb2514bi",
 		.data = &usb2514bi_data,
 	}, {
+		.compatible = "microchip,usb2517",
+		.data = &usb2517_data,
+	}, {
+		.compatible = "microchip,usb2517i",
+		.data = &usb2517i_data,
+	}, {
 		/* sentinel */
 	}
 };
@@ -574,6 +638,8 @@ static const struct i2c_device_id usb251xb_id[] = {
 	{ "usb2513bi", 0 },
 	{ "usb2514b", 0 },
 	{ "usb2514bi", 0 },
+	{ "usb2517", 0 },
+	{ "usb2517i", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, usb251xb_id);
-- 
2.12.0

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Greg KH]

On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> USB2517i hubs are very like USB251xb devices series. They have almost
> the same configuration registers space except number of ports, led
> configurations and lack of battery settings. All these peculiarities
> are reflected in this patch.

Please add one type of feature at a time per patch.  That makes it
easier to review and ensure you got it right.

thanks,

greg k-h

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

On Fri, Sep 15, 2017 at 04:45:14PM -0700, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> > USB2517i hubs are very like USB251xb devices series. They have almost
> > the same configuration registers space except number of ports, led
> > configurations and lack of battery settings. All these peculiarities
> > are reflected in this patch.
> 
> Please add one type of feature at a time per patch.  That makes it
> easier to review and ensure you got it right.
> 
> thanks,
> 
> greg k-h

Ok. The patch isn't that big though.

Regards,
-Sergey

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

On Fri, Sep 15, 2017 at 04:45:14PM -0700, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> > USB2517i hubs are very like USB251xb devices series. They have almost
> > the same configuration registers space except number of ports, led
> > configurations and lack of battery settings. All these peculiarities
> > are reflected in this patch.
> 
> Please add one type of feature at a time per patch.  That makes it
> easier to review and ensure you got it right.
> 
> thanks,
> 
> greg k-h

Ok. The patch isn't that big though.

Regards,
-Sergey

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Rob Herring]

On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> USB2517i hubs are very like USB251xb devices series. They have almost
> the same configuration registers space except number of ports, led
> configurations and lack of battery settings. All these peculiarities
> are reflected in this patch.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-

Though Greg wants the code split, I want the binding as one change. H/w 
doesn't gain features one by one.

It's preferred to split bindings to a separate patch.

>  drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
>  2 files changed, 78 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> index 3957d4eda..3d84626d3 100644
> --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> @@ -6,7 +6,8 @@ Hi-Speed Controller.
>  Required properties :
>   - compatible : Should be "microchip,usb251xb" or one of the specific types:
>  	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
> -	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
> +	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
> +	"microchip,usb2517", "microchip,usb2517i"
>   - reset-gpios : Should specify the gpio for hub reset
>   - reg : I2C address on the selected bus (default is <0x2C>)
>  
> @@ -36,6 +37,7 @@ Optional properties :
>  	an invalid value is given, the default is used instead.
>   - compound-device : indicate the hub is part of a compound device
>   - port-mapping-mode : enable port mapping mode
> + - speed-led-mode : led speed indiation mode selection (usb2517 only)

This is a boolean or has values? What are valid values?

This needs a vendor prefix. Somehow the other properties got in without.

>   - string-support : enable string descriptor support (required for manufacturer,
>  	product and serial string configuration)
>   - non-removable-ports : Should specify the ports which have a non-removable

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

On Wed, Sep 20, 2017 at 03:52:35PM -0500, Rob Herring <robh@kernel.org> wrote:
> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> > USB2517i hubs are very like USB251xb devices series. They have almost
> > the same configuration registers space except number of ports, led
> > configurations and lack of battery settings. All these peculiarities
> > are reflected in this patch.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-
> 
> Though Greg wants the code split, I want the binding as one change. H/w 
> doesn't gain features one by one.
> 
> It's preferred to split bindings to a separate patch.
> 

Folks, you are really driving people crazy. When I was reviewing a
kernel-patchset from a Logan-guy, I asked him to combine some of his patches,
since in fact their combination represented one solid driver. I was told to go
very far, and Greg supported him with it. I'm not going to be that rude and will
do as you asked me to. But really, isn't it possible to have some strict rule
created so a developer would always follow it thereby not being asked to
combine/split patches almost everytime?
The only way I see for now is to know each maintainer personal preferences.

> >  drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
> >  2 files changed, 78 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> > index 3957d4eda..3d84626d3 100644
> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> > @@ -6,7 +6,8 @@ Hi-Speed Controller.
> >  Required properties :
> >   - compatible : Should be "microchip,usb251xb" or one of the specific types:
> >  	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
> > -	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
> > +	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
> > +	"microchip,usb2517", "microchip,usb2517i"
> >   - reset-gpios : Should specify the gpio for hub reset
> >   - reg : I2C address on the selected bus (default is <0x2C>)
> >  
> > @@ -36,6 +37,7 @@ Optional properties :
> >  	an invalid value is given, the default is used instead.
> >   - compound-device : indicate the hub is part of a compound device
> >   - port-mapping-mode : enable port mapping mode
> > + - speed-led-mode : led speed indiation mode selection (usb2517 only)
> 
> This is a boolean or has values? What are valid values?
> 

It's boolean. Shall I rename it as:
"- speed-led-mode : enable led speed indication mode (usb2517 only)"?

> This needs a vendor prefix. Somehow the other properties got in without.
> 

Hmm, it's not vendor specific, but device-specific. USB2517 is produced
by the same vendor - microchip. The new device got almost the same functionality as
the others, except number or ports, LED feature and battery enable feature.
The last one isn't configurable by dts. The rest of the properties are the same
for all the compatible devices. So what properties you are talking about then?

> >   - string-support : enable string descriptor support (required for manufacturer,
> >  	product and serial string configuration)
> >   - non-removable-ports : Should specify the ports which have a non-removable

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Rob Herring]

On Wed, Sep 20, 2017 at 4:15 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 03:52:35PM -0500, Rob Herring <robh@kernel.org> wrote:
>> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
>> > USB2517i hubs are very like USB251xb devices series. They have almost
>> > the same configuration registers space except number of ports, led
>> > configurations and lack of battery settings. All these peculiarities
>> > are reflected in this patch.
>> >
>> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>> > ---
>> >  Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-
>>
>> Though Greg wants the code split, I want the binding as one change. H/w
>> doesn't gain features one by one.
>>
>> It's preferred to split bindings to a separate patch.
>>
>
> Folks, you are really driving people crazy. When I was reviewing a
> kernel-patchset from a Logan-guy, I asked him to combine some of his patches,
> since in fact their combination represented one solid driver. I was told to go
> very far, and Greg supported him with it. I'm not going to be that rude and will
> do as you asked me to. But really, isn't it possible to have some strict rule
> created so a developer would always follow it thereby not being asked to
> combine/split patches almost everytime?
> The only way I see for now is to know each maintainer personal preferences.

That rule is in Documentation/devicetree/bindings/submitting-patches.txt.

I generally only ask to respin and split bindings if there's other changes.

>> >  drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
>> >  2 files changed, 78 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > index 3957d4eda..3d84626d3 100644
>> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > @@ -6,7 +6,8 @@ Hi-Speed Controller.
>> >  Required properties :
>> >   - compatible : Should be "microchip,usb251xb" or one of the specific types:
>> >     "microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
>> > -   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
>> > +   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
>> > +   "microchip,usb2517", "microchip,usb2517i"
>> >   - reset-gpios : Should specify the gpio for hub reset
>> >   - reg : I2C address on the selected bus (default is <0x2C>)
>> >
>> > @@ -36,6 +37,7 @@ Optional properties :
>> >     an invalid value is given, the default is used instead.
>> >   - compound-device : indicate the hub is part of a compound device
>> >   - port-mapping-mode : enable port mapping mode
>> > + - speed-led-mode : led speed indiation mode selection (usb2517 only)
>>
>> This is a boolean or has values? What are valid values?
>>
>
> It's boolean. Shall I rename it as:
> "- speed-led-mode : enable led speed indication mode (usb2517 only)"?

Having the the word "boolean" in there would help.

>> This needs a vendor prefix. Somehow the other properties got in without.
>>
>
> Hmm, it's not vendor specific, but device-specific. USB2517 is produced
> by the same vendor - microchip. The new device got almost the same functionality as
> the others, except number or ports, LED feature and battery enable feature.
> The last one isn't configurable by dts. The rest of the properties are the same
> for all the compatible devices. So what properties you are talking about then?

Well, we don't name things after devices. Properties are either common
(either from DT Spec or a class of device (clocks, regulators, USB
device, USB hubs, etc.)) or vendor specific. I haven't looked at which
other ones specifically could be common for hubs or USB devices and
which ones should be MicroChip specific.

Rob

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

On Thu, Sep 21, 2017 at 11:53:29AM -0500, Rob Herring <robh@kernel.org> wrote:
> On Wed, Sep 20, 2017 at 4:15 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Wed, Sep 20, 2017 at 03:52:35PM -0500, Rob Herring <robh@kernel.org> wrote:
> >> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> >> > USB2517i hubs are very like USB251xb devices series. They have almost
> >> > the same configuration registers space except number of ports, led
> >> > configurations and lack of battery settings. All these peculiarities
> >> > are reflected in this patch.
> >> >
> >> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> >> > ---
> >> >  Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-
> >>
> >> Though Greg wants the code split, I want the binding as one change. H/w
> >> doesn't gain features one by one.
> >>
> >> It's preferred to split bindings to a separate patch.
> >>
> >
> > Folks, you are really driving people crazy. When I was reviewing a
> > kernel-patchset from a Logan-guy, I asked him to combine some of his patches,
> > since in fact their combination represented one solid driver. I was told to go
> > very far, and Greg supported him with it. I'm not going to be that rude and will
> > do as you asked me to. But really, isn't it possible to have some strict rule
> > created so a developer would always follow it thereby not being asked to
> > combine/split patches almost everytime?
> > The only way I see for now is to know each maintainer personal preferences.
> 
> That rule is in Documentation/devicetree/bindings/submitting-patches.txt.
> 
> I generally only ask to respin and split bindings if there's other changes.
> 

Great! I didn't know there is a document like that. Ok. From now I'll do as
it's prescribed there.

> >> >  drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
> >> >  2 files changed, 78 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > index 3957d4eda..3d84626d3 100644
> >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > @@ -6,7 +6,8 @@ Hi-Speed Controller.
> >> >  Required properties :
> >> >   - compatible : Should be "microchip,usb251xb" or one of the specific types:
> >> >     "microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
> >> > -   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
> >> > +   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
> >> > +   "microchip,usb2517", "microchip,usb2517i"
> >> >   - reset-gpios : Should specify the gpio for hub reset
> >> >   - reg : I2C address on the selected bus (default is <0x2C>)
> >> >
> >> > @@ -36,6 +37,7 @@ Optional properties :
> >> >     an invalid value is given, the default is used instead.
> >> >   - compound-device : indicate the hub is part of a compound device
> >> >   - port-mapping-mode : enable port mapping mode
> >> > + - speed-led-mode : led speed indiation mode selection (usb2517 only)
> >>
> >> This is a boolean or has values? What are valid values?
> >>
> >
> > It's boolean. Shall I rename it as:
> > "- speed-led-mode : enable led speed indication mode (usb2517 only)"?
> 
> Having the the word "boolean" in there would help.
> 
> >> This needs a vendor prefix. Somehow the other properties got in without.
> >>
> >
> > Hmm, it's not vendor specific, but device-specific. USB2517 is produced
> > by the same vendor - microchip. The new device got almost the same functionality as
> > the others, except number or ports, LED feature and battery enable feature.
> > The last one isn't configurable by dts. The rest of the properties are the same
> > for all the compatible devices. So what properties you are talking about then?
> 
> Well, we don't name things after devices. Properties are either common
> (either from DT Spec or a class of device (clocks, regulators, USB
> device, USB hubs, etc.)) or vendor specific. I haven't looked at which
> other ones specifically could be common for hubs or USB devices and
> which ones should be MicroChip specific.
> 

Alright. I found the recommended vendor-specific prefix, it's "microchip". What's 
next? The thing is, that this driver isn't usual USB root port controller driver.
It's the driver to perform the usb251x hubs configuration on boot time in accordance
with the hardware specifics. So to speak, this driver is something like EEPROM firmware
embedded in the kernel and configured by device tree node properties. I can't be sure,
whether all of these settings might be vendor specific, or some of them still can be
exposed by the hubs of other vendors. Most of the circuit designers just add real
EEPROMs to be connected to hubs, so their configurations would be loaded from them.
What shall we do with this bindings then? Shall we add the vendor-specific vendor to
the bindings file?

> Rob

Re: [PATCH 1/5] usb: usb251xb: Add USB2517/i hub support

[Serge Semin]

On Thu, Sep 21, 2017 at 11:53:29AM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Sep 20, 2017 at 4:15 PM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Sep 20, 2017 at 03:52:35PM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> On Sat, Sep 16, 2017 at 02:31:09AM +0300, Serge Semin wrote:
> >> > USB2517i hubs are very like USB251xb devices series. They have almost
> >> > the same configuration registers space except number of ports, led
> >> > configurations and lack of battery settings. All these peculiarities
> >> > are reflected in this patch.
> >> >
> >> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> >  Documentation/devicetree/bindings/usb/usb251xb.txt |  4 +-
> >>
> >> Though Greg wants the code split, I want the binding as one change. H/w
> >> doesn't gain features one by one.
> >>
> >> It's preferred to split bindings to a separate patch.
> >>
> >
> > Folks, you are really driving people crazy. When I was reviewing a
> > kernel-patchset from a Logan-guy, I asked him to combine some of his patches,
> > since in fact their combination represented one solid driver. I was told to go
> > very far, and Greg supported him with it. I'm not going to be that rude and will
> > do as you asked me to. But really, isn't it possible to have some strict rule
> > created so a developer would always follow it thereby not being asked to
> > combine/split patches almost everytime?
> > The only way I see for now is to know each maintainer personal preferences.
> 
> That rule is in Documentation/devicetree/bindings/submitting-patches.txt.
> 
> I generally only ask to respin and split bindings if there's other changes.
> 

Great! I didn't know there is a document like that. Ok. From now I'll do as
it's prescribed there.

> >> >  drivers/usb/misc/usb251xb.c                        | 84 +++++++++++++++++++---
> >> >  2 files changed, 78 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > index 3957d4eda..3d84626d3 100644
> >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > @@ -6,7 +6,8 @@ Hi-Speed Controller.
> >> >  Required properties :
> >> >   - compatible : Should be "microchip,usb251xb" or one of the specific types:
> >> >     "microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
> >> > -   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
> >> > +   "microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
> >> > +   "microchip,usb2517", "microchip,usb2517i"
> >> >   - reset-gpios : Should specify the gpio for hub reset
> >> >   - reg : I2C address on the selected bus (default is <0x2C>)
> >> >
> >> > @@ -36,6 +37,7 @@ Optional properties :
> >> >     an invalid value is given, the default is used instead.
> >> >   - compound-device : indicate the hub is part of a compound device
> >> >   - port-mapping-mode : enable port mapping mode
> >> > + - speed-led-mode : led speed indiation mode selection (usb2517 only)
> >>
> >> This is a boolean or has values? What are valid values?
> >>
> >
> > It's boolean. Shall I rename it as:
> > "- speed-led-mode : enable led speed indication mode (usb2517 only)"?
> 
> Having the the word "boolean" in there would help.
> 
> >> This needs a vendor prefix. Somehow the other properties got in without.
> >>
> >
> > Hmm, it's not vendor specific, but device-specific. USB2517 is produced
> > by the same vendor - microchip. The new device got almost the same functionality as
> > the others, except number or ports, LED feature and battery enable feature.
> > The last one isn't configurable by dts. The rest of the properties are the same
> > for all the compatible devices. So what properties you are talking about then?
> 
> Well, we don't name things after devices. Properties are either common
> (either from DT Spec or a class of device (clocks, regulators, USB
> device, USB hubs, etc.)) or vendor specific. I haven't looked at which
> other ones specifically could be common for hubs or USB devices and
> which ones should be MicroChip specific.
> 

Alright. I found the recommended vendor-specific prefix, it's "microchip". What's 
next? The thing is, that this driver isn't usual USB root port controller driver.
It's the driver to perform the usb251x hubs configuration on boot time in accordance
with the hardware specifics. So to speak, this driver is something like EEPROM firmware
embedded in the kernel and configured by device tree node properties. I can't be sure,
whether all of these settings might be vendor specific, or some of them still can be
exposed by the hubs of other vendors. Most of the circuit designers just add real
EEPROMs to be connected to hubs, so their configurations would be loaded from them.
What shall we do with this bindings then? Shall we add the vendor-specific vendor to
the bindings file?

> Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] usb: usb251xb: Fix property_u32 NULL pointer dereference

[Serge Semin]

The methods like of_property_read_u32 utilizing the specified
pointer permit only the pointer to a preallocated u32 storage as the
third argument. As a result the driver crashes on NULL pointer
dereference in case if "oc-delay-us" or "power-on-time-ms" declared
in dts file.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 2ef22758c..8101c6212 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -348,7 +348,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	struct device *dev = hub->dev;
 	struct device_node *np = dev->of_node;
 	int len, err, i;
-	u32 *property_u32 = NULL;
+	u32 property_u32 = 0;
 	const u32 *cproperty_u32;
 	const char *cproperty_char;
 	char str[USB251XB_STRING_BUFSIZE / 2];
@@ -425,16 +425,16 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "dynamic-power-switching", NULL))
 		hub->conf_data2 |= BIT(7);
 
-	if (!of_property_read_u32(np, "oc-delay-us", property_u32)) {
-		if (*property_u32 == 100) {
+	if (!of_property_read_u32(np, "oc-delay-us", &property_u32)) {
+		if (property_u32 == 100) {
 			/* 100 us*/
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 &= ~BIT(4);
-		} else if (*property_u32 == 4000) {
+		} else if (property_u32 == 4000) {
 			/* 4 ms */
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 |= BIT(4);
-		} else if (*property_u32 == 16000) {
+		} else if (property_u32 == 16000) {
 			/* 16 ms */
 			hub->conf_data2 |= BIT(5);
 			hub->conf_data2 |= BIT(4);
@@ -494,8 +494,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	}
 
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
-	if (!of_property_read_u32(np, "power-on-time-ms", property_u32))
-		hub->power_on_time = min_t(u8, *property_u32 / 2, 255);
+	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
+		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
 
 	if (of_property_read_u16_array(np, "language-id", &hub->lang_id, 1))
 		hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
-- 
2.12.0

[PATCH 3/5] usb: usb251xb: Add max power/current dts nodes

[Serge Semin]

This parameters may be varied in accordance with hardware specifics.
So lets add the corresponding settings to the usb251x driver dts
specification.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  6 ++++++
 drivers/usb/misc/usb251xb.c                        | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3d84626d3..dd59a32e7 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -44,6 +44,12 @@ Optional properties :
 	device connected.
  - sp-disabled-ports : Specifies the ports which will be self-power disabled
  - bp-disabled-ports : Specifies the ports which will be bus-power disabled
+ - sp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a self-powered hub. The value
+	is given in mA in a 0 - 100 range (default is 1mA).
+ - bp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a bus-powered hub. The value
+	is given in mA in a 0 - 510 range (default is 100mA).
  - power-on-time-ms : Specifies the time it takes from the time the host
 	initiates the power-on sequence to a port until the port has adequate
 	power. The value is given in ms in a 0 - 510 range (default is 100ms).
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 8101c6212..d6a0ff4ec 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -493,6 +493,22 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		}
 	}
 
+	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
+	if (!of_property_read_u32(np, "sp-max-power", &property_u32))
+		hub->max_power_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
+	if (!of_property_read_u32(np, "bp-max-power", &property_u32))
+		hub->max_power_bp = min_t(u8, property_u32 / 2, 255);
+
+	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
+	if (!of_property_read_u32(np, "sp-max-current", &property_u32))
+		hub->max_current_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
+	if (!of_property_read_u32(np, "bp-max-current", &property_u32))
+		hub->max_current_bp = min_t(u8, property_u32 / 2, 255);
+
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
 	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
 		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
@@ -532,10 +548,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
 	 */
-	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
 	hub->boost_57 = USB251XB_DEF_BOOST_57;
-- 
2.12.0

[PATCH 4/5] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

The driver used to be developed with legacy GPIO API support. It's
better to use descriptor-based interface for several reasons. First
of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
nodes, which is essential since different hardware may have different
GPIOs connectivity including the logical value inversion. Secondly,
by requesting the reset GPIO descriptor the driver prevent the other
applications from changing its value. And last but not least the
legacy GPIO interface should be avoided in the new code due to it
obsolescence.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  2 +-
 drivers/usb/misc/usb251xb.c                        | 33 +++++++++-------------
 2 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index dd59a32e7..7c981d556 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -8,10 +8,10 @@ Required properties :
 	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
 	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
 	"microchip,usb2517", "microchip,usb2517i"
- - reset-gpios : Should specify the gpio for hub reset
  - reg : I2C address on the selected bus (default is <0x2C>)
 
 Optional properties :
+ - reset-gpios : Should specify the gpio for hub reset
  - 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)
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index d6a0ff4ec..1d47acd09 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -20,12 +20,11 @@
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/nls.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/slab.h>
 
 /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
@@ -127,7 +126,7 @@ struct usb251xb {
 	struct device *dev;
 	struct i2c_client *i2c;
 	u8 skip_config;
-	int gpio_reset;
+	struct gpio_desc *gpio_reset;
 	u16 vendor_id;
 	u16 product_id;
 	u16 device_id;
@@ -235,13 +234,13 @@ static const struct usb251xb_data usb2517i_data = {
 
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
-	if (!gpio_is_valid(hub->gpio_reset))
+	if (!hub->gpio_reset)
 		return;
 
-	gpio_set_value_cansleep(hub->gpio_reset, state);
+	gpiod_set_value_cansleep(hub->gpio_reset, state);
 
 	/* wait for hub recovery/stabilization */
-	if (state)
+	if (!state)
 		usleep_range(500, 750);	/* >=500us at power on */
 	else
 		usleep_range(1, 10);	/* >=1us at power down */
@@ -260,7 +259,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 		i2c_wb[0] = 0x01;
 		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
 
-		usb251xb_reset(hub, 1);
+		usb251xb_reset(hub, 0);
 
 		err = i2c_smbus_write_i2c_block_data(hub->i2c,
 				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
@@ -310,7 +309,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, 1);
+	usb251xb_reset(hub, 0);
 
 	/* write registers */
 	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
@@ -363,19 +362,13 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	else
 		hub->skip_config = 0;
 
-	hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
-	if (hub->gpio_reset == -EPROBE_DEFER)
+	hub->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (PTR_ERR(hub->gpio_reset) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
-	if (gpio_is_valid(hub->gpio_reset)) {
-		err = devm_gpio_request_one(dev, hub->gpio_reset,
-					    GPIOF_OUT_INIT_LOW,
-					    "usb251xb reset");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as reset pin (%d)\n",
-				hub->gpio_reset, err);
-			return err;
-		}
+	} else if (IS_ERR(hub->gpio_reset)) {
+		err = PTR_ERR(hub->gpio_reset);
+		dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+		return err;
 	}
 
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
-- 
2.12.0

[PATCH 5/5] usb: usb251xb: Add copyrights

[Serge Semin]

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 1 +
 1 file changed, 1 insertions(+), 0 deletion(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 1d47acd09..43afa04ad 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -3,6 +3,7 @@
  * Configuration via SMBus.
  *
  * Copyright (c) 2017 SKIDATA AG
+ * Copyright (c) 2017 T-platforms
  *
  * This work is based on the USB3503 driver by Dongjin Kim and
  * a not-accepted patch by Fabien Lahoudere, see:
-- 
2.12.0

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/misc/usb251xb.c | 1 +
>  1 file changed, 1 insertions(+), 0 deletion(-)

I can't take patches without any changelog text, sorry.

> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 1d47acd09..43afa04ad 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -3,6 +3,7 @@
>   * Configuration via SMBus.
>   *
>   * Copyright (c) 2017 SKIDATA AG
> + * Copyright (c) 2017 T-platforms

I don't see the connection between a random company, and a random
gmail.com email address.  Please explain :)

thanks,

greg k-h

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/misc/usb251xb.c | 1 +
>  1 file changed, 1 insertions(+), 0 deletion(-)

I can't take patches without any changelog text, sorry.

> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 1d47acd09..43afa04ad 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -3,6 +3,7 @@
>   * Configuration via SMBus.
>   *
>   * Copyright (c) 2017 SKIDATA AG
> + * Copyright (c) 2017 T-platforms

I don't see the connection between a random company, and a random
gmail.com email address.  Please explain :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH wrote:
> On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/usb/misc/usb251xb.c | 1 +
> >  1 file changed, 1 insertions(+), 0 deletion(-)
> 
> I can't take patches without any changelog text, sorry.
> 
> > 
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 1d47acd09..43afa04ad 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> I don't see the connection between a random company, and a random
> gmail.com email address.  Please explain :)

Also, based on the changes in this series, I fail to see how you could
even claim copyright on the entire file.  Please explain that as well
when you resend this patch.

Note, I'm not a lawyer, and this isn't legal advice, please contact your
company's lawyers, they know this type of thing, and now the normal
rules for claiming copyright of a whole file.  They also know that
copyright notices in files don't mean anything, but lawyers love to
cargo-cult things more than any other profession :)

thanks,

greg k-h

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH wrote:
> On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/usb/misc/usb251xb.c | 1 +
> >  1 file changed, 1 insertions(+), 0 deletion(-)
> 
> I can't take patches without any changelog text, sorry.
> 
> > 
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 1d47acd09..43afa04ad 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> I don't see the connection between a random company, and a random
> gmail.com email address.  Please explain :)

Also, based on the changes in this series, I fail to see how you could
even claim copyright on the entire file.  Please explain that as well
when you resend this patch.

Note, I'm not a lawyer, and this isn't legal advice, please contact your
company's lawyers, they know this type of thing, and now the normal
rules for claiming copyright of a whole file.  They also know that
copyright notices in files don't mean anything, but lawyers love to
cargo-cult things more than any other profession :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Serge Semin]

On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  drivers/usb/misc/usb251xb.c | 1 +
> >  1 file changed, 1 insertions(+), 0 deletion(-)
> 
> I can't take patches without any changelog text, sorry.
> 

I'll merge it into the previous patch then. There is nothing to changeloging here. 

> > 
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 1d47acd09..43afa04ad 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> I don't see the connection between a random company, and a random
> gmail.com email address.  Please explain :)

Why do you see gmail.com only? I cc-ed this patchset to my work email:
Sergey.Semin@t-platforms.ru
Do I really need to send it from my work e-mail? I have met such rule before.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Serge Semin]

On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/usb/misc/usb251xb.c | 1 +
> >  1 file changed, 1 insertions(+), 0 deletion(-)
> 
> I can't take patches without any changelog text, sorry.
> 

I'll merge it into the previous patch then. There is nothing to changeloging here. 

> > 
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 1d47acd09..43afa04ad 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> I don't see the connection between a random company, and a random
> gmail.com email address.  Please explain :)

Why do you see gmail.com only? I cc-ed this patchset to my work email:
Sergey.Semin-vHJ8rsvMqnUPfZBKTuL5GA@public.gmane.org
Do I really need to send it from my work e-mail? I have met such rule before.

> 
> thanks,
> 
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Sat, Sep 16, 2017 at 02:45:17AM +0300, Serge Semin wrote:
> On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > ---
> > >  drivers/usb/misc/usb251xb.c | 1 +
> > >  1 file changed, 1 insertions(+), 0 deletion(-)
> > 
> > I can't take patches without any changelog text, sorry.
> > 
> 
> I'll merge it into the previous patch then. There is nothing to changeloging here. 
> 
> > > 
> > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > index 1d47acd09..43afa04ad 100644
> > > --- a/drivers/usb/misc/usb251xb.c
> > > +++ b/drivers/usb/misc/usb251xb.c
> > > @@ -3,6 +3,7 @@
> > >   * Configuration via SMBus.
> > >   *
> > >   * Copyright (c) 2017 SKIDATA AG
> > > + * Copyright (c) 2017 T-platforms
> > 
> > I don't see the connection between a random company, and a random
> > gmail.com email address.  Please explain :)
> 
> Why do you see gmail.com only? I cc-ed this patchset to my work email:
> Sergey.Semin@t-platforms.ru
> Do I really need to send it from my work e-mail? I have met such rule before.

The authorship of these patches are coming from a gmail.com address, how
do I know what that relates to some random email you cc: on the patch :)

thanks,

greg k-h

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Greg KH]

On Sat, Sep 16, 2017 at 02:45:17AM +0300, Serge Semin wrote:
> On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/usb/misc/usb251xb.c | 1 +
> > >  1 file changed, 1 insertions(+), 0 deletion(-)
> > 
> > I can't take patches without any changelog text, sorry.
> > 
> 
> I'll merge it into the previous patch then. There is nothing to changeloging here. 
> 
> > > 
> > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > index 1d47acd09..43afa04ad 100644
> > > --- a/drivers/usb/misc/usb251xb.c
> > > +++ b/drivers/usb/misc/usb251xb.c
> > > @@ -3,6 +3,7 @@
> > >   * Configuration via SMBus.
> > >   *
> > >   * Copyright (c) 2017 SKIDATA AG
> > > + * Copyright (c) 2017 T-platforms
> > 
> > I don't see the connection between a random company, and a random
> > gmail.com email address.  Please explain :)
> 
> Why do you see gmail.com only? I cc-ed this patchset to my work email:
> Sergey.Semin-vHJ8rsvMqnUPfZBKTuL5GA@public.gmane.org
> Do I really need to send it from my work e-mail? I have met such rule before.

The authorship of these patches are coming from a gmail.com address, how
do I know what that relates to some random email you cc: on the patch :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] usb: usb251xb: Add copyrights

[Serge Semin]

On Fri, Sep 15, 2017 at 04:53:13PM -0700, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 16, 2017 at 02:45:17AM +0300, Serge Semin wrote:
> > On Fri, Sep 15, 2017 at 04:40:28PM -0700, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Sep 16, 2017 at 02:31:13AM +0300, Serge Semin wrote:
> > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > > > ---
> > > >  drivers/usb/misc/usb251xb.c | 1 +
> > > >  1 file changed, 1 insertions(+), 0 deletion(-)
> > > 
> > > I can't take patches without any changelog text, sorry.
> > > 
> > 
> > I'll merge it into the previous patch then. There is nothing to changeloging here. 
> > 
> > > > 
> > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > > index 1d47acd09..43afa04ad 100644
> > > > --- a/drivers/usb/misc/usb251xb.c
> > > > +++ b/drivers/usb/misc/usb251xb.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Configuration via SMBus.
> > > >   *
> > > >   * Copyright (c) 2017 SKIDATA AG
> > > > + * Copyright (c) 2017 T-platforms
> > > 
> > > I don't see the connection between a random company, and a random
> > > gmail.com email address.  Please explain :)
> > 
> > Why do you see gmail.com only? I cc-ed this patchset to my work email:
> > Sergey.Semin@t-platforms.ru
> > Do I really need to send it from my work e-mail? I have met such rule before.
> 
> The authorship of these patches are coming from a gmail.com address, how
> do I know what that relates to some random email you cc: on the patch :)
> 
> thanks,
> 
> greg k-h

Does it really matter? I could create any domain and add some random company
as copyright. How would you know the copyrighted company is real then? The
only way would be just google it. That's it. But still the site can be faked
if it is really necessary.
I thought these things are a matter of trust. I submitted a patchset from my
personal e-mail, but created it while working for the company. It's highly
possible, that my personal email is going to be mine for much longer time, then
I'll be working in the company. In this case, the better way to declare it as
primary email, instead of the company one, so to have a longer support. Isn't it?
Why would I need to fake a copyrighted company anyway?

-Sergey

[PATCH 0/9 v2] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to make the following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Changelog v2:
- Split first patch into smaller ones
- Fix invalid BOOST_14 register definition
- Combine copyrights adding patch into the last one

Serge Semin (9):
  usb: usb251xb: Add USB2517i specific struct and IDs
  usb: usb251xb: Add USB251x specific port count setting
  usb: usb251xb: Add 5,6,7 ports mapping def setting
  usb: usb251xb: Add 5,6,7 ports boost settings
  usb: usb251xb: Add battery enable setting flag
  usb: usb251xb: Add USB2517 LED settings
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts property support
  usb: usb251xb: Use GPIO descriptor consumer interface

 Documentation/devicetree/bindings/usb/usb251xb.txt |  12 +-
 drivers/usb/misc/usb251xb.c                        | 160 +++++++++++++++------
 2 files changed, 128 insertions(+), 44 deletions(-)

-- 
2.12.0

[PATCH 0/9 v2] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to make the following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Changelog v2:
- Split first patch into smaller ones
- Fix invalid BOOST_14 register definition
- Combine copyrights adding patch into the last one

Serge Semin (9):
  usb: usb251xb: Add USB2517i specific struct and IDs
  usb: usb251xb: Add USB251x specific port count setting
  usb: usb251xb: Add 5,6,7 ports mapping def setting
  usb: usb251xb: Add 5,6,7 ports boost settings
  usb: usb251xb: Add battery enable setting flag
  usb: usb251xb: Add USB2517 LED settings
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts property support
  usb: usb251xb: Use GPIO descriptor consumer interface

 Documentation/devicetree/bindings/usb/usb251xb.txt |  12 +-
 drivers/usb/misc/usb251xb.c                        | 160 +++++++++++++++------
 2 files changed, 128 insertions(+), 44 deletions(-)

-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/9 v2] usb: usb251xb: Add USB2517i specific struct and IDs

[Serge Semin]

There are USB2517 and USB2517i hubs, which have almost the same
registers space as already supported USB251xbi series. The difference
it in DIDs and in few functions. This patch adds the USB2517/i data
structures to the driver, so it would have different setting depending
on the device discovered on i2c-bus.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt  |  3 ++-
 drivers/usb/misc/usb251xb.c                         | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3957d4eda..1682d4087 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -6,7 +6,8 @@ Hi-Speed Controller.
 Required properties :
  - compatible : Should be "microchip,usb251xb" or one of the specific types:
 	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
-	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
+	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
+	"microchip,usb2517", "microchip,usb2517i"
  - reset-gpios : Should specify the gpio for hub reset
  - reg : I2C address on the selected bus (default is <0x2C>)
 
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 91f66d68b..96a8c20ac 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -38,6 +38,7 @@
 #define USB251XB_DEF_PRODUCT_ID_12	0x2512 /* USB2512B/12Bi */
 #define USB251XB_DEF_PRODUCT_ID_13	0x2513 /* USB2513B/13Bi */
 #define USB251XB_DEF_PRODUCT_ID_14	0x2514 /* USB2514B/14Bi */
+#define USB251XB_DEF_PRODUCT_ID_17	0x2517 /* USB2517i */
 
 #define USB251XB_ADDR_DEVICE_ID_LSB	0x04
 #define USB251XB_ADDR_DEVICE_ID_MSB	0x05
@@ -82,7 +83,7 @@
 
 #define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
 #define USB251XB_ADDR_PRODUCT_STRING		0x54
-#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
+#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi/7i"
 
 #define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
 #define USB251XB_ADDR_SERIAL_STRING		0x92
@@ -186,6 +187,16 @@ static const struct usb251xb_data usb2514bi_data = {
 	.product_str = "USB2514Bi",
 };
 
+static const struct usb251xb_data usb2517_data = {
+	.product_id = 0x2517,
+	.product_str = "USB2517",
+};
+
+static const struct usb251xb_data usb2517i_data = {
+	.product_id = 0x2517,
+	.product_str = "USB2517i",
+};
+
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
 	if (!gpio_is_valid(hub->gpio_reset))
@@ -511,6 +522,12 @@ static const struct of_device_id usb251xb_of_match[] = {
 		.compatible = "microchip,usb2514bi",
 		.data = &usb2514bi_data,
 	}, {
+		.compatible = "microchip,usb2517",
+		.data = &usb2517_data,
+	}, {
+		.compatible = "microchip,usb2517i",
+		.data = &usb2517i_data,
+	}, {
 		/* sentinel */
 	}
 };
@@ -574,6 +591,8 @@ static const struct i2c_device_id usb251xb_id[] = {
 	{ "usb2513bi", 0 },
 	{ "usb2514b", 0 },
 	{ "usb2514bi", 0 },
+	{ "usb2517", 0 },
+	{ "usb2517i", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, usb251xb_id);
-- 
2.12.0

Re: [PATCH 1/9 v2] usb: usb251xb: Add USB2517i specific struct and IDs

[Richard Leitner]

Hi Serge,

On 09/16/2017 12:42 PM, Serge Semin wrote:
> There are USB2517 and USB2517i hubs, which have almost the same
> registers space as already supported USB251xbi series. The difference
> it in DIDs and in few functions. This patch adds the USB2517/i data
> structures to the driver, so it would have different setting depending
> on the device discovered on i2c-bus.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt  |  3 ++-
>  drivers/usb/misc/usb251xb.c                         | 21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)

...

Please also mention support for the USB2517(i) hubs in the Kconfig
helptext of the driver @ drivers/usb/misc/Kconfig. Thanks.

Apart from that the patch looks good to me.

regards,
Richard.L

Re: [PATCH 1/9 v2] usb: usb251xb: Add USB2517i specific struct and IDs

[Richard Leitner]

Hi Serge,

On 09/16/2017 12:42 PM, Serge Semin wrote:
> There are USB2517 and USB2517i hubs, which have almost the same
> registers space as already supported USB251xbi series. The difference
> it in DIDs and in few functions. This patch adds the USB2517/i data
> structures to the driver, so it would have different setting depending
> on the device discovered on i2c-bus.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt  |  3 ++-
>  drivers/usb/misc/usb251xb.c                         | 21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)

...

Please also mention support for the USB2517(i) hubs in the Kconfig
helptext of the driver @ drivers/usb/misc/Kconfig. Thanks.

Apart from that the patch looks good to me.

regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/9 v2] usb: usb251xb: Add USB251x specific port count setting

[Serge Semin]

USB251xb as well as USB2517 datasheet states, that all these
hubs differ by number of ports declared as the last digit in the
model name. So USB2512 got two ports, USB2513 - three, and so on.
Such setting must be reflected in the device specific data
structure and corresponding dts property should be checked whether
it doesn't get out of available ports.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 96a8c20ac..5cb0e5570 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -154,46 +154,55 @@ struct usb251xb {
 
 struct usb251xb_data {
 	u16 product_id;
+	u8 port_cnt;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514Bi",
 };
 
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517",
 };
 
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517i",
 };
 
@@ -422,8 +431,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->non_rem_dev |= BIT(port);
+			else
+				dev_warn(dev, "port %u doesn't exist\n", port);
 		}
 	}
 
@@ -433,8 +444,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_sp |= BIT(port);
+			else
+				dev_warn(dev, "port %u doesn't exist\n", port);
 		}
 	}
 
@@ -444,8 +457,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_bp |= BIT(port);
+			else
+				dev_warn(dev, "port %u doesn't exist\n", port);
 		}
 	}
 
-- 
2.12.0

Re: [PATCH 2/9 v2] usb: usb251xb: Add USB251x specific port count setting

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB251xb as well as USB2517 datasheet states, that all these
> hubs differ by number of ports declared as the last digit in the
> model name. So USB2512 got two ports, USB2513 - three, and so on.
> Such setting must be reflected in the device specific data
> structure and corresponding dts property should be checked whether
> it doesn't get out of available ports.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/misc/usb251xb.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 96a8c20ac..5cb0e5570 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c

...

>  
> @@ -422,8 +431,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->non_rem_dev |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

I'd prefer to add the (invalid) property trying to be set in this
warning messages. So someone is able to find the invalid dt setting
faster. Something like;

	dev_warn(dev, "requested NRD port %u doesn't exist\n", port);

>  		}
>  	}
>  
> @@ -433,8 +444,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->port_disable_sp |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

... same as above. For example:

	dev_warn(dev, "requested PDS port %u doesn't exist\n", port);

>  		}
>  	}
>  
> @@ -444,8 +457,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->port_disable_bp |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

... same as above. For example:

	dev_warn(dev, "requested PDB port %u doesn't exist\n", port);


Apart from that this patch looks fine for me. Thanks for the spot of the
hardcoded max ports check.

regards,
Richard.L

Re: [PATCH 2/9 v2] usb: usb251xb: Add USB251x specific port count setting

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB251xb as well as USB2517 datasheet states, that all these
> hubs differ by number of ports declared as the last digit in the
> model name. So USB2512 got two ports, USB2513 - three, and so on.
> Such setting must be reflected in the device specific data
> structure and corresponding dts property should be checked whether
> it doesn't get out of available ports.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/misc/usb251xb.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 96a8c20ac..5cb0e5570 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c

...

>  
> @@ -422,8 +431,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->non_rem_dev |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

I'd prefer to add the (invalid) property trying to be set in this
warning messages. So someone is able to find the invalid dt setting
faster. Something like;

	dev_warn(dev, "requested NRD port %u doesn't exist\n", port);

>  		}
>  	}
>  
> @@ -433,8 +444,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->port_disable_sp |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

... same as above. For example:

	dev_warn(dev, "requested PDS port %u doesn't exist\n", port);

>  		}
>  	}
>  
> @@ -444,8 +457,10 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>  		for (i = 0; i < len / sizeof(u32); i++) {
>  			u32 port = be32_to_cpu(cproperty_u32[i]);
>  
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>  				hub->port_disable_bp |= BIT(port);
> +			else
> +				dev_warn(dev, "port %u doesn't exist\n", port);

... same as above. For example:

	dev_warn(dev, "requested PDB port %u doesn't exist\n", port);


Apart from that this patch looks fine for me. Thanks for the spot of the
hardcoded max ports check.

regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/9 v2] usb: usb251xb: Add 5,6,7 ports mapping def setting

[Serge Semin]

USB2517 got three additionl downstream ports, which can
as well be mapped to another logical ports. USB2551xb driver
currently doesn't fully support such setting configuration
from dts file. This patch doesn't change this, but adds
usb2517 spcific ports default liner mapping.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 5cb0e5570..3de0de93b 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -103,7 +103,11 @@
 #define USB251XB_ADDR_PORT_MAP_12	0xFB
 #define USB251XB_DEF_PORT_MAP_12	0x00
 #define USB251XB_ADDR_PORT_MAP_34	0xFC
-#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB2513B/i & USB2514B/i only */
+#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB251{3B/i,4B/i,7/i} only */
+#define USB251XB_ADDR_PORT_MAP_56	0xFD
+#define USB251XB_DEF_PORT_MAP_56	0x00 /* USB2517/i only */
+#define USB251XB_ADDR_PORT_MAP_7	0xFE
+#define USB251XB_DEF_PORT_MAP_7		0x00 /* USB2517/i only */
 
 #define USB251XB_ADDR_STATUS_COMMAND		0xFF
 #define USB251XB_STATUS_COMMAND_SMBUS_DOWN	0x04
@@ -149,6 +153,8 @@ struct usb251xb {
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
+	u8  port_map56;
+	u8  port_map7;
 	u8  status;
 };
 
@@ -278,6 +284,8 @@ static int usb251xb_connect(struct usb251xb *hub)
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_56]       = hub->port_map56;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_7]        = hub->port_map7;
 	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
 
 	usb251xb_reset(hub, 1);
@@ -513,6 +521,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
+	hub->port_map56 = USB251XB_DEF_PORT_MAP_56;
+	hub->port_map7  = USB251XB_DEF_PORT_MAP_7;
 
 	return 0;
 }
-- 
2.12.0

Re: [PATCH 3/9 v2] usb: usb251xb: Add 5,6,7 ports mapping def setting

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB2517 got three additionl downstream ports, which can
> as well be mapped to another logical ports. USB2551xb driver

typo in "USB251xb"

> currently doesn't fully support such setting configuration
> from dts file. This patch doesn't change this, but adds
> usb2517 spcific ports default liner mapping.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/misc/usb251xb.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

Code itself looks good to me, therefore feel free to add:

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

regards,
Richard.L

Re: [PATCH 3/9 v2] usb: usb251xb: Add 5,6,7 ports mapping def setting

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB2517 got three additionl downstream ports, which can
> as well be mapped to another logical ports. USB2551xb driver

typo in "USB251xb"

> currently doesn't fully support such setting configuration
> from dts file. This patch doesn't change this, but adds
> usb2517 spcific ports default liner mapping.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/misc/usb251xb.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

Code itself looks good to me, therefore feel free to add:

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

regards,
Richard.L

[PATCH 4/9 v2] usb: usb251xb: Add 5,6,7 ports boost settings

[Serge Semin]

USB electrical signaling drive strength boost bit is also supported
by USB2517 hub. Since it got three addition ports, the designers
needed to add one more register for initialization. It turned out
to be formerly reserved 0xF7. As before we just initialize it with
default zeros.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 3de0de93b..44fa7d084 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -94,8 +94,10 @@
 
 #define USB251XB_ADDR_BOOST_UP	0xF6
 #define USB251XB_DEF_BOOST_UP	0x00
-#define USB251XB_ADDR_BOOST_X	0xF8
-#define USB251XB_DEF_BOOST_X	0x00
+#define USB251XB_ADDR_BOOST_57	0xF7
+#define USB251XB_DEF_BOOST_57	0x00
+#define USB251XB_ADDR_BOOST_14	0xF8
+#define USB251XB_DEF_BOOST_14	0x00
 
 #define USB251XB_ADDR_PORT_SWAP	0xFA
 #define USB251XB_DEF_PORT_SWAP	0x00
@@ -149,7 +151,8 @@ struct usb251xb {
 	char serial[USB251XB_STRING_BUFSIZE];
 	u8  bat_charge_en;
 	u8  boost_up;
-	u8  boost_x;
+	u8  boost_57;
+	u8  boost_14;
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
@@ -280,7 +283,8 @@ static int usb251xb_connect(struct usb251xb *hub)
 	       USB251XB_STRING_BUFSIZE);
 	i2c_wb[USB251XB_ADDR_BATTERY_CHARGING_ENABLE] = hub->bat_charge_en;
 	i2c_wb[USB251XB_ADDR_BOOST_UP]          = hub->boost_up;
-	i2c_wb[USB251XB_ADDR_BOOST_X]           = hub->boost_x;
+	i2c_wb[USB251XB_ADDR_BOOST_57]          = hub->boost_57;
+	i2c_wb[USB251XB_ADDR_BOOST_14]          = hub->boost_14;
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
@@ -517,7 +521,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
-	hub->boost_x = USB251XB_DEF_BOOST_X;
+	hub->boost_57 = USB251XB_DEF_BOOST_57;
+	hub->boost_14 = USB251XB_DEF_BOOST_14;
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
-- 
2.12.0

[PATCH 4/9 v2] usb: usb251xb: Add 5,6,7 ports boost settings

[Serge Semin]

USB electrical signaling drive strength boost bit is also supported
by USB2517 hub. Since it got three addition ports, the designers
needed to add one more register for initialization. It turned out
to be formerly reserved 0xF7. As before we just initialize it with
default zeros.

Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/misc/usb251xb.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 3de0de93b..44fa7d084 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -94,8 +94,10 @@
 
 #define USB251XB_ADDR_BOOST_UP	0xF6
 #define USB251XB_DEF_BOOST_UP	0x00
-#define USB251XB_ADDR_BOOST_X	0xF8
-#define USB251XB_DEF_BOOST_X	0x00
+#define USB251XB_ADDR_BOOST_57	0xF7
+#define USB251XB_DEF_BOOST_57	0x00
+#define USB251XB_ADDR_BOOST_14	0xF8
+#define USB251XB_DEF_BOOST_14	0x00
 
 #define USB251XB_ADDR_PORT_SWAP	0xFA
 #define USB251XB_DEF_PORT_SWAP	0x00
@@ -149,7 +151,8 @@ struct usb251xb {
 	char serial[USB251XB_STRING_BUFSIZE];
 	u8  bat_charge_en;
 	u8  boost_up;
-	u8  boost_x;
+	u8  boost_57;
+	u8  boost_14;
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
@@ -280,7 +283,8 @@ static int usb251xb_connect(struct usb251xb *hub)
 	       USB251XB_STRING_BUFSIZE);
 	i2c_wb[USB251XB_ADDR_BATTERY_CHARGING_ENABLE] = hub->bat_charge_en;
 	i2c_wb[USB251XB_ADDR_BOOST_UP]          = hub->boost_up;
-	i2c_wb[USB251XB_ADDR_BOOST_X]           = hub->boost_x;
+	i2c_wb[USB251XB_ADDR_BOOST_57]          = hub->boost_57;
+	i2c_wb[USB251XB_ADDR_BOOST_14]          = hub->boost_14;
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
@@ -517,7 +521,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
-	hub->boost_x = USB251XB_DEF_BOOST_X;
+	hub->boost_57 = USB251XB_DEF_BOOST_57;
+	hub->boost_14 = USB251XB_DEF_BOOST_14;
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9 v2] usb: usb251xb: Add 5,6,7 ports boost settings

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB electrical signaling drive strength boost bit is also supported
> by USB2517 hub. Since it got three addition ports, the designers
> needed to add one more register for initialization. It turned out
> to be formerly reserved 0xF7. As before we just initialize it with
> default zeros.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/usb/misc/usb251xb.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Looks good to me. Feel free to add:

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

regards,
Richard.L

Re: [PATCH 4/9 v2] usb: usb251xb: Add 5,6,7 ports boost settings

[Richard Leitner]

On 09/16/2017 12:42 PM, Serge Semin wrote:
> USB electrical signaling drive strength boost bit is also supported
> by USB2517 hub. Since it got three addition ports, the designers
> needed to add one more register for initialization. It turned out
> to be formerly reserved 0xF7. As before we just initialize it with
> default zeros.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/misc/usb251xb.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Looks good to me. Feel free to add:

Acked-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>

regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/9 v2] usb: usb251xb: Add battery enable setting flag

[Serge Semin]

Battery charging settings are supported by USB251xb hubs only.
USB2517i isn't one of them. So we need to reflect it within the
device-specific data structure. The driver doesn't support dts
property to change this setting, but instead defaults it with zero.
So the flag isn't used anywhere in the driver, but still can be helpful
in future, when necessity of corresponding dts setting arises.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 44fa7d084..0834729d1 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -164,54 +164,63 @@ struct usb251xb {
 struct usb251xb_data {
 	u16 product_id;
 	u8 port_cnt;
+	bool bat_support;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.bat_support = true,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.bat_support = true,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.bat_support = true,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.bat_support = true,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.bat_support = true,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.bat_support = true,
 	.product_str = "USB2514Bi",
 };
 
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.bat_support = false,
 	.product_str = "USB2517",
 };
 
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.bat_support = false,
 	.product_str = "USB2517i",
 };
 
-- 
2.12.0

[PATCH 6/9 v2] usb: usb251xb: Add USB2517 LED settings

[Serge Semin]

USB2517 supports two LED modes: USB mode (default) and speed indication
mode. The last one can be switched on by corresponding dts property.
Since USB251xb hubs doesn't support LEDs settings, we need to ignore
this setting.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt         |  1 +
 drivers/usb/misc/usb251xb.c                                | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 1682d4087..3d84626d3 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -37,6 +37,7 @@ Optional properties :
 	an invalid value is given, the default is used instead.
  - compound-device : indicate the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
+ - speed-led-mode : led speed indiation mode selection (usb2517 only)
  - string-support : enable string descriptor support (required for manufacturer,
 	product and serial string configuration)
  - non-removable-ports : Should specify the ports which have a non-removable
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 0834729d1..51cc53ddc 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -49,7 +49,7 @@
 #define USB251XB_ADDR_CONFIG_DATA_2	0x07
 #define USB251XB_DEF_CONFIG_DATA_2	0x20
 #define USB251XB_ADDR_CONFIG_DATA_3	0x08
-#define USB251XB_DEF_CONFIG_DATA_3	0x02
+#define USB251XB_DEF_CONFIG_DATA_3	0x00
 
 #define USB251XB_ADDR_NON_REMOVABLE_DEVICES	0x09
 #define USB251XB_DEF_NON_REMOVABLE_DEVICES	0x00
@@ -164,6 +164,7 @@ struct usb251xb {
 struct usb251xb_data {
 	u16 product_id;
 	u8 port_cnt;
+	bool led_support;
 	bool bat_support;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
@@ -171,6 +172,7 @@ struct usb251xb_data {
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2512B",
 };
@@ -178,6 +180,7 @@ static const struct usb251xb_data usb2512b_data = {
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2512Bi",
 };
@@ -185,6 +188,7 @@ static const struct usb251xb_data usb2512bi_data = {
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2513B",
 };
@@ -192,6 +196,7 @@ static const struct usb251xb_data usb2513b_data = {
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2513Bi",
 };
@@ -199,6 +204,7 @@ static const struct usb251xb_data usb2513bi_data = {
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2514B",
 };
@@ -206,6 +212,7 @@ static const struct usb251xb_data usb2514b_data = {
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2514Bi",
 };
@@ -213,6 +220,7 @@ static const struct usb251xb_data usb2514bi_data = {
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.led_support = true,
 	.bat_support = false,
 	.product_str = "USB2517",
 };
@@ -220,6 +228,7 @@ static const struct usb251xb_data usb2517_data = {
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.led_support = true,
 	.bat_support = false,
 	.product_str = "USB2517i",
 };
@@ -443,6 +452,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "port-mapping-mode", NULL))
 		hub->conf_data3 |= BIT(3);
 
+	if (data->led_support && of_get_property(np, "speed-led-mode", NULL))
+		hub->conf_data3 |= BIT(1);
+
 	if (of_get_property(np, "string-support", NULL))
 		hub->conf_data3 |= BIT(0);
 
-- 
2.12.0

[PATCH 7/9 v2] usb: usb251xb: Fix property_u32 NULL pointer dereference

[Serge Semin]

The methods like of_property_read_u32 utilizing the specified
pointer permit only the pointer to a preallocated u32 storage as the
third argument. As a result the driver crashes on NULL pointer
dereference in case if "oc-delay-us" or "power-on-time-ms" declared
in dts file.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 51cc53ddc..c308b0006 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -348,7 +348,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	struct device *dev = hub->dev;
 	struct device_node *np = dev->of_node;
 	int len, err, i;
-	u32 *property_u32 = NULL;
+	u32 property_u32 = 0;
 	const u32 *cproperty_u32;
 	const char *cproperty_char;
 	char str[USB251XB_STRING_BUFSIZE / 2];
@@ -425,16 +425,16 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "dynamic-power-switching", NULL))
 		hub->conf_data2 |= BIT(7);
 
-	if (!of_property_read_u32(np, "oc-delay-us", property_u32)) {
-		if (*property_u32 == 100) {
+	if (!of_property_read_u32(np, "oc-delay-us", &property_u32)) {
+		if (property_u32 == 100) {
 			/* 100 us*/
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 &= ~BIT(4);
-		} else if (*property_u32 == 4000) {
+		} else if (property_u32 == 4000) {
 			/* 4 ms */
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 |= BIT(4);
-		} else if (*property_u32 == 16000) {
+		} else if (property_u32 == 16000) {
 			/* 16 ms */
 			hub->conf_data2 |= BIT(5);
 			hub->conf_data2 |= BIT(4);
@@ -498,8 +498,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	}
 
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
-	if (!of_property_read_u32(np, "power-on-time-ms", property_u32))
-		hub->power_on_time = min_t(u8, *property_u32 / 2, 255);
+	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
+		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
 
 	if (of_property_read_u16_array(np, "language-id", &hub->lang_id, 1))
 		hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
-- 
2.12.0

[PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

This parameters may be varied in accordance with hardware specifics.
So lets add the corresponding settings to the usb251x driver dts
specification.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
 drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3d84626d3..dd59a32e7 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -44,6 +44,12 @@ Optional properties :
 	device connected.
  - sp-disabled-ports : Specifies the ports which will be self-power disabled
  - bp-disabled-ports : Specifies the ports which will be bus-power disabled
+ - sp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a self-powered hub. The value
+	is given in mA in a 0 - 100 range (default is 1mA).
+ - bp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a bus-powered hub. The value
+	is given in mA in a 0 - 510 range (default is 100mA).
  - power-on-time-ms : Specifies the time it takes from the time the host
 	initiates the power-on sequence to a port until the port has adequate
 	power. The value is given in ms in a 0 - 510 range (default is 100ms).
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index c308b0006..71994b883 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -497,6 +497,22 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		}
 	}
 
+	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
+	if (!of_property_read_u32(np, "sp-max-power", &property_u32))
+		hub->max_power_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
+	if (!of_property_read_u32(np, "bp-max-power", &property_u32))
+		hub->max_power_bp = min_t(u8, property_u32 / 2, 255);
+
+	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
+	if (!of_property_read_u32(np, "sp-max-current", &property_u32))
+		hub->max_current_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
+	if (!of_property_read_u32(np, "bp-max-current", &property_u32))
+		hub->max_current_bp = min_t(u8, property_u32 / 2, 255);
+
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
 	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
 		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
@@ -536,10 +552,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
 	 */
-	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
 	hub->boost_57 = USB251XB_DEF_BOOST_57;
-- 
2.12.0

[PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

This parameters may be varied in accordance with hardware specifics.
So lets add the corresponding settings to the usb251x driver dts
specification.

Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
 drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3d84626d3..dd59a32e7 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -44,6 +44,12 @@ Optional properties :
 	device connected.
  - sp-disabled-ports : Specifies the ports which will be self-power disabled
  - bp-disabled-ports : Specifies the ports which will be bus-power disabled
+ - sp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a self-powered hub. The value
+	is given in mA in a 0 - 100 range (default is 1mA).
+ - bp-max-{power,current} : Indicates the power/current consumed by hub from
+	an upstream port (VBUS) when operation as a bus-powered hub. The value
+	is given in mA in a 0 - 510 range (default is 100mA).
  - power-on-time-ms : Specifies the time it takes from the time the host
 	initiates the power-on sequence to a port until the port has adequate
 	power. The value is given in ms in a 0 - 510 range (default is 100ms).
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index c308b0006..71994b883 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -497,6 +497,22 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		}
 	}
 
+	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
+	if (!of_property_read_u32(np, "sp-max-power", &property_u32))
+		hub->max_power_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
+	if (!of_property_read_u32(np, "bp-max-power", &property_u32))
+		hub->max_power_bp = min_t(u8, property_u32 / 2, 255);
+
+	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
+	if (!of_property_read_u32(np, "sp-max-current", &property_u32))
+		hub->max_current_sp = min_t(u8, property_u32 / 2, 50);
+
+	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
+	if (!of_property_read_u32(np, "bp-max-current", &property_u32))
+		hub->max_current_bp = min_t(u8, property_u32 / 2, 255);
+
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
 	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
 		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
@@ -536,10 +552,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
 	 */
-	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
 	hub->boost_57 = USB251XB_DEF_BOOST_57;
-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Rob Herring]

On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
> This parameters may be varied in accordance with hardware specifics.
> So lets add the corresponding settings to the usb251x driver dts
> specification.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
>  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> index 3d84626d3..dd59a32e7 100644
> --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> @@ -44,6 +44,12 @@ Optional properties :
>  	device connected.
>   - sp-disabled-ports : Specifies the ports which will be self-power disabled
>   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
> + - sp-max-{power,current} : Indicates the power/current consumed by hub from
> +	an upstream port (VBUS) when operation as a self-powered hub. The value
> +	is given in mA in a 0 - 100 range (default is 1mA).
> + - bp-max-{power,current} : Indicates the power/current consumed by hub from
> +	an upstream port (VBUS) when operation as a bus-powered hub. The value
> +	is given in mA in a 0 - 510 range (default is 100mA).

These need units as defined in property-units.txt.

Why do you need power and current? Can't you calculate power?

>   - power-on-time-ms : Specifies the time it takes from the time the host
>  	initiates the power-on sequence to a port until the port has adequate
>  	power. The value is given in ms in a 0 - 510 range (default is 100ms).

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote:
> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
> > This parameters may be varied in accordance with hardware specifics.
> > So lets add the corresponding settings to the usb251x driver dts
> > specification.
> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
> >  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> > index 3d84626d3..dd59a32e7 100644
> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> > @@ -44,6 +44,12 @@ Optional properties :
> >  	device connected.
> >   - sp-disabled-ports : Specifies the ports which will be self-power disabled
> >   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from
> > +	an upstream port (VBUS) when operation as a self-powered hub. The value
> > +	is given in mA in a 0 - 100 range (default is 1mA).
> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from
> > +	an upstream port (VBUS) when operation as a bus-powered hub. The value
> > +	is given in mA in a 0 - 510 range (default is 100mA).
> 
> These need units as defined in property-units.txt.
> 

Ok.

> Why do you need power and current? Can't you calculate power?
> 

These are different parameters of the device. They got different configuration
registers and descriptions:
max_power* - ... This value also includes the power consumption of a
permanently attached peripheral if the hub is configured as a compound
device, and the embedded peripheral reports 0mA in its descriptors.
max_current* - ... This value does NOT include the power consumption of a
permanently attached peripheral if the hub is configured as a compound
device.

Additionally as you can see, they both are measured in "mA", so it isn't
a real physical power.

> >   - power-on-time-ms : Specifies the time it takes from the time the host
> >  	initiates the power-on sequence to a port until the port has adequate
> >  	power. The value is given in ms in a 0 - 510 range (default is 100ms).

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Rob Herring]

On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote:
>> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
>> > This parameters may be varied in accordance with hardware specifics.
>> > So lets add the corresponding settings to the usb251x driver dts
>> > specification.
>> >
>> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
>> > ---
>> >  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
>> >  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
>> >  2 files changed, 22 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > index 3d84626d3..dd59a32e7 100644
>> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > @@ -44,6 +44,12 @@ Optional properties :
>> >     device connected.
>> >   - sp-disabled-ports : Specifies the ports which will be self-power disabled
>> >   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
>> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from
>> > +   an upstream port (VBUS) when operation as a self-powered hub. The value
>> > +   is given in mA in a 0 - 100 range (default is 1mA).
>> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from
>> > +   an upstream port (VBUS) when operation as a bus-powered hub. The value
>> > +   is given in mA in a 0 - 510 range (default is 100mA).
>>
>> These need units as defined in property-units.txt.
>>
>
> Ok.
>
>> Why do you need power and current? Can't you calculate power?
>>
>
> These are different parameters of the device. They got different configuration
> registers and descriptions:
> max_power* - ... This value also includes the power consumption of a
> permanently attached peripheral if the hub is configured as a compound
> device, and the embedded peripheral reports 0mA in its descriptors.
> max_current* - ... This value does NOT include the power consumption of a
> permanently attached peripheral if the hub is configured as a compound
> device.

Then the names here should somehow reflect the above. Perhaps
"composite-current" and "hub-current" or something like that.

>
> Additionally as you can see, they both are measured in "mA", so it isn't
> a real physical power.

Well, I can't because there's no units.

Rob

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Rob Herring]

On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
>> > This parameters may be varied in accordance with hardware specifics.
>> > So lets add the corresponding settings to the usb251x driver dts
>> > specification.
>> >
>> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> >  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
>> >  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
>> >  2 files changed, 22 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > index 3d84626d3..dd59a32e7 100644
>> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
>> > @@ -44,6 +44,12 @@ Optional properties :
>> >     device connected.
>> >   - sp-disabled-ports : Specifies the ports which will be self-power disabled
>> >   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
>> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from
>> > +   an upstream port (VBUS) when operation as a self-powered hub. The value
>> > +   is given in mA in a 0 - 100 range (default is 1mA).
>> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from
>> > +   an upstream port (VBUS) when operation as a bus-powered hub. The value
>> > +   is given in mA in a 0 - 510 range (default is 100mA).
>>
>> These need units as defined in property-units.txt.
>>
>
> Ok.
>
>> Why do you need power and current? Can't you calculate power?
>>
>
> These are different parameters of the device. They got different configuration
> registers and descriptions:
> max_power* - ... This value also includes the power consumption of a
> permanently attached peripheral if the hub is configured as a compound
> device, and the embedded peripheral reports 0mA in its descriptors.
> max_current* - ... This value does NOT include the power consumption of a
> permanently attached peripheral if the hub is configured as a compound
> device.

Then the names here should somehow reflect the above. Perhaps
"composite-current" and "hub-current" or something like that.

>
> Additionally as you can see, they both are measured in "mA", so it isn't
> a real physical power.

Well, I can't because there's no units.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote:
> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh@kernel.org> wrote:
> >> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
> >> > This parameters may be varied in accordance with hardware specifics.
> >> > So lets add the corresponding settings to the usb251x driver dts
> >> > specification.
> >> >
> >> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> >> > ---
> >> >  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
> >> >  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
> >> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > index 3d84626d3..dd59a32e7 100644
> >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > @@ -44,6 +44,12 @@ Optional properties :
> >> >     device connected.
> >> >   - sp-disabled-ports : Specifies the ports which will be self-power disabled
> >> >   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
> >> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from
> >> > +   an upstream port (VBUS) when operation as a self-powered hub. The value
> >> > +   is given in mA in a 0 - 100 range (default is 1mA).
> >> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from
> >> > +   an upstream port (VBUS) when operation as a bus-powered hub. The value
> >> > +   is given in mA in a 0 - 510 range (default is 100mA).
> >>
> >> These need units as defined in property-units.txt.
> >>
> >
> > Ok.
> >
> >> Why do you need power and current? Can't you calculate power?
> >>
> >
> > These are different parameters of the device. They got different configuration
> > registers and descriptions:
> > max_power* - ... This value also includes the power consumption of a
> > permanently attached peripheral if the hub is configured as a compound
> > device, and the embedded peripheral reports 0mA in its descriptors.
> > max_current* - ... This value does NOT include the power consumption of a
> > permanently attached peripheral if the hub is configured as a compound
> > device.
> 
> Then the names here should somehow reflect the above. Perhaps
> "composite-current" and "hub-current" or something like that.
> 

I left the naming in accordance with the device datasheet. I thought it would be
better since the driver user would still need to consult with the device
documentation to properly set them. I don't really get how the difference is reflected
with the naming declared there though. So what naming would you prefer then? Might be
something like:
{sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
permanently attached peripherals.
{sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
include the permanently attached peripherals.

Or is it better to leave it in compliance with the documentation naming?

> >
> > Additionally as you can see, they both are measured in "mA", so it isn't
> > a real physical power.
> 
> Well, I can't because there's no units.
> 

What this line means then?
- sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
- bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).

Maybe I don't know something and the description line should state the units somehow
clearer?

> Rob

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, Sep 20, 2017 at 03:52:55PM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> On Sat, Sep 16, 2017 at 01:42:19PM +0300, Serge Semin wrote:
> >> > This parameters may be varied in accordance with hardware specifics.
> >> > So lets add the corresponding settings to the usb251x driver dts
> >> > specification.
> >> >
> >> > Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> > ---
> >> >  Documentation/devicetree/bindings/usb/usb251xb.txt   |  6 ++++++
> >> >  drivers/usb/misc/usb251xb.c                          | 20 ++++++++++++++++----
> >> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > index 3d84626d3..dd59a32e7 100644
> >> > --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> >> > @@ -44,6 +44,12 @@ Optional properties :
> >> >     device connected.
> >> >   - sp-disabled-ports : Specifies the ports which will be self-power disabled
> >> >   - bp-disabled-ports : Specifies the ports which will be bus-power disabled
> >> > + - sp-max-{power,current} : Indicates the power/current consumed by hub from
> >> > +   an upstream port (VBUS) when operation as a self-powered hub. The value
> >> > +   is given in mA in a 0 - 100 range (default is 1mA).
> >> > + - bp-max-{power,current} : Indicates the power/current consumed by hub from
> >> > +   an upstream port (VBUS) when operation as a bus-powered hub. The value
> >> > +   is given in mA in a 0 - 510 range (default is 100mA).
> >>
> >> These need units as defined in property-units.txt.
> >>
> >
> > Ok.
> >
> >> Why do you need power and current? Can't you calculate power?
> >>
> >
> > These are different parameters of the device. They got different configuration
> > registers and descriptions:
> > max_power* - ... This value also includes the power consumption of a
> > permanently attached peripheral if the hub is configured as a compound
> > device, and the embedded peripheral reports 0mA in its descriptors.
> > max_current* - ... This value does NOT include the power consumption of a
> > permanently attached peripheral if the hub is configured as a compound
> > device.
> 
> Then the names here should somehow reflect the above. Perhaps
> "composite-current" and "hub-current" or something like that.
> 

I left the naming in accordance with the device datasheet. I thought it would be
better since the driver user would still need to consult with the device
documentation to properly set them. I don't really get how the difference is reflected
with the naming declared there though. So what naming would you prefer then? Might be
something like:
{sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
permanently attached peripherals.
{sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
include the permanently attached peripherals.

Or is it better to leave it in compliance with the documentation naming?

> >
> > Additionally as you can see, they both are measured in "mA", so it isn't
> > a real physical power.
> 
> Well, I can't because there's no units.
> 

What this line means then?
- sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
- bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).

Maybe I don't know something and the description line should state the units somehow
clearer?

> Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Richard Leitner]

On 09/21/2017 07:10 PM, Serge Semin wrote:
> On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote:
>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote:

...

>>> These are different parameters of the device. They got different configuration
>>> registers and descriptions:
>>> max_power* - ... This value also includes the power consumption of a
>>> permanently attached peripheral if the hub is configured as a compound
>>> device, and the embedded peripheral reports 0mA in its descriptors.
>>> max_current* - ... This value does NOT include the power consumption of a
>>> permanently attached peripheral if the hub is configured as a compound
>>> device.
>>
>> Then the names here should somehow reflect the above. Perhaps
>> "composite-current" and "hub-current" or something like that.
>>
> 
> I left the naming in accordance with the device datasheet. I thought it would be
> better since the driver user would still need to consult with the device
> documentation to properly set them. I don't really get how the difference is reflected
> with the naming declared there though. So what naming would you prefer then? Might be
> something like:
> {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
> permanently attached peripherals.
> {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
> include the permanently attached peripherals.
> 
> Or is it better to leave it in compliance with the documentation naming?

I'd prefer the naming to be in accordance with the device datasheet too.
Changing it will IMHO generate avoidable misunderstandings...

But if we should go with Robs proposal please make sure the name from
the device datasheet is mentioned somewhere in the description of the
binding.

> 
>>>
>>> Additionally as you can see, they both are measured in "mA", so it isn't
>>> a real physical power.
>>
>> Well, I can't because there's no units.
>>
> 
> What this line means then?
> - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
> - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).
> 
> Maybe I don't know something and the description line should state the units somehow
> clearer?

Append the unit to the binding name, just like in "power-on-time-ms". As
there's no "Standard Unit Suffix" for mA stated in the documentation
(Documentation/devicetree/bindings/property-units.txt) I think it should
be "-milliamp"?

regards,
Richard.L

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Richard Leitner]

On 09/21/2017 07:10 PM, Serge Semin wrote:
> On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote:
>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote:

...

>>> These are different parameters of the device. They got different configuration
>>> registers and descriptions:
>>> max_power* - ... This value also includes the power consumption of a
>>> permanently attached peripheral if the hub is configured as a compound
>>> device, and the embedded peripheral reports 0mA in its descriptors.
>>> max_current* - ... This value does NOT include the power consumption of a
>>> permanently attached peripheral if the hub is configured as a compound
>>> device.
>>
>> Then the names here should somehow reflect the above. Perhaps
>> "composite-current" and "hub-current" or something like that.
>>
> 
> I left the naming in accordance with the device datasheet. I thought it would be
> better since the driver user would still need to consult with the device
> documentation to properly set them. I don't really get how the difference is reflected
> with the naming declared there though. So what naming would you prefer then? Might be
> something like:
> {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
> permanently attached peripherals.
> {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
> include the permanently attached peripherals.
> 
> Or is it better to leave it in compliance with the documentation naming?

I'd prefer the naming to be in accordance with the device datasheet too.
Changing it will IMHO generate avoidable misunderstandings...

But if we should go with Robs proposal please make sure the name from
the device datasheet is mentioned somewhere in the description of the
binding.

> 
>>>
>>> Additionally as you can see, they both are measured in "mA", so it isn't
>>> a real physical power.
>>
>> Well, I can't because there's no units.
>>
> 
> What this line means then?
> - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
> - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).
> 
> Maybe I don't know something and the description line should state the units somehow
> clearer?

Append the unit to the binding name, just like in "power-on-time-ms". As
there's no "Standard Unit Suffix" for mA stated in the documentation
(Documentation/devicetree/bindings/property-units.txt) I think it should
be "-milliamp"?

regards,
Richard.L

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Rob Herring]

On Wed, Oct 4, 2017 at 3:12 AM, Richard Leitner
<richard.leitner@skidata.com> wrote:
>
> On 09/21/2017 07:10 PM, Serge Semin wrote:
>> On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh@kernel.org> wrote:
>>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
>
> ...
>
>>>> These are different parameters of the device. They got different configuration
>>>> registers and descriptions:
>>>> max_power* - ... This value also includes the power consumption of a
>>>> permanently attached peripheral if the hub is configured as a compound
>>>> device, and the embedded peripheral reports 0mA in its descriptors.
>>>> max_current* - ... This value does NOT include the power consumption of a
>>>> permanently attached peripheral if the hub is configured as a compound
>>>> device.
>>>
>>> Then the names here should somehow reflect the above. Perhaps
>>> "composite-current" and "hub-current" or something like that.
>>>
>>
>> I left the naming in accordance with the device datasheet. I thought it would be
>> better since the driver user would still need to consult with the device
>> documentation to properly set them. I don't really get how the difference is reflected
>> with the naming declared there though. So what naming would you prefer then? Might be
>> something like:
>> {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
>> permanently attached peripherals.
>> {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
>> include the permanently attached peripherals.
>>
>> Or is it better to leave it in compliance with the documentation naming?
>
> I'd prefer the naming to be in accordance with the device datasheet too.
> Changing it will IMHO generate avoidable misunderstandings...

Okay, then add a vendor prefix.

> But if we should go with Robs proposal please make sure the name from
> the device datasheet is mentioned somewhere in the description of the
> binding.
>
>>
>>>>
>>>> Additionally as you can see, they both are measured in "mA", so it isn't
>>>> a real physical power.
>>>
>>> Well, I can't because there's no units.
>>>
>>
>> What this line means then?
>> - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
>> - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).
>>
>> Maybe I don't know something and the description line should state the units somehow
>> clearer?

The reason we have units in the name is so we don't have to go lookup
the documentation to find the units and so people don't use the same
property name with different units.

> Append the unit to the binding name, just like in "power-on-time-ms". As
> there's no "Standard Unit Suffix" for mA stated in the documentation
> (Documentation/devicetree/bindings/property-units.txt) I think it should
> be "-milliamp"?

The reason being is we align around microamps and try to avoid
everyone picking their own units (celliamps anyone?). So unless
microamps is not enough range for you, I would stick with that.

If you have both units and vendor prefix, then I care less about the
poorly written datasheet naming used.

Rob

Re: [PATCH 8/9 v2] usb: usb251xb: Add max power/current dts property support

[Rob Herring]

On Wed, Oct 4, 2017 at 3:12 AM, Richard Leitner
<richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org> wrote:
>
> On 09/21/2017 07:10 PM, Serge Semin wrote:
>> On Thu, Sep 21, 2017 at 11:26:04AM -0500, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> ...
>
>>>> These are different parameters of the device. They got different configuration
>>>> registers and descriptions:
>>>> max_power* - ... This value also includes the power consumption of a
>>>> permanently attached peripheral if the hub is configured as a compound
>>>> device, and the embedded peripheral reports 0mA in its descriptors.
>>>> max_current* - ... This value does NOT include the power consumption of a
>>>> permanently attached peripheral if the hub is configured as a compound
>>>> device.
>>>
>>> Then the names here should somehow reflect the above. Perhaps
>>> "composite-current" and "hub-current" or something like that.
>>>
>>
>> I left the naming in accordance with the device datasheet. I thought it would be
>> better since the driver user would still need to consult with the device
>> documentation to properly set them. I don't really get how the difference is reflected
>> with the naming declared there though. So what naming would you prefer then? Might be
>> something like:
>> {sp,bp}-max-total-current - for so named {sp,bp}-max-power, since it includes all the
>> permanently attached peripherals.
>> {sp,bp}-max-removable-current - for so named {sp,bp}-max-current, since it doesn't
>> include the permanently attached peripherals.
>>
>> Or is it better to leave it in compliance with the documentation naming?
>
> I'd prefer the naming to be in accordance with the device datasheet too.
> Changing it will IMHO generate avoidable misunderstandings...

Okay, then add a vendor prefix.

> But if we should go with Robs proposal please make sure the name from
> the device datasheet is mentioned somewhere in the description of the
> binding.
>
>>
>>>>
>>>> Additionally as you can see, they both are measured in "mA", so it isn't
>>>> a real physical power.
>>>
>>> Well, I can't because there's no units.
>>>
>>
>> What this line means then?
>> - sp-max-{power,current} : ... The value is given in mA in a 0 - 100 range (default is 1mA).
>> - bp-max-{power,current} : ... The value is given in mA in a 0 - 510 range (default is 100mA).
>>
>> Maybe I don't know something and the description line should state the units somehow
>> clearer?

The reason we have units in the name is so we don't have to go lookup
the documentation to find the units and so people don't use the same
property name with different units.

> Append the unit to the binding name, just like in "power-on-time-ms". As
> there's no "Standard Unit Suffix" for mA stated in the documentation
> (Documentation/devicetree/bindings/property-units.txt) I think it should
> be "-milliamp"?

The reason being is we align around microamps and try to avoid
everyone picking their own units (celliamps anyone?). So unless
microamps is not enough range for you, I would stick with that.

If you have both units and vendor prefix, then I care less about the
poorly written datasheet naming used.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

The driver used to be developed with legacy GPIO API support. It's
better to use descriptor-based interface for several reasons. First
of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
nodes, which is essential since different hardware may have different
GPIOs connectivity including the logical value inversion. Secondly,
by requesting the reset GPIO descriptor the driver prevent the other
applications from changing its value. And last but not least the
legacy GPIO interface should be avoided in the new code due to it
obsolescence.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  2 +-
 drivers/usb/misc/usb251xb.c                        | 34 +++++++++-------------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index dd59a32e7..7c981d556 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -8,10 +8,10 @@ Required properties :
 	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
 	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
 	"microchip,usb2517", "microchip,usb2517i"
- - reset-gpios : Should specify the gpio for hub reset
  - reg : I2C address on the selected bus (default is <0x2C>)
 
 Optional properties :
+ - reset-gpios : Should specify the gpio for hub reset
  - 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)
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 71994b883..c2dd9742f 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -3,6 +3,7 @@
  * Configuration via SMBus.
  *
  * Copyright (c) 2017 SKIDATA AG
+ * Copyright (c) 2017 T-platforms
  *
  * This work is based on the USB3503 driver by Dongjin Kim and
  * a not-accepted patch by Fabien Lahoudere, see:
@@ -20,12 +21,11 @@
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/nls.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/slab.h>
 
 /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
@@ -127,7 +127,7 @@ struct usb251xb {
 	struct device *dev;
 	struct i2c_client *i2c;
 	u8 skip_config;
-	int gpio_reset;
+	struct gpio_desc *gpio_reset;
 	u16 vendor_id;
 	u16 product_id;
 	u16 device_id;
@@ -235,13 +235,13 @@ static const struct usb251xb_data usb2517i_data = {
 
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
-	if (!gpio_is_valid(hub->gpio_reset))
+	if (!hub->gpio_reset)
 		return;
 
-	gpio_set_value_cansleep(hub->gpio_reset, state);
+	gpiod_set_value_cansleep(hub->gpio_reset, state);
 
 	/* wait for hub recovery/stabilization */
-	if (state)
+	if (!state)
 		usleep_range(500, 750);	/* >=500us at power on */
 	else
 		usleep_range(1, 10);	/* >=1us at power down */
@@ -260,7 +260,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 		i2c_wb[0] = 0x01;
 		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
 
-		usb251xb_reset(hub, 1);
+		usb251xb_reset(hub, 0);
 
 		err = i2c_smbus_write_i2c_block_data(hub->i2c,
 				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
@@ -310,7 +310,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, 1);
+	usb251xb_reset(hub, 0);
 
 	/* write registers */
 	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
@@ -363,19 +363,13 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	else
 		hub->skip_config = 0;
 
-	hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
-	if (hub->gpio_reset == -EPROBE_DEFER)
+	hub->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (PTR_ERR(hub->gpio_reset) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
-	if (gpio_is_valid(hub->gpio_reset)) {
-		err = devm_gpio_request_one(dev, hub->gpio_reset,
-					    GPIOF_OUT_INIT_LOW,
-					    "usb251xb reset");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as reset pin (%d)\n",
-				hub->gpio_reset, err);
-			return err;
-		}
+	} else if (IS_ERR(hub->gpio_reset)) {
+		err = PTR_ERR(hub->gpio_reset);
+		dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+		return err;
 	}
 
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
-- 
2.12.0

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Rob Herring]

On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> The driver used to be developed with legacy GPIO API support. It's
> better to use descriptor-based interface for several reasons. First
> of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
> nodes, which is essential since different hardware may have different
> GPIOs connectivity including the logical value inversion. Secondly,
> by requesting the reset GPIO descriptor the driver prevent the other
> applications from changing its value. And last but not least the
> legacy GPIO interface should be avoided in the new code due to it
> obsolescence.

All this has nothing to do with the binding. Please make the binding 
change separate and state why it should be optional.

> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt |  2 +-
>  drivers/usb/misc/usb251xb.c                        | 34 +++++++++-------------
>  2 files changed, 15 insertions(+), 21 deletions(-)

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

On Wed, Sep 20, 2017 at 03:52:57PM -0500, Rob Herring <robh@kernel.org> wrote:
> On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > The driver used to be developed with legacy GPIO API support. It's
> > better to use descriptor-based interface for several reasons. First
> > of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
> > nodes, which is essential since different hardware may have different
> > GPIOs connectivity including the logical value inversion. Secondly,
> > by requesting the reset GPIO descriptor the driver prevent the other
> > applications from changing its value. And last but not least the
> > legacy GPIO interface should be avoided in the new code due to it
> > obsolescence.
> 
> All this has nothing to do with the binding. Please make the binding 
> change separate and state why it should be optional.
> 

Ok.

> > 
> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/usb251xb.txt |  2 +-
> >  drivers/usb/misc/usb251xb.c                        | 34 +++++++++-------------
> >  2 files changed, 15 insertions(+), 21 deletions(-)

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Greg KH]

On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 71994b883..c2dd9742f 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -3,6 +3,7 @@
>   * Configuration via SMBus.
>   *
>   * Copyright (c) 2017 SKIDATA AG
> + * Copyright (c) 2017 T-platforms

Again, no, please consult with your corporate lawyers why this isn't ok.

greg k-h

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 71994b883..c2dd9742f 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> Again, no, please consult with your corporate lawyers why this isn't ok.
> 
> greg k-h

I still can't see why this isn't right. We submitted the patchset. It is not
that big and still it isn't just two lines. As I've seen all over the kernel, It is
a common practice to have multiple copyrights in kernel files. We are not claiming
the copyright to the whole file, but to the contribution only. I got a consent to
contribute when I was employed by the company. What's wrong with that? Shall I
send the patchset from my corporate e-mail then?

-Sergey

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index 71994b883..c2dd9742f 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -3,6 +3,7 @@
> >   * Configuration via SMBus.
> >   *
> >   * Copyright (c) 2017 SKIDATA AG
> > + * Copyright (c) 2017 T-platforms
> 
> Again, no, please consult with your corporate lawyers why this isn't ok.
> 
> greg k-h

I still can't see why this isn't right. We submitted the patchset. It is not
that big and still it isn't just two lines. As I've seen all over the kernel, It is
a common practice to have multiple copyrights in kernel files. We are not claiming
the copyright to the whole file, but to the contribution only. I got a consent to
contribute when I was employed by the company. What's wrong with that? Shall I
send the patchset from my corporate e-mail then?

-Sergey

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Greg KH]

On Thu, Sep 21, 2017 at 05:51:29PM +0300, Serge Semin wrote:
> On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > index 71994b883..c2dd9742f 100644
> > > --- a/drivers/usb/misc/usb251xb.c
> > > +++ b/drivers/usb/misc/usb251xb.c
> > > @@ -3,6 +3,7 @@
> > >   * Configuration via SMBus.
> > >   *
> > >   * Copyright (c) 2017 SKIDATA AG
> > > + * Copyright (c) 2017 T-platforms
> > 
> > Again, no, please consult with your corporate lawyers why this isn't ok.
> > 
> > greg k-h
> 
> I still can't see why this isn't right. We submitted the patchset. It is not
> that big and still it isn't just two lines. As I've seen all over the kernel, It is
> a common practice to have multiple copyrights in kernel files. We are not claiming
> the copyright to the whole file, but to the contribution only. I got a consent to
> contribute when I was employed by the company. What's wrong with that? Shall I
> send the patchset from my corporate e-mail then?

Well, yes, I need some way to properly identify that this corporation
did do the changes.  I said that before, I don't know why you ignored
that.

And yes, multiple copyrights are just fine, but again, please talk to
your corporate lawyer about why these changes don't seem to warrant that
"mark".  If they do think that they do warrant that, great, I will be
glad to discuss that with them, off-list if needed.

For even more fun, try discussing with your lawyers about why copyright
marks like this don't even mean anything anymore, and haven't for 20+
years now (can't remember the actual date...)  But that's a different
topic, and one not really relevant here.

thanks,

greg k-h

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

On Thu, Sep 21, 2017 at 05:07:14PM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 21, 2017 at 05:51:29PM +0300, Serge Semin wrote:
> > On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > > index 71994b883..c2dd9742f 100644
> > > > --- a/drivers/usb/misc/usb251xb.c
> > > > +++ b/drivers/usb/misc/usb251xb.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Configuration via SMBus.
> > > >   *
> > > >   * Copyright (c) 2017 SKIDATA AG
> > > > + * Copyright (c) 2017 T-platforms
> > > 
> > > Again, no, please consult with your corporate lawyers why this isn't ok.
> > > 
> > > greg k-h
> > 
> > I still can't see why this isn't right. We submitted the patchset. It is not
> > that big and still it isn't just two lines. As I've seen all over the kernel, It is
> > a common practice to have multiple copyrights in kernel files. We are not claiming
> > the copyright to the whole file, but to the contribution only. I got a consent to
> > contribute when I was employed by the company. What's wrong with that? Shall I
> > send the patchset from my corporate e-mail then?
> 
> Well, yes, I need some way to properly identify that this corporation
> did do the changes.  I said that before, I don't know why you ignored
> that.
> 
> And yes, multiple copyrights are just fine, but again, please talk to
> your corporate lawyer about why these changes don't seem to warrant that
> "mark".  If they do think that they do warrant that, great, I will be
> glad to discuss that with them, off-list if needed.
> 
> For even more fun, try discussing with your lawyers about why copyright
> marks like this don't even mean anything anymore, and haven't for 20+
> years now (can't remember the actual date...)  But that's a different
> topic, and one not really relevant here.
> 
> thanks,
> 
> greg k-h

Alright then. We'll remove the Copyright mark and I'll resend the patchset
from my corporate e-mail. Hope it will solve this issue.
Could you review the rest of the patchset?

Regards,
-Sergey

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

On Thu, Sep 21, 2017 at 05:07:14PM +0200, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Thu, Sep 21, 2017 at 05:51:29PM +0300, Serge Semin wrote:
> > On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > > On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > > index 71994b883..c2dd9742f 100644
> > > > --- a/drivers/usb/misc/usb251xb.c
> > > > +++ b/drivers/usb/misc/usb251xb.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Configuration via SMBus.
> > > >   *
> > > >   * Copyright (c) 2017 SKIDATA AG
> > > > + * Copyright (c) 2017 T-platforms
> > > 
> > > Again, no, please consult with your corporate lawyers why this isn't ok.
> > > 
> > > greg k-h
> > 
> > I still can't see why this isn't right. We submitted the patchset. It is not
> > that big and still it isn't just two lines. As I've seen all over the kernel, It is
> > a common practice to have multiple copyrights in kernel files. We are not claiming
> > the copyright to the whole file, but to the contribution only. I got a consent to
> > contribute when I was employed by the company. What's wrong with that? Shall I
> > send the patchset from my corporate e-mail then?
> 
> Well, yes, I need some way to properly identify that this corporation
> did do the changes.  I said that before, I don't know why you ignored
> that.
> 
> And yes, multiple copyrights are just fine, but again, please talk to
> your corporate lawyer about why these changes don't seem to warrant that
> "mark".  If they do think that they do warrant that, great, I will be
> glad to discuss that with them, off-list if needed.
> 
> For even more fun, try discussing with your lawyers about why copyright
> marks like this don't even mean anything anymore, and haven't for 20+
> years now (can't remember the actual date...)  But that's a different
> topic, and one not really relevant here.
> 
> thanks,
> 
> greg k-h

Alright then. We'll remove the Copyright mark and I'll resend the patchset
from my corporate e-mail. Hope it will solve this issue.
Could you review the rest of the patchset?

Regards,
-Sergey

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9 v2] usb: usb251xb: Use GPIO descriptor consumer interface

[Greg KH]

On Fri, Sep 22, 2017 at 06:26:54PM +0300, Serge Semin wrote:
> On Thu, Sep 21, 2017 at 05:07:14PM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Thu, Sep 21, 2017 at 05:51:29PM +0300, Serge Semin wrote:
> > > On Thu, Sep 21, 2017 at 10:23:38AM +0200, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Sat, Sep 16, 2017 at 01:42:20PM +0300, Serge Semin wrote:
> > > > > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > > > > index 71994b883..c2dd9742f 100644
> > > > > --- a/drivers/usb/misc/usb251xb.c
> > > > > +++ b/drivers/usb/misc/usb251xb.c
> > > > > @@ -3,6 +3,7 @@
> > > > >   * Configuration via SMBus.
> > > > >   *
> > > > >   * Copyright (c) 2017 SKIDATA AG
> > > > > + * Copyright (c) 2017 T-platforms
> > > > 
> > > > Again, no, please consult with your corporate lawyers why this isn't ok.
> > > > 
> > > > greg k-h
> > > 
> > > I still can't see why this isn't right. We submitted the patchset. It is not
> > > that big and still it isn't just two lines. As I've seen all over the kernel, It is
> > > a common practice to have multiple copyrights in kernel files. We are not claiming
> > > the copyright to the whole file, but to the contribution only. I got a consent to
> > > contribute when I was employed by the company. What's wrong with that? Shall I
> > > send the patchset from my corporate e-mail then?
> > 
> > Well, yes, I need some way to properly identify that this corporation
> > did do the changes.  I said that before, I don't know why you ignored
> > that.
> > 
> > And yes, multiple copyrights are just fine, but again, please talk to
> > your corporate lawyer about why these changes don't seem to warrant that
> > "mark".  If they do think that they do warrant that, great, I will be
> > glad to discuss that with them, off-list if needed.
> > 
> > For even more fun, try discussing with your lawyers about why copyright
> > marks like this don't even mean anything anymore, and haven't for 20+
> > years now (can't remember the actual date...)  But that's a different
> > topic, and one not really relevant here.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Alright then. We'll remove the Copyright mark and I'll resend the patchset
> from my corporate e-mail. Hope it will solve this issue.
> Could you review the rest of the patchset?

I'll wait for the resend, as Rob already pointed out issues, so it is
long-gone from my patchqueue.

thanks,

greg k-h

[PATCH 00/10 v3] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to make the following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Changelog v2:
- Split first patch into smaller ones
- Fix invalid BOOST_14 register definition
- Combine copyrights adding patch into the last one

Changelog v3:
- Move bindings documentation updates to a separate patch
- Add "boolean" info to the corresponding properties
- Rename {sp,bp}-max-{power,current} with a better readable
{sp,bp}-max-{total,removable}-current-microamp
- Discard Copyright line
- Add better reason text for dev_warn()'s
- Set led-speed-mode being default
- Fix some types

Serge Semin (10):
  usb: usb251xb: Update usb251xb bindings
  usb: usb251xb: Add USB2517i specific struct and IDs
  usb: usb251xb: Add USB251x specific port count setting
  usb: usb251xb: Add 5,6,7 ports mapping def setting
  usb: usb251xb: Add 5,6,7 ports boost settings
  usb: usb251xb: Add battery enable setting flag
  usb: usb251xb: Add USB2517 LED settings
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts property support
  usb: usb251xb: Use GPIO descriptor consumer interface

 .../devicetree/bindings/usb/usb251xb.txt |  46 ++++--
 kernel/drivers/usb/misc/Kconfig          |   4 +-
 kernel/drivers/usb/misc/usb251xb.c       | 166 +++++++++++++++------
 3 files changed, 158 insertions(+), 58 deletions(-)

-- 
2.12.0

[PATCH 00/10 v3] usb: usb251xb: Add USB2517i hub support and fix some bugs

[Serge Semin]

Primarily it was intended to just add USB2517 hub support to the driver.
But after tests a bug and inconistency were discovered. So it was decided
to make the following changes:

Changelog v1:
- Add USB2517/i hub specifics support to the driver
- Fix property_u32 NULL-pointer dereference
- Add new {bus,self}-max-{power,curret} dts properties
- Replace legacy GPIO API usage with descriptor-based one

Changelog v2:
- Split first patch into smaller ones
- Fix invalid BOOST_14 register definition
- Combine copyrights adding patch into the last one

Changelog v3:
- Move bindings documentation updates to a separate patch
- Add "boolean" info to the corresponding properties
- Rename {sp,bp}-max-{power,current} with a better readable
{sp,bp}-max-{total,removable}-current-microamp
- Discard Copyright line
- Add better reason text for dev_warn()'s
- Set led-speed-mode being default
- Fix some types

Serge Semin (10):
  usb: usb251xb: Update usb251xb bindings
  usb: usb251xb: Add USB2517i specific struct and IDs
  usb: usb251xb: Add USB251x specific port count setting
  usb: usb251xb: Add 5,6,7 ports mapping def setting
  usb: usb251xb: Add 5,6,7 ports boost settings
  usb: usb251xb: Add battery enable setting flag
  usb: usb251xb: Add USB2517 LED settings
  usb: usb251xb: Fix property_u32 NULL pointer dereference
  usb: usb251xb: Add max power/current dts property support
  usb: usb251xb: Use GPIO descriptor consumer interface

 .../devicetree/bindings/usb/usb251xb.txt |  46 ++++--
 kernel/drivers/usb/misc/Kconfig          |   4 +-
 kernel/drivers/usb/misc/usb251xb.c       | 166 +++++++++++++++------
 3 files changed, 158 insertions(+), 58 deletions(-)

-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10 v3] usb: usb251xb: Update usb251xb bindings

[Serge Semin]

Since hub usb2517 is going to be supported by the usb251xb driver,
the bindings need to be properly updated. Particularly:
- add "microchip,usb2517" and "microchip,usb2517i" compatible strings.
- add "boolean" description to all the properties, which really accept
a boolean value including a new one "led-{usb,speed}-mode".
- move reset-gpios property to the optional section. It isn't
defined as required in the code and shouldn't be required at all, since
hardware may handle reset pins by itself.
- add new led-{usb,speed}-mode mode property. USB2517 device supports
two LED modes: USB mode and speed (default) indication mode. The last one
can be switched on by this property.
- add {bp,sp}-max-{total,removable}-current-microamp property measured in
microamp. It hasn't been defined as property before. Since the limitation
specified by these parameters is hardware specific it needs to be defined
in dts.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 .../devicetree/bindings/usb/usb251xb.txt           | 46 +++++++++++++++-------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 3957d4eda..168ff819e 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -1,16 +1,17 @@
 Microchip USB 2.0 Hi-Speed Hub Controller
 
-The device node for the configuration of a Microchip USB251xB/xBi USB 2.0
+The device node for the configuration of a Microchip USB251x/xBi USB 2.0
 Hi-Speed Controller.
 
 Required properties :
  - compatible : Should be "microchip,usb251xb" or one of the specific types:
 	"microchip,usb2512b", "microchip,usb2512bi", "microchip,usb2513b",
-	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi"
- - reset-gpios : Should specify the gpio for hub reset
+	"microchip,usb2513bi", "microchip,usb2514b", "microchip,usb2514bi",
+	"microchip,usb2517", "microchip,usb2517i"
  - reg : I2C address on the selected bus (default is <0x2C>)
 
 Optional properties :
+ - reset-gpios : Should specify the gpio for hub reset
  - 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)
@@ -19,29 +20,47 @@ Optional properties :
  - manufacturer : Set USB Manufacturer string (max 31 characters long)
  - product : Set USB Product string (max 31 characters long)
  - serial : Set USB Serial string (max 31 characters long)
- - {bus,self}-powered : selects between self- and bus-powered operation (default
-	is self-powered)
- - disable-hi-speed : disable USB Hi-Speed support
+ - {bus,self}-powered : selects between self- and bus-powered operation
+	(boolean, default is self-powered)
+ - disable-hi-speed : disable USB Hi-Speed support (boolean)
  - {multi,single}-tt : selects between multi- and single-transaction-translator
-	(default is multi-tt)
- - disable-eop : disable End of Packet generation in full-speed mode
+	(boolean, default is multi-tt)
+ - disable-eop : disable End of Packet generation in full-speed mode (boolean)
  - {ganged,individual}-sensing : select over-current sense type in self-powered
-	mode (default is individual)
+	mode (boolean, default is individual)
  - {ganged,individual}-port-switching : select port power switching mode
-	(default is individual)
+	(boolean, default is individual)
  - dynamic-power-switching : enable auto-switching from self- to bus-powered
-	operation if the local power source is removed or unavailable
+	operation if the local power source is removed or unavailable (boolean)
  - oc-delay-us : Delay time (in microseconds) for filtering the over-current
 	sense inputs. Valid values are 100, 4000, 8000 (default) and 16000. If
 	an invalid value is given, the default is used instead.
- - compound-device : indicate the hub is part of a compound device
- - port-mapping-mode : enable port mapping mode
+ - compound-device : indicate the hub is part of a compound device (boolean)
+ - port-mapping-mode : enable port mapping mode (boolean)
+ - led-{usb,speed}-mode : led usb/speed indication mode selection
+	(boolean, default is speed mode)
  - string-support : enable string descriptor support (required for manufacturer,
 	product and serial string configuration)
  - non-removable-ports : Should specify the ports which have a non-removable
 	device connected.
  - sp-disabled-ports : Specifies the ports which will be self-power disabled
  - bp-disabled-ports : Specifies the ports which will be bus-power disabled
+ - sp-max-total-current-microamp: Specifies max current consumed by the hub
+	from VBUS when operating in self-powered hub. It includes the hub
+	silicon along with all associated circuitry including a permanently
+	attached peripheral (range: 0 - 100000 uA, default 1000 uA)
+ - bp-max-total-current-microamp: Specifies max current consumed by the hub
+	from VBUS when operating in self-powered hub. It includes the hub
+	silicon along with all associated circuitry including a permanently
+	attached peripheral (range: 0 - 510000 uA, default 100000 uA)
+ - sp-max-removable-current-microamp: Specifies max current consumed by the hub
+	from VBUS when operating in self-powered hub. It includes the hub
+	silicon along with all associated circuitry excluding a permanently
+	attached peripheral (range: 0 - 100000 uA, default 1000 uA)
+ - bp-max-removable-current-microamp: Specifies max current consumed by the hub
+	from VBUS when operating in self-powered hub. It includes the hub
+	silicon along with all associated circuitry excluding a permanently
+	attached peripheral (range: 0 - 510000 uA, default 100000 uA)
  - power-on-time-ms : Specifies the time it takes from the time the host
 	initiates the power-on sequence to a port until the port has adequate
 	power. The value is given in ms in a 0 - 510 range (default is 100ms).
@@ -56,7 +75,6 @@ Examples:
 	usb2514b@2c {
 		compatible = "microchip,usb2514b";
 		reg = <0x2c>;
-		reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
 		vendor-id = /bits/ 16 <0x0000>;
 		product-id = /bits/ 16 <0x0000>;
 		string-support;
-- 
2.12.0

Re: [PATCH 01/10 v3] usb: usb251xb: Update usb251xb bindings

[Rob Herring]

On Sun, Oct 22, 2017 at 11:38:03PM +0300, Serge Semin wrote:
> Since hub usb2517 is going to be supported by the usb251xb driver,
> the bindings need to be properly updated. Particularly:
> - add "microchip,usb2517" and "microchip,usb2517i" compatible strings.
> - add "boolean" description to all the properties, which really accept
> a boolean value including a new one "led-{usb,speed}-mode".
> - move reset-gpios property to the optional section. It isn't
> defined as required in the code and shouldn't be required at all, since
> hardware may handle reset pins by itself.
> - add new led-{usb,speed}-mode mode property. USB2517 device supports
> two LED modes: USB mode and speed (default) indication mode. The last one
> can be switched on by this property.
> - add {bp,sp}-max-{total,removable}-current-microamp property measured in
> microamp. It hasn't been defined as property before. Since the limitation
> specified by these parameters is hardware specific it needs to be defined
> in dts.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  .../devicetree/bindings/usb/usb251xb.txt           | 46 +++++++++++++++-------
>  1 file changed, 32 insertions(+), 14 deletions(-)

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

Re: [PATCH 01/10 v3] usb: usb251xb: Update usb251xb bindings

[Rob Herring]

On Sun, Oct 22, 2017 at 11:38:03PM +0300, Serge Semin wrote:
> Since hub usb2517 is going to be supported by the usb251xb driver,
> the bindings need to be properly updated. Particularly:
> - add "microchip,usb2517" and "microchip,usb2517i" compatible strings.
> - add "boolean" description to all the properties, which really accept
> a boolean value including a new one "led-{usb,speed}-mode".
> - move reset-gpios property to the optional section. It isn't
> defined as required in the code and shouldn't be required at all, since
> hardware may handle reset pins by itself.
> - add new led-{usb,speed}-mode mode property. USB2517 device supports
> two LED modes: USB mode and speed (default) indication mode. The last one
> can be switched on by this property.
> - add {bp,sp}-max-{total,removable}-current-microamp property measured in
> microamp. It hasn't been defined as property before. Since the limitation
> specified by these parameters is hardware specific it needs to be defined
> in dts.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/usb251xb.txt           | 46 +++++++++++++++-------
>  1 file changed, 32 insertions(+), 14 deletions(-)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/10 v3] usb: usb251xb: Add USB2517i specific struct and IDs

[Serge Semin]

There are USB2517 and USB2517i hubs, which have almost the same
registers space as already supported USB251xBi series. The difference
it in DIDs and in a few functions. This patch adds the USB2517/i data
structures to the driver, so it would have different setting depending
on the device discovered on i2c-bus.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/Kconfig    |  4 ++--
 drivers/usb/misc/usb251xb.c | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 37dd1c018..27b9fcbdf 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -247,8 +247,8 @@ config USB_HUB_USB251XB
 	depends on I2C
 	help
 	  This option enables support for configuration via SMBus of the
-	  Microchip USB251xB/xBi USB 2.0 Hub Controller series.
-	  Configuration parameters may be set in devicetree or platform data.
+	  Microchip USB251x/xBi USB 2.0 Hub Controller series. Configuration
+	  parameters may be set in devicetree or platform data.
 	  Say Y or M here if you need to configure such a device via SMBus.
 
 config USB_HSIC_USB3503
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 91f66d68b..22c32ea3f 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -38,6 +38,7 @@
 #define USB251XB_DEF_PRODUCT_ID_12	0x2512 /* USB2512B/12Bi */
 #define USB251XB_DEF_PRODUCT_ID_13	0x2513 /* USB2513B/13Bi */
 #define USB251XB_DEF_PRODUCT_ID_14	0x2514 /* USB2514B/14Bi */
+#define USB251XB_DEF_PRODUCT_ID_17	0x2517 /* USB2517/17i */
 
 #define USB251XB_ADDR_DEVICE_ID_LSB	0x04
 #define USB251XB_ADDR_DEVICE_ID_MSB	0x05
@@ -82,7 +83,7 @@
 
 #define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
 #define USB251XB_ADDR_PRODUCT_STRING		0x54
-#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
+#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi/7i"
 
 #define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
 #define USB251XB_ADDR_SERIAL_STRING		0x92
@@ -186,6 +187,16 @@ static const struct usb251xb_data usb2514bi_data = {
 	.product_str = "USB2514Bi",
 };
 
+static const struct usb251xb_data usb2517_data = {
+	.product_id = 0x2517,
+	.product_str = "USB2517",
+};
+
+static const struct usb251xb_data usb2517i_data = {
+	.product_id = 0x2517,
+	.product_str = "USB2517i",
+};
+
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
 	if (!gpio_is_valid(hub->gpio_reset))
@@ -511,6 +522,12 @@ static const struct of_device_id usb251xb_of_match[] = {
 		.compatible = "microchip,usb2514bi",
 		.data = &usb2514bi_data,
 	}, {
+		.compatible = "microchip,usb2517",
+		.data = &usb2517_data,
+	}, {
+		.compatible = "microchip,usb2517i",
+		.data = &usb2517i_data,
+	}, {
 		/* sentinel */
 	}
 };
@@ -574,6 +591,8 @@ static const struct i2c_device_id usb251xb_id[] = {
 	{ "usb2513bi", 0 },
 	{ "usb2514b", 0 },
 	{ "usb2514bi", 0 },
+	{ "usb2517", 0 },
+	{ "usb2517i", 0 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, usb251xb_id);
@@ -590,5 +609,5 @@ static struct i2c_driver usb251xb_i2c_driver = {
 module_i2c_driver(usb251xb_i2c_driver);
 
 MODULE_AUTHOR("Richard Leitner <richard.leitner@skidata.com>");
-MODULE_DESCRIPTION("USB251xB/xBi USB 2.0 Hub Controller Driver");
+MODULE_DESCRIPTION("USB251x/xBi USB 2.0 Hub Controller Driver");
 MODULE_LICENSE("GPL");
-- 
2.12.0

Re: [PATCH 02/10 v3] usb: usb251xb: Add USB2517i specific struct and IDs

[Richard Leitner]

Hi,
please see comments below for some nit-picks.

On 10/22/2017 10:38 PM, Serge Semin wrote:
> There are USB2517 and USB2517i hubs, which have almost the same
> registers space as already supported USB251xBi series. The difference
> it in DIDs and in a few functions. This patch adds the USB2517/i data
> structures to the driver, so it would have different setting depending
> on the device discovered on i2c-bus.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/usb/misc/Kconfig    |  4 ++--
>   drivers/usb/misc/usb251xb.c | 23 +++++++++++++++++++++--
>   2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 37dd1c018..27b9fcbdf 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -247,8 +247,8 @@ config USB_HUB_USB251XB
>   	depends on I2C
>   	help
>   	  This option enables support for configuration via SMBus of the
> -	  Microchip USB251xB/xBi USB 2.0 Hub Controller series.
> -	  Configuration parameters may be set in devicetree or platform data.
> +	  Microchip USB251x/xBi USB 2.0 Hub Controller series. Configuration
> +	  parameters may be set in devicetree or platform data.

Here you have "USB251x/xBi"...

>   	  Say Y or M here if you need to configure such a device via SMBus.
>   
>   config USB_HSIC_USB3503
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 91f66d68b..22c32ea3f 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c

...

> @@ -82,7 +83,7 @@
>   
>   #define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
>   #define USB251XB_ADDR_PRODUCT_STRING		0x54
> -#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
> +#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi/7i"

but here you have "USB251xB/xBi/7i"...

>   
>   #define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
>   #define USB251XB_ADDR_SERIAL_STRING		0x92

...

> @@ -590,5 +609,5 @@ static struct i2c_driver usb251xb_i2c_driver = {
>   module_i2c_driver(usb251xb_i2c_driver);
>   
>   MODULE_AUTHOR("Richard Leitner <richard.leitner@skidata.com>");
> -MODULE_DESCRIPTION("USB251xB/xBi USB 2.0 Hub Controller Driver");
> +MODULE_DESCRIPTION("USB251x/xBi USB 2.0 Hub Controller Driver");

... and here again "USB251x/xBi"...

>   MODULE_LICENSE("GPL");
> 

I'd prefer to use just one "name" for the driver. If you be precise that 
would be "USB251xB/xBi/xi". But that seems a bit overloaded to me. AFAIK 
the "B" and "i" prepended to the type have no affect on the I2C 
interface/register map, so maybe "USB251x" would be sufficient?

kind regards,
Richard.L

Re: [PATCH 02/10 v3] usb: usb251xb: Add USB2517i specific struct and IDs

[Richard Leitner]

Hi,
please see comments below for some nit-picks.

On 10/22/2017 10:38 PM, Serge Semin wrote:
> There are USB2517 and USB2517i hubs, which have almost the same
> registers space as already supported USB251xBi series. The difference
> it in DIDs and in a few functions. This patch adds the USB2517/i data
> structures to the driver, so it would have different setting depending
> on the device discovered on i2c-bus.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/usb/misc/Kconfig    |  4 ++--
>   drivers/usb/misc/usb251xb.c | 23 +++++++++++++++++++++--
>   2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 37dd1c018..27b9fcbdf 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -247,8 +247,8 @@ config USB_HUB_USB251XB
>   	depends on I2C
>   	help
>   	  This option enables support for configuration via SMBus of the
> -	  Microchip USB251xB/xBi USB 2.0 Hub Controller series.
> -	  Configuration parameters may be set in devicetree or platform data.
> +	  Microchip USB251x/xBi USB 2.0 Hub Controller series. Configuration
> +	  parameters may be set in devicetree or platform data.

Here you have "USB251x/xBi"...

>   	  Say Y or M here if you need to configure such a device via SMBus.
>   
>   config USB_HSIC_USB3503
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 91f66d68b..22c32ea3f 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c

...

> @@ -82,7 +83,7 @@
>   
>   #define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
>   #define USB251XB_ADDR_PRODUCT_STRING		0x54
> -#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
> +#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi/7i"

but here you have "USB251xB/xBi/7i"...

>   
>   #define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
>   #define USB251XB_ADDR_SERIAL_STRING		0x92

...

> @@ -590,5 +609,5 @@ static struct i2c_driver usb251xb_i2c_driver = {
>   module_i2c_driver(usb251xb_i2c_driver);
>   
>   MODULE_AUTHOR("Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>");
> -MODULE_DESCRIPTION("USB251xB/xBi USB 2.0 Hub Controller Driver");
> +MODULE_DESCRIPTION("USB251x/xBi USB 2.0 Hub Controller Driver");

... and here again "USB251x/xBi"...

>   MODULE_LICENSE("GPL");
> 

I'd prefer to use just one "name" for the driver. If you be precise that 
would be "USB251xB/xBi/xi". But that seems a bit overloaded to me. AFAIK 
the "B" and "i" prepended to the type have no affect on the I2C 
interface/register map, so maybe "USB251x" would be sufficient?

kind regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10 v3] usb: usb251xb: Add USB251x specific port count setting

[Serge Semin]

USB251xb as well as USB2517 datasheet states, that all these
hubs differ by number of ports declared as the last digit in the
model name. So USB2512 got two ports, USB2513 - three, and so on.
Such setting must be reflected in the device specific data
structure and corresponding dts property should be checked whether
it doesn't get out of available ports.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 22c32ea3f..9586da7eb 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -154,46 +154,55 @@ struct usb251xb {
 
 struct usb251xb_data {
 	u16 product_id;
+	u8 port_cnt;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514Bi",
 };
 
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517",
 };
 
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517i",
 };
 
@@ -422,8 +431,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->non_rem_dev |= BIT(port);
+			else
+				dev_warn(dev, "NRD port %u doesn't exist\n",
+					port);
 		}
 	}
 
@@ -433,8 +445,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_sp |= BIT(port);
+			else
+				dev_warn(dev, "PDS port %u doesn't exist\n",
+					port);
 		}
 	}
 
@@ -444,8 +459,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_bp |= BIT(port);
+			else
+				dev_warn(dev, "PDB port %u doesn't exist\n",
+					port);
 		}
 	}
 
-- 
2.12.0

[PATCH 03/10 v3] usb: usb251xb: Add USB251x specific port count setting

[Serge Semin]

USB251xb as well as USB2517 datasheet states, that all these
hubs differ by number of ports declared as the last digit in the
model name. So USB2512 got two ports, USB2513 - three, and so on.
Such setting must be reflected in the device specific data
structure and corresponding dts property should be checked whether
it doesn't get out of available ports.

Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/misc/usb251xb.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 22c32ea3f..9586da7eb 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -154,46 +154,55 @@ struct usb251xb {
 
 struct usb251xb_data {
 	u16 product_id;
+	u8 port_cnt;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
+	.port_cnt = 2,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
+	.port_cnt = 3,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
+	.port_cnt = 4,
 	.product_str = "USB2514Bi",
 };
 
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517",
 };
 
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
+	.port_cnt = 7,
 	.product_str = "USB2517i",
 };
 
@@ -422,8 +431,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->non_rem_dev |= BIT(port);
+			else
+				dev_warn(dev, "NRD port %u doesn't exist\n",
+					port);
 		}
 	}
 
@@ -433,8 +445,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_sp |= BIT(port);
+			else
+				dev_warn(dev, "PDS port %u doesn't exist\n",
+					port);
 		}
 	}
 
@@ -444,8 +459,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		for (i = 0; i < len / sizeof(u32); i++) {
 			u32 port = be32_to_cpu(cproperty_u32[i]);
 
-			if ((port >= 1) && (port <= 4))
+			if ((port >= 1) && (port <= data->port_cnt))
 				hub->port_disable_bp |= BIT(port);
+			else
+				dev_warn(dev, "PDB port %u doesn't exist\n",
+					port);
 		}
 	}
 
-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/10 v3] usb: usb251xb: Add USB251x specific port count setting

[Richard Leitner]

On 10/22/2017 10:38 PM, Serge Semin wrote:
> USB251xb as well as USB2517 datasheet states, that all these
> hubs differ by number of ports declared as the last digit in the
> model name. So USB2512 got two ports, USB2513 - three, and so on.
> Such setting must be reflected in the device specific data
> structure and corresponding dts property should be checked whether
> it doesn't get out of available ports.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/usb/misc/usb251xb.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)

...
> @@ -422,8 +431,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->non_rem_dev |= BIT(port);
> +			else
> +				dev_warn(dev, "NRD port %u doesn't exist\n",
> +					port);

Please match the alignment of the second line with the open parenthesis.

>   		}
>   	}
>   
> @@ -433,8 +445,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->port_disable_sp |= BIT(port);
> +			else
> +				dev_warn(dev, "PDS port %u doesn't exist\n",
> +					port);

... same here ...

>   		}
>   	}
>   
> @@ -444,8 +459,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->port_disable_bp |= BIT(port);
> +			else
> +				dev_warn(dev, "PDB port %u doesn't exist\n",
> +					port);

... and here.

>   		}
>   	}

Otherwise feel free to add:

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

regards,
Richard.L

Re: [PATCH 03/10 v3] usb: usb251xb: Add USB251x specific port count setting

[Richard Leitner]

On 10/22/2017 10:38 PM, Serge Semin wrote:
> USB251xb as well as USB2517 datasheet states, that all these
> hubs differ by number of ports declared as the last digit in the
> model name. So USB2512 got two ports, USB2513 - three, and so on.
> Such setting must be reflected in the device specific data
> structure and corresponding dts property should be checked whether
> it doesn't get out of available ports.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/usb/misc/usb251xb.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)

...
> @@ -422,8 +431,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->non_rem_dev |= BIT(port);
> +			else
> +				dev_warn(dev, "NRD port %u doesn't exist\n",
> +					port);

Please match the alignment of the second line with the open parenthesis.

>   		}
>   	}
>   
> @@ -433,8 +445,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->port_disable_sp |= BIT(port);
> +			else
> +				dev_warn(dev, "PDS port %u doesn't exist\n",
> +					port);

... same here ...

>   		}
>   	}
>   
> @@ -444,8 +459,11 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		for (i = 0; i < len / sizeof(u32); i++) {
>   			u32 port = be32_to_cpu(cproperty_u32[i]);
>   
> -			if ((port >= 1) && (port <= 4))
> +			if ((port >= 1) && (port <= data->port_cnt))
>   				hub->port_disable_bp |= BIT(port);
> +			else
> +				dev_warn(dev, "PDB port %u doesn't exist\n",
> +					port);

... and here.

>   		}
>   	}

Otherwise feel free to add:

Acked-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>

regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/10 v3] usb: usb251xb: Add 5,6,7 ports mapping def setting

[Serge Semin]

USB2517 got three additionl downstream ports, which can
as well be mapped to another logical ports. USB251xb driver
currently doesn't fully support such setting configuration
from dts file. This patch doesn't change this, but adds
usb2517 spcific ports default liner mapping.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
Acked-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/misc/usb251xb.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 9586da7eb..a05bd2e1e 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -103,7 +103,11 @@
 #define USB251XB_ADDR_PORT_MAP_12	0xFB
 #define USB251XB_DEF_PORT_MAP_12	0x00
 #define USB251XB_ADDR_PORT_MAP_34	0xFC
-#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB2513B/i & USB2514B/i only */
+#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB251{3B/i,4B/i,7/i} only */
+#define USB251XB_ADDR_PORT_MAP_56	0xFD
+#define USB251XB_DEF_PORT_MAP_56	0x00 /* USB2517/i only */
+#define USB251XB_ADDR_PORT_MAP_7	0xFE
+#define USB251XB_DEF_PORT_MAP_7		0x00 /* USB2517/i only */
 
 #define USB251XB_ADDR_STATUS_COMMAND		0xFF
 #define USB251XB_STATUS_COMMAND_SMBUS_DOWN	0x04
@@ -149,6 +153,8 @@ struct usb251xb {
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
+	u8  port_map56;
+	u8  port_map7;
 	u8  status;
 };
 
@@ -278,6 +284,8 @@ static int usb251xb_connect(struct usb251xb *hub)
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_56]       = hub->port_map56;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_7]        = hub->port_map7;
 	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
 
 	usb251xb_reset(hub, 1);
@@ -516,6 +524,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
+	hub->port_map56 = USB251XB_DEF_PORT_MAP_56;
+	hub->port_map7  = USB251XB_DEF_PORT_MAP_7;
 
 	return 0;
 }
-- 
2.12.0

[PATCH 05/10 v3] usb: usb251xb: Add 5,6,7 ports boost settings

[Serge Semin]

USB electrical signaling drive strength boost bit is also supported
by USB2517 hub. Since it got three addition ports, the designers
needed to add one more register for initialization. It turned out
to be formerly reserved 0xF7. As before we just initialize it with
default zeros.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
Acked-by: Richard Leitner <richard.leitner@skidata.com>
---
 drivers/usb/misc/usb251xb.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index a05bd2e1e..0f309943f 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -94,8 +94,10 @@
 
 #define USB251XB_ADDR_BOOST_UP	0xF6
 #define USB251XB_DEF_BOOST_UP	0x00
-#define USB251XB_ADDR_BOOST_X	0xF8
-#define USB251XB_DEF_BOOST_X	0x00
+#define USB251XB_ADDR_BOOST_57	0xF7
+#define USB251XB_DEF_BOOST_57	0x00
+#define USB251XB_ADDR_BOOST_14	0xF8
+#define USB251XB_DEF_BOOST_14	0x00
 
 #define USB251XB_ADDR_PORT_SWAP	0xFA
 #define USB251XB_DEF_PORT_SWAP	0x00
@@ -149,7 +151,8 @@ struct usb251xb {
 	char serial[USB251XB_STRING_BUFSIZE];
 	u8  bat_charge_en;
 	u8  boost_up;
-	u8  boost_x;
+	u8  boost_57;
+	u8  boost_14;
 	u8  port_swap;
 	u8  port_map12;
 	u8  port_map34;
@@ -280,7 +283,8 @@ static int usb251xb_connect(struct usb251xb *hub)
 	       USB251XB_STRING_BUFSIZE);
 	i2c_wb[USB251XB_ADDR_BATTERY_CHARGING_ENABLE] = hub->bat_charge_en;
 	i2c_wb[USB251XB_ADDR_BOOST_UP]          = hub->boost_up;
-	i2c_wb[USB251XB_ADDR_BOOST_X]           = hub->boost_x;
+	i2c_wb[USB251XB_ADDR_BOOST_57]          = hub->boost_57;
+	i2c_wb[USB251XB_ADDR_BOOST_14]          = hub->boost_14;
 	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
 	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
@@ -520,7 +524,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
-	hub->boost_x = USB251XB_DEF_BOOST_X;
+	hub->boost_57 = USB251XB_DEF_BOOST_57;
+	hub->boost_14 = USB251XB_DEF_BOOST_14;
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
 	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
-- 
2.12.0

[PATCH 06/10 v3] usb: usb251xb: Add battery enable setting flag

[Serge Semin]

Battery charging settings are supported by USB251xb hubs only.
USB2517i isn't one of them. So we need to reflect it within the
device-specific data structure. The driver doesn't support dts
property changing this setting, but instead defaults it with zero.
So the flag isn't used anywhere in the driver, but still can be helpful
in future, when necessity of the corresponding dts setting arises.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 0f309943f..e6ccea57a 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -164,54 +164,63 @@ struct usb251xb {
 struct usb251xb_data {
 	u16 product_id;
 	u8 port_cnt;
+	bool bat_support;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
 
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.bat_support = true,
 	.product_str = "USB2512B",
 };
 
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.bat_support = true,
 	.product_str = "USB2512Bi",
 };
 
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.bat_support = true,
 	.product_str = "USB2513B",
 };
 
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.bat_support = true,
 	.product_str = "USB2513Bi",
 };
 
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.bat_support = true,
 	.product_str = "USB2514B",
 };
 
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.bat_support = true,
 	.product_str = "USB2514Bi",
 };
 
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.bat_support = false,
 	.product_str = "USB2517",
 };
 
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.bat_support = false,
 	.product_str = "USB2517i",
 };
 
-- 
2.12.0

[PATCH 07/10 v3] usb: usb251xb: Add USB2517 LED settings

[Serge Semin]

USB2517 supports two LED modes: USB mode and speed (default) indication
mode. The last one can be switched on by corresponding dts property.
Since USB251xb hubs doesn't support LEDs settings, we need to ignore
this setting.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index e6ccea57a..80df778e0 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -164,6 +164,7 @@ struct usb251xb {
 struct usb251xb_data {
 	u16 product_id;
 	u8 port_cnt;
+	bool led_support;
 	bool bat_support;
 	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
 };
@@ -171,6 +172,7 @@ struct usb251xb_data {
 static const struct usb251xb_data usb2512b_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2512B",
 };
@@ -178,6 +180,7 @@ static const struct usb251xb_data usb2512b_data = {
 static const struct usb251xb_data usb2512bi_data = {
 	.product_id = 0x2512,
 	.port_cnt = 2,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2512Bi",
 };
@@ -185,6 +188,7 @@ static const struct usb251xb_data usb2512bi_data = {
 static const struct usb251xb_data usb2513b_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2513B",
 };
@@ -192,6 +196,7 @@ static const struct usb251xb_data usb2513b_data = {
 static const struct usb251xb_data usb2513bi_data = {
 	.product_id = 0x2513,
 	.port_cnt = 3,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2513Bi",
 };
@@ -199,6 +204,7 @@ static const struct usb251xb_data usb2513bi_data = {
 static const struct usb251xb_data usb2514b_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2514B",
 };
@@ -206,6 +212,7 @@ static const struct usb251xb_data usb2514b_data = {
 static const struct usb251xb_data usb2514bi_data = {
 	.product_id = 0x2514,
 	.port_cnt = 4,
+	.led_support = false,
 	.bat_support = true,
 	.product_str = "USB2514Bi",
 };
@@ -213,6 +220,7 @@ static const struct usb251xb_data usb2514bi_data = {
 static const struct usb251xb_data usb2517_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.led_support = true,
 	.bat_support = false,
 	.product_str = "USB2517",
 };
@@ -220,6 +228,7 @@ static const struct usb251xb_data usb2517_data = {
 static const struct usb251xb_data usb2517i_data = {
 	.product_id = 0x2517,
 	.port_cnt = 7,
+	.led_support = true,
 	.bat_support = false,
 	.product_str = "USB2517i",
 };
@@ -443,6 +452,9 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "port-mapping-mode", NULL))
 		hub->conf_data3 |= BIT(3);
 
+	if (data->led_support && of_get_property(np, "led-usb-mode", NULL))
+		hub->conf_data3 &= ~BIT(1);
+
 	if (of_get_property(np, "string-support", NULL))
 		hub->conf_data3 |= BIT(0);
 
-- 
2.12.0

[PATCH 08/10 v3] usb: usb251xb: Fix property_u32 NULL pointer dereference

[Serge Semin]

The methods like of_property_read_u32 utilizing the specified
pointer permit only the pointer to a preallocated u32 storage as the
third argument. As a result the driver crashes on NULL pointer
dereference in case if "oc-delay-us" or "power-on-time-ms" declared
in dts file.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 80df778e0..29432fd3b 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -348,7 +348,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	struct device *dev = hub->dev;
 	struct device_node *np = dev->of_node;
 	int len, err, i;
-	u32 *property_u32 = NULL;
+	u32 property_u32 = 0;
 	const u32 *cproperty_u32;
 	const char *cproperty_char;
 	char str[USB251XB_STRING_BUFSIZE / 2];
@@ -425,16 +425,16 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	if (of_get_property(np, "dynamic-power-switching", NULL))
 		hub->conf_data2 |= BIT(7);
 
-	if (!of_property_read_u32(np, "oc-delay-us", property_u32)) {
-		if (*property_u32 == 100) {
+	if (!of_property_read_u32(np, "oc-delay-us", &property_u32)) {
+		if (property_u32 == 100) {
 			/* 100 us*/
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 &= ~BIT(4);
-		} else if (*property_u32 == 4000) {
+		} else if (property_u32 == 4000) {
 			/* 4 ms */
 			hub->conf_data2 &= ~BIT(5);
 			hub->conf_data2 |= BIT(4);
-		} else if (*property_u32 == 16000) {
+		} else if (property_u32 == 16000) {
 			/* 16 ms */
 			hub->conf_data2 |= BIT(5);
 			hub->conf_data2 |= BIT(4);
@@ -501,8 +501,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	}
 
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
-	if (!of_property_read_u32(np, "power-on-time-ms", property_u32))
-		hub->power_on_time = min_t(u8, *property_u32 / 2, 255);
+	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
+		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
 
 	if (of_property_read_u16_array(np, "language-id", &hub->lang_id, 1))
 		hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
-- 
2.12.0

[PATCH 09/10 v3] usb: usb251xb: Add max power/current dts property support

[Serge Semin]

This parameters may be varied in accordance with hardware specifics.
So lets add the corresponding settings to the usb251xb driver dts
specification.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 29432fd3b..669b98be2 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -500,6 +500,26 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		}
 	}
 
+	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
+	if (!of_property_read_u32(np, "sp-max-total-current-microamp",
+	    &property_u32))
+		hub->max_power_sp = min_t(u8, property_u32 / 2000, 50);
+
+	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
+	if (!of_property_read_u32(np, "bp-max-total-current-microamp",
+	    &property_u32))
+		hub->max_power_bp = min_t(u8, property_u32 / 2000, 255);
+
+	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
+	if (!of_property_read_u32(np, "sp-max-removable-current-microamp",
+	    &property_u32))
+		hub->max_current_sp = min_t(u8, property_u32 / 2000, 50);
+
+	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
+	if (!of_property_read_u32(np, "bp-max-removable-current-microamp",
+	    &property_u32))
+		hub->max_current_bp = min_t(u8, property_u32 / 2000, 255);
+
 	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
 	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
 		hub->power_on_time = min_t(u8, property_u32 / 2, 255);
@@ -539,10 +559,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
 	 */
-	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
 	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
 	hub->boost_up = USB251XB_DEF_BOOST_UP;
 	hub->boost_57 = USB251XB_DEF_BOOST_57;
-- 
2.12.0

Re: [PATCH 09/10 v3] usb: usb251xb: Add max power/current dts property support

[Richard Leitner]

Hi,
again some nit-picks below...

On 10/22/2017 10:38 PM, Serge Semin wrote:
> This parameters may be varied in accordance with hardware specifics.
> So lets add the corresponding settings to the usb251xb driver dts
> specification.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/usb/misc/usb251xb.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 29432fd3b..669b98be2 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -500,6 +500,26 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		}
>   	}
>   
> +	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
> +	if (!of_property_read_u32(np, "sp-max-total-current-microamp",
> +	    &property_u32))

Please match the "correct" (of_property_read_u32) opening parenthesis 
here...

> +		hub->max_power_sp = min_t(u8, property_u32 / 2000, 50);
> +
> +	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
> +	if (!of_property_read_u32(np, "bp-max-total-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_power_bp = min_t(u8, property_u32 / 2000, 255);
> +
> +	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
> +	if (!of_property_read_u32(np, "sp-max-removable-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_current_sp = min_t(u8, property_u32 / 2000, 50);
> +
> +	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
> +	if (!of_property_read_u32(np, "bp-max-removable-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_current_bp = min_t(u8, property_u32 / 2000, 255);
> +
>   	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
>   	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
>   		hub->power_on_time = min_t(u8, property_u32 / 2, 255);

Thanks!

kind regards,
Richard.L

Re: [PATCH 09/10 v3] usb: usb251xb: Add max power/current dts property support

[Richard Leitner]

Hi,
again some nit-picks below...

On 10/22/2017 10:38 PM, Serge Semin wrote:
> This parameters may be varied in accordance with hardware specifics.
> So lets add the corresponding settings to the usb251xb driver dts
> specification.
> 
> Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/usb/misc/usb251xb.c | 24 ++++++++++++++++++++----
>   1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 29432fd3b..669b98be2 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -500,6 +500,26 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
>   		}
>   	}
>   
> +	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
> +	if (!of_property_read_u32(np, "sp-max-total-current-microamp",
> +	    &property_u32))

Please match the "correct" (of_property_read_u32) opening parenthesis 
here...

> +		hub->max_power_sp = min_t(u8, property_u32 / 2000, 50);
> +
> +	hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
> +	if (!of_property_read_u32(np, "bp-max-total-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_power_bp = min_t(u8, property_u32 / 2000, 255);
> +
> +	hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
> +	if (!of_property_read_u32(np, "sp-max-removable-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_current_sp = min_t(u8, property_u32 / 2000, 50);
> +
> +	hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
> +	if (!of_property_read_u32(np, "bp-max-removable-current-microamp",
> +	    &property_u32))

... and here ...

> +		hub->max_current_bp = min_t(u8, property_u32 / 2000, 255);
> +
>   	hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
>   	if (!of_property_read_u32(np, "power-on-time-ms", &property_u32))
>   		hub->power_on_time = min_t(u8, property_u32 / 2, 255);

Thanks!

kind regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/10 v3] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

The driver used to be developed with legacy GPIO API support. It's
better to use descriptor-based interface for several reasons. First
of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
nodes, which is essential since different hardware may have different
GPIOs connectivity including the logical value inversion. Secondly,
by requesting the reset GPIO descriptor the driver prevent the other
applications from changing its value. And last but not least the
legacy GPIO interface should be avoided in the new code due to it
obsolescence.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/usb/misc/usb251xb.c | 33 +++++++++++----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 669b98be2..097923656 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -20,12 +20,11 @@
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/nls.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/slab.h>
 
 /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
@@ -127,7 +126,7 @@ struct usb251xb {
 	struct device *dev;
 	struct i2c_client *i2c;
 	u8 skip_config;
-	int gpio_reset;
+	struct gpio_desc *gpio_reset;
 	u16 vendor_id;
 	u16 product_id;
 	u16 device_id;
@@ -235,13 +234,13 @@ static const struct usb251xb_data usb2517i_data = {
 
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
-	if (!gpio_is_valid(hub->gpio_reset))
+	if (!hub->gpio_reset)
 		return;
 
-	gpio_set_value_cansleep(hub->gpio_reset, state);
+	gpiod_set_value_cansleep(hub->gpio_reset, state);
 
 	/* wait for hub recovery/stabilization */
-	if (state)
+	if (!state)
 		usleep_range(500, 750);	/* >=500us at power on */
 	else
 		usleep_range(1, 10);	/* >=1us at power down */
@@ -260,7 +259,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 		i2c_wb[0] = 0x01;
 		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
 
-		usb251xb_reset(hub, 1);
+		usb251xb_reset(hub, 0);
 
 		err = i2c_smbus_write_i2c_block_data(hub->i2c,
 				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
@@ -310,7 +309,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, 1);
+	usb251xb_reset(hub, 0);
 
 	/* write registers */
 	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
@@ -363,19 +362,13 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	else
 		hub->skip_config = 0;
 
-	hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
-	if (hub->gpio_reset == -EPROBE_DEFER)
+	hub->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (PTR_ERR(hub->gpio_reset) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
-	if (gpio_is_valid(hub->gpio_reset)) {
-		err = devm_gpio_request_one(dev, hub->gpio_reset,
-					    GPIOF_OUT_INIT_LOW,
-					    "usb251xb reset");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as reset pin (%d)\n",
-				hub->gpio_reset, err);
-			return err;
-		}
+	} else if (IS_ERR(hub->gpio_reset)) {
+		err = PTR_ERR(hub->gpio_reset);
+		dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+		return err;
 	}
 
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
-- 
2.12.0

[PATCH 10/10 v3] usb: usb251xb: Use GPIO descriptor consumer interface

[Serge Semin]

The driver used to be developed with legacy GPIO API support. It's
better to use descriptor-based interface for several reasons. First
of all the legacy API doesn't support the ACTIVE_LOW/HIGH flag of dts
nodes, which is essential since different hardware may have different
GPIOs connectivity including the logical value inversion. Secondly,
by requesting the reset GPIO descriptor the driver prevent the other
applications from changing its value. And last but not least the
legacy GPIO interface should be avoided in the new code due to it
obsolescence.

Signed-off-by: Serge Semin <fancer.lancer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/misc/usb251xb.c | 33 +++++++++++----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 669b98be2..097923656 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -20,12 +20,11 @@
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/nls.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/slab.h>
 
 /* Internal Register Set Addresses & Default Values acc. to DS00001692C */
@@ -127,7 +126,7 @@ struct usb251xb {
 	struct device *dev;
 	struct i2c_client *i2c;
 	u8 skip_config;
-	int gpio_reset;
+	struct gpio_desc *gpio_reset;
 	u16 vendor_id;
 	u16 product_id;
 	u16 device_id;
@@ -235,13 +234,13 @@ static const struct usb251xb_data usb2517i_data = {
 
 static void usb251xb_reset(struct usb251xb *hub, int state)
 {
-	if (!gpio_is_valid(hub->gpio_reset))
+	if (!hub->gpio_reset)
 		return;
 
-	gpio_set_value_cansleep(hub->gpio_reset, state);
+	gpiod_set_value_cansleep(hub->gpio_reset, state);
 
 	/* wait for hub recovery/stabilization */
-	if (state)
+	if (!state)
 		usleep_range(500, 750);	/* >=500us at power on */
 	else
 		usleep_range(1, 10);	/* >=1us at power down */
@@ -260,7 +259,7 @@ static int usb251xb_connect(struct usb251xb *hub)
 		i2c_wb[0] = 0x01;
 		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
 
-		usb251xb_reset(hub, 1);
+		usb251xb_reset(hub, 0);
 
 		err = i2c_smbus_write_i2c_block_data(hub->i2c,
 				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
@@ -310,7 +309,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, 1);
+	usb251xb_reset(hub, 0);
 
 	/* write registers */
 	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
@@ -363,19 +362,13 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	else
 		hub->skip_config = 0;
 
-	hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
-	if (hub->gpio_reset == -EPROBE_DEFER)
+	hub->gpio_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (PTR_ERR(hub->gpio_reset) == -EPROBE_DEFER) {
 		return -EPROBE_DEFER;
-	if (gpio_is_valid(hub->gpio_reset)) {
-		err = devm_gpio_request_one(dev, hub->gpio_reset,
-					    GPIOF_OUT_INIT_LOW,
-					    "usb251xb reset");
-		if (err) {
-			dev_err(dev,
-				"unable to request GPIO %d as reset pin (%d)\n",
-				hub->gpio_reset, err);
-			return err;
-		}
+	} else if (IS_ERR(hub->gpio_reset)) {
+		err = PTR_ERR(hub->gpio_reset);
+		dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
+		return err;
 	}
 
 	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
-- 
2.12.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html