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

[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/00001692C.pdf

Signed-off-by: Richard Leitner <richard.leitner@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 0000000..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 0x0000)
+ - 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
+	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).
+
+Examples:
+	usb2512b@2c {
+		compatible = "microchip,usb2512b";
+		hub-reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+	};
+
+	usb2514b@2c {
+		compatible = "microchip,usb2514b";
+		reg = <0x2c>;
+		reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
+		vendor-id = /bits/ 16 <0x0000>;
+		product-id = /bits/ 16 <0x0000>;
+		string-support;
+		manufacturer = "Foo";
+		product = "Foo-Bar";
+		serial = "1234567890A";
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 107c10e..ecc139b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8201,6 +8201,14 @@ F:	drivers/media/platform/atmel/atmel-isc.c
 F:	drivers/media/platform/atmel/atmel-isc-regs.h
 F:	devicetree/bindings/media/atmel-isc.txt
 
+MICROCHIP USB251XB DRIVER
+M:	Richard Leitner <richard.leitner@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
 M:	Chen Yu <yu.c.chen@intel.com>
 L:	platform-driver-x86@vger.kernel.org
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 47b3577..1d1d70d 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -233,6 +233,15 @@ config USB_EZUSB_FX2
 	  Say Y here if you need EZUSB device support.
 	  (Cypress FX/FX2/FX2LP microcontrollers)
 
+config USB_HUB_USB251XB
+	tristate "USB251XB Hub Controller Configuration Driver"
+	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.
+	  Say Y or M here if you need to configure such a device via SMBus.
+
 config USB_HSIC_USB3503
        tristate "USB3503 HSIC to USB20 Driver"
        depends on I2C
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 3d19927..f6ac6c9 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_USB_TRANCEVIBRATOR)	+= trancevibrator.o
 obj-$(CONFIG_USB_USS720)		+= uss720.o
 obj-$(CONFIG_USB_SEVSEG)		+= usbsevseg.o
 obj-$(CONFIG_USB_YUREX)			+= yurex.o
+obj-$(CONFIG_USB_HUB_USB251XB)		+= usb251xb.o
 obj-$(CONFIG_USB_HSIC_USB3503)		+= usb3503.o
 obj-$(CONFIG_USB_HSIC_USB4604)		+= usb4604.o
 obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
