2022-02-23 08:26:24

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: 回复: 回复 : Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC) wrote:
> Ok, thanks your reply.
>
> Here is my question, we must modify the driver "onboard_usb_hub.c" if we want to use it. But it's hard to complete because it's an opensource code.

I do not understand. We do not deal with code that is not in the kernel
source tree, as we have no idea what is out there. Please just submit
your changes to be merged into the tree and all will be fine.

thanks,

greg k-h


2022-02-28 09:58:19

by Linyu Yuan

[permalink] [raw]
Subject: RE: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: A dd onboard_usb_hub driver

> From: [email protected] <[email protected]>
> Sent: Wednesday, February 23, 2022 3:56 PM
> To: Tao Wang (Consultant) (QUIC) <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Linyu Yuan (QUIC) <[email protected]>
> Subject: Re: ?^?`: ?^?`: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
>
> On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC)
> wrote:
> > Ok, thanks your reply.
> >
> > Here is my question, we must modify the driver "onboard_usb_hub.c" if
> we want to use it. But it's hard to complete because it's an opensource code.
>
> I do not understand. We do not deal with code that is not in the kernel
> source tree, as we have no idea what is out there. Please just submit
> your changes to be merged into the tree and all will be fine.

Hi Greg and mka,

Let's make it clear that we are talking about once this driver is approved into usb tree,
If we use different USB HUB which have VID/PID not defined in this driver,
We need to update this driver.

But if we defined VID/PID in device tree(for a specific board, manufacture should know VID/PID from HUB it used),
dynamic parsed by the driver, then we don't need to change this driver (increase VID/PID table).

>
> thanks,
>
> greg k-h

2022-02-28 19:01:36

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: 回复: 回复 : Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

On Mon, Feb 28, 2022 at 03:08:34AM +0000, Linyu Yuan (QUIC) wrote:
> > From: [email protected] <[email protected]>
> > Sent: Wednesday, February 23, 2022 3:56 PM
> > To: Tao Wang (Consultant) (QUIC) <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Linyu Yuan (QUIC) <[email protected]>
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> >
> > On Wed, Feb 23, 2022 at 07:40:31AM +0000, Tao Wang (Consultant) (QUIC)
> > wrote:
> > > Ok, thanks your reply.
> > >
> > > Here is my question, we must modify the driver "onboard_usb_hub.c" if
> > we want to use it. But it's hard to complete because it's an opensource code.
> >
> > I do not understand. We do not deal with code that is not in the kernel
> > source tree, as we have no idea what is out there. Please just submit
> > your changes to be merged into the tree and all will be fine.
>
> Hi Greg and mka,
>
> Let's make it clear that we are talking about once this driver is approved into usb tree,
> If we use different USB HUB which have VID/PID not defined in this driver,
> We need to update this driver.
>
> But if we defined VID/PID in device tree(for a specific board, manufacture should know VID/PID from HUB it used),
> dynamic parsed by the driver, then we don't need to change this driver (increase VID/PID table).

As per my earlier reply, the kernel/USB core uses the VID:PID reported
by the USB device, the compatible string in the device tree is purely
informational. That's not something that could be changed by this
driver.

And even if the VID:PID from the device tree was used: how is the
kernel supposed to know that the onboard_hub driver should be
probed for a given VID:PID from the device tree, without listing
the VID:PID (or compatible string) in the driver (which is what
you seem to seek to avoid)?

2022-03-01 04:08:19

by Linyu Yuan

[permalink] [raw]
Subject: RE: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

> From: [email protected] <[email protected]>
> Sent: Tuesday, March 1, 2022 2:29 AM
> To: Linyu Yuan (QUIC) <[email protected]>
> Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
>
> >
> > Hi Greg and mka,
> >
> > Let's make it clear that we are talking about once this driver is approved
> into usb tree,
> > If we use different USB HUB which have VID/PID not defined in this driver,
> > We need to update this driver.
> >
> > But if we defined VID/PID in device tree(for a specific board, manufacture
> should know VID/PID from HUB it used),
> > dynamic parsed by the driver, then we don't need to change this driver
> (increase VID/PID table).
>
> As per my earlier reply, the kernel/USB core uses the VID:PID reported
> by the USB device, the compatible string in the device tree is purely
> informational. That's not something that could be changed by this
> driver.
I can't fully understand this comment, could you please share step if we want to add a new HUB support, what should we do ? nothing ?

