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@xxxxxxxxx>
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))
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
