[PATCH v4] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
Changes v4:
- reintroduce forgotten Changelog
Changes v3:
- Improve sysfs file description as recommended by greg k-h
Changes v2:
- rename oc_count to over_current_count
- add entry to Documentation/ABI
- add detailled description to commit message
---
 Documentation/ABI/testing/sysfs-bus-usb | 10 ++
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..c702c78f24d8 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner <richard.leit...@skidata.com>
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned 32 bit value
+   which wraps to 0 after its maximum is reached.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu <baolu...@linux.intel.com>
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(_dev->dev, "over-current change\n");
+   dev_dbg(_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
_attr_connect_type.attr,
+   _attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

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

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/15/2018 10:26 AM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 16:44 +0100 schrieb Richard Leitner:
>> On 03/14/2018 04:27 PM, Oliver Neukum wrote:
>>> Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner:
>>>>
>>> Well, but it does not. Removing a redundant definition is a clear
>>> benefit. But you are not removing a definition. You are introducing
>>> a preprocessor constant. Why?
>>> What is its benefit?
>>
>> AFAIK pci_ids.h collects PCI vendor and device IDs in one single 
>> point. As the PCI vendor ID of Netlogic is used in multiple files
>> IMHO it would be a good idea to add it to pci_ids.h and furthermore
>> remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where
>> it's currently defined).
>>
>> Or am I getting things wrong?
> 
> I think so, yes. We are giving names to constants as a form
> of comment or to change them at multiple places at once and
> consistently.
> 
> So
> 
> #define XYZ_NETDEV_RESET_RETRIES  2
> 
> makes clearly sense. So does
> 
> #define XYZ_MAGIC_VALUE1  0xab4e
> 
> because it tells you that you have a magic value.
> But you will never redefine a PCI vendor ID. In fact you
> must not. And if you have a comparison like
> 
> dev->vID == 0x1234
> 
> if you change this to
> 
> dev->vID == SOME_VENDOR_ID
> 
> what good does this to you? You already knew it was a vendor ID.
> Now you can name it at a glance. So what? If you have a device
> you will have to check whether you have some OEM version. You
> will always go and check the raw number. And if you have a log
> and need to check whether the check will be true, you will have
> a number.
> Using a constant there is nothing but trouble. Yet one more grep.

Thank you for that explanation. But IMHO it was clearer with a
human-readable name in such comparisons...

For example in the following I see at the first glance which
device from which vendor is affected and I don't need any
additional comments or ID databases...
   if (pdev->vendor == PCI_VENDOR_ID_TI &&
   pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)

...but if that's not the preferred way of doing things I'm
perfectly fine with that.

Furthermore to me it sounds you are saying that the complete
pci_ids.h should be thrown over-board?!?

regards;richard.l

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

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/14/2018 04:27 PM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 14:31 +0100 schrieb Richard Leitner:
>> Hi Oliver,
>> thank you for your feedback!
>>
>> On 03/14/2018 01:17 PM, Oliver Neukum wrote:
>>> Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner:
>>>> From: Richard Leitner <richard.leit...@skidata.com>
>>>>
>>>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>>>> pci_ids.h
>>>
>>> Hi,
>>>
>>> in general, why?
>>> Does this patch generate any benefit for any developer
>>> reading the source? I don't see it. Does it cause an
>>> issue for anybody who has a log file with the nummerical
>>> ID and needs to grep for it? Yes it does.
>>
>> I'll send a v2 where I also use this definition in 
>> arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from
>> arch/mips/include/asm/netlogic/xlp-hal/iomap.h.
>>
>> Therefore it will remove this definition from the iomap.h
>> and move it to pci_ids.h
>>
>> This will IMHO be a clear benefit as it removes a redundant
>> definition.
> 
> Well, but it does not. Removing a redundant definition is a clear
> benefit. But you are not removing a definition. You are introducing
> a preprocessor constant. Why?
> What is its benefit?

AFAIK pci_ids.h collects PCI vendor and device IDs in one single 
point. As the PCI vendor ID of Netlogic is used in multiple files
IMHO it would be a good idea to add it to pci_ids.h and furthermore
remove it from arch/mips/include/asm/netlogic/xlp-hal/iomap.h (where
it's currently defined).

Or am I getting things wrong?

regards;richard.l
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

Hi Oliver,
thank you for your feedback!

On 03/14/2018 01:17 PM, Oliver Neukum wrote:
> Am Mittwoch, den 14.03.2018, 11:29 +0100 schrieb Richard Leitner:
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>> pci_ids.h
> 
> Hi,
> 
> in general, why?
> Does this patch generate any benefit for any developer
> reading the source? I don't see it. Does it cause an
> issue for anybody who has a log file with the nummerical
> ID and needs to grep for it? Yes it does.

I'll send a v2 where I also use this definition in 
arch/mips/netlogic/xlp/ instead of PCI_VENDOR_NETLOGIC from
arch/mips/include/asm/netlogic/xlp-hal/iomap.h.

Therefore it will remove this definition from the iomap.h
and move it to pci_ids.h

This will IMHO be a clear benefit as it removes a redundant
definition.

> 
> Where is the point of this patch?
> 
>   Regards
>   Oliver
> 

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

Re: [PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

On 03/14/2018 11:48 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:32AM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> Replace the hardcoded PCI vendor ID of Netlogic with a definition in
>> pci_ids.h
> 
> Why?  It's only being used in one file, so it should not be in
> pci_ids.h, right?

It's also used as PCI_VENDOR_NETLOGIC in arch/mips/netlogic/xlp/.

Should this be replaced with PCI_VENDOR_ID_NETLOGIC from pci_ids.h?

> 
> thanks,
> 
> greg k-h
> 

thank you!

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

Re: [PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

On 03/14/2018 11:49 AM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:29:33AM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
>> the harcoded values with them.
>>
>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
>> ---
>>  drivers/usb/host/pci-quirks.c | 6 --
>>  drivers/usb/host/xhci-pci.c   | 4 ++--
>>  include/linux/pci_ids.h   | 2 ++
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>

> Now this patch was fine :)
> 
> Care to redo this series?

Sure. Will send a v2 soon.

> 
> thanks,
> 
> greg k-h
> 

thank you!

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

[PATCH 1/3] usb: host: pci: use existing Intel PCI ID macros

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

Instead of the hardcoded hexadecimal PCI IDs use the existing macros
from pci_ids.h for Intel IDs.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 10 +-
 drivers/usb/host/xhci-pci.c   |  3 ++-
 include/linux/pci_ids.h   |  1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 67ad4bb6919a..4f4a9f36a68e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -858,8 +858,8 @@ static void ehci_bios_handoff(struct pci_dev *pdev,
 *
 * The HASEE E200 hangs when the semaphore is set (bugzilla #77021).
 */
-   if (pdev->vendor == 0x8086 && (pdev->device == 0x283a ||
-   pdev->device == 0x27cc)) {
+   if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+   (pdev->device == 0x283a || pdev->device == 0x27cc)) {
if (dmi_check_system(ehci_dmi_nohandoff_table))
try_handoff = 0;
}
@@ -1168,9 +1168,9 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
val = readl(base + ext_cap_offset);
 
/* Auto handoff never worked for these devices. Force it and continue */
-   if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
-   (pdev->vendor == PCI_VENDOR_ID_RENESAS
-&& pdev->device == 0x0014)) {
+   if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
+   (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
writel(val, base + ext_cap_offset);
}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 5262fa571a5d..a5bfd890190c 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -213,7 +213,8 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
pdev->device == PCI_DEVICE_ID_ASMEDIA_1042A_XHCI)
xhci->quirks |= XHCI_ASMEDIA_MODIFY_FLOWCONTROL;
 
-   if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+   if (pdev->vendor == PCI_VENDOR_ID_TI &&
+   pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
if (xhci->quirks & XHCI_RESET_ON_RESUME)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index a6b30667a331..e8d1af82a688 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -838,6 +838,7 @@
 #define PCI_DEVICE_ID_TI_XX12  0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM   0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A  0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0  0x8241
 #define PCI_DEVICE_ID_TI_1130  0xac12
 #define PCI_DEVICE_ID_TI_1031  0xac13
 #define PCI_DEVICE_ID_TI_1131  0xac15
-- 
2.11.0

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

[PATCH 2/3] usb: host: pci: introduce PCI vendor ID for Netlogic

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

Replace the hardcoded PCI vendor ID of Netlogic with a definition in
pci_ids.h

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 2 +-
 include/linux/pci_ids.h   | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 4f4a9f36a68e..39d163729b89 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1243,7 +1243,7 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
/* Skip Netlogic mips SoC's internal PCI USB controller.
 * This device does not need/support EHCI/OHCI handoff
 */
-   if (pdev->vendor == 0x184e) /* vendor Netlogic */
+   if (pdev->vendor == PCI_VENDOR_ID_NETLOGIC)
return;
if (pdev->class != PCI_CLASS_SERIAL_USB_UHCI &&
pdev->class != PCI_CLASS_SERIAL_USB_OHCI &&
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index e8d1af82a688..d23a97868dee 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -3067,4 +3067,6 @@
 
 #define PCI_VENDOR_ID_OCZ  0x1b85
 
+#define PCI_VENDOR_ID_NETLOGIC 0x184e
+
 #endif /* _LINUX_PCI_IDS_H */
-- 
2.11.0

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

[PATCH 3/3] usb: host: pci: replace hardcoded renesas PCI IDs

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

Introduce Renesas uPD72020{1,2} PCI device IDs in pci_ids.h and replace
the harcoded values with them.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 drivers/usb/host/pci-quirks.c | 6 --
 drivers/usb/host/xhci-pci.c   | 4 ++--
 include/linux/pci_ids.h   | 2 ++
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 39d163729b89..5e1ad523622e 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -1170,7 +1170,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
/* Auto handoff never worked for these devices. Force it and continue */
if ((pdev->vendor == PCI_VENDOR_ID_TI &&
 pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
-   (pdev->vendor == PCI_VENDOR_ID_RENESAS && pdev->device == 0x0014)) {
+   (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
+pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)) {
val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
writel(val, base + ext_cap_offset);
}
@@ -1282,7 +1283,8 @@ bool usb_xhci_needs_pci_reset(struct pci_dev *pdev)
 * quirk, or the system will be in a rather bad state.
 */
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   (pdev->device == 0x0014 || pdev->device == 0x0015))
+   (pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201 ||
+pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202))
return true;
 
return false;
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a5bfd890190c..a453e4c35ac7 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -189,10 +189,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
xhci->quirks |= XHCI_BROKEN_STREAMS;
}
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0014)
+   pdev->device == PCI_DEVICE_ID_RENESAS_UPD720201)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
if (pdev->vendor == PCI_VENDOR_ID_RENESAS &&
-   pdev->device == 0x0015)
+   pdev->device == PCI_DEVICE_ID_RENESAS_UPD720202)
xhci->quirks |= XHCI_RESET_ON_RESUME;
if (pdev->vendor == PCI_VENDOR_ID_VIA)
xhci->quirks |= XHCI_RESET_ON_RESUME;
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index d23a97868dee..eb52f0e9b651 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2427,6 +2427,8 @@
 #define PCI_DEVICE_ID_RENESAS_SH7763   0x0004
 #define PCI_DEVICE_ID_RENESAS_SH7785   0x0007
 #define PCI_DEVICE_ID_RENESAS_SH7786   0x0010
+#define PCI_DEVICE_ID_RENESAS_UPD7202010x0014
+#define PCI_DEVICE_ID_RENESAS_UPD7202020x0015
 
 #define PCI_VENDOR_ID_SOLARFLARE   0x1924
 #define PCI_DEVICE_ID_SOLARFLARE_SFC4000A_00x0703
-- 
2.11.0

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

[PATCH 0/3] usb: host: pci: PCI ID consolidation

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

Centralize some hardcoded PCI IDs as definitions in the global
include/linux/pci_ids.h file. This is done to reduce the amount of
scattered PCI ID definitions and hardcoded values across the kernel.

Richard Leitner (3):
  usb: host: pci: use existing Intel PCI ID macros
  usb: host: pci: introduce PCI vendor ID for Netlogic
  usb: host: pci: replace hardcoded renesas PCI IDs

 drivers/usb/host/pci-quirks.c | 16 +---
 drivers/usb/host/xhci-pci.c   |  7 ---
 include/linux/pci_ids.h   |  5 +
 3 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.11.0

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

[PATCH v3] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/ABI/testing/sysfs-bus-usb | 10 ++
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..c702c78f24d8 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,16 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner <richard.leit...@skidata.com>
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned 32 bit value
+   which wraps to 0 after its maximum is reached.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu <baolu...@linux.intel.com>
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(_dev->dev, "over-current change\n");
+   dev_dbg(_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
_attr_connect_type.attr,
+   _attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

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

Re: [PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

Hi Greg,

On 03/09/2018 06:19 PM, Greg KH wrote:
>> diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
>> b/Documentation/ABI/testing/sysfs-bus-usb
>> index 0bd731cbb50c..27020293c85b 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-usb
>> +++ b/Documentation/ABI/testing/sysfs-bus-usb
>> @@ -189,6 +189,15 @@ Description:
>>  The file will read "hotplug", "wired" and "not used" if the
>>  information is available, and "unknown" otherwise.
>>  
>> +What:   /sys/bus/usb/devices/.../(hub 
>> interface)/portX/over_current_count
>> +Date:   February 2018
>> +Contact:Richard Leitner <richard.leit...@skidata.com>
>> +Description:
>> +Most hubs are able to detect over-current situations on their
>> +ports and report them to the kernel. This attribute is to expose
>> +the number of over-current situation occurred on a specific port
>> +to user space. This file will contain an unsigned int.
> "unsigned int" is very vague :)
> 
> How about "this is a 32bit value"?
> 
> And what happens when this wraps?  Is that allowed?

Thank you for your feedback. I'll improve the description for v3!

> 
> thanks,
> 
> greg k-h

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

Re: [PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

On 02/20/2018 12:55 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Richard Leitner <d...@g0hl1n.net> writes:
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant.
>>
>> In our case we have a series of USB hardware (using the cp210x driver)
>> which communicates using a proprietary protocol. These devices sometimes
>> trigger an over-current situation on some hubs. In case of such an
>> over-current situation the USB devices offer an interface for reducing
>> the max used power. As these conditions are quite rare and imply
>> performance reductions of the device we don't want to reduce the max
>> power always.
>>
>> Therefore give user-space applications the possibility to react
>> adequately by introducing an over_current_counter in the usb port struct
>> which is exported via sysfs.
> 
> why don't you just provide more than one configuration with several
> bMaxPower fields? Then host OS should choose correct configuration based
> on power budget.

Thank you for that suggestion!
Generally speaking that would be possible. Nonetheless we
a) don't have the possibility to change the firmware or the USB 
configuration of the device.
b) in those corner-cases (where the over-current situation occurs)
the device consumes more than 500mA (which couldn't be 
configured in bMaxPower AFAIK?). Nonetheless most
hubs don't detect the over-current (AFAIK) due to
electrical tolerances of their components. Our problem are
the hubs which trigger the over-current situation (which
is fine from a USB perspective).

I know it's probably not a very nice solution, so if anybody has a better
proposal please let me know :-)

