[PATCH] misc: Port USB251xB/xBi Hi-Speed Hub Controller Driver from Linux

[Stefan Herbrechtsmeier]

Hi Marek,

sorry for the late comments but I think the driver doesn't work as expected.

Am 10.04.2022 um 06:27 schrieb Marek Vasut:
> 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.
> 
> This is ported from Linux as of Linux kernel commit
> 5c2b9c61ae5d8 ("usb: usb251xb: add boost-up property support")
> 
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Michal Simek <michal.simek at xilinx.com>
> Cc: Simon Glass <sjg at chromium.org>
> ---
>   drivers/misc/Kconfig    |   9 +
>   drivers/misc/Makefile   |   1 +
>   drivers/misc/usb251xb.c | 605 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 615 insertions(+)
>   create mode 100644 drivers/misc/usb251xb.c
> 
> diff --git a/drivers/misc/usb251xb.c b/drivers/misc/usb251xb.c
> new file mode 100644
> index 00000000000..077edc25045
> --- /dev/null
> +++ b/drivers/misc/usb251xb.c
> @@ -0,0 +1,605 @@

[snip]

> +static int usb251xb_of_to_plat(struct udevice *dev)
> +{
> +	struct usb251xb_data *data =
> +		(struct usb251xb_data *)dev_get_driver_data(dev);
> +	struct usb251xb *hub = dev_get_priv(dev);
> +	char str[USB251XB_STRING_BUFSIZE / 2];
> +	const char *cproperty_char;
> +	u32 property_u32 = 0;
> +	int len, err;
> +
> +	if (dev_read_bool(dev, "skip-config"))
> +		hub->skip_config = 1;
> +	else
> +		hub->skip_config = 0;
> +
> +	err = gpio_request_by_name(dev, "reset-gpios", 0, &hub->gpio_reset,
> +				   GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +	if (err && err != -ENOENT) {
> +		dev_err(dev, "unable to request GPIO reset pin (%d)\n", err);
> +		return err;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_DM_REGULATOR)) {
> +		err = device_get_supply_regulator(dev, "vdd-supply",
> +						  &hub->vdd);
> +		if (err && err != -ENOENT) {
> +			dev_err(dev, "Warning: cannot get power supply\n");
> +			return err;
> +		}
> +	}

Shouldn't the gpio and regulator request be part of the probe?

> +
> +	if (dev_read_u32(dev, "vendor-id", &hub->vendor_id))
> +		hub->vendor_id = USB251XB_DEF_VENDOR_ID;
> +
> +	if (dev_read_u32(dev, "product-id", &hub->product_id))
> +		hub->product_id = data->product_id;
> +
> +	if (dev_read_u32(dev, "device-id", &hub->device_id))
> +		hub->device_id = USB251XB_DEF_DEVICE_ID;
> +

[snip]

> +	if (dev_read_u32(dev, "language-id", &hub->lang_id))
> +		hub->lang_id = USB251XB_DEF_LANGUAGE_ID;
> +

This doesn't work because the ids are 16 bit [1,2] and the dev_read_u32 
function checks the size.

> +	if (!dev_read_u32(dev, "boost-up", &hub->boost_up))
> +		hub->boost_up = USB251XB_DEF_BOOST_UP;

This looks like a 8 bit value [1].

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/misc/usb251xb.c
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb251xb.txt

Regards,
   Stefan