Re: [PATCH 1/1] Add driver for smsc usb251x i2c configuration

2016-08-04 Thread Rob Herring 
On Tue, Aug 02, 2016 at 04:31:54PM +0200, Fabien Lahoudere wrote:
> This driver copy the configuration of the controller EEPROM via i2c.
> Configuration information is available in Documentation/usb/usb251x.txt
> 
> Signed-off-by: Fabien Lahoudere 
> ---
>  Documentation/devicetree/bindings/usb/usb251x.txt |  27 +++
>  drivers/usb/misc/Kconfig  |   9 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/usb251x.c| 265 
> ++
>  4 files changed, 302 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb251x.txt
>  create mode 100644 drivers/usb/misc/usb251x.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251x.txt 
> b/Documentation/devicetree/bindings/usb/usb251x.txt
> new file mode 100644
> index 000..2b0863a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb251x.txt
> @@ -0,0 +1,27 @@
> +SMSC USB 2.0 Hi-Speed Hub Controller
> +
> +Required properties:
> +- compatible = "smsc,usb251x";
> +- reg = <0x2c>;
> +
> +Optional properties:
> +- smsc,usb251x-cfg-data1 : u8, set configuration data 1 (byte 0x06)
> +- smsc,usb251x-cfg-data2 : u8, set configuration data 2 (byte 0x07)
> +- smsc,usb251x-cfg-data3 : u8, set configuration data 3 (byte 0x08)
> +- smsc,usb251x-portmap12 : u8, set port mapping for ports 1 and 2 (byte 0xfb)
> +- smsc,usb251x-portmap34 : u8, set port mapping for ports 3 and 4 (byte 0xfc)
> +- smsc,usb251x-portmap56 : u8, set port mapping for ports 5 and 6 (byte 0xfd)
> +- smsc,usb251x-portmap7 : u8, set port mapping for port 7 (byte 0xfe)
> +- smsc,usb251x-status-command : u8, configure smbus behaviour (byte 0xff)

This is to override what's in the EEPROM or for when there is no EEPROM 
(or both)? 

What about the other values? Are they likely to need to be added later? 
I think I'd rather see either specific properties for specific settings 
if only a few or a single property for the whole EEPROM.

Rob
--
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/1] Add driver for smsc usb251x i2c configuration

2016-08-02 Thread Fabien Lahoudere 
This driver copy the configuration of the controller EEPROM via i2c.
Configuration information is available in Documentation/usb/usb251x.txt

Signed-off-by: Fabien Lahoudere 
---
 Documentation/devicetree/bindings/usb/usb251x.txt |  27 +++
 drivers/usb/misc/Kconfig  |   9 +
 drivers/usb/misc/Makefile |   1 +
 drivers/usb/misc/usb251x.c| 265 ++
 4 files changed, 302 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/usb251x.txt
 create mode 100644 drivers/usb/misc/usb251x.c

diff --git a/Documentation/devicetree/bindings/usb/usb251x.txt 
b/Documentation/devicetree/bindings/usb/usb251x.txt
new file mode 100644
index 000..2b0863a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb251x.txt
@@ -0,0 +1,27 @@
+SMSC USB 2.0 Hi-Speed Hub Controller
+
+Required properties:
+- compatible = "smsc,usb251x";
+- reg = <0x2c>;
+
+Optional properties:
+- smsc,usb251x-cfg-data1 : u8, set configuration data 1 (byte 0x06)
+- smsc,usb251x-cfg-data2 : u8, set configuration data 2 (byte 0x07)
+- smsc,usb251x-cfg-data3 : u8, set configuration data 3 (byte 0x08)
+- smsc,usb251x-portmap12 : u8, set port mapping for ports 1 and 2 (byte 0xfb)
+- smsc,usb251x-portmap34 : u8, set port mapping for ports 3 and 4 (byte 0xfc)
+- smsc,usb251x-portmap56 : u8, set port mapping for ports 5 and 6 (byte 0xfd)
+- smsc,usb251x-portmap7 : u8, set port mapping for port 7 (byte 0xfe)
+- smsc,usb251x-status-command : u8, configure smbus behaviour (byte 0xff)
+
+Example:
+
+   usb251x: usb251x@2c {
+   compatible = "smsc,usb251x";
+   reg = <0x2c>;
+   smsc,usb251x-cfg-data3 = <0x09>;
+   smsc,usb251x-portmap12 = <0x21>;
+   smsc,usb251x-portmap12 = <0x43>;
+   smsc,usb251x-status-command = <0x1>;
+   };
+
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index eb8f8d3..89c532f 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -240,6 +240,15 @@ config USB_HSIC_USB3503
help
  This option enables support for SMSC USB3503 HSIC to USB 2.0 Driver.
 