Thanks!

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

[PATCH v2] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
Tested on an i.MX6DL based board

Changes v2:
- rename oc_count to over_current_count
- add entry to Documentation/ABI
- add detailled description to commit message
---
 Documentation/ABI/testing/sysfs-bus-usb |  9 +
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..27020293c85b 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,15 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.
 
+What:  /sys/bus/usb/devices/.../(hub 
interface)/portX/over_current_count
+Date:      February 2018
+Contact:   Richard Leitner <richard.leit...@skidata.com>
+Description:
+   Most hubs are able to detect over-current situations on their
+   ports and report them to the kernel. This attribute is to expose
+   the number of over-current situation occurred on a specific port
+   to user space. This file will contain an unsigned int.
+
 What:  /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
 Date:  November 2015
 Contact:   Lu Baolu <baolu...@linux.intel.com>
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->over_current_count++;
 
-   dev_dbg(_dev->dev, "over-current change\n");
+   dev_dbg(_dev->dev, "over-current change #%u\n",
+   port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int over_current_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t over_current_count_show(struct device *dev,
+  struct device_attribute *attr, char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
_attr_connect_type.attr,
+   _attr_over_current_count.attr,
NULL,
 };
 
-- 
2.11.0

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

Re: [PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

On 02/19/2018 04:59 PM, Greg KH wrote:
> On Mon, Feb 19, 2018 at 01:01:07PM +0100, Richard Leitner wrote:
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
>>
>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> When you add/remove/modify a sysfs attribute, you always have to
> document in Documentation/ABI/

Ok. Thank you. Seems I missed that, Sorry!

>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
>>  
>>  if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>>  u16 status = 0, unused;
>> +port_dev->oc_count++;
>>  
>> -dev_dbg(_dev->dev, "over-current change\n");
>> +dev_dbg(_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_count);
>>  usb_clear_port_feature(hdev, port1,
>>  USB_PORT_FEAT_C_OVER_CURRENT);
>>  msleep(100);/* Cool down */
>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
>> index 2a700ccc868c..b5cf567bf9e2 100644
>> --- a/drivers/usb/core/hub.h
>> +++ b/drivers/usb/core/hub.h
>> @@ -100,6 +100,7 @@ struct usb_port {
>>  unsigned int is_superspeed:1;
>>  unsigned int usb3_lpm_u1_permit:1;
>>  unsigned int usb3_lpm_u2_permit:1;
>> +unsigned int oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(connect_type);
>>  
>> +static ssize_t oc_count_show(struct device *dev, struct device_attribute 
>> *attr,
>> + char *buf)
>> +{
>> +struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +return sprintf(buf, "%u\n", port_dev->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I don't see what userspace can do with this number, as there's not much
> it can do with it.

I've answered this question to Felipe already:

On 02/19/2018 03:12 PM, Richard Leitner wrote:
> In our case we have a series of USB hardware (using the cp210x driver) which
> communicates using a proprietary protocol. These devices sometimes trigger an
> over-current situation on some hubs. In case of such an over-current situation
> the USB devices offer an interface for reducing the max used power.
> As these conditions are quite rare and imply performance reductions of the
> device we don't want to use reduce the max power always.
> 
> With this patch I want to give the user-space application the possibility to
> react adequately to such over-current situations.
> 
> I hope this explains my motivation for this patch in a comprehensible way.

I'm of course always open for a better/cleaner/safer way of doing this ;-)

Thank you!

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

Re: [PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

Hi,

On 02/19/2018 02:53 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Richard Leitner <d...@g0hl1n.net> writes:
> 
>> From: Richard Leitner <richard.leit...@skidata.com>
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
> 
> relevant how? What can the application do with that knowledge?

In our case we have a series of USB hardware (using the cp210x driver) which
communicates using a proprietary protocol. These devices sometimes trigger an
over-current situation on some hubs. In case of such an over-current situation
the USB devices offer an interface for reducing the max used power.
As these conditions are quite rare and imply performance reductions of the
device we don't want to use reduce the max power always.

With this patch I want to give the user-space application the possibility to
react adequately to such over-current situations.

I hope this explains my motivation for this patch in a comprehensible way.

> 
>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
>>  
>>  if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
>>  u16 status = 0, unused;
>> +port_dev->oc_count++;
>>  
>> -dev_dbg(_dev->dev, "over-current change\n");
>> +dev_dbg(_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_count);
>>  usb_clear_port_feature(hdev, port1,
>>  USB_PORT_FEAT_C_OVER_CURRENT);
>>  msleep(100);/* Cool down */
>> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
>> index 2a700ccc868c..b5cf567bf9e2 100644
>> --- a/drivers/usb/core/hub.h
>> +++ b/drivers/usb/core/hub.h
>> @@ -100,6 +100,7 @@ struct usb_port {
>>  unsigned int is_superspeed:1;
>>  unsigned int usb3_lpm_u1_permit:1;
>>  unsigned int usb3_lpm_u2_permit:1;
>> +unsigned int oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 100644
>> --- a/drivers/usb/core/port.c
>> +++ b/drivers/usb/core/port.c
>> @@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(connect_type);
>>  
>> +static ssize_t oc_count_show(struct device *dev, struct device_attribute 
>> *attr,
>> + char *buf)
>> +{
>> +struct usb_port *port_dev = to_usb_port(dev);
>> +
>> +return sprintf(buf, "%u\n", port_dev->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I would actually spell this out human readable: over_current_count
> 

Would be fine for me.

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

[PATCH] usb: core: introduce per-port over-current counters

[Richard Leitner]

From: Richard Leitner <richard.leit...@skidata.com>

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant. Therefore
introduce a oc_counter in the usb port struct which is exported via
sysfs.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
Tested on an i.MX6DL based board.
---
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..448fba1e1827 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)
 
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+   port_dev->oc_count++;
 
-   dev_dbg(_dev->dev, "over-current change\n");
+   dev_dbg(_dev->dev, "over-current change #%u\n",
+   port_dev->oc_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100);/* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..b5cf567bf9e2 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+   unsigned int oc_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..0bfe410eb8a7 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t oc_count_show(struct device *dev, struct device_attribute *attr,
+char *buf)
+{
+   struct usb_port *port_dev = to_usb_port(dev);
+
+   return sprintf(buf, "%u\n", port_dev->oc_count);
+}
+static DEVICE_ATTR_RO(oc_count);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
  struct device_attribute *attr, char *buf)
 {
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
_attr_connect_type.attr,
+   _attr_oc_count.attr,
NULL,
 };
 
-- 
2.11.0

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

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 
---
  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",
+   _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",
+   _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",
+   _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",
+   _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", _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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.lan...@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.leit...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.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.lan...@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.leit...@skidata.com>

regards,
Richard.L
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.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  wrote:
>> On Wed, Sep 20, 2017 at 4:27 PM, Serge Semin  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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.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.lan...@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.leit...@skidata.com>

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

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 
> ---
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
> ---
>  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 linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] USB: misc: remove unneeded MODULE_VERSION() usage

[Richard Leitner]

On 07/19/2017 02:17 PM, Greg Kroah-Hartman wrote:
> MODULE_VERSION is useless for in-kernel drivers, so just remove all
> usage of it in the USB misc drivers.  Along with this, some
> DRIVER_VERSION macros were removed as they are also pointless.

...

> diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> index 91f66d68bcb7..135c91c434bf 100644
> --- a/drivers/usb/misc/usb251xb.c
> +++ b/drivers/usb/misc/usb251xb.c
> @@ -114,7 +114,6 @@
>  
>  #define DRIVER_NAME  "usb251xb"
>  #define DRIVER_DESC  "Microchip USB 2.0 Hi-Speed Hub Controller"
> -#define DRIVER_VERSION   "1.0"
>  
>  struct usb251xb {
>   struct device *dev;

As I'm pretty new to kernel development I don't know if it's
needed/wanted in this case, but if so please feel free to add

Acked-by: Richard Leitner <richard.leit...@skidata.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Microchip USB Hub Driver Harmonization

[Richard Leitner]

On 05/17/2017 06:01 PM, Krzysztof Kozlowski wrote:

On Wed, May 17, 2017 at 12:58:38PM +0200, Richard Leitner wrote:

...

1. Currently usb251xb uses i2c_smbus_*, usb3503 uses regmap_* and
usb4604 uses i2c_master_* functions for the hub configuration. What
would be the preferred solution?


regmap? It is already widely used for I2C drivers. I think most (or even
all?) new I2C drivers use regmap. It hides the real bus between common
regmap API.


Ok. Thanks for that information. Then I will go for regmap.




2. What would be a good prefix for common headers/functions/macros/etc.?
I thought of "mcusbhub"... Would that be OK? Or are there any
conventions/better proposals on that?


If you are going to develop one driver for entire family, then you could
even choose just one name. Let's say the most generic.

I don't quite understand the meaning behind "harmonizing drivers".


I'm currently evaluating if it is feasible to create one common driver 
for all USB hubs. Due to the fact there are currently only 3 hub 
types implemented mainline (the Microchip homepage lists 36 USB Hub 
products [1]) it involves quite a lot data-sheet reading ;-)


As a first step (and also the final one if implementing a common driver 
is not feasible) I thought of creating a common header/source file which 
implements all "re-useable" functions/macros/etc. Would that also be an 
accepted solution?





3. Currently only usb3503 supports "platform data". Is this still needed
or may it be removed?


I think it is still used, e.g. by:
arch/arm/boot/dts/exynos5250-spring.dts


Please correct me if I'm wrong, but isn't that DT only?

The reason why I'm asking if it may be removed is because the only file 
including "linux/platform_data/usb3503.h" is the usb3503.c driver itself 
and it's also the only file using "struct usb3503_platform_data".


Thanks & kind regards,
RichardL

[1] 
https://www.microchip.com/design-centers/usb/usb-hubs-and-devices/products/overview

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

Microchip USB Hub Driver Harmonization

[Richard Leitner]

Hello,
due to the fact (all?) the Microchip (former SMSC) USB hubs share the
same I2C configuration interface, I'm currently working on harmonizing
those USB Hub drivers. Currently this affects the usb251xb, usb3503 and
usb4604 drivers. To avoid preventable efforts (and patch versions) I
have some question on the preferred implementation:

1. Currently usb251xb uses i2c_smbus_*, usb3503 uses regmap_* and
usb4604 uses i2c_master_* functions for the hub configuration. What
would be the preferred solution?

2. What would be a good prefix for common headers/functions/macros/etc.?
I thought of "mcusbhub"... Would that be OK? Or are there any
conventions/better proposals on that?

3. Currently only usb3503 supports "platform data". Is this still needed
or may it be removed?

Thanks & kind regards,
RichardL
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/4] usb: misc: fix/improve USB251xB/xBi dt bindings

[Richard Leitner]

This patchset improves/fixes the USB251xB/xBi devicetree bindings as
recommended by Rob Herring.

CHANGES v2:
- Require exact values for oc-delay-us property
- Remove erroneous be32_to_cpu call in power-on-time-ms prop parsing
- Remove reference to inexistent file from MAINTAINERS

Richard Leitner (4):
  usb: usb251xb: remove max_{power,current}_{sp,bp} properties
  usb: usb251xb: dt: add unit suffix to oc-delay and power-on-time
  doc: dt-bindings: usb251xb: mark reg as required
  MAINTAINERS: usb251xb: remove reference inexistent file

 Documentation/devicetree/bindings/usb/usb251xb.txt | 53 +++
 MAINTAINERS|  1 -
 drivers/usb/misc/usb251xb.c| 59 +-
 3 files changed, 42 insertions(+), 71 deletions(-)

-- 
2.1.4

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

[PATCH v2 3/4] doc: dt-bindings: usb251xb: mark reg as required

[Richard Leitner]

Mark the reg property as required and furthermore fix some typos and
spellings in the documentation.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 23 +++---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 91499ae..3957d4e 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -7,18 +7,18 @@ 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"
- - hub-reset-gpios : Should specify the gpio for hub reset
+ - reset-gpios : Should specify the gpio for hub reset
+ - reg : I2C address on the selected bus (default is <0x2C>)
 
 Optional properties :
- - reg : I2C address on the selected bus (default is <0x2C>)
  - skip-config : Skip Hub configuration, but only send the USB-Attach command
- - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
- - product-id : USB Product ID of the hub (16 bit, default depends on type)
- - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
- - language-id : USB Language ID (16 bit, default is 0x)
- - manufacturer : USB Manufacturer string (max 31 characters long)
- - product : USB Product string (max 31 characters long)
- - serial : USB Serial string (max 31 characters long)
+ - 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)
+ - device-id : Set USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : Set USB Language ID (16 bit, default is 0x)
+ - 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
@@ -34,7 +34,7 @@ Optional properties :
  - 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 : indicated the hub is part of a compound device
+ - compound-device : indicate the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
  - string-support : enable string descriptor support (required for 
manufacturer,
product and serial string configuration)
@@ -49,7 +49,8 @@ Optional properties :
 Examples:
usb2512b@2c {
compatible = "microchip,usb2512b";
-   hub-reset-gpios = < 4 GPIO_ACTIVE_LOW>;
+   reg = <0x2c>;
+   reset-gpios = < 4 GPIO_ACTIVE_LOW>;
};
 
usb2514b@2c {
-- 
2.1.4

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

[PATCH v2 1/4] usb: usb251xb: remove max_{power,current}_{sp,bp} properties

[Richard Leitner]

Remove the max_{power,current}_{sp,bp} properties of the usb251xb driver
from devicetree. This is done to simplify the dt bindings as requested
by Rob Herring in https://lkml.org/lkml/2017/2/15/1283. If those
properties are ever needed by somebody they can be enabled again easily.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
Acked-by: Rob Herring <r...@kernel.org>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 20 --
 drivers/usb/misc/usb251xb.c| 24 --
 2 files changed, 4 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 0c065f7..a5efd10 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -40,26 +40,6 @@ 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
- - max-sp-power : Specifies the maximum current the hub consumes from an
-   upstream port when operating as self-powered hub including the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 2).
- - max-bp-power : Specifies the maximum current the hub consumes from an
-   upstream port when operating as bus-powered hub including the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 100).
- - max-sp-current : Specifies the maximum current the hub consumes from an
-   upstream port when operating as self-powered hub EXCLUDING the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 2).
- - max-bp-current : Specifies the maximum current the hub consumes from an
-   upstream port when operating as bus-powered hub EXCLUDING the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 100).
  - power-on-time : 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 4e18600..3f9c306 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -432,26 +432,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
}
}
 
-   hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-   if (!of_property_read_u32(np, "max-sp-power", property_u32))
-   hub->max_power_sp = min_t(u8, be32_to_cpu(*property_u32) / 2,
- 250);
-
-   hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-   if (!of_property_read_u32(np, "max-bp-power", property_u32))
-   hub->max_power_bp = min_t(u8, be32_to_cpu(*property_u32) / 2,
- 250);
-
-   hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-   if (!of_property_read_u32(np, "max-sp-current", property_u32))
-   hub->max_current_sp = min_t(u8, be32_to_cpu(*property_u32) / 2,
-   250);
-
-   hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
-   if (!of_property_read_u32(np, "max-bp-current", property_u32))
-   hub->max_current_bp = min_t(u8, be32_to_cpu(*property_u32) / 2,
-   250);
-
hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
if (!of_property_read_u32(np, "power-on-time", property_u32))
hub->power_on_time = min_t(u8, be32_to_cpu(*property_u32) / 2,
@@ -492,6 +472,10 @@ 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_x = USB251XB_DEF_BOOST_X;
-- 
2.1.4

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

[PATCH v2 2/4] usb: usb251xb: dt: add unit suffix to oc-delay and power-on-time

[Richard Leitner]

Rename oc-delay-* to oc-delay-us and make it expect a time value.
Furthermore add -ms suffix to power-on-time. There changes were
suggested by Rob Herring in https://lkml.org/lkml/2017/2/15/1283.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 10 ---
 drivers/usb/misc/usb251xb.c| 35 --
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index a5efd10..91499ae 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -31,7 +31,9 @@ Optional properties :
(default is individual)
  - dynamic-power-switching : enable auto-switching from self- to bus-powered
operation if the local power source is removed or unavailable
- - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - 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 : indicated the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
  - string-support : enable string descriptor support (required for 
manufacturer,
@@ -40,9 +42,9 @@ 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
- - power-on-time : 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).
+ - 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).
 
 Examples:
usb2512b@2c {
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 3f9c306..91f66d68 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -375,18 +375,24 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
if (of_get_property(np, "dynamic-power-switching", NULL))
hub->conf_data2 |= BIT(7);
 
-   if (of_get_property(np, "oc-delay-100us", NULL)) {
-   hub->conf_data2 &= ~BIT(5);
-   hub->conf_data2 &= ~BIT(4);
-   } else if (of_get_property(np, "oc-delay-4ms", NULL)) {
-   hub->conf_data2 &= ~BIT(5);
-   hub->conf_data2 |= BIT(4);
-   } else if (of_get_property(np, "oc-delay-8ms", NULL)) {
-   hub->conf_data2 |= BIT(5);
-   hub->conf_data2 &= ~BIT(4);
-   } else if (of_get_property(np, "oc-delay-16ms", NULL)) {
-   hub->conf_data2 |= BIT(5);
-   hub->conf_data2 |= BIT(4);
+   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) {
+   /* 4 ms */
+   hub->conf_data2 &= ~BIT(5);
+   hub->conf_data2 |= BIT(4);
+   } else if (*property_u32 == 16000) {
+   /* 16 ms */
+   hub->conf_data2 |= BIT(5);
+   hub->conf_data2 |= BIT(4);
+   } else {
+   /* 8 ms (DEFAULT) */
+   hub->conf_data2 |= BIT(5);
+   hub->conf_data2 &= ~BIT(4);
+   }
}
 
if (of_get_property(np, "compound-device", NULL))
@@ -433,9 +439,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", property_u32))
-   hub->power_on_time = min_t(u8, be32_to_cpu(*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", >lang_id, 1))
hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
-- 
2.1.4

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

[PATCH v2 4/4] MAINTAINERS: usb251xb: remove reference inexistent file

[Richard Leitner]

The platform_data header file was dropped in the merged version of the
USB251xB driver. Therefore remove its reference from the MAINTAINERS file.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index c265a5f..c776906 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8307,7 +8307,6 @@ M:    Richard Leitner <richard.leit...@skidata.com>
 L: linux-usb@vger.kernel.org
 S: Maintained
 F: drivers/usb/misc/usb251xb.c
-F: include/linux/platform_data/usb251xb.h
 F: Documentation/devicetree/bindings/usb/usb251xb.txt
 
 MICROSOFT SURFACE PRO 3 BUTTON DRIVER
-- 
2.1.4

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

Re: [PATCH v5] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/27/2017 10:48 AM, Jan Lübbe wrote:
> On Di, 2017-02-21 at 15:57 +0100, Richard Leitner wrote:
>>>>> This is a lot of properties. Are you really finding a need for all of
>>>>> them? Is this to handle h/w designers too cheap to put down the EEPROM?
>>>>> Maybe better to just define an eeprom property in the format the h/w
>>>>> expects.
>>>>
>>>>
>>>> I need about 15 of these properties. I just exposed them all to dt because 
>>>> I
>>>> thought they could be useful for somebody.
>>>>
>>>> Yes, these are a subset of the settings which can be configured via an
>>>> external EEPROM (By strapping some pins of the hub you can select if it
>>>> loads its configuration from an EEPROM or receives it via SMBus).
>>>>
>>>> My first thought was also to define only a byte array in dt, but IMHO these
>>>> options are much more readable and convenient for everybody. I'd also be
>>>> fine with removing the properties I don't need from dt. But that may lead 
>>>> to
>>>> future patches where somebody needs some of the options not already exposed
>>>> to dt.
>>>
>>> Okay. It's really a judgement call. If this is most of the settings,
>>> then it's fine. If it was only a fraction of them, then maybe we'd
>>> want to do just a byte array. Sounds like it is the former.
>>
>> In fact there are 6 more parameters available according to the
>> datasheet. So how should I proceed here? Remove the one's I'm not using
>> at the moment, leave them as they are or add the missing 6 too?
> 
> Rob, several of these properties look more like configuration rather
> than HW description ('skip-config', '*-id', 'manufacturer', 'product',
> 'serial'). Is DT the right place for this? I would expect userspace to
> provide the configuration.

Jan, on the one hand you're right, some (most) of the properties are
configuration parameters for the hub chip. On the other hand setting
these parameters (and therefore configuring the hub) avoids an
additional EEPROM chip and IMHO that's some kind of describing an
"imaginary HW". Isn't it? :-)

If DT is not the right place for it: Which userspace tool/procedure
would be appropriate for it? In my case the hub needs to be available
(in a configured state) when the initramfs gets executed.

Thanks for your feedback & regards,
Richard L
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/21/2017 03:37 PM, Rob Herring wrote:
> On Thu, Feb 16, 2017 at 12:36 AM, Richard Leitner <m...@g0hl1n.net> wrote:
>> On 02/16/2017 03:30 AM, Rob Herring wrote:
>>>
>>> On Fri, Feb 10, 2017 at 09:19:27AM +0100, Richard Leitner wrote:
>>>>
>>>> This patch adds a driver for configuration of the Microchip USB251xB/xBi
>>>> USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
>>>> configuration interface and two to four USB 2.0 downstream ports.
>>>>
>>>> Furthermore add myself as a maintainer for this driver.
>>>>
>>>> The datasheet can be found at the manufacturers website, see [1]. All
>>>> device-tree exposed configuration features have been tested on a i.MX6
>>>> platform with a USB2512B hub.
>>>>
>>>> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf
>>>>
>>>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
>>>> ---
>>>> CHANGES v5:
>>>
>>>
>>> V5 and the first I see it?
>>
>>
>> Just double-checked it, you (robh...@kernel.org) were on CC since v1.
> 
> Indeed. You just sent new versions faster than I get to them. New
> versions puts you at the back of my queue. Sending out 5 versions in a
> week is a lot.

Ooh. I'm sorry. It was just that I got feedback quite fast and had the
time to implement the changes.

BTW: I already sent a patch fixing most of your issues. Please just
ignore that, as not everything is fixed in there already. I'll send an
improved version end of this week.

> 
> [...]
> 
>>>> +
>>>> +Optional properties :
>>>> + - reg : I2C address on the selected bus (default is <0x2C>)
>>>
>>>
>>> Why is this optional?
>>
>>
>> Due to the fact the address is hardcoded insinde the chip I thought we could
>> set it to 0x2C by default if reg is not given.
> 
> That's true for pretty much every I2C chip.
> 
>> Should it be required?
> 
> Yes. The only time it would not be present is the I2C bus is not
> physically connected. In that case though, this should be a child of
> the USB host node.

Ok.

[...]

>>>
>>>> + - compound-device : indicated the hub is part of a compound device
>>>> + - port-mapping-mode : enable port mapping 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
>>>> + - max-sp-power : Specifies the maximum current the hub consumes from an
>>>> +   upstream port when operating as self-powered hub including the
>>>> power
>>>> +   consumption of a permanently attached peripheral if the hub is
>>>> +   configured as a compound device. The value is given in mA in a 0
>>>> - 500
>>>> +   range (default is 2).
>>>> + - max-bp-power : Specifies the maximum current the hub consumes from an
>>>> +   upstream port when operating as bus-powered hub including the
>>>> power
>>>> +   consumption of a permanently attached peripheral if the hub is
>>>> +   configured as a compound device. The value is given in mA in a 0
>>>> - 500
>>>> +   range (default is 100).
>>>> + - max-sp-current : Specifies the maximum current the hub consumes from
>>>> an
>>>> +   upstream port when operating as self-powered hub EXCLUDING the
>>>> power
>>>> +   consumption of a permanently attached peripheral if the hub is
>>>> +   configured as a compound device. The value is given in mA in a 0
>>>> - 500
>>>> +   range (default is 2).
>>>> + - max-bp-current : Specifies the maximum current the hub consumes from
>>>> an
>>>> +   upstream port when operating as bus-powered hub EXCLUDING the
>>>> power
>>>> +   consumption of a permanently attached peripheral if the hub is
>>>> +   configured as a compound device. The value is given in mA in a 0
>>>> - 500
>>>> +   range (default is 100).
>>>> +

[PATCH 1/3] usb: usb251xb: remove max_{power,current}_{sp,bp} properties

[Richard Leitner]

Remove the max_{power,current}_{sp,bp} properties of the usb251xb driver
from devicetree. This is done to simplify the dt bindings as requested
by Rob Herring in https://lkml.org/lkml/2017/2/15/1283. If those
properties are ever needed by somebody they can be enabled again easily.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 20 --
 drivers/usb/misc/usb251xb.c| 24 --
 2 files changed, 4 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 0c065f7..a5efd10 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -40,26 +40,6 @@ 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
- - max-sp-power : Specifies the maximum current the hub consumes from an
-   upstream port when operating as self-powered hub including the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 2).
- - max-bp-power : Specifies the maximum current the hub consumes from an
-   upstream port when operating as bus-powered hub including the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 100).
- - max-sp-current : Specifies the maximum current the hub consumes from an
-   upstream port when operating as self-powered hub EXCLUDING the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 2).
- - max-bp-current : Specifies the maximum current the hub consumes from an
-   upstream port when operating as bus-powered hub EXCLUDING the power
-   consumption of a permanently attached peripheral if the hub is
-   configured as a compound device. The value is given in mA in a 0 - 500
-   range (default is 100).
  - power-on-time : 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 4e18600..3f9c306 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -432,26 +432,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
}
}
 
-   hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
-   if (!of_property_read_u32(np, "max-sp-power", property_u32))
-   hub->max_power_sp = min_t(u8, be32_to_cpu(*property_u32) / 2,
- 250);
-
-   hub->max_power_bp = USB251XB_DEF_MAX_POWER_BUS;
-   if (!of_property_read_u32(np, "max-bp-power", property_u32))
-   hub->max_power_bp = min_t(u8, be32_to_cpu(*property_u32) / 2,
- 250);
-
-   hub->max_current_sp = USB251XB_DEF_MAX_CURRENT_SELF;
-   if (!of_property_read_u32(np, "max-sp-current", property_u32))
-   hub->max_current_sp = min_t(u8, be32_to_cpu(*property_u32) / 2,
-   250);
-
-   hub->max_current_bp = USB251XB_DEF_MAX_CURRENT_BUS;
-   if (!of_property_read_u32(np, "max-bp-current", property_u32))
-   hub->max_current_bp = min_t(u8, be32_to_cpu(*property_u32) / 2,
-   250);
-
hub->power_on_time = USB251XB_DEF_POWER_ON_TIME;
if (!of_property_read_u32(np, "power-on-time", property_u32))
hub->power_on_time = min_t(u8, be32_to_cpu(*property_u32) / 2,
@@ -492,6 +472,10 @@ 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_x = USB251XB_DEF_BOOST_X;
-- 
2.1.4

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

[PATCH 0/3] usb: misc: improve USB251xB/xBi dt bindings

[Richard Leitner]

This patchset improves/fixes the USB251xB/xBi devicetree bindings as
recommended by Rob Herring.

Richard Leitner (3):
  usb: usb251xb: remove max_{power,current}_{sp,bp} properties
  usb: usb251xb: dt: add unit suffix to oc-delay and power-on-time
  doc: dt-bindings: usb251xb: mark reg as required

 Documentation/devicetree/bindings/usb/usb251xb.txt | 52 +++-
 drivers/usb/misc/usb251xb.c| 56 +-
 2 files changed, 40 insertions(+), 68 deletions(-)

-- 
2.1.4

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

[PATCH 3/3] doc: dt-bindings: usb251xb: mark reg as required

[Richard Leitner]

Mark the reg property as required and furthermore fix some typos and
spellings in the documentation.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 23 +++---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index 4d6bd73..0408d9e 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -7,18 +7,18 @@ 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"
- - hub-reset-gpios : Should specify the gpio for hub reset
+ - reset-gpios : Should specify the gpio for hub reset
+ - reg : I2C address on the selected bus (default is <0x2C>)
 
 Optional properties :
- - reg : I2C address on the selected bus (default is <0x2C>)
  - skip-config : Skip Hub configuration, but only send the USB-Attach command
- - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
- - product-id : USB Product ID of the hub (16 bit, default depends on type)
- - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
- - language-id : USB Language ID (16 bit, default is 0x)
- - manufacturer : USB Manufacturer string (max 31 characters long)
- - product : USB Product string (max 31 characters long)
- - serial : USB Serial string (max 31 characters long)
+ - 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)
+ - device-id : Set USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : Set USB Language ID (16 bit, default is 0x)
+ - 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
@@ -33,7 +33,7 @@ Optional properties :
operation if the local power source is removed or unavailable
  - oc-delay-us : microseconds over current timer delay, valid values are 100,
4000, 8000 (default) and 16000. All other values are rounded up.
- - compound-device : indicated the hub is part of a compound device
+ - compound-device : indicate the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
  - string-support : enable string descriptor support (required for 
manufacturer,
product and serial string configuration)
@@ -48,7 +48,8 @@ Optional properties :
 Examples:
usb2512b@2c {
compatible = "microchip,usb2512b";
-   hub-reset-gpios = < 4 GPIO_ACTIVE_LOW>;
+   reg = <0x2c>;
+   reset-gpios = < 4 GPIO_ACTIVE_LOW>;
};
 
usb2514b@2c {
-- 
2.1.4

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

[PATCH 2/3] usb: usb251xb: dt: add unit suffix to oc-delay and power-on-time

[Richard Leitner]

Rename oc-delay-* to oc-delay-us and make it expect a time value.
Furthermore add -ms suffix to power-on-time. These changes were
suggested by Rob Herring in https://lkml.org/lkml/2017/2/15/1283.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  9 +++---
 drivers/usb/misc/usb251xb.c| 32 +-
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
index a5efd10..4d6bd73 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -31,7 +31,8 @@ Optional properties :
(default is individual)
  - dynamic-power-switching : enable auto-switching from self- to bus-powered
operation if the local power source is removed or unavailable
- - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - oc-delay-us : microseconds over current timer delay, valid values are 100,
+   4000, 8000 (default) and 16000. All other values are rounded up.
  - compound-device : indicated the hub is part of a compound device
  - port-mapping-mode : enable port mapping mode
  - string-support : enable string descriptor support (required for 
manufacturer,
@@ -40,9 +41,9 @@ 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
- - power-on-time : 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).
+ - 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).
 
 Examples:
usb2512b@2c {
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 3f9c306..58b887a 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -375,18 +375,24 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
if (of_get_property(np, "dynamic-power-switching", NULL))
hub->conf_data2 |= BIT(7);
 
-   if (of_get_property(np, "oc-delay-100us", NULL)) {
-   hub->conf_data2 &= ~BIT(5);
-   hub->conf_data2 &= ~BIT(4);
-   } else if (of_get_property(np, "oc-delay-4ms", NULL)) {
-   hub->conf_data2 &= ~BIT(5);
-   hub->conf_data2 |= BIT(4);
-   } else if (of_get_property(np, "oc-delay-8ms", NULL)) {
-   hub->conf_data2 |= BIT(5);
-   hub->conf_data2 &= ~BIT(4);
-   } else if (of_get_property(np, "oc-delay-16ms", NULL)) {
-   hub->conf_data2 |= BIT(5);
-   hub->conf_data2 |= BIT(4);
+   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) {
+   /* 4 ms */
+   hub->conf_data2 &= ~BIT(5);
+   hub->conf_data2 |= BIT(4);
+   } else if (*property_u32 < 8000) {
+   /* 8 ms */
+   hub->conf_data2 |= BIT(5);
+   hub->conf_data2 &= ~BIT(4);
+   } else {
+   /* 16 ms */
+   hub->conf_data2 |= BIT(5);
+   hub->conf_data2 |= BIT(4);
+   }
}
 
if (of_get_property(np, "compound-device", NULL))
@@ -433,7 +439,7 @@ 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", property_u32))
+   if (!of_property_read_u32(np, "power-on-time-ms", property_u32))
hub->power_on_time = min_t(u8, be32_to_cpu(*property_u32) / 2,
   255);
 
-- 
2.1.4

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

Re: [PATCH v5] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/16/2017 03:30 AM, Rob Herring wrote:

On Fri, Feb 10, 2017 at 09:19:27AM +0100, Richard Leitner wrote:

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
CHANGES v5:


V5 and the first I see it?


Just double-checked it, you (robh...@kernel.org) were on CC since v1.

@greg-kh: I got the notification that v5 was already applied to your usb 
tree. So how should I proceed here? Send a v6 or send a separate patch 
fixing the dt issues mentioned by robh?





- Put includes in alphabetical order
- Fix some indentations
- Replace {set,clr}_bit_in_byte with BIT() macros
- Fix multiline comments
- Use of_property_read_u32() instead of of_get_property() where possible
- Use min_t() instead of by-hand implemented if's
- Use strlcpy and ternary operator instead of strncpy in if/else
- Remove useless & improve some other outputs
- Omit i2c remove function
- Use module_i2c_driver() instead of module_{init,exit}()
CHANGES v4:
- use utf8s_to_utf16s() instead of ascii2utf16le()
- remove ascii2utf16le() in lib/string.c again
- remove changes in drivers/usb/core/hcd.c again
  (I will post a separate patch for using utf8s_to_utf16s()
  in there too)

CHANGES v3:
- move ascii2utf16le() to lib/string.c and also use it also for
ascii2desc in drivers/usb/core/hcd.c
- remove platform data support from usb251xb driver

CHANGES v2:
- fix max-{b,s}p-current property name
- add descriptor string handling from platform_data
- fix non-dt handling
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 605 +
 5 files changed, 706 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..0c065f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset


Just "reset-gpios". And you need to define the active state.


OK.




+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)


Why is this optional?


Due to the fact the address is hardcoded insinde the chip I thought we 
could set it to 0x2C by default if reg is not given.


Should it be required?




+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)


These are to set the ids or need to match what they are set too?


These are to set the VID/PID of the Hub.




+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)