new file mode 100644
index 0000000..4e18600
--- /dev/null
+++ b/drivers/usb/misc/usb251xb.c
@@ -0,0 +1,605 @@
+/*
+ * Driver for Microchip USB251xB USB 2.0 Hi-Speed Hub Controller
+ * Configuration via SMBus.
+ *
+ * Copyright (c) 2017 SKIDATA AG
+ *
+ * This work is based on the USB3503 driver by Dongjin Kim and
+ * a not-accepted patch by Fabien Lahoudere, see:
+ * https://patchwork.kernel.org/patch/9257715/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/nls.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
+
+/* Internal Register Set Addresses & Default Values acc. to DS00001692C */
+#define USB251XB_ADDR_VENDOR_ID_LSB	0x00
+#define USB251XB_ADDR_VENDOR_ID_MSB	0x01
+#define USB251XB_DEF_VENDOR_ID		0x0424
+
+#define USB251XB_ADDR_PRODUCT_ID_LSB	0x02
+#define USB251XB_ADDR_PRODUCT_ID_MSB	0x03
+#define USB251XB_DEF_PRODUCT_ID_12	0x2512 /* USB2512B/12Bi */
+#define USB251XB_DEF_PRODUCT_ID_13	0x2513 /* USB2513B/13Bi */
+#define USB251XB_DEF_PRODUCT_ID_14	0x2514 /* USB2514B/14Bi */
+
+#define USB251XB_ADDR_DEVICE_ID_LSB	0x04
+#define USB251XB_ADDR_DEVICE_ID_MSB	0x05
+#define USB251XB_DEF_DEVICE_ID		0x0BB3
+
+#define USB251XB_ADDR_CONFIG_DATA_1	0x06
+#define USB251XB_DEF_CONFIG_DATA_1	0x9B
+#define USB251XB_ADDR_CONFIG_DATA_2	0x07
+#define USB251XB_DEF_CONFIG_DATA_2	0x20
+#define USB251XB_ADDR_CONFIG_DATA_3	0x08
+#define USB251XB_DEF_CONFIG_DATA_3	0x02
+
+#define USB251XB_ADDR_NON_REMOVABLE_DEVICES	0x09
+#define USB251XB_DEF_NON_REMOVABLE_DEVICES	0x00
+
+#define USB251XB_ADDR_PORT_DISABLE_SELF	0x0A
+#define USB251XB_DEF_PORT_DISABLE_SELF	0x00
+#define USB251XB_ADDR_PORT_DISABLE_BUS	0x0B
+#define USB251XB_DEF_PORT_DISABLE_BUS	0x00
+
+#define USB251XB_ADDR_MAX_POWER_SELF	0x0C
+#define USB251XB_DEF_MAX_POWER_SELF	0x01
+#define USB251XB_ADDR_MAX_POWER_BUS	0x0D
+#define USB251XB_DEF_MAX_POWER_BUS	0x32
+
+#define USB251XB_ADDR_MAX_CURRENT_SELF	0x0E
+#define USB251XB_DEF_MAX_CURRENT_SELF	0x01
+#define USB251XB_ADDR_MAX_CURRENT_BUS	0x0F
+#define USB251XB_DEF_MAX_CURRENT_BUS	0x32
+
+#define USB251XB_ADDR_POWER_ON_TIME	0x10
+#define USB251XB_DEF_POWER_ON_TIME	0x32
+
+#define USB251XB_ADDR_LANGUAGE_ID_HIGH	0x11
+#define USB251XB_ADDR_LANGUAGE_ID_LOW	0x12
+#define USB251XB_DEF_LANGUAGE_ID	0x0000
+
+#define USB251XB_STRING_BUFSIZE			62
+#define USB251XB_ADDR_MANUFACTURER_STRING_LEN	0x13
+#define USB251XB_ADDR_MANUFACTURER_STRING	0x16
+#define USB251XB_DEF_MANUFACTURER_STRING	"Microchip"
+
+#define USB251XB_ADDR_PRODUCT_STRING_LEN	0x14
+#define USB251XB_ADDR_PRODUCT_STRING		0x54
+#define USB251XB_DEF_PRODUCT_STRING		"USB251xB/xBi"
+
+#define USB251XB_ADDR_SERIAL_STRING_LEN		0x15
+#define USB251XB_ADDR_SERIAL_STRING		0x92
+#define USB251XB_DEF_SERIAL_STRING		""
+
+#define USB251XB_ADDR_BATTERY_CHARGING_ENABLE	0xD0
+#define USB251XB_DEF_BATTERY_CHARGING_ENABLE	0x00
+
+#define USB251XB_ADDR_BOOST_UP	0xF6
+#define USB251XB_DEF_BOOST_UP	0x00
+#define USB251XB_ADDR_BOOST_X	0xF8
+#define USB251XB_DEF_BOOST_X	0x00
+
+#define USB251XB_ADDR_PORT_SWAP	0xFA
+#define USB251XB_DEF_PORT_SWAP	0x00
+
+#define USB251XB_ADDR_PORT_MAP_12	0xFB
+#define USB251XB_DEF_PORT_MAP_12	0x00
+#define USB251XB_ADDR_PORT_MAP_34	0xFC
+#define USB251XB_DEF_PORT_MAP_34	0x00 /* USB2513B/i & USB2514B/i only */
+
+#define USB251XB_ADDR_STATUS_COMMAND		0xFF
+#define USB251XB_STATUS_COMMAND_SMBUS_DOWN	0x04
+#define USB251XB_STATUS_COMMAND_RESET		0x02
+#define USB251XB_STATUS_COMMAND_ATTACH		0x01
+
+#define USB251XB_I2C_REG_SZ	0x100
+#define USB251XB_I2C_WRITE_SZ	0x10
+
+#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;
+	struct i2c_client *i2c;
+	u8 skip_config;
+	int gpio_reset;
+	u16 vendor_id;
+	u16 product_id;
+	u16 device_id;
+	u8  conf_data1;
+	u8  conf_data2;
+	u8  conf_data3;
+	u8  non_rem_dev;
+	u8  port_disable_sp;
+	u8  port_disable_bp;
+	u8  max_power_sp;
+	u8  max_power_bp;
+	u8  max_current_sp;
+	u8  max_current_bp;
+	u8  power_on_time;
+	u16 lang_id;
+	u8 manufacturer_len;
+	u8 product_len;
+	u8 serial_len;
+	char manufacturer[USB251XB_STRING_BUFSIZE];
+	char product[USB251XB_STRING_BUFSIZE];
+	char serial[USB251XB_STRING_BUFSIZE];
+	u8  bat_charge_en;
+	u8  boost_up;
+	u8  boost_x;
+	u8  port_swap;
+	u8  port_map12;
+	u8  port_map34;
+	u8  status;
+};
+
+struct usb251xb_data {
+	u16 product_id;
+	char product_str[USB251XB_STRING_BUFSIZE / 2]; /* ASCII string */
+};
+
+static const struct usb251xb_data usb2512b_data = {
+	.product_id = 0x2512,
+	.product_str = "USB2512B",
+};
+
+static const struct usb251xb_data usb2512bi_data = {
+	.product_id = 0x2512,
+	.product_str = "USB2512Bi",
+};
+
+static const struct usb251xb_data usb2513b_data = {
+	.product_id = 0x2513,
+	.product_str = "USB2513B",
+};
+
+static const struct usb251xb_data usb2513bi_data = {
+	.product_id = 0x2513,
+	.product_str = "USB2513Bi",
+};
+
+static const struct usb251xb_data usb2514b_data = {
+	.product_id = 0x2514,
+	.product_str = "USB2514B",
+};
+
+static const struct usb251xb_data usb2514bi_data = {
+	.product_id = 0x2514,
+	.product_str = "USB2514Bi",
+};
+
+static void usb251xb_reset(struct usb251xb *hub, int state)
+{
+	if (!gpio_is_valid(hub->gpio_reset))
+		return;
+
+	gpio_set_value_cansleep(hub->gpio_reset, state);
+
+	/* wait for hub recovery/stabilization */
+	if (state)
+		usleep_range(500, 750);	/* >=500us at power on */
+	else
+		usleep_range(1, 10);	/* >=1us at power down */
+}
+
+static int usb251xb_connect(struct usb251xb *hub)
+{
+	struct device *dev = hub->dev;
+	int err, i;
+	char i2c_wb[USB251XB_I2C_REG_SZ];
+
+	memset(i2c_wb, 0, USB251XB_I2C_REG_SZ);
+
+	if (hub->skip_config) {
+		dev_info(dev, "Skip hub configuration, only attach.\n");
+		i2c_wb[0] = 0x01;
+		i2c_wb[1] = USB251XB_STATUS_COMMAND_ATTACH;
+
+		usb251xb_reset(hub, 1);
+
+		err = i2c_smbus_write_i2c_block_data(hub->i2c,
+				USB251XB_ADDR_STATUS_COMMAND, 2, i2c_wb);
+		if (err) {
+			dev_err(dev, "attaching hub failed: %d\n", err);
+			return err;
+		}
+		return 0;
+	}
+
+	i2c_wb[USB251XB_ADDR_VENDOR_ID_MSB]     = (hub->vendor_id >> 8) & 0xFF;
+	i2c_wb[USB251XB_ADDR_VENDOR_ID_LSB]     = hub->vendor_id & 0xFF;
+	i2c_wb[USB251XB_ADDR_PRODUCT_ID_MSB]    = (hub->product_id >> 8) & 0xFF;
+	i2c_wb[USB251XB_ADDR_PRODUCT_ID_LSB]    = hub->product_id & 0xFF;
+	i2c_wb[USB251XB_ADDR_DEVICE_ID_MSB]     = (hub->device_id >> 8) & 0xFF;
+	i2c_wb[USB251XB_ADDR_DEVICE_ID_LSB]     = hub->device_id & 0xFF;
+	i2c_wb[USB251XB_ADDR_CONFIG_DATA_1]     = hub->conf_data1;
+	i2c_wb[USB251XB_ADDR_CONFIG_DATA_2]     = hub->conf_data2;
+	i2c_wb[USB251XB_ADDR_CONFIG_DATA_3]     = hub->conf_data3;
+	i2c_wb[USB251XB_ADDR_NON_REMOVABLE_DEVICES] = hub->non_rem_dev;
+	i2c_wb[USB251XB_ADDR_PORT_DISABLE_SELF] = hub->port_disable_sp;
+	i2c_wb[USB251XB_ADDR_PORT_DISABLE_BUS]  = hub->port_disable_bp;
+	i2c_wb[USB251XB_ADDR_MAX_POWER_SELF]    = hub->max_power_sp;
+	i2c_wb[USB251XB_ADDR_MAX_POWER_BUS]     = hub->max_power_bp;
+	i2c_wb[USB251XB_ADDR_MAX_CURRENT_SELF]  = hub->max_current_sp;
+	i2c_wb[USB251XB_ADDR_MAX_CURRENT_BUS]   = hub->max_current_bp;
+	i2c_wb[USB251XB_ADDR_POWER_ON_TIME]     = hub->power_on_time;
+	i2c_wb[USB251XB_ADDR_LANGUAGE_ID_HIGH]  = (hub->lang_id >> 8) & 0xFF;
+	i2c_wb[USB251XB_ADDR_LANGUAGE_ID_LOW]   = hub->lang_id & 0xFF;
+	i2c_wb[USB251XB_ADDR_MANUFACTURER_STRING_LEN] = hub->manufacturer_len;
+	i2c_wb[USB251XB_ADDR_PRODUCT_STRING_LEN]      = hub->product_len;
+	i2c_wb[USB251XB_ADDR_SERIAL_STRING_LEN]       = hub->serial_len;
+	memcpy(&i2c_wb[USB251XB_ADDR_MANUFACTURER_STRING], hub->manufacturer,
+	       USB251XB_STRING_BUFSIZE);
+	memcpy(&i2c_wb[USB251XB_ADDR_SERIAL_STRING], hub->serial,
+	       USB251XB_STRING_BUFSIZE);
+	memcpy(&i2c_wb[USB251XB_ADDR_PRODUCT_STRING], hub->product,
+	       USB251XB_STRING_BUFSIZE);
+	i2c_wb[USB251XB_ADDR_BATTERY_CHARGING_ENABLE] = hub->bat_charge_en;
+	i2c_wb[USB251XB_ADDR_BOOST_UP]          = hub->boost_up;
+	i2c_wb[USB251XB_ADDR_BOOST_X]           = hub->boost_x;
+	i2c_wb[USB251XB_ADDR_PORT_SWAP]         = hub->port_swap;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_12]       = hub->port_map12;
+	i2c_wb[USB251XB_ADDR_PORT_MAP_34]       = hub->port_map34;
+	i2c_wb[USB251XB_ADDR_STATUS_COMMAND] = USB251XB_STATUS_COMMAND_ATTACH;
+
+	usb251xb_reset(hub, 1);
+
+	/* write registers */
+	for (i = 0; i < (USB251XB_I2C_REG_SZ / USB251XB_I2C_WRITE_SZ); i++) {
+		int offset = i * USB251XB_I2C_WRITE_SZ;
+		char wbuf[USB251XB_I2C_WRITE_SZ + 1];
+
+		/* The first data byte transferred tells the hub how many data
+		 * bytes will follow (byte count).
+		 */
+		wbuf[0] = USB251XB_I2C_WRITE_SZ;
+		memcpy(&wbuf[1], &i2c_wb[offset], USB251XB_I2C_WRITE_SZ);
+
+		dev_dbg(dev, "writing %d byte block %d to 0x%02X\n",
+			USB251XB_I2C_WRITE_SZ, i, offset);
+
+		err = i2c_smbus_write_i2c_block_data(hub->i2c, offset,
+						     USB251XB_I2C_WRITE_SZ + 1,
+						     wbuf);
+		if (err)
+			goto out_err;
+	}
+
+	dev_info(dev, "Hub configuration was successful.\n");
+	return 0;
+
+out_err:
+	dev_err(dev, "configuring block %d failed: %d\n", i, err);
+	return err;
+}
+
+#ifdef CONFIG_OF
+static int usb251xb_get_ofdata(struct usb251xb *hub,
+			       struct usb251xb_data *data)
+{
+	struct device *dev = hub->dev;
+	struct device_node *np = dev->of_node;
+	int len, err, i;
+	u32 *property_u32 = NULL;
+	const u32 *cproperty_u32;
+	const char *cproperty_char;
+	char str[USB251XB_STRING_BUFSIZE / 2];
+
+	if (!np) {
+		dev_err(dev, "failed to get ofdata\n");
+		return -ENODEV;
+	}
+
+	if (of_get_property(np, "skip-config", NULL))
+		hub->skip_config = 1;
+	else
+		hub->skip_config = 0;
+
+	hub->gpio_reset = of_get_named_gpio(np, "reset-gpios", 0);
+	if (hub->gpio_reset == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (gpio_is_valid(hub->gpio_reset)) {
+		err = devm_gpio_request_one(dev, hub->gpio_reset,
+					    GPIOF_OUT_INIT_LOW,
+					    "usb251xb reset");
+		if (err) {
+			dev_err(dev,
+				"unable to request GPIO %d as reset pin (%d)\n",
+				hub->gpio_reset, err);
+			return err;
+		}
+	}
+
+	if (of_property_read_u16_array(np, "vendor-id", &hub->vendor_id, 1))
+		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
+
+	if (of_property_read_u16_array(np, "product-id",
+				       &hub->product_id, 1))
+		hub->product_id = data->product_id;
+
+	if (of_property_read_u16_array(np, "device-id", &hub->device_id, 1))
+		hub->device_id = USB251XB_DEF_DEVICE_ID;
+
+	hub->conf_data1 = USB251XB_DEF_CONFIG_DATA_1;
+	if (of_get_property(np, "self-powered", NULL)) {
+		hub->conf_data1 |= BIT(7);
+
+		/* Configure Over-Current sens when self-powered */
+		hub->conf_data1 &= ~BIT(2);
+		if (of_get_property(np, "ganged-sensing", NULL))
+			hub->conf_data1 &= ~BIT(1);
+		else if (of_get_property(np, "individual-sensing", NULL))
+			hub->conf_data1 |= BIT(1);
+	} else if (of_get_property(np, "bus-powered", NULL)) {
+		hub->conf_data1 &= ~BIT(7);
+
+		/* Disable Over-Current sense when bus-powered */
+		hub->conf_data1 |= BIT(2);
+	}
+
+	if (of_get_property(np, "disable-hi-speed", NULL))
+		hub->conf_data1 |= BIT(5);
+
+	if (of_get_property(np, "multi-tt", NULL))
+		hub->conf_data1 |= BIT(4);
+	else if (of_get_property(np, "single-tt", NULL))
+		hub->conf_data1 &= ~BIT(4);
+
+	if (of_get_property(np, "disable-eop", NULL))
+		hub->conf_data1 |= BIT(3);
+
+	if (of_get_property(np, "individual-port-switching", NULL))
+		hub->conf_data1 |= BIT(0);
+	else if (of_get_property(np, "ganged-port-switching", NULL))
+		hub->conf_data1 &= ~BIT(0);
+
+	hub->conf_data2 = USB251XB_DEF_CONFIG_DATA_2;
+	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_get_property(np, "compound-device", NULL))
+		hub->conf_data2 |= BIT(3);
+
+	hub->conf_data3 = USB251XB_DEF_CONFIG_DATA_3;
+	if (of_get_property(np, "port-mapping-mode", NULL))
+		hub->conf_data3 |= BIT(3);
+
+	if (of_get_property(np, "string-support", NULL))
+		hub->conf_data3 |= BIT(0);
+
+	hub->non_rem_dev = USB251XB_DEF_NON_REMOVABLE_DEVICES;
+	cproperty_u32 = of_get_property(np, "non-removable-ports", &len);
+	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
+		for (i = 0; i < len / sizeof(u32); i++) {
+			u32 port = be32_to_cpu(cproperty_u32[i]);
+
+			if ((port >= 1) && (port <= 4))
+				hub->non_rem_dev |= BIT(port);
+		}
+	}
+
+	hub->port_disable_sp = USB251XB_DEF_PORT_DISABLE_SELF;
+	cproperty_u32 = of_get_property(np, "sp-disabled-ports", &len);
+	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
+		for (i = 0; i < len / sizeof(u32); i++) {
+			u32 port = be32_to_cpu(cproperty_u32[i]);
+
+			if ((port >= 1) && (port <= 4))
+				hub->port_disable_sp |= BIT(port);
+		}
+	}
+
+	hub->port_disable_bp = USB251XB_DEF_PORT_DISABLE_BUS;
+	cproperty_u32 = of_get_property(np, "bp-disabled-ports", &len);
+	if (cproperty_u32 && (len / sizeof(u32)) > 0) {
+		for (i = 0; i < len / sizeof(u32); i++) {
+			u32 port = be32_to_cpu(cproperty_u32[i]);
+
+			if ((port >= 1) && (port <= 4))
+				hub->port_disable_bp |= BIT(port);
+		}
+	}
+
+	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,
+					   255);
+
+	if (of_property_read_u16_array(np, "language-id", &hub->lang_id, 1))
+		hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
+
+	cproperty_char = of_get_property(np, "manufacturer", NULL);
+	strlcpy(str, cproperty_char ? : USB251XB_DEF_MANUFACTURER_STRING,
+		sizeof(str));
+	hub->manufacturer_len = strlen(str) & 0xFF;
+	memset(hub->manufacturer, 0, USB251XB_STRING_BUFSIZE);
+	len = min_t(size_t, USB251XB_STRING_BUFSIZE / 2, strlen(str));
+	len = utf8s_to_utf16s(str, len, UTF16_LITTLE_ENDIAN,
+			      (wchar_t *)hub->manufacturer,
+			      USB251XB_STRING_BUFSIZE);
+
+	cproperty_char = of_get_property(np, "product", NULL);
+	strlcpy(str, cproperty_char ? : data->product_str, sizeof(str));
+	hub->product_len = strlen(str) & 0xFF;
+	memset(hub->product, 0, USB251XB_STRING_BUFSIZE);
+	len = min_t(size_t, USB251XB_STRING_BUFSIZE / 2, strlen(str));
+	len = utf8s_to_utf16s(str, len, UTF16_LITTLE_ENDIAN,
+			      (wchar_t *)hub->product,
+			      USB251XB_STRING_BUFSIZE);
+
+	cproperty_char = of_get_property(np, "serial", NULL);
+	strlcpy(str, cproperty_char ? : USB251XB_DEF_SERIAL_STRING,
+		sizeof(str));
+	hub->serial_len = strlen(str) & 0xFF;
+	memset(hub->serial, 0, USB251XB_STRING_BUFSIZE);
+	len = min_t(size_t, USB251XB_STRING_BUFSIZE / 2, strlen(str));
+	len = utf8s_to_utf16s(str, len, UTF16_LITTLE_ENDIAN,
+			      (wchar_t *)hub->serial,
+			      USB251XB_STRING_BUFSIZE);
+
+	/* The following parameters are currently not exposed to devicetree, but
+	 * may be as soon as needed.
+	 */
+	hub->bat_charge_en = USB251XB_DEF_BATTERY_CHARGING_ENABLE;
+	hub->boost_up = USB251XB_DEF_BOOST_UP;
+	hub->boost_x = USB251XB_DEF_BOOST_X;
+	hub->port_swap = USB251XB_DEF_PORT_SWAP;
+	hub->port_map12 = USB251XB_DEF_PORT_MAP_12;
+	hub->port_map34 = USB251XB_DEF_PORT_MAP_34;
+
+	return 0;
+}
+
+static const struct of_device_id usb251xb_of_match[] = {
+	{
+		.compatible = "microchip,usb2512b",
+		.data = &usb2512b_data,
+	}, {
+		.compatible = "microchip,usb2512bi",
+		.data = &usb2512bi_data,
+	}, {
+		.compatible = "microchip,usb2513b",
+		.data = &usb2513b_data,
+	}, {
+		.compatible = "microchip,usb2513bi",
+		.data = &usb2513bi_data,
+	}, {
+		.compatible = "microchip,usb2514b",
+		.data = &usb2514b_data,
+	}, {
+		.compatible = "microchip,usb2514bi",
+		.data = &usb2514bi_data,
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, usb251xb_of_match);
+#else /* CONFIG_OF */
+static int usb251xb_get_ofdata(struct usb251xb *hub,
+			       struct usb251xb_data *data)
+{
+	return 0;
+}
+#endif /* CONFIG_OF */
+
+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;
+
+	if (np) {
+		err = usb251xb_get_ofdata(hub,
+					  (struct usb251xb_data *)of_id->data);
+		if (err) {
+			dev_err(dev, "failed to get ofdata: %d\n", err);
+			return err;
+		}
+	}
+
+	err = usb251xb_connect(hub);
+	if (err) {
+		dev_err(dev, "Failed to connect hub (%d)\n", err);
+		return err;
+	}
+
+	dev_info(dev, "Hub probed successfully\n");
+
+	return 0;
+}
+
+static int usb251xb_i2c_probe(struct i2c_client *i2c,
+			      const struct i2c_device_id *id)
+{
+	struct usb251xb *hub;
+
+	hub = devm_kzalloc(&i2c->dev, sizeof(struct usb251xb), GFP_KERNEL);
+	if (!hub)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, hub);
+	hub->dev = &i2c->dev;
+	hub->i2c = i2c;
+
+	return usb251xb_probe(hub);
+}
+
+static const struct i2c_device_id usb251xb_id[] = {
+	{ "usb2512b", 0 },
+	{ "usb2512bi", 0 },
+	{ "usb2513b", 0 },
+	{ "usb2513bi", 0 },
+	{ "usb2514b", 0 },
+	{ "usb2514bi", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, usb251xb_id);
+
+static struct i2c_driver usb251xb_i2c_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = of_match_ptr(usb251xb_of_match),
+	},
+	.probe    = usb251xb_i2c_probe,
+	.id_table = usb251xb_id,
+};
+
+module_i2c_driver(usb251xb_i2c_driver);
+
+MODULE_AUTHOR("Richard Leitner <richard.leitner@skidata.com>");
+MODULE_DESCRIPTION("USB251xB/xBi USB 2.0 Hub Controller Driver");
+MODULE_LICENSE("GPL");
-- 
2.1.4

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

