[PATCH 0/3] usb: usb251xb: Add upstream port lanes polarity inversion

[PATCH 0/3] usb: usb251xb: Add upstream port lanes polarity inversion

[Serge Semin]

A while ago I created an original patchset with functionality of any ports
inversion setting implemented. But while I was busy with another work,
Marco' version of the similar code has already been upstreamed.
So I decided to at least refactor my patchset extracting alterations,
which I think might be useful in the driver.

Even though the Marco' implementation was correct in general, it lacked a
small specific detail. The polarity inversion property might be setup for
downstream and upstream ports. Both of these cases are covered by current
'swap-dx-lanes' property implementation, but there is no any mentioning
about number 0 being considered as Upstream port of the hub. This might be
confusing seeing the rest of the port-related properties accept
only the ports starting from 1 (which is starting number of downstream
ports) and don't support number 0. So in order to unify the 'swap-dx-lanes'
property with the rest port-related ones, I'd suggest to make it being
responsible for downstream ports only, while upstream port lanes inversion
would be enabled by setting a new 'swap-us-lanes' boolean property. Current
property names mnemonic also fits quiet well for this update.

In addition the usb251xb_get_ofdata() method is getting to be too
complicated with obvious identical peaces of code. In particular it
concerns the ports-related properties like 'non-removable-ports',
'sp-disabled-ports' and 'bp-disabled-ports'. We can simplify the code
responsible for the properties handling by moving a common part into
a dedicated method. Needless to say that in case if the suggestion made
above regarding the 'swap-dx-lanes' property alteration is accepted,
the same could be done for parsing it' code as well.

So all of these updates are implemented in the patches of this small
series.


Serge Semin (3):
  usb: usb251xb: Add US lanes inversion dts-bindings
  usb: usb251xb: Create a ports field collector method
  usb: usb251xb: Add US port lanes inversion property

 .../devicetree/bindings/usb/usb251xb.txt      |  6 +-
 drivers/usb/misc/usb251xb.c                   | 73 +++++++------------
 2 files changed, 32 insertions(+), 47 deletions(-)

-- 
2.21.0

[PATCH 1/3] usb: usb251xb: Add US lanes inversion dts-bindings

[Serge Semin]

Since a separate US ports lanes polarity inversion property is going to
be available the bindings doc-file should be updated with information
about swap-us-lanes bool property, which will be responsible for it.

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

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 17915f64b8ee..bc7945e9dbfe 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -64,8 +64,10 @@ Optional properties :
  - 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).
- - swap-dx-lanes : Specifies the ports which will swap the differential-pair
-	(D+/D-), default is not-swapped.
+ - swap-dx-lanes : Specifies the downstream ports which will swap the
+	differential-pair (D+/D-), default is not-swapped.
+ - swap-us-lanes : Selects the upstream port differential-pair (D+/D-)
+	swapping (boolean, default is not-swapped)
 
 Examples:
 	usb2512b@2c {
-- 
2.21.0

[PATCH 2/3] usb: usb251xb: Create a ports field collector method

[Serge Semin]

Seeing the ports field collection functionality is used four times per
just one function, it's better to have a dedicated method performing
the task. Note that this fix filters the port 0 out from the lanes
swapping property the same way as it has been programmed for the rest
multi-ports properties. But unlike the rest of ports config registers
the BIT(0) of the Port Lanes Swap register refers to the Upstream Port
lanes inversion. This fact hasn't been documented in the driver bindings
nor there were any mentioning about port 0 being treated as upstream
port. Lets then leave this fix as is for the properties unification
and create an additional "swap-us-lanes" in the next patch.

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

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 04684849d683..4ef34df948ad 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -331,18 +331,31 @@ static int usb251xb_connect(struct usb251xb *hub)
 }
 
 #ifdef CONFIG_OF
+static void usb251xb_get_ports_field(struct usb251xb *hub,
+				    const char *prop_name, u8 port_cnt, u8 *fld)
+{
+	struct device *dev = hub->dev;
+	struct property *prop;
+	const __be32 *p;
+	u32 port;
+
+	of_property_for_each_u32(dev->of_node, prop_name, prop, p, port) {
+		if ((port >= 1) && (port <= port_cnt))
+			*fld |= BIT(port);
+		else
+			dev_warn(dev, "port %u doesn't exist\n", port);
+	}
+}
+
 static int usb251xb_get_ofdata(struct usb251xb *hub,
 			       struct usb251xb_data *data)
 {
 	struct device *dev = hub->dev;
 	struct device_node *np = dev->of_node;
-	int len, err, i;
-	u32 port, property_u32 = 0;
-	const u32 *cproperty_u32;
+	int len, err;
+	u32 property_u32 = 0;
 	const char *cproperty_char;
 	char str[USB251XB_STRING_BUFSIZE / 2];
-	struct property *prop;
-	const __be32 *p;
 
 	if (!np) {
 		dev_err(dev, "failed to get ofdata\n");
@@ -444,46 +457,16 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 		hub->conf_data3 |= BIT(0);
 
 	hub->non_rem_dev = USB251XB_DEF_NON_REMOVABLE_DEVICES;
-	cproperty_u32 = of_get_property(np, "non-removable-ports", &len);
-	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
-		for (i = 0; i < len / sizeof(u32); i++) {
-			u32 port = be32_to_cpu(cproperty_u32[i]);
-
-			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);
-		}
-	}
+	usb251xb_get_ports_field(hub, "non-removable-ports", data->port_cnt,
+				 &hub->non_rem_dev);
 
 	hub->port_disable_sp = USB251XB_DEF_PORT_DISABLE_SELF;