If do nothing, can we remove id_table from onboard_hub_usbdev_driver ?
>
> And even if the VID:PID from the device tree was used: how is the
> kernel supposed to know that the onboard_hub driver should be
> probed for a given VID:PID from the device tree, without listing
> the VID:PID (or compatible string) in the driver (which is what
> you seem to seek to avoid)?
In my opinion, if it need update VID/PID table in this driver to support a new HUB,
we can parse VID/PID from device tree and create dynamic VID/PID entry to id_table of onboard_hub_usbdev_driver.

Hope you can understand what I said.

2022-03-01 18:43:20

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: 回复: 回复 : Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

On Tue, Mar 01, 2022 at 02:30:00AM +0000, Linyu Yuan (QUIC) wrote:
> > From: [email protected] <[email protected]>
> > Sent: Tuesday, March 1, 2022 2:29 AM
> > To: Linyu Yuan (QUIC) <[email protected]>
> > Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> >
> > >
> > > Hi Greg and mka,
> > >
> > > Let's make it clear that we are talking about once this driver is approved
> > into usb tree,
> > > If we use different USB HUB which have VID/PID not defined in this driver,
> > > We need to update this driver.
> > >
> > > But if we defined VID/PID in device tree(for a specific board, manufacture
> > should know VID/PID from HUB it used),
> > > dynamic parsed by the driver, then we don't need to change this driver
> > (increase VID/PID table).
> >
> > As per my earlier reply, the kernel/USB core uses the VID:PID reported
> > by the USB device, the compatible string in the device tree is purely
> > informational. That's not something that could be changed by this
> > driver.
> I can't fully understand this comment, could you please share step if we want to add a new HUB support, what should we do ? nothing ?

Add the VID:PID and compatible strings to onboard_usb_hub.c, analogous
to those for the RTS5411 and RTS5414. More work will be needed if the
hub needs a special power up or power down sequence (multiple regulators,
GPIOs, ...)

> If do nothing, can we remove id_table from onboard_hub_usbdev_driver ?
> >
> > And even if the VID:PID from the device tree was used: how is the
> > kernel supposed to know that the onboard_hub driver should be
> > probed for a given VID:PID from the device tree, without listing
> > the VID:PID (or compatible string) in the driver (which is what
> > you seem to seek to avoid)?
> In my opinion, if it need update VID/PID table in this driver to support a new HUB,
> we can parse VID/PID from device tree and create dynamic VID/PID entry to id_table of onboard_hub_usbdev_driver.
>
> Hope you can understand what I said.

Not really.

I doubt that what you are suggesting would work. The easiest thing
to convince people would probably be to send a patch (based on this
one) with a working implementation of your idea.

2022-03-02 23:50:37

by Linyu Yuan

[permalink] [raw]
Subject: RE: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

> From: [email protected] <[email protected]>
> Sent: Wednesday, March 2, 2022 12:33 AM
> To: Linyu Yuan (QUIC) <[email protected]>
> Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
>
> > In my opinion, if it need update VID/PID table in this driver to support a
> new HUB,
> > we can parse VID/PID from device tree and create dynamic VID/PID entry
> to id_table of onboard_hub_usbdev_driver.
> >
> > Hope you can understand what I said.
>
> Not really.
>
> I doubt that what you are suggesting would work. The easiest thing
> to convince people would probably be to send a patch (based on this
> one) with a working implementation of your idea.

I show my idea, but not test,

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index e280409..1811317 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -10,6 +10,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/list.h>
+#include <linux/slab.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/of.h>
@@ -173,6 +174,58 @@ static void onboard_hub_remove_usbdev(struct onboard_hub *hub, const struct usb_
mutex_unlock(&hub->lock);
}