[Rob Herring]

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/00001692C.pdf
> 
> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
> CHANGES v5:

V5 and the first I see it?

> 	- 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 0000000..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.

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

Why is this optional? 

> + - 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?

> + - device-id : USB Device ID of the hub (16 bit, default is 0x0bb3)
> + - language-id : USB Language ID (16 bit, default is 0x0000)
> + - 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)

Make the property value be the time (e.g. oc-delay-us).

> + - 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).
> + - 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).

Various properties need unit suffixes (see property-units.txt) and 
either be common properties or need vendor prefixes.

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.

> +
> +Examples:
> +	usb2512b@2c {
> +		compatible = "microchip,usb2512b";
> +		hub-reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
> +	};
> +
> +	usb2514b@2c {
> +		compatible = "microchip,usb2514b";
> +		reg = <0x2c>;
> +		reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
> +		vendor-id = /bits/ 16 <0x0000>;
> +		product-id = /bits/ 16 <0x0000>;
> +		string-support;
> +		manufacturer = "Foo";
> +		product = "Foo-Bar";
> +		serial = "1234567890A";
> +	};

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/00001692C.pdf
>>
>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>> ---
>> CHANGES v5:
>
> V5 and the first I see it?

Just double-checked it, you (robh+dt@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 0000000..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 0x0000)
>> + - 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)
>
> Make the property value be the time (e.g. oc-delay-us).

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).
>> + - 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).
>
> Various properties need unit suffixes (see property-units.txt) and
> either be common properties or need vendor prefixes.

