On Thu, 2 Aug 2012 kuninori.morimoto.gx@xxxxxxxxxxx wrote:
> This patch enables to call platform specific power callback function.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> drivers/usb/host/ehci-platform.c | 33 +++++++++++++++++++++++++++++----
> include/linux/usb/ehci_pdriver.h | 2 ++
> 2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index 4b1d896..679aa16 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -82,10 +82,11 @@ static int __devinit ehci_platform_probe(struct platform_device *dev)
> {
> struct usb_hcd *hcd;
> struct resource *res_mem;
> + struct usb_ehci_pdata *pdata = dev->dev.platform_data;
> int irq;
> int err = -ENOMEM;
>
> - BUG_ON(!dev->dev.platform_data);
> + BUG_ON(!pdata);
We should avoid using BUG_ON in drivers. Even though it's present in
the original code, removing it would be better than keeping it. You
can change it to WARN_ON.
> --- a/include/linux/usb/ehci_pdriver.h
> +++ b/include/linux/usb/ehci_pdriver.h
> @@ -41,6 +41,8 @@ struct usb_ehci_pdata {
> unsigned big_endian_mmio:1;
> unsigned port_power_on:1;
> unsigned port_power_off:1;
> +
> + void (*power)(struct device *dev, int in_pm, int enable);
I don't like having these two separate arguments. Having multiple
function pointers would be better:
void (*power_off)(struct platform_device *pdev);
/* Turn off all power and clocks */
void (*power_suspend)(struct platform_device *pdev);
/* Turn on only VBUS suspend power and hotplug
detection, turn off everything else */
void (*power_on)(struct platform_device *pdev);
/* Turn on all power and clocks */
Also, I think it's better for the first argument to point to a platform
device instead of a regular device. Otherwise people will have to
convert the pointer back and forth.
Everything else looks good.
Similar comments apply to the ohci-platform patch.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