+#define MAX_HUB_NUM 4
+#define MAX_TABLE_SIZE (MAX_HUB_NUM * 2)
+static struct usb_device_id onboard_hub_id_table[MAX_TABLE_SIZE + 1];
+MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
+
+static void onboard_hub_add_idtable(__u16 vid, __u16 pid)
+{
+ int i;
+
+ for (i = 0; i < MAX_TABLE_SIZE; i++) {
+ if (!onboard_hub_id_table[i].idVendor)
+ break;
+
+ if (onboard_hub_id_table[i].idVendor == vid &&
+ onboard_hub_id_table[i].idProduct == pid)
+ return;
+ }
+ if (i == MAX_TABLE_SIZE)
+ return;
+
+ onboard_hub_id_table[i].idVendor = vid;
+ onboard_hub_id_table[i].idProduct = pid;
+ onboard_hub_id_table[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
+}
+
+static int onboard_hub_parse_idtable(struct device_node *np)
+{
+ int size = of_property_count_elems_of_size(np, "vidpid", sizeof(int));
+ int ret, i;
+ u16 *ids;
+
+ if (!size || size % 2)
+ return -EINVAL;
+
+ ids = kzalloc(sizeof(u16) * size, GFP_KERNEL);
+ if (!ids)
+ return -ENOMEM;
+
+ ret = of_property_read_u16_array(np, "vidpid", ids, size);
+ if (ret) {
+ kfree(ids);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < size; i+=2)
+ onboard_hub_add_idtable(ids[i], ids[i+1]);
+
+ kfree(ids);
+
+ return 0;
+}
+
static ssize_t always_powered_in_suspend_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -210,6 +263,10 @@ static int onboard_hub_probe(struct platform_device *pdev)
struct onboard_hub *hub;
int err;

+ err = onboard_hub_parse_idtable(dev->of_node);
+ if (err)
+ return err;
+
hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
if (!hub)
return -ENOMEM;
@@ -378,15 +435,6 @@ static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
onboard_hub_remove_usbdev(hub, udev);
}

