2018-04-05 22:21:35

by Mark Greer

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

On Wed, Apr 04, 2018 at 12:02:46AM +0530, Gaurav Dhingra wrote:
> Wrap comment to fix warning "prefer a maximum 75 chars per line"
>
> Signed-off-by: Gaurav Dhingra <[email protected]>
> ---
> drivers/staging/greybus/audio_codec.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index a1d5440..01838d9 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -23,7 +23,9 @@ enum {
> NUM_CODEC_DAIS,
> };
>
> -/* device_type should be same as defined in audio.h (Android media layer) */
> +/* device_type should be same as defined in audio.h
> + * (Android media layer)
> + */
> enum {
> GBAUDIO_DEVICE_NONE = 0x0,
> /* reserved bits */
> --
> 1.9.1

Hi Gaurav.

Thank you for the patch, it looks fine to me.

Reviewed-by: Mark Greer <[email protected]>


2018-04-06 05:23:43

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

On Fri, Apr 6, 2018 at 3:49 AM, Mark Greer <[email protected]> wrote:
> On Wed, Apr 04, 2018 at 12:02:46AM +0530, Gaurav Dhingra wrote:
>> Wrap comment to fix warning "prefer a maximum 75 chars per line"
>>
>> Signed-off-by: Gaurav Dhingra <[email protected]>
>> ---
>> drivers/staging/greybus/audio_codec.h | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
>> index a1d5440..01838d9 100644
>> --- a/drivers/staging/greybus/audio_codec.h
>> +++ b/drivers/staging/greybus/audio_codec.h
>> @@ -23,7 +23,9 @@ enum {
>> NUM_CODEC_DAIS,
>> };
>>
>> -/* device_type should be same as defined in audio.h (Android media layer) */
>> +/* device_type should be same as defined in audio.h

This isn't the right way to write a multi-line comment. It should be like:

/*
* XXXX
* XXXX
*/

>> + * (Android media layer)
>> + */
>> enum {
>> GBAUDIO_DEVICE_NONE = 0x0,
>> /* reserved bits */
>> --
>> 1.9.1
>
> Hi Gaurav.
>
> Thank you for the patch, it looks fine to me.
>
> Reviewed-by: Mark Greer <[email protected]>

2018-04-06 10:18:49

by Gaurav Dhingra

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

Hi,

Thanks for reviewing the patch.


On Friday 06 April 2018 10:52 AM, Viresh Kumar wrote:
> On Fri, Apr 6, 2018 at 3:49 AM, Mark Greer <[email protected]> wrote:
>> On Wed, Apr 04, 2018 at 12:02:46AM +0530, Gaurav Dhingra wrote:
>>> Wrap comment to fix warning "prefer a maximum 75 chars per line"
>>>
>>> Signed-off-by: Gaurav Dhingra <[email protected]>
>>> ---
>>> drivers/staging/greybus/audio_codec.h | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
>>> index a1d5440..01838d9 100644
>>> --- a/drivers/staging/greybus/audio_codec.h
>>> +++ b/drivers/staging/greybus/audio_codec.h
>>> @@ -23,7 +23,9 @@ enum {
>>> NUM_CODEC_DAIS,
>>> };
>>>
>>> -/* device_type should be same as defined in audio.h (Android media layer) */
>>> +/* device_type should be same as defined in audio.h
> This isn't the right way to write a multi-line comment. It should be like:
>
> /*
> * XXXX
> * XXXX
> */
I sent in an updated patchset. Though I forgot to add
[email protected] to "To" in mail. I tried to follow instructions
described on https://kernelnewbies.org/FirstKernelPatch for updating my
patch. Do you think I followed the instructions correctly? I was
thinking may be I need to update the already sent patch by adding *new
commit* to my already existing commit on that git branch, but instead I
tried to do what I understood from the website I mentioned above.
>>> + * (Android media layer)
>>> + */
>>> enum {
>>> GBAUDIO_DEVICE_NONE = 0x0,
>>> /* reserved bits */
>>> --
>>> 1.9.1
>> Hi Gaurav.
>>
>> Thank you for the patch, it looks fine to me.
>>
>> Reviewed-by: Mark Greer <[email protected]>

--
Gaurav Dhingra
(sent from Thunderbird email client)


2018-04-06 10:26:43

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

On 06-04-18, 15:47, Gaurav Dhingra wrote:
> I sent in an updated patchset. Though I forgot to add
> [email protected] to "To" in mail.

That's fine, but you still haven't sent it to all the relevant people.
You should have used the get_maintainer script (present in kernel
source) for that.

$ scripts/get_maintainer.pl drivers/staging/greybus/audio_codec.h

Vaibhav Agarwal <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
Mark Greer <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
Johan Hovold <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
Alex Elder <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
Greg Kroah-Hartman <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
[email protected] (moderated list:GREYBUS SUBSYSTEM)
[email protected] (open list:STAGING SUBSYSTEM)
[email protected] (open list)

> I tried to follow instructions
> described on https://kernelnewbies.org/FirstKernelPatch for updating my
> patch. Do you think I followed the instructions correctly?

Mostly yes, you did it fine.

> I was thinking
> may be I need to update the already sent patch by adding *new commit* to my
> already existing commit on that git branch, but instead I tried to do what I
> understood from the website I mentioned above.

Sorry, I find it difficult to understand what you wrote ;)

--
viresh

2018-04-06 10:29:08

by Viresh Kumar

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

On 06-04-18, 15:55, Viresh Kumar wrote:
> On 06-04-18, 15:47, Gaurav Dhingra wrote:
> > I sent in an updated patchset. Though I forgot to add
> > [email protected] to "To" in mail.
>
> That's fine, but you still haven't sent it to all the relevant people.
> You should have used the get_maintainer script (present in kernel
> source) for that.
>
> $ scripts/get_maintainer.pl drivers/staging/greybus/audio_codec.h
>
> Vaibhav Agarwal <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
> Mark Greer <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
> Johan Hovold <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
> Alex Elder <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
> Greg Kroah-Hartman <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
> [email protected] (moderated list:GREYBUS SUBSYSTEM)
> [email protected] (open list:STAGING SUBSYSTEM)
> [email protected] (open list)

Actually your cc list in V2 is fine, it wasn't correct in v1.

--
viresh

2018-04-06 10:30:59

by Gaurav Dhingra

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line



On Friday 06 April 2018 03:56 PM, Viresh Kumar wrote:
> On 06-04-18, 15:55, Viresh Kumar wrote:
>> On 06-04-18, 15:47, Gaurav Dhingra wrote:
>>> I sent in an updated patchset. Though I forgot to add
>>> [email protected] to "To" in mail.
>> That's fine, but you still haven't sent it to all the relevant people.
>> You should have used the get_maintainer script (present in kernel
>> source) for that.
>>
>> $ scripts/get_maintainer.pl drivers/staging/greybus/audio_codec.h
>>
>> Vaibhav Agarwal <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
>> Mark Greer <[email protected]> (maintainer:GREYBUS AUDIO PROTOCOLS DRIVERS)
>> Johan Hovold <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
>> Alex Elder <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
>> Greg Kroah-Hartman <[email protected]> (maintainer:GREYBUS SUBSYSTEM)
>> [email protected] (moderated list:GREYBUS SUBSYSTEM)
>> [email protected] (open list:STAGING SUBSYSTEM)
>> [email protected] (open list)
> Actually your cc list in V2 is fine, it wasn't correct in v1.
Yes, I too think it is correct. In v1 I forgot to remove '--nol' from
command shown on kernelnewbies/FirstKernelPatch webpage and that removed
a few relevant mailing lists.

--
Gaurav Dhingra
(sent from Thunderbird email client)


2018-04-06 16:03:48

by Mark Greer

[permalink] [raw]
Subject: Re: [PATCH] staging: greybus: Fix warning to limit chars per line

On Fri, Apr 06, 2018 at 10:52:17AM +0530, Viresh Kumar wrote:
> On Fri, Apr 6, 2018 at 3:49 AM, Mark Greer <[email protected]> wrote:
> > On Wed, Apr 04, 2018 at 12:02:46AM +0530, Gaurav Dhingra wrote:
> >> Wrap comment to fix warning "prefer a maximum 75 chars per line"
> >>
> >> Signed-off-by: Gaurav Dhingra <[email protected]>
> >> ---
> >> drivers/staging/greybus/audio_codec.h | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> >> index a1d5440..01838d9 100644
> >> --- a/drivers/staging/greybus/audio_codec.h
> >> +++ b/drivers/staging/greybus/audio_codec.h
> >> @@ -23,7 +23,9 @@ enum {
> >> NUM_CODEC_DAIS,
> >> };
> >>
> >> -/* device_type should be same as defined in audio.h (Android media layer) */
> >> +/* device_type should be same as defined in audio.h
>
> This isn't the right way to write a multi-line comment. It should be like:
>
> /*
> * XXXX
> * XXXX
> */

Ugh, yeah, you're right. I must have been sleeping.

Mark
--