[PATCH v5] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
CHANGES v5:
- Put includes in alphabetical order
- Fix some indentations
- Replace {set,clr}_bit_in_byte with BIT() macros
- Fix multiline comments
- Use of_property_read_u32() instead of of_get_property() where possible
- Use min_t() instead of by-hand implemented if's
- Use strlcpy and ternary operator instead of strncpy in if/else
- Remove useless & improve some other outputs
- Omit i2c remove function
- Use module_i2c_driver() instead of module_{init,exit}()
CHANGES v4:
- use utf8s_to_utf16s() instead of ascii2utf16le()
- remove ascii2utf16le() in lib/string.c again
- remove changes in drivers/usb/core/hcd.c again
  (I will post a separate patch for using utf8s_to_utf16s()
  in there too)

CHANGES v3:
- move ascii2utf16le() to lib/string.c and also use it also for
ascii2desc in drivers/usb/core/hcd.c
- remove platform data support from usb251xb driver

CHANGES v2:
- fix max-{b,s}p-current property name
- add descriptor string handling from platform_data
- fix non-dt handling
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 605 +
 5 files changed, 706 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..0c065f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset
+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)
+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)
+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)
+ - dynamic-power-switching : enable auto-switching from self- to bus-powered
+   operation if the local power source is removed or unavailable
+ - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - compound-device : indicated the hub is part of a compound device
+ - port-mapping-mode : enable port mapping 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
+ - max-sp-power : Specifies the maximum cur

Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/08/2017 09:16 PM, Andy Shevchenko wrote:
> On Wed, 2017-02-08 at 21:03 +0100, Richard Leitner wrote:
>> Should I keep my inline {clr,set}_bit_in_byte() 
>> functions an use BIT() in there, or delete them and use BIT()
>> directly 
>> in usb251xb_get_ofdata() ?
> 
> Does it make any sense?
> 
> Even just name of your function is longer than what it substitutes.