Ok. What exactly do you mean with common properties? I don't think it's 
the endianness settings described in common-properties.txt, is it?

Is "microchip," fine as vendor prefix?

>
> 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.

>
>> +
>> +Examples:
>> +	usb2512b@2c {
>> +		compatible = "microchip,usb2512b";
>> +		hub-reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
>> +	};
>> +
>> +	usb2514b@2c {
>> +		compatible = "microchip,usb2514b";
>> +		reg = <0x2c>;
>> +		reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
>> +		vendor-id = /bits/ 16 <0x0000>;
>> +		product-id = /bits/ 16 <0x0000>;
>> +		string-support;
>> +		manufacturer = "Foo";
>> +		product = "Foo-Bar";
>> +		serial = "1234567890A";
>> +	};
>

Thanks for your feedback!

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

[Greg KH]

On Thu, Feb 16, 2017 at 07:36:48AM +0100, Richard Leitner 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/00001692C.pdf
> > > 
> > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> > > ---
> > > CHANGES v5:
> > 
> > V5 and the first I see it?
> 
> Just double-checked it, you (robh+dt@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?

A separate patch fixing the issues is fine, I don't think it's worth
reverting your original patch and then adding an updated one to the
tree.

thanks,

greg k-h

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

[Rob Herring]

On Thu, Feb 16, 2017 at 12:36 AM, Richard Leitner <me@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/00001692C.pdf
>>>
>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>> ---
>>> CHANGES v5:
>>
>>
>> V5 and the first I see it?
>
>
> Just double-checked it, you (robh+dt@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.

[...]

>>> +
>>> +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.

>>> + - 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 0x0000)
>>> + - 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)
>>
>>
>> Make the property value be the time (e.g. oc-delay-us).
>
>
> 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).
>>> + - 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).
>>
>>
>> Various properties need unit suffixes (see property-units.txt) and
>> either be common properties or need vendor prefixes.
>
>
> Ok. What exactly do you mean with common properties? I don't think it's the
> endianness settings described in common-properties.txt, is it?

