Re: [PATCH 1/1] Add driver for smsc usb251x i2c configuration
From: Mark Rutland
Date: Tue Aug 02 2016 - 10:52:51 EST
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 <fabien.lahoudere@xxxxxxxxxxxxxxx>
> ---
> 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 0000000..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 is this? Is this a FW blob? Is it a series of commands?
How has this been derived?
A comment would be very helpful.
[...]
> +static void usb251x_set_config_from_of(const struct device_node *node,
> + unsigned char *table,
> + const char *pname, u8 offset)
> +{
> + int ret;
> + unsigned char value;
> +
> + ret = of_property_read_u8(node, pname, &value);
> + if (ret == 0)
> + table[offset] = value;
> +}
This doesn't match your example, which used u32 values, due to lack of a
/bits/ 8 prefix. For those properties, of_property_read_u8() would
always return the value 0.
How was this been tested?
Thanks,
Mark