Well, you're right :-)
So I'll change that for v5.

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

Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/08/2017 08:20 PM, Andy Shevchenko wrote:

On Wed, 2017-02-08 at 19:45 +0100, Richard Leitner wrote:

On 02/08/2017 05:40 PM, Andy Shevchenko wrote:

On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote:

On 02/08/2017 02:59 PM, Greg KH wrote:

On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:

On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:


So the preferred solution is the BIT() stuff?


I think BIT() macro in place. Otherwise you'll need a temporary unsigned
long variable. If you already have one, then __*_bit() would work.


As I have no temporary unsigned long variable in that scope I'll go for 
the BIT() approach. Should I keep my inline {clr,set}_bit_in_byte() 
functions an use BIT() in there, or delete them and use BIT() directly 
in usb251xb_get_ofdata() ?



+static int usb251xb_get_ofdata(struct usb251xb *hub,
+  struct usb251xb_data *data)
+{
+   return 0;
+}
+#endif /* CONFIG_OF */


I don't think it's a good idea to have those ugly #ifdef.


How can it be removed?


__maybe_unused for function, device_property_*() instead of
of_property_*() calls.

Something like that. But if you are insisting this is *only* OF
available hardware or we don't care, I'll not object.


In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO those
drivers should use the same solution here. But you guys are the ones
with tons of kernel coding experience, so just say how it should be
done :-)


I'll agree with whatever Greg wants here.


Ok. So I'll wait for Gregs response before posting a v5.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/08/2017 05:40 PM, Andy Shevchenko wrote:

On Wed, 2017-02-08 at 16:17 +0100, Richard Leitner wrote:

On 02/08/2017 02:59 PM, Greg KH wrote:

On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:

On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:

From: Richard Leitner <d...@g0hl1n.net>



+#define DRIVER_NAME "usb251xb"
+#define DRIVER_DESC "Microchip USB 2.0 Hi-Speed Hub Controller"
+#define DRIVER_VERSION "1.0"


Is it my MUA, or all above indentations are broken?


What do you mean?


Should the strings be aligned, like the following?
#define DRIVER_NAME "usb251xb"
#define DRIVER_DESC "Microchip USB .."
#define DRIVER_VERSION  "1.0"


Yep, tab vs. space indentation.


Ok, will do that for v5.


Above doesn't make much sense. Why not to use


BIT(bit)


and

& ~BIT(bit)

in place?


I thought we already had functions to do this for you.  Don't write
new
ones "by hand" either wya.


Which functions do you mean? I only found set_bit() and clear_bit()
from
atomic_ops. But those operate on "unsigned long" variables. From the
documentation:
Native atomic bit operations are defined to operate
on objects aligned to the size of an "unsigned long"
C data type, and are least of that size.


__set_bit(), __clear_bit() -- non-atomic variants, but you are right,
that (unsigned long) exactly the point I didn't propose them.


So the preferred solution is the BIT() stuff?


+   /* the first data byte transferred tells the
hub how
many data
+* bytes will follow (byte count)
+*/


I'm not sure this is good formatted comment for USB subsystem.


Looks fine to me, why do you think it is incorrect?


I would do like

/*
 * The multi-line
 * comment.
 */

Capital letter, period at the end, first empty line (unlike in net
subsystem).


So what's the preferred format? Empty line at the beginning or not?

The captital letter and period looks fine. I'll apply that for v5.




+#else /* CONFIG_OF */
+static int usb251xb_get_ofdata(struct usb251xb *hub,
+  struct usb251xb_data *data)
+{
+   return 0;
+}
+#endif /* CONFIG_OF */


I don't think it's a good idea to have those ugly #ifdef.


How can it be removed?


__maybe_unused for function, device_property_*() instead of
of_property_*() calls.

Something like that. But if you are insisting this is *only* OF
available hardware or we don't care, I'll not object.


In usb3503.c and usb4604.c we have that #ifdef CONFIG_OF too. IMHO those 
drivers should use the same solution here. But you guys are the ones 
with tons of kernel coding experience, so just say how it should be done :-)


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

Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/08/2017 02:59 PM, Greg KH wrote:
> On Wed, Feb 08, 2017 at 03:21:08PM +0200, Andy Shevchenko wrote:
>> On Wed, 2017-02-08 at 09:52 +0100, Richard Leitner wrote:
>>> From: Richard Leitner <d...@g0hl1n.net>
>>
>> If you want to fix the above you have to fix your Git configuration.

My git config is fine, just cherry-picked it from a remote and forgot I
committed it from another computer with another git config ;-)
Will fix that in v5 for sure!

>>
>>
>>> This patch adds a driver for configuration of the Microchip
>>> USB251xB/xBi
>>> USB 2.0 hub controller series with USB 2.0 upstream connectivity,
>>> SMBus
>>> configuration interface and two to four USB 2.0 downstream ports.
>>>
>>> Furthermore add myself as a maintainer for this driver.
>>>
>>> The datasheet can be found at the manufacturers website, see [1]. All
>>> device-tree exposed configuration features have been tested on a i.MX6
>>> platform with a USB2512B hub.
>>
>>> +++ b/drivers/usb/misc/usb251xb.c
>>> @@ -0,0 +1,674 @@
>>
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>
>> Alphabetical order?
> 
> Ick, no, who cares, really.  It's whatever order the author wants, don't
> be so picky.

Ok :-)
But somehow you're right Andy, alphabetical order seems to look better
here (will do that in v5).