+config USB_USB251X
+   tristate "USB251X device configurable via I2C"
+   depends on I2C
+   help
+ This option enables support for configuring SMSC USB251X USB hub.
+This driver write the hub configuration in EEPROM via i2C. Fields can 
be
+configured through device tree.
+See Documentation/devicetree/bindings/usb/usb251x.txt
+
 config USB_LINK_LAYER_TEST
tristate "USB Link Layer Test driver"
help
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 3d79faa..dac251a 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -27,5 +27,6 @@ obj-$(CONFIG_USB_HSIC_USB3503)+= usb3503.o
 obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
 obj-$(CONFIG_UCSI) += ucsi.o
 
+obj-$(CONFIG_USB_USB251X)  += usb251x.o
 obj-$(CONFIG_USB_SISUSBVGA)+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)  += lvstest.o
diff --git a/drivers/usb/misc/usb251x.c b/drivers/usb/misc/usb251x.c
new file mode 100644
index 000..b3750fc
--- /dev/null
+++ b/drivers/usb/misc/usb251x.c
@@ -0,0 +1,265 @@
+/*
+ * drivers/usb/misc/usb251x.c
+ *
+ * driver for SMSC USB251X USB Hub
+ *
+ * Authors: Rick Bronson 
+ *  Fabien Lahoudere 
+ *
+ * Register initialization is based on code examples provided by Philips
+ * Copyright (c) 2005 Koninklijke Philips Electronics N.V.
+ *
+ * This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* registers */
+#define USB251X_VENDOR_ID_LSB 0x00
+#define USB251X_VENDOR_ID_MSB 0x01
+#define USB251X_PRODUCT_ID_LSB 0x02
+#define USB251X_PRODUCT_ID_MSB 0x03
+#define USB251X_DEVICE_ID_LSB 0x04
+#define USB251X_DEVICE_ID_MSB 0x05
+#define USB251X_CONFIGURATION_DATA_BYTE_1 0x06
+#define USB251X_CONFIGURATION_DATA_BYTE_2 0x07
+#define USB251X_CONFIGURATION_DATA_BYTE_3 0x08
+#define USB251X_NON_REMOVABLE_DEVICES 0x09
+#define USB251X_PORT_DISABLE_SELF 0x0A
+#define USB251X_PORT_DISABLE_BUS 0x0B
+#define USB251X_MAX_POWER_SELF 0x0C
+#define USB251X_MAX_POWER_BUS 0x0D
+#define USB251X_HUB_CONTROLLER_MAX_CURRENT_SELF 0x0E
+#define USB251X_HUB_CONTROLLER_MAX_CURRENT_BUS 0x0F
+#define USB251X_POWER_ON_TIME 0x10
+#define USB251X_LANGUAGE_ID_HIGH 0x11
+#define USB251X_LANGUAGE_ID_LOW 0x12
+#define USB251X_MANUFACTURER_STRING_LENGTH 0x13
+#define USB251X_PRODUCT_STRING_LENGTH 0x14
+#define USB251X_SERIAL_STRING_LENGTH 0x15
+#define USB251X_MANUFACTURER_STRING 0x16
+#define USB251X_PRODUCT_STRING

Re: [PATCH 1/1] Add driver for smsc usb251x i2c configuration

2016-08-02 Thread Mark Rutland 
Hi,

On Tue, Aug 02, 2016 at 04:31:54PM +0200, Fabien Lahoudere wrote:
> This driver copy the configuration of the controller EEPROM via i2c.
> Configuration information is available in Documentation/usb/usb251x.txt
> 
> Signed-off-by: Fabien Lahoudere 
> ---
>  Documentation/devicetree/bindings/usb/usb251x.txt |  27 +++
>  drivers/usb/misc/Kconfig  |   9 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/usb251x.c| 265 
> ++
>  4 files changed, 302 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb251x.txt
>  create mode 100644 drivers/usb/misc/usb251x.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251x.txt 
> b/Documentation/devicetree/bindings/usb/usb251x.txt
> new file mode 100644
> index 000..2b0863a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb251x.txt
> @@ -0,0 +1,27 @@
> +SMSC USB 2.0 Hi-Speed Hub Controller
> +
> +Required properties:
> +- compatible = "smsc,usb251x";

