On Fri, Jul 15, 2022 at 09:32:59AM +0200, Alexander Stein wrote:
> Despite default reset upon probe, release reset line after powering up
> the hub and assert reset again before powering down.
>
> Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> * Use device specific sleep times, if present
> * Use fsleep instead of usleep_range
>
> drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
> drivers/usb/misc/onboard_usb_hub.h | 5 +++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 6b9b949d17d3..1dd7f4767def 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -7,6 +7,7 @@
>
> #include <linux/device.h>
> #include <linux/export.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> @@ -38,6 +39,8 @@ struct usbdev_node {
> struct onboard_hub {
> struct regulator *vdd;
> struct device *dev;
> + const struct onboard_hub_devtype_data *devtype_data;
This kind of device specific data is often call platform data, let's
call the struct 'onboard_hub_pdata' and the field 'pdata'?
> + struct gpio_desc *reset_gpio;
> bool always_powered_in_suspend;
> bool is_powered_on;
> bool going_away;
> @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> return err;
> }
>
> + if (hub->devtype_data)
Instead of these checks let's make sure all hubs have pdata. Actually your
onboard_hub_probe() already esnures that the field is assigned.
> + fsleep(hub->devtype_data->power_stable_time);
> + if (hub->reset_gpio)
> + gpiod_set_value_cansleep(hub->reset_gpio, 0);
I would have expected:
if (hub->reset_gpio) {
fsleep(hub->devtype_data->power_stable_time);
gpiod_set_value_cansleep(hub->reset_gpio, 0);
}
For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
VDD33 stable before de-assertion of GRSTz", so no delay without reset.
> +
> hub->is_powered_on = true;
>
> return 0;
> @@ -65,6 +73,12 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
> {
> int err;
>
> + if (hub->reset_gpio) {
> + gpiod_set_value_cansleep(hub->reset_gpio, 1);
> + if (hub->devtype_data)
> + fsleep(hub->devtype_data->reset_duration);
> + }
> +
> err = regulator_disable(hub->vdd);
> if (err) {
> dev_err(hub->dev, "failed to disable regulator: %d\n", err);
> @@ -219,6 +233,7 @@ static void onboard_hub_attach_usb_driver(struct work_struct *work)
>
> static int onboard_hub_probe(struct platform_device *pdev)
> {
> + const struct of_device_id *of_id;
> struct device *dev = &pdev->dev;
> struct onboard_hub *hub;
> int err;
> @@ -227,10 +242,24 @@ static int onboard_hub_probe(struct platform_device *pdev)
> if (!hub)
> return -ENOMEM;
>
> + of_id = of_match_device(onboard_hub_match, &pdev->dev);
> + if (of_id)
> + hub->devtype_data = of_id->data;
> + else
> + return -ENODEV;
Please change to:
if (!of_id)
return -ENODEV;
hub->pdata = of_id->data;
With the currently supported Realtek hubs will fail to probe. Please add a pdata for
the RTS541x hubs. The have no reset signal, so the values can be 0.
> +
> hub->vdd = devm_regulator_get(dev, "vdd");
> if (IS_ERR(hub->vdd))
> return PTR_ERR(hub->vdd);
>
> + hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(hub->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), "failed to get reset GPIO\n");
> +
> + if (hub->devtype_data && hub->reset_gpio)
drop check 'hub->devtype_data' check
> + fsleep(hub->devtype_data->reset_duration);
> +
> hub->dev = dev;
> mutex_init(&hub->lock);
> INIT_LIST_HEAD(&hub->udev_list);
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index d3a5b6938582..7e743f4c8aaa 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -6,6 +6,11 @@
> #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> #define _USB_MISC_ONBOARD_USB_HUB_H
>
> +struct onboard_hub_devtype_data {
> + unsigned long power_stable_time; /* power stabilization time in us */
> + unsigned long reset_duration; /* reset pulse width in us */
Let's encode the unit in the field name:
unsigned long power_stable_us;
unsigned long reset_us;
> +};
Is it necessary to define this struct in onboard_usb_hub.h? It seems it
could as well be defined in onboard_usb_hub.c.
