[PATCH v2] staging: greybus: arche-platform: fix line over 80 characters style warnings

[Alex Elder]

On 11/29/2016 03:11 PM, Andrea Ghittino wrote:
> Fixes greybus "line over 80 characters" style warnings 
> found by checkpatch.pl tool

I have a few comments.  Please update your patch and send
a new version.

> Signed-off-by: Andrea Ghittino <aghittino at gmail.com>

You state that this is v2; how does this version differ
from what you posted last time?

You should normally summarize a history of changes between
patch versions, in the space below the "---" line:

> ---

Here.

So you might do something like:

v2: - Rebased against the latest version of the code
    - Reworded the description of the patch
    - Cleaned up a white space problem reported by John Doe

> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> index 3fda0cd..755120a 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -168,7 +168,10 @@ static int standby_boot_seq(struct platform_device *pdev)
>  	if (apb->init_disabled)
>  		return 0;
>  
> -	/* Even if it is in OFF state, then we do not want to change the state */
> +	/*
> +	 * Even if it is in OFF state,
> +	 * then we do not want to change the state
> +	 */

The 80 characters per line rule is not all that critical.  That
being said, I don't object to fixing these issues.

However in this case I find the way you formatted it looks
strange to my eyes--comments with very short lines look a
little odd to me.  In my environment, I set up my editor to
wrap at about column 70, but you can go beyond that.  Doing
multi-line comments that are much shorter than that is unusual
though.

Two suggestions:
- Shorten the comment line by just moving a word or two to the
  next line, e.g.:
	/*
	 * Even if it is in OFF state, then we do not want to change
	 * the state
	 */
- Sometimes (and this particular example is one of those times)
  the comment can be slightly reworded, resulting in something
  just as good but fitting the 80 column limit.  E.g.:
	/* Don't change state if it is in standby, or off */
  (Note, I haven't looked closely at the code to verify that
  my wording here actually matches what it does.)

These general suggestions apply to a few other of your fixes,
below.

>  	if (apb->state == ARCHE_PLATFORM_STATE_STANDBY ||
>  			apb->state == ARCHE_PLATFORM_STATE_OFF)
>  		return 0;
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index 338c2d3..18ab2b4 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -295,7 +295,10 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  				arche_platform_set_wake_detect_state(arche_pdata,
>  								     WD_STATE_IDLE);
>  			} else {
> -				/* Check we are not in middle of irq thread already */
> +				/*
> +				 * Check we are not in middle
> +				 * of irq thread already
> +				 */
>  				if (arche_pdata->wake_detect_state !=
>  						WD_STATE_COLDBOOT_START) {
>  					arche_platform_set_wake_detect_state(arche_pdata,
> @@ -312,12 +315,14 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  		if (arche_pdata->wake_detect_state == WD_STATE_IDLE) {
>  			arche_pdata->wake_detect_start = jiffies;
>  			/*
> -			 * In the begining, when wake/detect goes low (first time), we assume
> -			 * it is meant for coldboot and set the flag. If wake/detect line stays low
> -			 * beyond 30msec, then it is coldboot else fallback to standby boot.
> +			 * In the begining, when wake/detect goes low

I think it would be nice to fix this spelling error (s/begin/beginn/)
too.  Some people might argue that belongs in another patch.  Either
way, since you're fixing this comment, consider fixing everything
that's wrong with it while you're at it.

> +			 * (first time), we assume it is meant for coldboot
> +			 * and set the flag. If wake/detect line stays low
> +			 * beyond 30msec, then it is coldboot else
> +			 * fallback to standby boot.
>  			 */
>  			arche_platform_set_wake_detect_state(arche_pdata,
> -							     WD_STATE_BOOT_INIT);
> +							     WD_STATE_BOOT_INIT);

I'm not sure what changed in the line above--it must be in the white
space or something.  Either mention that you're doing it in the
patch description, or eliminate this change from the patch.

					-Alex

>  		}
>  	}
>  
> @@ -561,7 +566,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	int ret;
>  
> -	arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata), GFP_KERNEL);
> +	arche_pdata = devm_kzalloc(&pdev->dev,
> +				   sizeof(*arche_pdata),
> +				   GFP_KERNEL);
>  	if (!arche_pdata)
>  		return -ENOMEM;
>  
> @@ -780,12 +787,18 @@ static SIMPLE_DEV_PM_OPS(arche_platform_pm_ops,
>  			arche_platform_resume);
>  
>  static const struct of_device_id arche_platform_of_match[] = {
> -	{ .compatible = "google,arche-platform", }, /* Use PID/VID of SVC device */
> +	{
> +		/* Use PID/VID of SVC device */
> +		.compatible = "google,arche-platform",
> +	},
>  	{ },
>  };
>  
>  static const struct of_device_id arche_combined_id[] = {
> -	{ .compatible = "google,arche-platform", }, /* Use PID/VID of SVC device */
> +	{
> +		/* Use PID/VID of SVC device */
> +		.compatible = "google,arche-platform",
> +	},
>  	{ .compatible = "usbffff,2", },
>  	{ },
>  };
> diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
> index f8862c6..e8010c8 100644
> --- a/drivers/staging/greybus/audio_codec.c
> +++ b/drivers/staging/greybus/audio_codec.c
> @@ -831,7 +831,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module)
>  		snd_soc_dapm_link_component_dai_widgets(codec->card,
>  							&codec->dapm);
>  #ifdef CONFIG_SND_JACK
> -		/* register jack devices for this module from codec->jack_list */
> +		/*
> +		 * register jack devices for this module from
> +		 * codec->jack_list
> +		 */
>  		list_for_each_entry(jack, &codec->jack_list, list) {
>  			if ((jack == &module->headset_jack)
>  			    || (jack == &module->button_jack))
>