No, not common-properties.txt (maybe that needs another name). Simply
documented in a common location that multiple device bindings can
share. So things that would be common to USB devices or USB hubs in
particular.

Looking through the list again, probably just the ones corresponding
to USB descriptors should be common.

> Is "microchip," fine as vendor prefix?

Yes, if that's the correct string for Microchip.

>> 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.

Rob

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 <me@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/00001692C.pdf
>>>>
>>>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
>>>> ---
>>>> CHANGES v5:
>>>
>>>
>>> V5 and the first I see it?
>>
>>
>> Just double-checked it, you (robh+dt@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).
>>>> + - 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).
>>>
>>>
>>> Various properties need unit suffixes (see property-units.txt) and
>>> either be common properties or need vendor prefixes.
>>
>>
>> Ok. What exactly do you mean with common properties? I don't think it's the
>> endianness settings described in common-properties.txt, is it?
> 
> No, not common-properties.txt (maybe that needs another name). Simply
> documented in a common location that multiple device bindings can
> share. So things that would be common to USB devices or USB hubs in
> particular.
> 
> Looking through the list again, probably just the ones corresponding
> to USB descriptors should be common.

Ok. But aren't for example "power-on-time" or "disabled-ports" also
properties (most) hubs share and should therefore be common?

> 
>> Is "microchip," fine as vendor prefix?
> 
> Yes, if that's the correct string for Microchip.

Ok.

> 
>>> 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?

Thank you for your feedback!

regards,
Richard L

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

[Jan Lübbe]

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.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

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

[Rob Herring]

On Mon, Feb 27, 2017 at 3:48 AM, Jan Lübbe <jlu@pengutronix.de> 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.

Configuration can be okay. The test I use is more who needs to
set/control these properties? Would an end-user want to control them?
If so, then they should be sysfs controls. If they are configuration
for a particular design, then DT is fine. Given these are all
typically EEPROM properties, then they aren't really end-user controls
and belong in DT.

Rob

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

[Jan Lübbe]

On Mo, 2017-02-27 at 16:27 -0600, Rob Herring wrote:
> On Mon, Feb 27, 2017 at 3:48 AM, Jan Lübbe <jlu@pengutronix.de> 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.
> 
> Configuration can be okay. The test I use is more who needs to
> set/control these properties? Would an end-user want to control them?
> If so, then they should be sysfs controls. If they are configuration
> for a particular design, then DT is fine. Given these are all
> typically EEPROM properties, then they aren't really end-user controls
> and belong in DT.

Thanks for explaining this, your test sounds very reasonable. And if it
turns out that runtime access is useful as well, the sysfs properties
could be added as needed.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |