Re: [PATCH] USB: OHCI: ohci-pxa27x: remove unuseful functions

Re: [PATCH] USB: OHCI: ohci-pxa27x: remove unuseful functions

[Alan Stern]

By the way, "unuseful" is not an English word.  It should be "useless".

On Wed, 30 Nov 2016 csmanjuvijay@gmail.com wrote:

> From: Manjunath Goudar <csmanjuvijay@gmail.com>
> 
> The ohci_hcd_pxa27x_drv_probe function is not doing anything other
> than calling usb_hcd_pxa27x_probe function so ohci_hcd_pxa27x_drv_probe
> function is unuseful that is why removed ohci_hcd_pxa27x_drv_probe
> function and renamed usb_hcd_pxa27x_probe function to
> ohci_hcd_pxa27x_drv_probe for proper naming.
> 
> The ohci_hcd_pxa27x_remove function is also not doing anything other than
> calling usb_hcd_pxa27x_remove that is why removed ohci_hcd_pxa27x_remove
> function and renamed usb_hcd_pxa27x_remove to ohci_hcd_pxa27x_remove for
> proper naming.

A few things should be improved.

> Signed-off-by: Manjunath Goudar <csmanjuvijay@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/usb/host/ohci-pxa27x.c | 41 +++++++++++++++--------------------------
>  1 file changed, 15 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
> index a667cf2..c5efcd1 100644
> --- a/drivers/usb/host/ohci-pxa27x.c
> +++ b/drivers/usb/host/ohci-pxa27x.c
> @@ -404,7 +404,7 @@ static int ohci_pxa_of_init(struct platform_device *pdev)
>  
>  
>  /**
> - * usb_hcd_pxa27x_probe - initialize pxa27x-based HCDs
> + * ohci_hcd_pxa27x_probe - initialize pxa27x-based HCDs
>   * Context: !in_interrupt()
>   *
>   * Allocates basic resources for this USB host controller, and
> @@ -412,7 +412,7 @@ static int ohci_pxa_of_init(struct platform_device *pdev)
>   * through the hotplug entry's driver_data.
>   *
>   */
> -int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device *pdev)
> +static int ohci_hcd_pxa27x_probe (struct platform_device *pdev)

You should remove the space between "probe" and "(".  checkpatch.pl 
will warn you about this.  Didn't you run it?