> 
>>> +#define DRIVER_NAME "usb251xb"
>>> +#define DRIVER_DESC "Microchip USB 2.0 Hi-Speed Hub Controller"
>>> +#define DRIVER_VERSION "1.0"
>>
>> Is it my MUA, or all above indentations are broken?
> 
> What do you mean?

Should the strings be aligned, like the following?
#define DRIVER_NAME "usb251xb"
#define DRIVER_DESC "Microchip USB .."
#define DRIVER_VERSION  "1.0"

> 
>>> +static inline void set_bit_in_byte(u8 bit, u8 *val)
>>> +{
>>> +   if (bit < 8)
>>> +   *val |= (1 << bit);
>>> +}
>>> +
>>> +static inline void clr_bit_in_byte(u8 bit, u8 *val)
>>> +{
>>> +   if (bit < 8)
>>> +   *val &= ~(1 << bit);
>>> +}
>>
>> Above doesn't make much sense. Why not to use
>>
>> | BIT(bit) 
>>
>> and
>>
>> & ~BIT(bit)
>>
>> in place?
> 
> I thought we already had functions to do this for you.  Don't write new
> ones "by hand" either wya.

Which functions do you mean? I only found set_bit() and clear_bit() from
atomic_ops. But those operate on "unsigned long" variables. From the
documentation:
Native atomic bit operations are defined to operate
on objects aligned to the size of an "unsigned long"
C data type, and are least of that size.

> 
>>> +   /* the first data byte transferred tells the hub how
>>> many data
>>> +* bytes will follow (byte count)
>>> +*/
>>
>> I'm not sure this is good formatted comment for USB subsystem.
> 
> Looks fine to me, why do you think it is incorrect?
> 
>>> +   /* the following parameters are currently not exposed to
>>> devicetree, but
>>> +* may be as soon as needed
>>> +*/
>>
>> Style of multi-line comment.
> 
> Nope, it's fine.
> 
>>> +#else /* CONFIG_OF */
>>> +static int usb251xb_get_ofdata(struct usb251xb *hub,
>>> +  struct usb251xb_data *data)
>>> +{
>>> +   return 0;
>>> +}
>>> +#endif /* CONFIG_OF */
>>
>> I don't think it's a good idea to have those ugly #ifdef.
> 
> How can it be removed?
> 
>>> +static int usb251xb_probe(struct usb251xb *hub)
>>> +{
>>> +   struct device *dev = hub->dev;
>>> +   struct device_node *np = dev->of_node;
>>> +   const struct of_device_id *of_id =
>>> of_match_device(usb251xb_of_match,
>>> +  dev);
>>> +   int err;
>>> +
>>
>>> +   dev_info(dev, DRIVER_DESC " " DRIVER_NAME "\n");
>>
>> Useless.
> 
> Agreed.

Ok, I will remove it in v5!

Thanks & regards,
Richard L

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

Re: [PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/08/2017 09:52 AM, Richard Leitner wrote:
> From: Richard Leitner <d...@g0hl1n.net>

Please drop/ignore that "From". This patch is from:
Richard Leitner <richard.leit...@skidata.com>

> 
> This patch adds a driver for configuration of the Microchip USB251xB/xBi
> USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
> configuration interface and two to four USB 2.0 downstream ports.
> 
> Furthermore add myself as a maintainer for this driver.
> 
> The datasheet can be found at the manufacturers website, see [1]. All
> device-tree exposed configuration features have been tested on a i.MX6
> platform with a USB2512B hub.
> 
> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf
> 
> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
> ---
> CHANGES v4:
>   - use utf8s_to_utf16s() instead of ascii2utf16le()
>   - remove ascii2utf16le() in lib/string.c again
>   - remove changes in drivers/usb/core/hcd.c again
> (I will post a separate patch for using utf8s_to_utf16s()
> in there too)
> 
> CHANGES v3:
>   - move ascii2utf16le() to lib/string.c and also use it also for
>   ascii2desc in drivers/usb/core/hcd.c
>   - remove platform data support from usb251xb driver
> 
> CHANGES v2:
>   - fix max-{b,s}p-current property name
>   - add descriptor string handling from platform_data
>   - fix non-dt handling
> ---
>  Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
>  MAINTAINERS|   8 +
>  drivers/usb/misc/Kconfig   |   9 +
>  drivers/usb/misc/Makefile  |   1 +
>  drivers/usb/misc/usb251xb.c| 674 
> +
>  5 files changed, 775 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
>  create mode 100644 drivers/usb/misc/usb251xb.c
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

From: Richard Leitner <d...@g0hl1n.net>

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
CHANGES v4:
- use utf8s_to_utf16s() instead of ascii2utf16le()
- remove ascii2utf16le() in lib/string.c again
- remove changes in drivers/usb/core/hcd.c again
  (I will post a separate patch for using utf8s_to_utf16s()
  in there too)

CHANGES v3:
- move ascii2utf16le() to lib/string.c and also use it also for
ascii2desc in drivers/usb/core/hcd.c
- remove platform data support from usb251xb driver

CHANGES v2:
- fix max-{b,s}p-current property name
- add descriptor string handling from platform_data
- fix non-dt handling
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 674 +
 5 files changed, 775 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..0c065f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset
+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)
+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)
+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)
+ - dynamic-power-switching : enable auto-switching from self- to bus-powered
+   operation if the local power source is removed or unavailable
+ - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - compound-device : indicated the hub is part of a compound device
+ - port-mapping-mode : enable port mapping 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
+ - max-sp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as bus-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   

Re: [PATCH v3 1/3] lib/string: introduce ascii2utf16le() helper

[Richard Leitner]

On 02/06/2017 05:48 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 02/06/2017 05:03 PM, Richard Leitner wrote:
> 
>> For USB string descriptors we need to convert ASCII strings to UTF16-LE.
>> Therefore make a simple helper function (based on ascii2desc from
>> drivers/usb/core/hcd.c) for that purpose.
>>
>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
> [...]
>> diff --git a/lib/string.c b/lib/string.c
>> index ed83562..a113e3e 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -952,3 +952,29 @@ char *strreplace(char *s, char old, char new)
>>  return s;
>>  }
>>  EXPORT_SYMBOL(strreplace);
>> +
>> +/**
>> + * ascii2utf16le() - Helper routine for producing UTF-16LE string
>> descriptors
>> + * @s: Null-terminated ASCII (actually ISO-8859-1) string
>> + * @buf: Buffer for UTF-16LE string
>> + * @len: Length (in bytes; may be odd) of UTF-16LE buffer.
>> + *
>> + * Return: The number of bytes filled in: 2*strlen(s) or @len,
>> whichever is less
>> + */
>> +unsigned int ascii2utf16le(char const *s, u8 *buf, unsigned int len)
>> +{
>> +unsigned int n, t = 2 * strlen(s);
>> +
>> +if (len > t)
>> +len = t;
>> +n = len;
>> +while (n--) {
>> +t = (unsigned char)*s++;
>> +*buf++ = t;
>> +if (!n--)
>> +break;
>> +*buf++ = t >> 8;
> 
>Isn't it always 0?

As I will remove this function and use utf8s_to_utf16s() instead (as
suggested by Alan Stern) IMHO this issue needs no more attention.

Nonetheless, thank you for your feedback!

> 
>> +}
>> +return len;
>> +}
>> +EXPORT_SYMBOL(ascii2utf16le);

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

Re: [PATCH v3 1/3] lib/string: introduce ascii2utf16le() helper

[Richard Leitner]

On 02/06/2017 04:40 PM, Alan Stern wrote:
> On Mon, 6 Feb 2017, Richard Leitner wrote:
>> So it would be OK to include linux/nls.h and use utf8s_to_utf16s() in
>> drivers/usb/{core/hcd.c,misc/usb251xb.c}?
> 
> Well, we already include linux/nls.h in drivers/usb/core/message.c and
> a few files under drivers/usb/gadget.  Putting it in a few more places
> shouldn't hurt.

OK. Thanks! I will change that for v4.

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

Re: [PATCH v3 1/3] lib/string: introduce ascii2utf16le() helper

[Richard Leitner]

On 02/06/2017 04:12 PM, Alan Stern wrote:
> On Mon, 6 Feb 2017, Richard Leitner wrote:
> 
>> For USB string descriptors we need to convert ASCII strings to UTF16-LE.
>> Therefore make a simple helper function (based on ascii2desc from
>> drivers/usb/core/hcd.c) for that purpose.
> 
> You know, we already have utf8s_to_utf16s() in fs/nls/nls_base.c.  
> Maybe it doesn't do exactly what you want, but it should be pretty 
> close.  Adding another helper function to do essentially the same thing 
> seems unnecessary.

Thanks for that pointer. I totally agree with you.

So it would be OK to include linux/nls.h and use utf8s_to_utf16s() in
drivers/usb/{core/hcd.c,misc/usb251xb.c}?

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

[PATCH v3 0/3] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

This patch series adds a driver for configuration of the Microchip USB251xB
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

CHANGES v3:
- move ascii2utf16le() to lib/string.c and also use it also for
ascii2desc in drivers/usb/core/hcd.c
- remove platform data support from usb251xb driver

CHANGES v2:
- fix max-{b,s}p-current property name
- add descriptor string handling from platform_data
- fix non-dt handling

Richard Leitner (3):
  lib/string: introduce ascii2utf16le() helper
  usb: core: hcd: use ascii2utf16le() in ascii2desc()
  usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/core/hcd.c |  22 +-
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 664 +
 include/linux/string.h |   1 +
 lib/string.c   |  26 +
 8 files changed, 802 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c

-- 
2.1.4

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

[PATCH v3 3/3] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 664 +
 5 files changed, 765 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..0c065f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset
+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)
+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)
+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)
+ - dynamic-power-switching : enable auto-switching from self- to bus-powered
+   operation if the local power source is removed or unavailable
+ - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - compound-device : indicated the hub is part of a compound device
+ - port-mapping-mode : enable port mapping 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
+ - max-sp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as bus-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 100).
+ - max-sp-current : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub EXCLUDING the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-current : Specifies the maximum current the hub consumes from an
+   upstream port when operating as bus-powered hub EXCLUDING the power
+   consumption of a permanently attached peripheral if the hub is
+   co

[PATCH v3 1/3] lib/string: introduce ascii2utf16le() helper

[Richard Leitner]

For USB string descriptors we need to convert ASCII strings to UTF16-LE.
Therefore make a simple helper function (based on ascii2desc from
drivers/usb/core/hcd.c) for that purpose.

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 include/linux/string.h |  1 +
 lib/string.c   | 26 ++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..48fd0c6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,7 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+unsigned int ascii2utf16le(char const *s, u8 *buf, unsigned int len);
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562..a113e3e 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -952,3 +952,29 @@ char *strreplace(char *s, char old, char new)
return s;
 }
 EXPORT_SYMBOL(strreplace);
+
+/**
+ * ascii2utf16le() - Helper routine for producing UTF-16LE string descriptors
+ * @s: Null-terminated ASCII (actually ISO-8859-1) string
+ * @buf: Buffer for UTF-16LE string
+ * @len: Length (in bytes; may be odd) of UTF-16LE buffer.
+ *
+ * Return: The number of bytes filled in: 2*strlen(s) or @len, whichever is 
less
+ */
+unsigned int ascii2utf16le(char const *s, u8 *buf, unsigned int len)
+{
+   unsigned int n, t = 2 * strlen(s);
+
+   if (len > t)
+   len = t;
+   n = len;
+   while (n--) {
+   t = (unsigned char)*s++;
+   *buf++ = t;
+   if (!n--)
+   break;
+   *buf++ = t >> 8;
+   }
+   return len;
+}
+EXPORT_SYMBOL(ascii2utf16le);
-- 
2.1.4

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

[PATCH v3 2/3] usb: core: hcd: use ascii2utf16le() in ascii2desc()

[Richard Leitner]

As string.c now provides ascii2utf16le() use it in ascii2desc() for
converting the string descriptor.

Furthermore fix checkpatch.pl issues in ascii2desc().

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 drivers/usb/core/hcd.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 479e223..9f84548 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -407,27 +407,25 @@ MODULE_PARM_DESC(authorized_default,
  * USB String descriptors can contain at most 126 characters; input
  * strings longer than that are truncated.
  */
-static unsigned
-ascii2desc(char const *s, u8 *buf, unsigned len)
+static unsigned int
+ascii2desc(char const *s, u8 *buf, unsigned int len)
 {
-   unsigned n, t = 2 + 2*strlen(s);
+   unsigned int t = 2 + 2 * strlen(s);
 
if (t > 254)
t = 254;/* Longest possible UTF string descriptor */
if (len > t)
len = t;
+   if (!len)
+   return 0;
 
t += USB_DT_STRING << 8;/* Now t is first 16 bits to store */
+   *buf++ = t;
+   if (len < 2)
+   return len;
+   *buf++ = t >> 8;
 
-   n = len;
-   while (n--) {
-   *buf++ = t;
-   if (!n--)
-   break;
-   *buf++ = t >> 8;
-   t = (unsigned char)*s++;
-   }
-   return len;
+   return ascii2utf16le(s, buf, (len - 2)) + 2;
 }
 
 /**
-- 
2.1.4

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

Re: [PATCH v2] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

On 02/05/2017 08:42 AM, Greg KH wrote:
> On Fri, Feb 03, 2017 at 11:55:24AM +0100, Richard Leitner wrote:
>> +/**
>> + * ascii2utf16le() - Helper routine for producing UTF-16LE string 
>> descriptors
>> + * @s: Null-terminated ASCII (actually ISO-8859-1) string
>> + * @buf: Buffer for UTF-16LE string
>> + * @len: Length (in bytes; may be odd) of UTF-16LE buffer.
>> + *
>> + * Return: The number of bytes filled in: 2*strlen(s) or @len, whichever is 
>> less
>> + *
>> + * Note:
>> + * The UTF-16LE USB String descriptors can contain at most 31 characters (as
>> + * specified in the datasheet); input strings longer than that are 
>> truncated.
>> + *
>> + * Based on ascii2desc from drivers/usb/core/hcd.c
>> + */
> 
> Don't we have a kernel function for this already?  If we need to export
> ascii2desc() from the USB core, we can do that, or better yet, move both
> of them to a string library function in the core part of the kernel.  We
> shouldn't have to duplicate this type of thin in an individual driver.

Ok. So I'll move the ascii2utf16le function to lib/string.c (?) and call
it from ascii2desc in drivers/usb/core/hcd.c (due to the fact ascii2desc
also prepends 2 bytes) and the USB251xB driver? Would that be OK?

> 
>> --- /dev/null
>> +++ b/include/linux/platform_data/usb251xb.h
>> @@ -0,0 +1,33 @@
> 
> Do you need a platform data structure here?  Shouldn't we be only using
> DT now?

I don't need the platform data support at all. I just added it because
it is also available in the USB3503 driver. So if it's OK for you I'd be
glad to remove it.

Thank you for your feedback, Greg!

regards,
Richard L

P.S.: A mainline-contribution-beginner question on the process:
Should I send a v3 with those changes applied now or wait for some more
feedback?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
CHANGES v2:
- fix max-{b,s}p-current property name
- add descriptor string handling from platform_data
- fix non-dt handling
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 771 +
 include/linux/platform_data/usb251xb.h |  33 +
 6 files changed, 905 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c
 create mode 100644 include/linux/platform_data/usb251xb.h

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..0c065f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset
+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)
+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)
+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)
+ - dynamic-power-switching : enable auto-switching from self- to bus-powered
+   operation if the local power source is removed or unavailable
+ - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - compound-device : indicated the hub is part of a compound device
+ - port-mapping-mode : enable port mapping 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
+ - max-sp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as bus-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 100).
+ - max-sp-current : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub EXCLUDING the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in 