-	cproperty_u32 = of_get_property(np, "sp-disabled-ports", &len);
-	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
-		for (i = 0; i < len / sizeof(u32); i++) {
-			u32 port = be32_to_cpu(cproperty_u32[i]);
-
-			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);
-		}
-	}
+	usb251xb_get_ports_field(hub, "sp-disabled-ports", data->port_cnt,
+				 &hub->port_disable_sp);
 
 	hub->port_disable_bp = USB251XB_DEF_PORT_DISABLE_BUS;
-	cproperty_u32 = of_get_property(np, "bp-disabled-ports", &len);
-	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
-		for (i = 0; i < len / sizeof(u32); i++) {
-			u32 port = be32_to_cpu(cproperty_u32[i]);
-
-			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);
-		}
-	}
+	usb251xb_get_ports_field(hub, "bp-disabled-ports", data->port_cnt,
+				 &hub->port_disable_bp);
 
 	hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
 	if (!of_property_read_u32(np, "sp-max-total-current-microamp",
@@ -546,10 +529,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	 * register controls the USB DP/DM signal swapping for each port.
 	 */
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
-	of_property_for_each_u32(np, "swap-dx-lanes", prop, p, port) {
-		if (port <= data->port_cnt)
-			hub->port_swap |= BIT(port);
-	}
+	usb251xb_get_ports_field(hub, "swap-dx-lanes", data->port_cnt,
+				 &hub->port_swap);
 
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
-- 
2.21.0

[PATCH 3/3] usb: usb251xb: Add US port lanes inversion property

[Serge Semin]

The driver bindings already declare the "swap-dx-lanes" property to
invert the downstream ports lanes polarity. The similar config
can be defined for a single upstream port - "swap-us-lanes". It's
going to be boolean since there is only one upstream port
on the hub.

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

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 4ef34df948ad..56f0a10633fc 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -531,6 +531,8 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	usb251xb_get_ports_field(hub, "swap-dx-lanes", data->port_cnt,
 				 &hub->port_swap);
+	if (of_get_property(np, "swap-us-lanes", NULL))
+		hub->port_swap |= BIT(0);
 
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
-- 
2.21.0

Re: [PATCH 1/3] usb: usb251xb: Add US lanes inversion dts-bindings

[Rob Herring]

On Thu, 25 Apr 2019 00:12:05 +0300, Serge Semin wrote:
> Since a separate US ports lanes polarity inversion property is going to
> be available the bindings doc-file should be updated with information
> about swap-us-lanes bool property, which will be responsible for it.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH 1/3] usb: usb251xb: Add US lanes inversion dts-bindings

[Richard Leitner]

On 24/04/2019 23:12, Serge Semin wrote:
> Since a separate US ports lanes polarity inversion property is going to
> be available the bindings doc-file should be updated with information
> about swap-us-lanes bool property, which will be responsible for it.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH 2/3] usb: usb251xb: Create a ports field collector method

[Richard Leitner]

On 24/04/2019 23:12, Serge Semin wrote:
> Seeing the ports field collection functionality is used four times per
> just one function, it's better to have a dedicated method performing
> the task. Note that this fix filters the port 0 out from the lanes
> swapping property the same way as it has been programmed for the rest
> multi-ports properties. But unlike the rest of ports config registers
> the BIT(0) of the Port Lanes Swap register refers to the Upstream Port
> lanes inversion. This fact hasn't been documented in the driver bindings
> nor there were any mentioning about port 0 being treated as upstream
> port. Lets then leave this fix as is for the properties unification
> and create an additional "swap-us-lanes" in the next patch.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/usb/misc/usb251xb.c | 71 ++++++++++++++-----------------------
>   1 file changed, 26 insertions(+), 45 deletions(-)
> 

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

Re: [PATCH 3/3] usb: usb251xb: Add US port lanes inversion property

[Richard Leitner]

On 24/04/2019 23:12, Serge Semin wrote:
> The driver bindings already declare the "swap-dx-lanes" property to
> invert the downstream ports lanes polarity. The similar config
> can be defined for a single upstream port - "swap-us-lanes". It's
> going to be boolean since there is only one upstream port
> on the hub.
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>   drivers/usb/misc/usb251xb.c | 2 ++
>   1 file changed, 2 insertions(+)

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