>  {
>  	int retval, irq;
>  	struct usb_hcd *hcd;
> @@ -423,6 +423,11 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device
>  	struct clk *usb_clk;
>  	unsigned int i;
>  
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	pr_debug ("In ohci_hcd_pxa27x_drv_probe");
> +

Leave these out.  The driver already checks for usb_disabled() in 
ohci_pxa27x_init(); there's no reason to check it again.  And the debug 
line seems rather unimportant.

>  	retval = ohci_pxa_of_init(pdev);
>  	if (retval)
>  		return retval;
> @@ -442,7 +447,7 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device
>  	if (IS_ERR(usb_clk))
>  		return PTR_ERR(usb_clk);
>  
> -	hcd = usb_create_hcd (driver, &pdev->dev, "pxa27x");
> +	hcd = usb_create_hcd (&ohci_pxa27x_hc_driver, &pdev->dev, "pxa27x");

Again, fix up warnings from checkpatch.pl.  Here and below.

Alan Stern

>  	if (!hcd)
>  		return -ENOMEM;
>  
> @@ -503,17 +508,18 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct platform_device
>  /* may be called with controller, bus, and devices active */
>  
>  /**
> - * usb_hcd_pxa27x_remove - shutdown processing for pxa27x-based HCDs
> + * ohci_hcd_pxa27x_remove - shutdown processing for pxa27x-based HCDs
>   * @dev: USB Host Controller being removed
>   * Context: !in_interrupt()
>   *
> - * Reverses the effect of usb_hcd_pxa27x_probe(), first invoking
> + * Reverses the effect of ohci_hcd_pxa27x_probe(), first invoking
>   * the HCD's stop() method.  It is always called from a thread
>   * context, normally "rmmod", "apmd", or something similar.
>   *
>   */
> -void usb_hcd_pxa27x_remove (struct usb_hcd *hcd, struct platform_device *pdev)
> +static int ohci_hcd_pxa27x_remove (struct platform_device *pdev)
>  {
> +	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  	struct pxa27x_ohci *pxa_ohci = to_pxa27x_ohci(hcd);
>  	unsigned int i;
>  
> @@ -524,28 +530,11 @@ void usb_hcd_pxa27x_remove (struct usb_hcd *hcd, struct platform_device *pdev)
>  		pxa27x_ohci_set_vbus_power(pxa_ohci, i, false);
>  
>  	usb_put_hcd(hcd);
> +	return 0;
>  }
>  
>  /*-------------------------------------------------------------------------*/
>  
> -static int ohci_hcd_pxa27x_drv_probe(struct platform_device *pdev)
> -{
> -	pr_debug ("In ohci_hcd_pxa27x_drv_probe");
> -
> -	if (usb_disabled())
> -		return -ENODEV;
> -
> -	return usb_hcd_pxa27x_probe(&ohci_pxa27x_hc_driver, pdev);
> -}
> -
> -static int ohci_hcd_pxa27x_drv_remove(struct platform_device *pdev)
> -{
> -	struct usb_hcd *hcd = platform_get_drvdata(pdev);
> -
> -	usb_hcd_pxa27x_remove(hcd, pdev);
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM
>  static int ohci_hcd_pxa27x_drv_suspend(struct device *dev)
>  {
> @@ -598,8 +587,8 @@ static const struct dev_pm_ops ohci_hcd_pxa27x_pm_ops = {
>  #endif
>  
>  static struct platform_driver ohci_hcd_pxa27x_driver = {
> -	.probe		= ohci_hcd_pxa27x_drv_probe,
> -	.remove		= ohci_hcd_pxa27x_drv_remove,
> +	.probe		= ohci_hcd_pxa27x_probe,
> +	.remove		= ohci_hcd_pxa27x_remove,
>  	.shutdown	= usb_hcd_platform_shutdown,
>  	.driver		= {
>  		.name	= "pxa27x-ohci",
> 

Re: [PATCH v2] USB: OHCI: ohci-pxa27x: remove useless functions

[Greg Kroah-Hartman]

On Wed, Nov 30, 2016 at 11:59:49PM +0000, csmanjuvijay@gmail.com wrote:
> From: Manjunath Goudar <csmanjuvijay@gmail.com>
> 
> The ohci_hcd_pxa27x_drv_probe function is not doing anything other
> than calling usb_hcd_pxa27x_probe function so ohci_hcd_pxa27x_drv_probe
> function is useless that is why removed ohci_hcd_pxa27x_drv_probe
> function and renamed usb_hcd_pxa27x_probe function to
> ohci_hcd_pxa27x_drv_probe for proper naming.
> 
> The ohci_hcd_pxa27x_remove function is also not doing anything other than
> calling usb_hcd_pxa27x_remove that is why removed ohci_hcd_pxa27x_remove
> function and renamed usb_hcd_pxa27x_remove to ohci_hcd_pxa27x_remove for
> proper naming.
> 
> Signed-off-by: Manjunath Goudar <csmanjuvijay@gmail.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Changelog v1 -> v2:
> 1.Removed warnings and errors of checkpatch.pl script.
> 2.Replaced unuseful with useless in patch commit message for proper meaning.
> 3.Removed usb_disabled() from ohci_hcd_pxa27x_drv_probe function because it
>   is already existing in ohci_pxa27x_init function
> ---
>  drivers/usb/host/ohci-pxa27x.c | 42 ++++++++++++++----------------------------
>  1 file changed, 14 insertions(+), 28 deletions(-)

changelog should go below --- line.


> 
> diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
> index a667cf2..171f28f2 100644
> --- a/drivers/usb/host/ohci-pxa27x.c
> +++ b/drivers/usb/host/ohci-pxa27x.c
> @@ -157,9 +157,9 @@ static int pxa27x_ohci_select_pmm(struct pxa27x_ohci *pxa_ohci, int mode)
>  		uhcrhdb |= (0x7<<17);
>  		break;
>  	default:
> -		printk( KERN_ERR
> +		printk(KERN_ERR

No, don't do checkpatch cleanups in portions of the code you are not
actually changing for your "real" patch.  That's a mess.

You can send checkpatch patches as follow-on patches, but not with this
one.

thanks,

greg k-h