Please us specific compatible strings rather than wildcards.

> +- reg = <0x2c>;
> +
> +Optional properties:
> +- smsc,usb251x-cfg-data1 : u8, set configuration data 1 (byte 0x06)
> +- smsc,usb251x-cfg-data2 : u8, set configuration data 2 (byte 0x07)
> +- smsc,usb251x-cfg-data3 : u8, set configuration data 3 (byte 0x08)
> +- smsc,usb251x-portmap12 : u8, set port mapping for ports 1 and 2 (byte 0xfb)
> +- smsc,usb251x-portmap34 : u8, set port mapping for ports 3 and 4 (byte 0xfc)
> +- smsc,usb251x-portmap56 : u8, set port mapping for ports 5 and 6 (byte 0xfd)
> +- smsc,usb251x-portmap7 : u8, set port mapping for port 7 (byte 0xfe)
> +- smsc,usb251x-status-command : u8, configure smbus behaviour (byte 0xff)

For device tree bindings we generally shy away from encoding raw values
in this manner. I'm very much not keen on this as-is.

What exactly do these values represent?

Why must these be configured through DT? When should a dts author
provide them?

I have more comments on the representation below.

> +
> +Example:
> +
> + usb251x: usb251x@2c {
> + compatible = "smsc,usb251x";
> + reg = <0x2c>;
> + smsc,usb251x-cfg-data3 = <0x09>;
> + smsc,usb251x-portmap12 = <0x21>;
> + smsc,usb251x-portmap12 = <0x43>;
> + smsc,usb251x-status-command = <0x1>;
> + };

Above these were describes as u8 values, but here they're treated as u32
due to the lack of a /bits/ 8 prefix on the values. Trying to store them
as u8 saves no space whatsoever, given values are always padded to 32
bits.

[...]

> +static unsigned char default_init_table[USB251X_ADDR_SZ] = {
> + 0x24, 0x04, 0x14, 0x25, 0xa0, 0x0b, 0x9b, 0x20, /* 00 - 07 */
> + 0x02, 0x00, 0x00, 0x00, 0x01, 0x32, 0x01, 0x32, /* 08 - 0F */
> + 0x32, 0x00, 0x00, 4, 30, 0x00, 'S', 0x00,   /* 10 - 17 */
> + 'M', 0x00, 'S', 0x00, 'C', 0x00, 0x00, 0x00,/* 18 - 1F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 20 - 27 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 28 - 2F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 30 - 37 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 38 - 3F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 40 - 47 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 48 - 4F */
> + 0x00, 0x00, 0x00, 0x00, 'U', 0x00, 'S', 0x00,   /* 50 - 57 */
> + 'B', 0x00, ' ', 0x00, '2', 0x00, '.', 0x00, /* 58 - 5F */
> + '0', 0x00, ' ', 0x00, 'H', 0x00, 'i', 0x00, /* 60 - 67 */
> + '-', 0x00, 'S', 0x00, 'p', 0x00, 'e', 0x00, /* 68 - 6F */
> + 'e', 0x00, 'd', 0x00, ' ', 0x00, 'H', 0x00, /* 70 - 77 */
> + 'u', 0x00, 'b', 0x00, ' ', 0x00, 'C', 0x00, /* 78 - 7F */
> + 'o', 0x00, 'n', 0x00, 't', 0x00, 'r', 0x00, /* 80 - 87 */
> + 'o', 0x00, 'l', 0x00, 'l', 0x00, 'e', 0x00, /* 88 - 8F */
> + 'r', 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  /* 90 - 97 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 98 - 9F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* A0 - A7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* A8 - AF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* B0 - B7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* B8 - BF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* C0 - C7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* C8 - CF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* D0 - D7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* D8 - DF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* E0 - E7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* E8 - EF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* F0 - F7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00  /* F8 - FF */
> +};

What exactly