Re: [RFC PATCH] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

Hi Greg,

On 02/03/2017 10:03 AM, Greg KH wrote:
> On Thu, Feb 02, 2017 at 02:44:29PM +0100, Richard Leitner wrote:
>> This patch adds a driver for configuration of the Microchip USB251xB/xBi
>> USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
>> configuration interface and two to four USB 2.0 downstream ports.
>>
>> Furthermore add myself as a maintainer for this driver.
>>
>> The datasheet can be found at the manufacturers website, see [1]. All
>> device-tree exposed configuration features have been tested on a i.MX6
>> platform with a USB2512B hub.
>>
>> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf
>>
>> Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
> 
> What is "RFC" about this?  If you don't think it's ready to be merged,
> I'll agree with that and so I've deleted it from my review queue :)

As it's my first patch which adds a new driver I thought (after reading
[1]) a "RFC" would be appropriate. Isn't it?

As stated in the commit message we tested it internally on an i.MX6
platform. Therefore, from my/our point of view, it works as expected and
should be ready to be merged (otherwise I wouldn't have submitted it to
the ML ;-) ).

So how should I proceed here? Re-send it without "RFC"? Wait for some
feedback? Something completely different?

thanks for your feedback/help!

kind regards,
Richard L

[1] https://kernelnewbies.org/PatchTipsAndTricks
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] usb: misc: add USB251xB/xBi Hi-Speed Hub Controller Driver

[Richard Leitner]

This patch adds a driver for configuration of the Microchip USB251xB/xBi
USB 2.0 hub controller series with USB 2.0 upstream connectivity, SMBus
configuration interface and two to four USB 2.0 downstream ports.

Furthermore add myself as a maintainer for this driver.

The datasheet can be found at the manufacturers website, see [1]. All
device-tree exposed configuration features have been tested on a i.MX6
platform with a USB2512B hub.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/1692C.pdf

Signed-off-by: Richard Leitner <richard.leit...@skidata.com>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt |  83 +++
 MAINTAINERS|   8 +
 drivers/usb/misc/Kconfig   |   9 +
 drivers/usb/misc/Makefile  |   1 +
 drivers/usb/misc/usb251xb.c| 757 +
 include/linux/platform_data/usb251xb.h |  33 +
 6 files changed, 891 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251xb.txt
 create mode 100644 drivers/usb/misc/usb251xb.c
 create mode 100644 include/linux/platform_data/usb251xb.h

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt 
b/Documentation/devicetree/bindings/usb/usb251xb.txt
new file mode 100644
index 000..9496b06
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -0,0 +1,83 @@
+Microchip USB 2.0 Hi-Speed Hub Controller
+
+The device node for the configuration of a Microchip USB251xB/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"
+ - hub-reset-gpios : Should specify the gpio for hub reset
+
+Optional properties :
+ - reg : I2C address on the selected bus (default is <0x2C>)
+ - skip-config : Skip Hub configuration, but only send the USB-Attach command
+ - vendor-id : USB Vendor ID of the hub (16 bit, default is 0x0424)
+ - product-id : USB Product ID of the hub (16 bit, default depends on type)
+ - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
+ - language-id : USB Language ID (16 bit, default is 0x)
+ - manufacturer : USB Manufacturer string (max 31 characters long)
+ - product : USB Product string (max 31 characters long)
+ - serial : 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
+ - {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
+ - {ganged,individual}-sensing : select over-current sense type in self-powered
+   mode (default is individual)
+ - {ganged,individual}-port-switching : select port power switching mode
+   (default is individual)
+ - dynamic-power-switching : enable auto-switching from self- to bus-powered
+   operation if the local power source is removed or unavailable
+ - oc-delay-{100us,4ms,8ms,16ms} : set over current timer delay (default is 
8ms)
+ - compound-device : indicated the hub is part of a compound device
+ - port-mapping-mode : enable port mapping 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
+ - max-sp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as bus-powered hub including the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 100).
+ - max-sp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as self-powered hub EXCLUDING the power
+   consumption of a permanently attached peripheral if the hub is
+   configured as a compound device. The value is given in mA in a 0 - 500
+   range (default is 2).
+ - max-bp-power : Specifies the maximum current the hub consumes from an
+   upstream port when operating as

[PATCH v2] usb: gadget: serial: replace {V,}DBG macro with dev_{v,}dbg

[Richard Leitner]

Replace the VDBG and DBG macro with the kernels proper debug macros
(dev_vdbg and dev_dbg) in f_acm.c, f_obex.c  f_serial.c

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
v2: - rebased on Linux v3.17-rc1
- fixed some PARENTHESIS_ALIGNMENT checkpatch.pl warnings
---
 drivers/usb/gadget/function/f_acm.c|   49 ++--
 drivers/usb/gadget/function/f_obex.c   |   28 +++---
 drivers/usb/gadget/function/f_serial.c |   19 +++--
 3 files changed, 56 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/gadget/function/f_acm.c 
b/drivers/usb/gadget/function/f_acm.c
index ab1065a..6da4685 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -313,15 +313,15 @@ static void acm_complete_set_line_coding(struct usb_ep 
*ep,
struct usb_composite_dev *cdev = acm-port.func.config-cdev;
 
if (req-status != 0) {
-   DBG(cdev, acm ttyGS%d completion, err %d\n,
-   acm-port_num, req-status);
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d completion, err %d\n,
+   acm-port_num, req-status);
return;
}
 
/* normal completion */
if (req-actual != sizeof(acm-port_line_coding)) {
-   DBG(cdev, acm ttyGS%d short resp, len %d\n,
-   acm-port_num, req-actual);
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d short resp, len %d\n,
+   acm-port_num, req-actual);
usb_ep_set_halt(ep);
} else {
struct usb_cdc_line_coding  *value = req-buf;
@@ -397,14 +397,16 @@ static int acm_setup(struct usb_function *f, const struct 
usb_ctrlrequest *ctrl)
 
default:
 invalid:
-   VDBG(cdev, invalid control req%02x.%02x v%04x i%04x l%d\n,
-   ctrl-bRequestType, ctrl-bRequest,
-   w_value, w_index, w_length);
+   dev_vdbg(cdev-gadget-dev,
+invalid control req%02x.%02x v%04x i%04x l%d\n,
+ctrl-bRequestType, ctrl-bRequest,
+w_value, w_index, w_length);
}
 
