| Message ID | 20240220-onboard_xvf3500-v4-6-dc1617cc5dd4@wolfvision.net |
|---|---|
| State | New |
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
On 21.02.24 21:25, Matthias Kaehlcke wrote: > On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: >> The current mechanism uses generic names for the power supplies, which >> conflicts with proper name definitions in the device bindings. >> >> Add a per-device property to include real supply names and keep generic >> names as a fallback mechanism for backward compatibility. >> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >> --- >> drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------ >> drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++ >> 2 files changed, 52 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >> index f43130a6786f..e66fcac93006 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.c >> +++ b/drivers/usb/misc/onboard_usb_dev.c >> @@ -29,18 +29,6 @@ >> >> #include "onboard_usb_dev.h" >> >> -/* >> - * Use generic names, as the actual names might differ between devices. If a new >> - * device requires more than the currently supported supplies, add a new one >> - * here. >> - */ >> -static const char * const supply_names[] = { >> - "vdd", >> - "vdd2", >> -}; >> - >> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names) >> - >> static void onboard_dev_attach_usb_driver(struct work_struct *work); >> >> static struct usb_device_driver onboard_dev_usbdev_driver; >> @@ -66,6 +54,33 @@ struct onboard_dev { >> struct clk *clk; >> }; >> >> +static int onboard_dev_get_regulator_bulk(struct device *dev, >> + struct onboard_dev *onboard_dev) >> +{ >> + unsigned int i; >> + int err; >> + >> + const char * const *supply_names = onboard_dev->pdata->supply_names; >> + >> + if (onboard_dev->pdata->num_supplies > MAX_SUPPLIES) >> + return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n", >> + MAX_SUPPLIES); >> + >> + if (!supply_names[0]) >> + supply_names = generic_supply_names; > > Please change to 'if (!supply_names)' and omit the initialization of > .supply_names for devices that use the generic supply names. > That looks much cleaner, I will apply it to the next version. >> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h >> index ebe83e19d818..59dced6bd339 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.h >> +++ b/drivers/usb/misc/onboard_usb_dev.h >> @@ -6,63 +6,86 @@ >> #ifndef _USB_MISC_ONBOARD_USB_DEV_H >> #define _USB_MISC_ONBOARD_USB_DEV_H >> >> +/* >> + * Fallback supply names for backwards compatibility. If the device requires >> + * more than the currently supported supplies, add a new one here, and if >> + * possible, the real name supplies to the device-specific data. >> + */ >> +static const char * const generic_supply_names[] = { >> + "vdd", >> + "vdd2", >> +}; >> + >> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) > > This will have to change when support for a device with more than 2 non-generic > supply names gets added. Please use a literal value for MAX_SUPPLIES instead of > ARRAY_SIZE. If the literal is 2 it would still need to change for future devices > with more supplies, but that change would be more straighforward. > I am not completely sure about this. Someone could increase MAX_SUPPLIES without adding a generic name. Actually two modifications will be necessary for every addition (name and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, and MAX_SUPPLIES is automatically increased. I understand that the whole point of this is getting rid of the generic names, but we still have to provide generic names for every extra supply, at least for code consistency and to avoid size mismatches between real an generic supply names. >> + >> struct onboard_dev_pdata { >> unsigned long reset_us; /* reset pulse width in us */ >> unsigned int num_supplies; /* number of supplies */ >> bool is_hub; >> + const char * const supply_names[MAX_SUPPLIES]; >> + >> }; >> >> static const struct onboard_dev_pdata microchip_usb424_data = { >> .reset_us = 1, >> .num_supplies = 1, >> + .supply_names = { NULL }, >> .is_hub = true, >> }; >> >> ... Thanks again for your feedback. Best regards, Javier Carrasco
On 21.02.24 22:18, Matthias Kaehlcke wrote: >>>> +/* >>>> + * Fallback supply names for backwards compatibility. If the device requires >>>> + * more than the currently supported supplies, add a new one here, and if >>>> + * possible, the real name supplies to the device-specific data. >>>> + */ >>>> +static const char * const generic_supply_names[] = { >>>> + "vdd", >>>> + "vdd2", >>>> +}; >>>> + >>>> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) >>> >>> This will have to change when support for a device with more than 2 non-generic >>> supply names gets added. Please use a literal value for MAX_SUPPLIES instead of >>> ARRAY_SIZE. If the literal is 2 it would still need to change for future devices >>> with more supplies, but that change would be more straighforward. >>> >> >> I am not completely sure about this. Someone could increase MAX_SUPPLIES >> without adding a generic name. > > That's perfectly fine and intended. MAX_SUPPLIES is a max, any list > shorther than that is valid. Any longer list will result in probe() > being aborted with a clear error message. > >> Actually two modifications will be necessary for every addition (name >> and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, >> and MAX_SUPPLIES is automatically increased. > > As per above it's not necessary to add a new name when MAX_SUPPLIES is > increased to support more non-generic names. It would only be necessary > if more generic names were added, my understanding is that this > should not happen because any newly supported onboard devices are > supposed to use device specific supply names. I don't like to idea of > adding unused pseudo supply names to the list, just for the sake of > using ARRAY_SIZE. > >> I understand that the whole point of this is getting rid of the generic >> names, but we still have to provide generic names for every extra >> supply, at least for code consistency and to avoid size mismatches >> between real an generic supply names. > > Please let me know if you still think the extra names are needed. Not really, the only case I could come up is if an existing device that uses generic names might end up requiring a third supply, which would also be generic. But even such an unlikely event would be cover without ARRAY_SIZE. Actually one could argue that every existing device could have "vdd" and "vdd2" as their supply names and remove checks and the generic array. Best regards, Javier Carrasco
On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: > The current mechanism uses generic names for the power supplies, which > conflicts with proper name definitions in the device bindings. > > Add a per-device property to include real supply names and keep generic > names as a fallback mechanism for backward compatibility. > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> For v5 you could consider making this [1/8] (i.e. before the renaming of the driver). That way support for new hubs doesn't necessarily have to wait for the entire series to land. Since this series is underway I think new bindings shouldn't use 'vdd-supply' but the device specific name of the supply.
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c index f43130a6786f..e66fcac93006 100644 --- a/drivers/usb/misc/onboard_usb_dev.c +++ b/drivers/usb/misc/onboard_usb_dev.c @@ -29,18 +29,6 @@ #include "onboard_usb_dev.h" -/* - * Use generic names, as the actual names might differ between devices. If a new - * device requires more than the currently supported supplies, add a new one - * here. - */ -static const char * const supply_names[] = { - "vdd", - "vdd2", -}; - -#define MAX_SUPPLIES ARRAY_SIZE(supply_names) - static void onboard_dev_attach_usb_driver(struct work_struct *work); static struct usb_device_driver onboard_dev_usbdev_driver; @@ -66,6 +54,33 @@ struct onboard_dev { struct clk *clk; }; +static int onboard_dev_get_regulator_bulk(struct device *dev, + struct onboard_dev *onboard_dev) +{ + unsigned int i; + int err; + + const char * const *supply_names = onboard_dev->pdata->supply_names; + + if (onboard_dev->pdata->num_supplies > MAX_SUPPLIES) + return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n", + MAX_SUPPLIES); + + if (!supply_names[0]) + supply_names = generic_supply_names; + + for (i = 0; i < onboard_dev->pdata->num_supplies; i++) + onboard_dev->supplies[i].supply = supply_names[i]; + + err = devm_regulator_bulk_get(dev, onboard_dev->pdata->num_supplies, + onboard_dev->supplies); + if (err) + dev_err(dev, "Failed to get regulator supplies: %pe\n", + ERR_PTR(err)); + + return err; +} + static int onboard_dev_power_on(struct onboard_dev *onboard_dev) { int err; @@ -266,7 +281,6 @@ static int onboard_dev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct onboard_dev *onboard_dev; - unsigned int i; int err; onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL); @@ -277,19 +291,9 @@ static int onboard_dev_probe(struct platform_device *pdev) if (!onboard_dev->pdata) return -EINVAL; - if (onboard_dev->pdata->num_supplies > MAX_SUPPLIES) - return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n", - MAX_SUPPLIES); - - for (i = 0; i < onboard_dev->pdata->num_supplies; i++) - onboard_dev->supplies[i].supply = supply_names[i]; - - err = devm_regulator_bulk_get(dev, onboard_dev->pdata->num_supplies, - onboard_dev->supplies); - if (err) { - dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err)); + err = onboard_dev_get_regulator_bulk(dev, onboard_dev); + if (err) return err; - } onboard_dev->clk = devm_clk_get_optional(dev, NULL); if (IS_ERR(onboard_dev->clk)) diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h index ebe83e19d818..59dced6bd339 100644 --- a/drivers/usb/misc/onboard_usb_dev.h +++ b/drivers/usb/misc/onboard_usb_dev.h @@ -6,63 +6,86 @@ #ifndef _USB_MISC_ONBOARD_USB_DEV_H #define _USB_MISC_ONBOARD_USB_DEV_H +/* + * Fallback supply names for backwards compatibility. If the device requires + * more than the currently supported supplies, add a new one here, and if + * possible, the real name supplies to the device-specific data. + */ +static const char * const generic_supply_names[] = { + "vdd", + "vdd2", +}; + +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) + struct onboard_dev_pdata { unsigned long reset_us; /* reset pulse width in us */ unsigned int num_supplies; /* number of supplies */ bool is_hub; + const char * const supply_names[MAX_SUPPLIES]; + }; static const struct onboard_dev_pdata microchip_usb424_data = { .reset_us = 1, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata microchip_usb5744_data = { .reset_us = 0, .num_supplies = 2, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata realtek_rts5411_data = { .reset_us = 0, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata ti_tusb8041_data = { .reset_us = 3000, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata cypress_hx3_data = { .reset_us = 10000, .num_supplies = 2, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata cypress_hx2vl_data = { .reset_us = 1, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata genesys_gl850g_data = { .reset_us = 3, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata genesys_gl852g_data = { .reset_us = 50, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, }; static const struct onboard_dev_pdata vialab_vl817_data = { .reset_us = 10, .num_supplies = 1, + .supply_names = { NULL }, .is_hub = true, };
The current mechanism uses generic names for the power supplies, which conflicts with proper name definitions in the device bindings. Add a per-device property to include real supply names and keep generic names as a fallback mechanism for backward compatibility. Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> --- drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------ drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++ 2 files changed, 52 insertions(+), 25 deletions(-)