-static const struct usb_device_id onboard_hub_id_table[] = {
- { USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
- { USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
- { USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
- { USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
- {}
-};
-MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
-
static struct usb_device_driver onboard_hub_usbdev_driver = {
.name = "onboard-usb-hub",
.probe = onboard_hub_usbdev_probe,

2022-03-04 19:49:14

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: 回复: 回复 : Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

On Wed, Mar 02, 2022 at 05:14:30AM +0000, Linyu Yuan (QUIC) wrote:
> > From: [email protected] <[email protected]>
> > Sent: Wednesday, March 2, 2022 12:33 AM
> > To: Linyu Yuan (QUIC) <[email protected]>
> > Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > onboard_usb_hub driver
> >
> > > In my opinion, if it need update VID/PID table in this driver to support a
> > new HUB,
> > > we can parse VID/PID from device tree and create dynamic VID/PID entry
> > to id_table of onboard_hub_usbdev_driver.
> > >
> > > Hope you can understand what I said.
> >
> > Not really.
> >
> > I doubt that what you are suggesting would work. The easiest thing
> > to convince people would probably be to send a patch (based on this
> > one) with a working implementation of your idea.
>
> I show my idea, but not test,
>
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index e280409..1811317 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -10,6 +10,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> +#include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> @@ -173,6 +174,58 @@ static void onboard_hub_remove_usbdev(struct onboard_hub *hub, const struct usb_
> mutex_unlock(&hub->lock);
> }
>
> +#define MAX_HUB_NUM 4
> +#define MAX_TABLE_SIZE (MAX_HUB_NUM * 2)
> +static struct usb_device_id onboard_hub_id_table[MAX_TABLE_SIZE + 1];
> +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> +
> +static void onboard_hub_add_idtable(__u16 vid, __u16 pid)
> +{
> + int i;
> +
> + for (i = 0; i < MAX_TABLE_SIZE; i++) {
> + if (!onboard_hub_id_table[i].idVendor)
> + break;
> +
> + if (onboard_hub_id_table[i].idVendor == vid &&
> + onboard_hub_id_table[i].idProduct == pid)
> + return;
> + }
> + if (i == MAX_TABLE_SIZE)
> + return;
> +
> + onboard_hub_id_table[i].idVendor = vid;
> + onboard_hub_id_table[i].idProduct = pid;
> + onboard_hub_id_table[i].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
> +}
> +
> +static int onboard_hub_parse_idtable(struct device_node *np)
> +{
> + int size = of_property_count_elems_of_size(np, "vidpid", sizeof(int));
> + int ret, i;
> + u16 *ids;
> +
> + if (!size || size % 2)
> + return -EINVAL;
> +
> + ids = kzalloc(sizeof(u16) * size, GFP_KERNEL);
> + if (!ids)
> + return -ENOMEM;
> +
> + ret = of_property_read_u16_array(np, "vidpid", ids, size);
> + if (ret) {
> + kfree(ids);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < size; i+=2)
> + onboard_hub_add_idtable(ids[i], ids[i+1]);
> +
> + kfree(ids);
> +
> + return 0;
> +}
> +
> static ssize_t always_powered_in_suspend_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -210,6 +263,10 @@ static int onboard_hub_probe(struct platform_device *pdev)
> struct onboard_hub *hub;
> int err;
>
> + err = onboard_hub_parse_idtable(dev->of_node);
> + if (err)
> + return err;
> +
> hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> if (!hub)
> return -ENOMEM;
> @@ -378,15 +435,6 @@ static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
> onboard_hub_remove_usbdev(hub, udev);
> }
>
> -static const struct usb_device_id onboard_hub_id_table[] = {
> - { USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
> - { USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> - { USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> - { USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> - {}
> -};
> -MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> -
> static struct usb_device_driver onboard_hub_usbdev_driver = {
> .name = "onboard-usb-hub",
> .probe = onboard_hub_usbdev_probe,

I see multiple issues with this approach:

1. The new device tree property 'vidpid'. It is (or should be) redundant
with the compatible string, I very much doubt you could convince DT
maintainers to add it.
2. You don't want to modify the driver to enabled support for new USB hubs.
That means you would have to use a compatible string that is already in
the driver, but which doesn't match the VID:PID of the hub. While this
might work it's a hack.
3. If the USB hub is probed before the platform device it won't use this
driver because the VID:PID isn't in the device id table.
4. Possible race conditions when changing the device id table on the fly

2022-03-07 05:25:13

by Linyu Yuan

[permalink] [raw]
Subject: RE: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add onboard_usb_hub driver

> From: [email protected] <[email protected]>
> Sent: Saturday, March 5, 2022 12:48 AM
> To: Linyu Yuan (QUIC) <[email protected]>
> Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> onboard_usb_hub driver
>
> On Wed, Mar 02, 2022 at 05:14:30AM +0000, Linyu Yuan (QUIC) wrote:
> > > From: [email protected] <[email protected]>
> > > Sent: Wednesday, March 2, 2022 12:33 AM
> > > To: Linyu Yuan (QUIC) <[email protected]>
> > > Cc: [email protected]; Tao Wang (Consultant) (QUIC)
> > > <[email protected]>; [email protected];
> [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; linux-
> [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected];
> [email protected]
> > > Subject: Re: 回复: 回复: Re: [PATCH v20 3/5] usb: misc: Add
> > > onboard_usb_hub driver
> > >
> > > > In my opinion, if it need update VID/PID table in this driver to support a
> > > new HUB,
> > > > we can parse VID/PID from device tree and create dynamic VID/PID
> entry
> > > to id_table of onboard_hub_usbdev_driver.
> > > >
> > > > Hope you can understand what I said.
> > >
> > > Not really.
> > >
> > > I doubt that what you are suggesting would work. The easiest thing
> > > to convince people would probably be to send a patch (based on this
> > > one) with a working implementation of your idea.
> >
> > I show my idea, but not test,
> >
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> b/drivers/usb/misc/onboard_usb_hub.c
> > index e280409..1811317 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -10,6 +10,7 @@
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > #include <linux/list.h>
> > +#include <linux/slab.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/of.h>
> > @@ -173,6 +174,58 @@ static void onboard_hub_remove_usbdev(struct
> onboard_hub *hub, const struct usb_
> > mutex_unlock(&hub->lock);
> > }
> >
> > +#define MAX_HUB_NUM 4
> > +#define MAX_TABLE_SIZE (MAX_HUB_NUM * 2)
> > +static struct usb_device_id onboard_hub_id_table[MAX_TABLE_SIZE + 1];
> > +MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > +
> > +static void onboard_hub_add_idtable(__u16 vid, __u16 pid)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < MAX_TABLE_SIZE; i++) {
> > + if (!onboard_hub_id_table[i].idVendor)
> > + break;
> > +
> > + if (onboard_hub_id_table[i].idVendor == vid &&
> > + onboard_hub_id_table[i].idProduct == pid)
> > + return;
> > + }
> > + if (i == MAX_TABLE_SIZE)
> > + return;
> > +
> > + onboard_hub_id_table[i].idVendor = vid;
> > + onboard_hub_id_table[i].idProduct = pid;
> > + onboard_hub_id_table[i].match_flags =
> USB_DEVICE_ID_MATCH_DEVICE;
> > +}
> > +
> > +static int onboard_hub_parse_idtable(struct device_node *np)
> > +{
> > + int size = of_property_count_elems_of_size(np, "vidpid", sizeof(int));
> > + int ret, i;
> > + u16 *ids;
> > +
> > + if (!size || size % 2)
> > + return -EINVAL;
> > +
> > + ids = kzalloc(sizeof(u16) * size, GFP_KERNEL);
> > + if (!ids)
> > + return -ENOMEM;
> > +
> > + ret = of_property_read_u16_array(np, "vidpid", ids, size);
> > + if (ret) {
> > + kfree(ids);
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < size; i+=2)
> > + onboard_hub_add_idtable(ids[i], ids[i+1]);
> > +
> > + kfree(ids);
> > +
> > + return 0;
> > +}
> > +
> > static ssize_t always_powered_in_suspend_show(struct device *dev,
> struct device_attribute *attr,
> > char *buf)
> > {
> > @@ -210,6 +263,10 @@ static int onboard_hub_probe(struct
> platform_device *pdev)
> > struct onboard_hub *hub;
> > int err;
> >
> > + err = onboard_hub_parse_idtable(dev->of_node);
> > + if (err)
> > + return err;
> > +
> > hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> > if (!hub)
> > return -ENOMEM;
> > @@ -378,15 +435,6 @@ static void
> onboard_hub_usbdev_disconnect(struct usb_device *udev)
> > onboard_hub_remove_usbdev(hub, udev);
> > }
> >
> > -static const struct usb_device_id onboard_hub_id_table[] = {
> > - { USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1
> */
> > - { USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1
> */
> > - { USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2
> */
> > - { USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1
> */
> > - {}
> > -};
> > -MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > -
> > static struct usb_device_driver onboard_hub_usbdev_driver = {
> > .name = "onboard-usb-hub",
> > .probe = onboard_hub_usbdev_probe,
>
> I see multiple issues with this approach:
>
> 1. The new device tree property 'vidpid'. It is (or should be) redundant
> with the compatible string, I very much doubt you could convince DT
> maintainers to add it.
> 2. You don't want to modify the driver to enabled support for new USB hubs.
> That means you would have to use a compatible string that is already in
> the driver, but which doesn't match the VID:PID of the hub. While this
> might work it's a hack.
> 3. If the USB hub is probed before the platform device it won't use this
> driver because the VID:PID isn't in the device id table.
This is another topic which I don't know if discuss or not,

What is power state requirement of hub for this driver ?
Do it expect hub power off when system enter linux kernel stage ?
If so, all this driver may not work as hub may enumerated before this driver load.

> 4. Possible race conditions when changing the device id table on the fly
If hub is default power off, there is no race.