/* respond with data transfer or status phase? */
if (value = 0) {
-   DBG(cdev, acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
+   dev_dbg(cdev-gadget-dev,
+   acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
acm-port_num, ctrl-bRequestType, ctrl-bRequest,
w_value, w_index, w_length);
req-zero = 0;
@@ -428,10 +430,12 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
if (intf == acm-ctrl_id) {
if (acm-notify-driver_data) {
-   VDBG(cdev, reset acm control interface %d\n, intf);
+   dev_vdbg(cdev-gadget-dev,
+reset acm control interface %d\n, intf);
usb_ep_disable(acm-notify);
} else {
-   VDBG(cdev, init acm ctrl interface %d\n, intf);
+   dev_vdbg(cdev-gadget-dev,
+init acm ctrl interface %d\n, intf);
if (config_ep_by_speed(cdev-gadget, f, acm-notify))
return -EINVAL;
}
@@ -440,11 +444,13 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
} else if (intf == acm-data_id) {
if (acm-port.in-driver_data) {
-   DBG(cdev, reset acm ttyGS%d\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev,
+   reset acm ttyGS%d\n, acm-port_num);
gserial_disconnect(acm-port);
}
if (!acm-port.in-desc || !acm-port.out-desc) {
-   DBG(cdev, activate acm ttyGS%d\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev,
+   activate acm ttyGS%d\n, acm-port_num);
if (config_ep_by_speed(cdev-gadget, f,
   acm-port.in) ||
config_ep_by_speed(cdev-gadget, f,
@@ -467,7 +473,7 @@ static void acm_disable(struct usb_function *f)
struct f_acm*acm = func_to_acm(f);
struct usb_composite_dev *cdev = f-config-cdev;
 
-   DBG(cdev, acm ttyGS%d deactivated\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d deactivated\n, acm-port_num);
gserial_disconnect(acm-port);
usb_ep_disable(acm-notify);
acm-notify-driver_data = NULL;
@@ -537,8 +543,8 @@ static int acm_notify_serial_state(struct f_acm *acm)
 
spin_lock(acm-lock);
if (acm-notify_req) {
-   DBG(cdev, acm ttyGS%d serial state %04x\n,
-   acm-port_num, acm

[PATCH] usb: gadget: serial: remove PREFIX macro

[Richard Leitner]

Remove the ttyGS PREFIX macro from u_serial.c and replace all occurences with
the hardcoded ttyGS string.

This macro was mostly used in a few debug/warning messages and a lot of
hardcoded ttyGS existed beneath. It may have been used for renaming the
tty, but if done so most debug messages would have ignored this.

Due to the fact the usage of this PREFIX in all debug calls would have
resulted in a hard to read/grep code it is removed completely.

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
changes from RFC to PATCH:
- rebased on v3.17-rc1
- fixed some PARENTHESIS_ALIGNMENT checkpatch.pl warnings
---
 drivers/usb/gadget/function/u_serial.c |   30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/function/u_serial.c 
b/drivers/usb/gadget/function/u_serial.c
index ad0aca8..491082a 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -55,11 +55,8 @@
  * for a telephone or fax link.  And ttyGS2 might be something that just
  * needs a simple byte stream interface for some messaging protocol that
  * is managed in userspace ... OBEX, PTP, and MTP have been mentioned.
- */
-
-#define PREFIX ttyGS
-
-/*
+ *
+ *
  * gserial is the lifecycle interface, used by USB functions
  * gs_port is the I/O nexus, used by the tty driver
  * tty_struct links to the tty/filesystem framework
@@ -385,9 +382,9 @@ __acquires(port-port_lock)
list_del(req-list);
req-zero = (gs_buf_data_avail(port-port_write_buf) == 0);
 
-   pr_vdebug(PREFIX %d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
-   port-port_num, len, *((u8 *)req-buf),
-   *((u8 *)req-buf+1), *((u8 *)req-buf+2));
+   pr_vdebug(ttyGS%d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
+ port-port_num, len, *((u8 *)req-buf),
+ *((u8 *)req-buf+1), *((u8 *)req-buf+2));
 
/* Drop lock while we call out of driver; completions
 * could be issued while we do so.  Disconnection may
@@ -503,13 +500,13 @@ static void gs_rx_push(unsigned long _port)
switch (req-status) {
case -ESHUTDOWN:
disconnect = true;
-   pr_vdebug(PREFIX %d: shutdown\n, port-port_num);
+   pr_vdebug(ttyGS%d: shutdown\n, port-port_num);
break;
 
default:
/* presumably a transient fault */
-   pr_warning(PREFIX %d: unexpected RX status %d\n,
-   port-port_num, req-status);
+   pr_warn(ttyGS%d: unexpected RX status %d\n,
+   port-port_num, req-status);
/* FALLTHROUGH */
case 0:
/* normal completion */
@@ -537,9 +534,8 @@ static void gs_rx_push(unsigned long _port)
if (count != size) {
/* stop pushing; TTY layer can't handle more */
port-n_read += count;
-   pr_vdebug(PREFIX %d: rx block %d/%d\n,
-   port-port_num,
-   count, req-actual);
+   pr_vdebug(ttyGS%d: rx block %d/%d\n,
+ port-port_num, count, req-actual);
break;
}
port-n_read = 0;
@@ -569,7 +565,7 @@ static void gs_rx_push(unsigned long _port)
if (do_push)
tasklet_schedule(port-push);
else
-   pr_warning(PREFIX %d: RX not scheduled?\n,
+   pr_warn(ttyGS%d: RX not scheduled?\n,
port-port_num);
}
}
@@ -985,7 +981,7 @@ static void gs_unthrottle(struct tty_struct *tty)
 * read queue backs up enough we'll be NAKing OUT packets.
 */
tasklet_schedule(port-push);
-   pr_vdebug(PREFIX %d: unthrottle\n, port-port_num);
+   pr_vdebug(ttyGS%d: unthrottle\n, port-port_num);
}
spin_unlock_irqrestore(port-port_lock, flags);
 }
@@ -1295,7 +1291,7 @@ static int userial_init(void)
return -ENOMEM;
 
gs_tty_driver-driver_name = g_serial;
-   gs_tty_driver-name = PREFIX;
+   gs_tty_driver-name = ttyGS;
/* uses dynamically assigned dev_t values */
 
gs_tty_driver-type = TTY_DRIVER_TYPE_SERIAL;
-- 
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http

Re: [RFC] usb: gadget: serial: remove PREFIX macro

[Richard Leitner]

Hi,
this patch was lying around for some time...
are there any comments or objections on this?

regards,
richard

On Fri, 18 Jul 2014 11:39:46 +0200
Richard Leitner richard.leit...@skidata.com wrote:

 Remove the ttyGS PREFIX macro from u_serial.c and replace all occurences with
 the hardcoded ttyGS string.
 
 This macro was mostly used in a few debug/warning messages and a lot of
 hardcoded ttyGS existed beneath. It may have been used for renaming the
 tty, but if done so most debug messages would have ignored this because
 of the hardcoded strings.
 
 Due to the fact the usage of this PREFIX instead of the hardcoded strings
 in all debug calls would have resulted in a hard to read/grep code it
 is removed completely.
 
 Signed-off-by: Richard Leitner richard.leit...@skidata.com
 ---
 note: previous discussion was at https://lkml.org/lkml/2014/6/27/193
 ---
  drivers/usb/gadget/u_serial.c |   21 +
  1 file changed, 9 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
 index ad0aca8..7e7a6b0 100644
 --- a/drivers/usb/gadget/u_serial.c
 +++ b/drivers/usb/gadget/u_serial.c
 @@ -55,11 +55,8 @@
   * for a telephone or fax link.  And ttyGS2 might be something that just
   * needs a simple byte stream interface for some messaging protocol that
   * is managed in userspace ... OBEX, PTP, and MTP have been mentioned.
 - */
 -
 -#define PREFIX   ttyGS
 -
 -/*
 + *
 + *
   * gserial is the lifecycle interface, used by USB functions
   * gs_port is the I/O nexus, used by the tty driver
   * tty_struct links to the tty/filesystem framework
 @@ -385,7 +382,7 @@ __acquires(port-port_lock)
   list_del(req-list);
   req-zero = (gs_buf_data_avail(port-port_write_buf) == 0);
  
 - pr_vdebug(PREFIX %d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
 + pr_vdebug(ttyGS%d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
   port-port_num, len, *((u8 *)req-buf),
   *((u8 *)req-buf+1), *((u8 *)req-buf+2));
  
 @@ -503,12 +500,12 @@ static void gs_rx_push(unsigned long _port)
   switch (req-status) {
   case -ESHUTDOWN:
   disconnect = true;
 - pr_vdebug(PREFIX %d: shutdown\n, port-port_num);
 + pr_vdebug(ttyGS%d: shutdown\n, port-port_num);
   break;
  
   default:
   /* presumably a transient fault */
 - pr_warning(PREFIX %d: unexpected RX status %d\n,
 + pr_warn(ttyGS%d: unexpected RX status %d\n,
   port-port_num, req-status);
   /* FALLTHROUGH */
   case 0:
 @@ -537,7 +534,7 @@ static void gs_rx_push(unsigned long _port)
   if (count != size) {
   /* stop pushing; TTY layer can't handle more */
   port-n_read += count;
 - pr_vdebug(PREFIX %d: rx block %d/%d\n,
 + pr_vdebug(ttyGS%d: rx block %d/%d\n,
   port-port_num,
   count, req-actual);
   break;
 @@ -569,7 +566,7 @@ static void gs_rx_push(unsigned long _port)
   if (do_push)
   tasklet_schedule(port-push);
   else
 - pr_warning(PREFIX %d: RX not scheduled?\n,
 + pr_warn(ttyGS%d: RX not scheduled?\n,
   port-port_num);
   }
   }
 @@ -985,7 +982,7 @@ static void gs_unthrottle(struct tty_struct *tty)
* read queue backs up enough we'll be NAKing OUT packets.
*/
   tasklet_schedule(port-push);
 - pr_vdebug(PREFIX %d: unthrottle\n, port-port_num);
 + pr_vdebug(ttyGS%d: unthrottle\n, port-port_num);
   }
   spin_unlock_irqrestore(port-port_lock, flags);
  }
 @@ -1295,7 +1292,7 @@ static int userial_init(void)
   return -ENOMEM;
  
   gs_tty_driver-driver_name = g_serial;
 - gs_tty_driver-name = PREFIX;
 + gs_tty_driver-name = ttyGS;
   /* uses dynamically assigned dev_t values */
  
   gs_tty_driver-type = TTY_DRIVER_TYPE_SERIAL;
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] usb: gadget: serial: remove PREFIX macro

[Richard Leitner]

Hi,

On Mon, 11 Aug 2014 08:50:34 -0500
Felipe Balbi ba...@ti.com wrote:

 On Mon, Aug 11, 2014 at 03:04:35PM +0200, Richard Leitner wrote:
  Hi,
  this patch was lying around for some time...
  are there any comments or objections on this?
 
 you realise we're still in the middle of the merge window, right ?
 

yeah I know, but I didn't knew that I shouldn't ask for the status of my
patches during merge windows...

Sorry for that, this was one of my first patches submitted, so I hope you'll be
clement with beginners like me ;-)

I'll come back to you when the merge window is closed.

thanks  regards,
richard
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: gadget: serial: replace {V,}DBG macro with dev_{v,}dbg

[Richard Leitner]

Replace the VDBG and DBG macro with the kernels proper debug macros
(dev_vdbg and dev_dbg) in f_acm.c, f_obex.c  f_serial.c

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
 drivers/usb/gadget/f_acm.c|   41 -
 drivers/usb/gadget/f_obex.c   |   22 ++
 drivers/usb/gadget/f_serial.c |   11 +++
 3 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index ab1065a..beac376 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -313,14 +313,14 @@ static void acm_complete_set_line_coding(struct usb_ep 
*ep,
struct usb_composite_dev *cdev = acm-port.func.config-cdev;
 
if (req-status != 0) {
-   DBG(cdev, acm ttyGS%d completion, err %d\n,
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d completion, err %d\n,
acm-port_num, req-status);
return;
}
 
/* normal completion */
if (req-actual != sizeof(acm-port_line_coding)) {
-   DBG(cdev, acm ttyGS%d short resp, len %d\n,
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d short resp, len %d\n,
acm-port_num, req-actual);
usb_ep_set_halt(ep);
} else {
@@ -397,14 +397,16 @@ static int acm_setup(struct usb_function *f, const struct 
usb_ctrlrequest *ctrl)
 
default:
 invalid:
-   VDBG(cdev, invalid control req%02x.%02x v%04x i%04x l%d\n,
+   dev_vdbg(cdev-gadget-dev,
+   invalid control req%02x.%02x v%04x i%04x l%d\n,
ctrl-bRequestType, ctrl-bRequest,
w_value, w_index, w_length);
}
 
/* respond with data transfer or status phase? */
if (value = 0) {
-   DBG(cdev, acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
+   dev_dbg(cdev-gadget-dev,
+   acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
acm-port_num, ctrl-bRequestType, ctrl-bRequest,
w_value, w_index, w_length);
req-zero = 0;
@@ -428,10 +430,12 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
if (intf == acm-ctrl_id) {
if (acm-notify-driver_data) {
-   VDBG(cdev, reset acm control interface %d\n, intf);
+   dev_vdbg(cdev-gadget-dev,
+reset acm control interface %d\n, intf);
usb_ep_disable(acm-notify);
} else {
-   VDBG(cdev, init acm ctrl interface %d\n, intf);
+   dev_vdbg(cdev-gadget-dev,
+init acm ctrl interface %d\n, intf);
if (config_ep_by_speed(cdev-gadget, f, acm-notify))
return -EINVAL;
}
@@ -440,11 +444,13 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
} else if (intf == acm-data_id) {
if (acm-port.in-driver_data) {
-   DBG(cdev, reset acm ttyGS%d\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev,
+   reset acm ttyGS%d\n, acm-port_num);
gserial_disconnect(acm-port);
}
if (!acm-port.in-desc || !acm-port.out-desc) {
-   DBG(cdev, activate acm ttyGS%d\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev,
+   activate acm ttyGS%d\n, acm-port_num);
if (config_ep_by_speed(cdev-gadget, f,
   acm-port.in) ||
config_ep_by_speed(cdev-gadget, f,
@@ -467,7 +473,7 @@ static void acm_disable(struct usb_function *f)
struct f_acm*acm = func_to_acm(f);
struct usb_composite_dev *cdev = f-config-cdev;
 
-   DBG(cdev, acm ttyGS%d deactivated\n, acm-port_num);
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d deactivated\n, acm-port_num);
gserial_disconnect(acm-port);
usb_ep_disable(acm-notify);
acm-notify-driver_data = NULL;
@@ -537,8 +543,8 @@ static int acm_notify_serial_state(struct f_acm *acm)
 
spin_lock(acm-lock);
if (acm-notify_req) {
-   DBG(cdev, acm ttyGS%d serial state %04x\n,
-   acm-port_num, acm-serial_state);
+   dev_dbg(cdev-gadget-dev, acm ttyGS%d serial state %04x\n,
+   acm-port_num, acm-serial_state);
status = acm_cdc_notify(acm, USB_CDC_NOTIFY_SERIAL_STATE,
0, acm-serial_state, 
sizeof(acm-serial_state));
} else {
@@ -691,12 +697,13 @@ acm_bind(struct usb_configuration *c, struct usb_function 
*f)
if (status

[RFC] usb: gadget: serial: remove PREFIX macro

[Richard Leitner]

Remove the ttyGS PREFIX macro from u_serial.c and replace all occurences with
the hardcoded ttyGS string.

This macro was mostly used in a few debug/warning messages and a lot of
hardcoded ttyGS existed beneath. It may have been used for renaming the
tty, but if done so most debug messages would have ignored this because
of the hardcoded strings.

Due to the fact the usage of this PREFIX instead of the hardcoded strings
in all debug calls would have resulted in a hard to read/grep code it
is removed completely.

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
note: previous discussion was at https://lkml.org/lkml/2014/6/27/193
---
 drivers/usb/gadget/u_serial.c |   21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/u_serial.c b/drivers/usb/gadget/u_serial.c
index ad0aca8..7e7a6b0 100644
--- a/drivers/usb/gadget/u_serial.c
+++ b/drivers/usb/gadget/u_serial.c
@@ -55,11 +55,8 @@
  * for a telephone or fax link.  And ttyGS2 might be something that just
  * needs a simple byte stream interface for some messaging protocol that
  * is managed in userspace ... OBEX, PTP, and MTP have been mentioned.
- */
-
-#define PREFIX ttyGS
-
-/*
+ *
+ *
  * gserial is the lifecycle interface, used by USB functions
  * gs_port is the I/O nexus, used by the tty driver
  * tty_struct links to the tty/filesystem framework
@@ -385,7 +382,7 @@ __acquires(port-port_lock)
list_del(req-list);
req-zero = (gs_buf_data_avail(port-port_write_buf) == 0);
 
-   pr_vdebug(PREFIX %d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
+   pr_vdebug(ttyGS%d: tx len=%d, 0x%02x 0x%02x 0x%02x ...\n,
port-port_num, len, *((u8 *)req-buf),
*((u8 *)req-buf+1), *((u8 *)req-buf+2));
 
@@ -503,12 +500,12 @@ static void gs_rx_push(unsigned long _port)
switch (req-status) {
case -ESHUTDOWN:
disconnect = true;
-   pr_vdebug(PREFIX %d: shutdown\n, port-port_num);
+   pr_vdebug(ttyGS%d: shutdown\n, port-port_num);
break;
 
default:
/* presumably a transient fault */
-   pr_warning(PREFIX %d: unexpected RX status %d\n,
+   pr_warn(ttyGS%d: unexpected RX status %d\n,
port-port_num, req-status);
/* FALLTHROUGH */
case 0:
@@ -537,7 +534,7 @@ static void gs_rx_push(unsigned long _port)
if (count != size) {
/* stop pushing; TTY layer can't handle more */
port-n_read += count;
-   pr_vdebug(PREFIX %d: rx block %d/%d\n,
+   pr_vdebug(ttyGS%d: rx block %d/%d\n,
port-port_num,
count, req-actual);
break;
@@ -569,7 +566,7 @@ static void gs_rx_push(unsigned long _port)
if (do_push)
tasklet_schedule(port-push);
else
-   pr_warning(PREFIX %d: RX not scheduled?\n,
+   pr_warn(ttyGS%d: RX not scheduled?\n,
port-port_num);
}
}
@@ -985,7 +982,7 @@ static void gs_unthrottle(struct tty_struct *tty)
 * read queue backs up enough we'll be NAKing OUT packets.
 */
tasklet_schedule(port-push);
-   pr_vdebug(PREFIX %d: unthrottle\n, port-port_num);
+   pr_vdebug(ttyGS%d: unthrottle\n, port-port_num);
}
spin_unlock_irqrestore(port-port_lock, flags);
 }
@@ -1295,7 +1292,7 @@ static int userial_init(void)
return -ENOMEM;
 
gs_tty_driver-driver_name = g_serial;
-   gs_tty_driver-name = PREFIX;
+   gs_tty_driver-name = ttyGS;
/* uses dynamically assigned dev_t values */
 
gs_tty_driver-type = TTY_DRIVER_TYPE_SERIAL;
-- 
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

On Mon, 30 Jun 2014 07:45:43 -0700
Greg Kroah-Hartman gre...@linuxfoundation.org wrote:

 From: Of Richard Leitner
  Replaces all hardcoded ttyGS strings with the PREFIX macro.
  Due to the fact the strings are spread over different source files 
  the
  PREFIX definition is moved to u_serial.h

 Lots of changes like:
  -   DBG(cdev, acm ttyGS%d completion, err %d\n,
  -   acm-port_num, req-status);
  +   DBG(cdev, acm %s%d completion, err %d\n,
  +   PREFIX, acm-port_num, req-status);

 
 Use the proper debug macros that the kernel provides you (i.e.
 dev_dbg() and family) and then you don't need to put the string in there
 at all, the kernel will do it automatically for you, in the correct
 format, so that all userspace tools can properly track what is going on.
 
 So please remove the horrid DBG() macro entirely.
 

It may be a clumsy question, but where do I get the device struct for the ttyGS 
from? (acm-port.ioport-port_tty ?!?)
The one for the USB gadget is cdev-gadget-dev if I've seen it correctly?

Thank you for your help!

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

Re: [PATCHv2] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

On Tue, 1 Jul 2014 09:31:49 +0200
Richard Leitner richard.leit...@skidata.com wrote:

 On Mon, 30 Jun 2014 07:45:43 -0700
 Greg Kroah-Hartman gre...@linuxfoundation.org wrote:
 
  From: Of Richard Leitner
   Replaces all hardcoded ttyGS strings with the PREFIX macro.
   Due to the fact the strings are spread over different source 
   files the
   PREFIX definition is moved to u_serial.h
 
  Lots of changes like:
   - DBG(cdev, acm ttyGS%d completion, err %d\n,
   - acm-port_num, req-status);
   + DBG(cdev, acm %s%d completion, err %d\n,
   + PREFIX, acm-port_num, req-status);
 
  
  Use the proper debug macros that the kernel provides you (i.e.
  dev_dbg() and family) and then you don't need to put the string in there
  at all, the kernel will do it automatically for you, in the correct
  format, so that all userspace tools can properly track what is going on.
  
  So please remove the horrid DBG() macro entirely.
I'll prepare a patch for that...

But that doesn't solve my original problem with the hardcoded ttyGS...


 [...] where do I get the device struct for the ttyGS from?

In u_serial with something like:

-   pr_debug(gs_open: ttyGS%d (%p,%p)\n, port-port_num, tty, file);
+   pr_debug(gs_open: %s (%p,%p)\n, dev_name(tty-dev), tty, file);

Or is there a better approach?

What will be a good solution for f_acm/f_obex instead of PREFIX? Or should 
PREFIX be renamed and used?

Thanks for your help and patience!

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

Re: [PATCH] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

Hello,
thanks for your reply!

On Fri, 27 Jun 2014 10:46:28 -0700
Greg Kroah-Hartman gre...@linuxfoundation.org wrote:

 On Fri, Jun 27, 2014 at 01:37:21PM +0200, Richard Leitner wrote:
  Replace all hardcoded ttyGS strings with the PREFIX macro.
 
 Why?
Because IMHO if PREFIX is available it should be used everywhere possible.
Furthermore if you change the PREFIX the debug output wouldn't be consistent 
any more.

 
  Therefore the PREFIX definition is moved to u_serial.h.
 
 Why?
Otherwise f_acm.c, f_obex.c and f_serial.c wouldn't know it.
Isn't u_serial.h the right place for it?

 
  Furthermore the modified files are checkpatch.pl compliant now.
 
 You are doing two different things here in the same patch.  Please split
 it up into two different patches.
Ok, I'll do that.

 
 thanks,
 
 greg k-h

thanks  regards,
richard
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

Replaces all hardcoded ttyGS strings with the PREFIX macro.
Due to the fact the strings are spread over different source files the
PREFIX definition is moved to u_serial.h

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
v2: removed checkpatch.pl resovling (will be in a separate patch)
---
 drivers/usb/gadget/f_acm.c|   36 ++--
 drivers/usb/gadget/f_obex.c   |   27 ++-
 drivers/usb/gadget/f_serial.c |   10 +-
 drivers/usb/gadget/u_serial.c |   30 +++---
 drivers/usb/gadget/u_serial.h |1 +
 5 files changed, 53 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index ab1065a..c6e6457 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -313,15 +313,15 @@ static void acm_complete_set_line_coding(struct usb_ep 
*ep,
struct usb_composite_dev *cdev = acm-port.func.config-cdev;
 
if (req-status != 0) {
-   DBG(cdev, acm ttyGS%d completion, err %d\n,
-   acm-port_num, req-status);
+   DBG(cdev, acm %s%d completion, err %d\n,
+   PREFIX, acm-port_num, req-status);
return;
}
 
/* normal completion */
if (req-actual != sizeof(acm-port_line_coding)) {
-   DBG(cdev, acm ttyGS%d short resp, len %d\n,
-   acm-port_num, req-actual);
+   DBG(cdev, acm %s%d short resp, len %d\n,
+   PREFIX, acm-port_num, req-actual);
usb_ep_set_halt(ep);
} else {
struct usb_cdc_line_coding  *value = req-buf;
@@ -404,15 +404,15 @@ invalid:
 
/* respond with data transfer or status phase? */
if (value = 0) {
-   DBG(cdev, acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
-   acm-port_num, ctrl-bRequestType, ctrl-bRequest,
-   w_value, w_index, w_length);
+   DBG(cdev, acm %s%d req%02x.%02x v%04x i%04x l%d\n,
+   PREFIX, acm-port_num, ctrl-bRequestType,
+   ctrl-bRequest, w_value, w_index, w_length);
req-zero = 0;
req-length = value;
value = usb_ep_queue(cdev-gadget-ep0, req, GFP_ATOMIC);
if (value  0)
-   ERROR(cdev, acm response on ttyGS%d, err %d\n,
-   acm-port_num, value);
+   ERROR(cdev, acm response on %s%d, err %d\n,
+   PREFIX, acm-port_num, value);
}
 
/* device either stalls (value  0) or reports success */
@@ -440,11 +440,11 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
} else if (intf == acm-data_id) {
if (acm-port.in-driver_data) {
-   DBG(cdev, reset acm ttyGS%d\n, acm-port_num);
+   DBG(cdev, reset acm %s%d\n, PREFIX, acm-port_num);
gserial_disconnect(acm-port);
}
if (!acm-port.in-desc || !acm-port.out-desc) {
-   DBG(cdev, activate acm ttyGS%d\n, acm-port_num);
+   DBG(cdev, activate acm %s%d\n, PREFIX, acm-port_num);
if (config_ep_by_speed(cdev-gadget, f,
   acm-port.in) ||
config_ep_by_speed(cdev-gadget, f,
@@ -467,7 +467,7 @@ static void acm_disable(struct usb_function *f)
struct f_acm*acm = func_to_acm(f);
struct usb_composite_dev *cdev = f-config-cdev;
 
-   DBG(cdev, acm ttyGS%d deactivated\n, acm-port_num);
+   DBG(cdev, acm %s%d deactivated\n, PREFIX, acm-port_num);
gserial_disconnect(acm-port);
usb_ep_disable(acm-notify);
acm-notify-driver_data = NULL;
@@ -522,8 +522,8 @@ static int acm_cdc_notify(struct f_acm *acm, u8 type, u16 
value,
 
if (status  0) {
ERROR(acm-port.func.config-cdev,
-   acm ttyGS%d can't notify serial state, %d\n,
-   acm-port_num, status);
+   acm %s%d can't notify serial state, %d\n,
+   PREFIX, acm-port_num, status);
acm-notify_req = req;
}
 
@@ -537,8 +537,8 @@ static int acm_notify_serial_state(struct f_acm *acm)
 
spin_lock(acm-lock);
if (acm-notify_req) {
-   DBG(cdev, acm ttyGS%d serial state %04x\n,
-   acm-port_num, acm-serial_state);
+   DBG(cdev, acm %s%d serial state %04x\n,
+   PREFIX, acm-port_num, acm-serial_state);
status = acm_cdc_notify(acm, USB_CDC_NOTIFY_SERIAL_STATE,
0, acm-serial_state, 
sizeof(acm

Re: [PATCHv2] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

Hi,

On Mon, 30 Jun 2014 08:41:18 +
David Laight david.lai...@aculab.com wrote:

 From: Of Richard Leitner
  Replaces all hardcoded ttyGS strings with the PREFIX macro.
  Due to the fact the strings are spread over different source files the
  PREFIX definition is moved to u_serial.h
 
 Lots of changes like:
  -   DBG(cdev, acm ttyGS%d completion, err %d\n,
  -   acm-port_num, req-status);
  +   DBG(cdev, acm %s%d completion, err %d\n,
  +   PREFIX, acm-port_num, req-status);
 
 I'm not sure this improves the code.

Maybe you're right, the readability of the code wouldn't be better afterwards,
but as I already mentioned IMHO if there is such a macro it should be used 
everywhere or nowhere...

 Do you see a need to ever change PREFIX?

To be honest: I don't know who would ever need this (me probably not).

So the other idea is to carve it out completely?

 
 Even if it is a reasonable change, the name PREFIX is hopeless.
 It would have to be something like ACM_TTYNAME_PREFIX.

good point.

 
   David

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

Re: [PATCHv2] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

Hi,

On Mon, 30 Jun 2014 09:08:01 +
David Laight david.lai...@aculab.com wrote:

   From: Of Richard Leitner
Replaces all hardcoded ttyGS strings with the PREFIX macro.
Due to the fact the strings are spread over different source files the
PREFIX definition is moved to u_serial.h
  
   Lots of changes like:
-   DBG(cdev, acm ttyGS%d completion, err %d\n,
-   acm-port_num, req-status);
+   DBG(cdev, acm %s%d completion, err %d\n,
+   PREFIX, acm-port_num, req-status);
  
   I'm not sure this improves the code.
  
  Maybe you're right, the readability of the code wouldn't be better 
  afterwards,

but as I already mentioned IMHO if there is such a macro it should be used 
everywhere or nowhere...

 
 Indeed it will make most attempts to grep for the error message fail.

So your thought is to carve it out completely?

Are there any other opinions/ideas on that?

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

[PATCH] usb: gadget: serial: replace hardcoded ttyGS with PREFIX

[Richard Leitner]

Replace all hardcoded ttyGS strings with the PREFIX macro.
Therefore the PREFIX definition is moved to u_serial.h.

Furthermore the modified files are checkpatch.pl compliant now.

Signed-off-by: Richard Leitner richard.leit...@skidata.com
---
 drivers/usb/gadget/f_acm.c|   49 +
 drivers/usb/gadget/f_obex.c   |   27 ---
 drivers/usb/gadget/f_serial.c |   12 +-
 drivers/usb/gadget/u_serial.c |   37 ---
 drivers/usb/gadget/u_serial.h |1 +
 5 files changed, 65 insertions(+), 61 deletions(-)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
index ab1065a..6a4d836 100644
--- a/drivers/usb/gadget/f_acm.c
+++ b/drivers/usb/gadget/f_acm.c
@@ -96,11 +96,11 @@ static inline struct f_acm *port_to_acm(struct gserial *p)
 
 static struct usb_interface_assoc_descriptor
 acm_iad_descriptor = {
-   .bLength =  sizeof acm_iad_descriptor,
+   .bLength =  sizeof(acm_iad_descriptor),
.bDescriptorType =  USB_DT_INTERFACE_ASSOCIATION,
 
/* .bFirstInterface =   DYNAMIC, */
-   .bInterfaceCount =  2,  // control + data
+   .bInterfaceCount =  2,  /* control + data */
.bFunctionClass =   USB_CLASS_COMM,
.bFunctionSubClass =USB_CDC_SUBCLASS_ACM,
.bFunctionProtocol =USB_CDC_ACM_PROTO_AT_V25TER,
@@ -253,7 +253,7 @@ static struct usb_endpoint_descriptor acm_ss_out_desc = {
 };
 
 static struct usb_ss_ep_comp_descriptor acm_ss_bulk_comp_desc = {
-   .bLength =  sizeof acm_ss_bulk_comp_desc,
+   .bLength =  sizeof(acm_ss_bulk_comp_desc),
.bDescriptorType =  USB_DT_SS_ENDPOINT_COMP,
 };
 
@@ -284,7 +284,7 @@ static struct usb_descriptor_header *acm_ss_function[] = {
 static struct usb_string acm_string_defs[] = {
[ACM_CTRL_IDX].s = CDC Abstract Control Model (ACM),
[ACM_DATA_IDX].s = CDC ACM Data,
-   [ACM_IAD_IDX ].s = CDC Serial,
+   [ACM_IAD_IDX].s  = CDC Serial,
{  } /* end of list */
 };
 
@@ -313,15 +313,15 @@ static void acm_complete_set_line_coding(struct usb_ep 
*ep,
struct usb_composite_dev *cdev = acm-port.func.config-cdev;
 
if (req-status != 0) {
-   DBG(cdev, acm ttyGS%d completion, err %d\n,
-   acm-port_num, req-status);
+   DBG(cdev, acm %s%d completion, err %d\n,
+   PREFIX, acm-port_num, req-status);
return;
}
 
/* normal completion */
if (req-actual != sizeof(acm-port_line_coding)) {
-   DBG(cdev, acm ttyGS%d short resp, len %d\n,
-   acm-port_num, req-actual);
+   DBG(cdev, acm %s%d short resp, len %d\n,
+   PREFIX, acm-port_num, req-actual);
usb_ep_set_halt(ep);
} else {
struct usb_cdc_line_coding  *value = req-buf;
@@ -404,15 +404,15 @@ invalid:
 
/* respond with data transfer or status phase? */
if (value = 0) {
-   DBG(cdev, acm ttyGS%d req%02x.%02x v%04x i%04x l%d\n,
-   acm-port_num, ctrl-bRequestType, ctrl-bRequest,
-   w_value, w_index, w_length);
+   DBG(cdev, acm %s%d req%02x.%02x v%04x i%04x l%d\n,
+   PREFIX, acm-port_num, ctrl-bRequestType,
+   ctrl-bRequest, w_value, w_index, w_length);
req-zero = 0;
req-length = value;
value = usb_ep_queue(cdev-gadget-ep0, req, GFP_ATOMIC);
if (value  0)
-   ERROR(cdev, acm response on ttyGS%d, err %d\n,
-   acm-port_num, value);
+   ERROR(cdev, acm response on %s%d, err %d\n,
+   PREFIX, acm-port_num, value);
}
 
/* device either stalls (value  0) or reports success */
@@ -440,11 +440,11 @@ static int acm_set_alt(struct usb_function *f, unsigned 
intf, unsigned alt)
 
} else if (intf == acm-data_id) {
if (acm-port.in-driver_data) {
-   DBG(cdev, reset acm ttyGS%d\n, acm-port_num);
+   DBG(cdev, reset acm %s%d\n, PREFIX, acm-port_num);
gserial_disconnect(acm-port);
}
if (!acm-port.in-desc || !acm-port.out-desc) {
-   DBG(cdev, activate acm ttyGS%d\n, acm-port_num);
+   DBG(cdev, activate acm %s%d\n, PREFIX, acm-port_num);
if (config_ep_by_speed(cdev-gadget, f,
   acm-port.in) ||
config_ep_by_speed(cdev-gadget, f,
@@ -467,7 +467,7 @@ static void acm_disable(struct usb_function *f)
struct f_acm*acm = func_to_acm(f);
struct