[PATCH] hw/audio/es1370: Avoid four-letter word

[PATCH] hw/audio/es1370: Avoid four-letter word

[Thomas Huth]

Using certain four-letter words is not good style in source code,
so let's avoid that.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/audio/es1370.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
index 91c47330ad..bd460c810e 100644
--- a/hw/audio/es1370.c
+++ b/hw/audio/es1370.c
@@ -670,8 +670,7 @@ static void es1370_transfer_audio (ES1370State *s, struct chan *d, int loop_sel,
     cnt += (transferred + d->leftover) >> 2;
 
     if (s->sctl & loop_sel) {
-        /* Bah, how stupid is that having a 0 represent true value?
-           i just spent few hours on this shit */
+        /* Bah, how stupid is that having a 0 represent true value? */
         AUD_log ("es1370: warning", "non looping mode\n");
     } else {
         d->frame_cnt = size;
-- 
2.41.0

Re: [PATCH] hw/audio/es1370: Avoid four-letter word

[Daniel P. Berrangé]

On Thu, Nov 09, 2023 at 06:35:44PM +0100, Thomas Huth wrote:
> Using certain four-letter words is not good style in source code,
> so let's avoid that.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/audio/es1370.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] hw/audio/es1370: Avoid four-letter word

[Peter Maydell]

On Thu, 9 Nov 2023 at 17:36, Thomas Huth <thuth@redhat.com> wrote:
>
> Using certain four-letter words is not good style in source code,
> so let's avoid that.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/audio/es1370.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
> index 91c47330ad..bd460c810e 100644
> --- a/hw/audio/es1370.c
> +++ b/hw/audio/es1370.c
> @@ -670,8 +670,7 @@ static void es1370_transfer_audio (ES1370State *s, struct chan *d, int loop_sel,
>      cnt += (transferred + d->leftover) >> 2;
>
>      if (s->sctl & loop_sel) {
> -        /* Bah, how stupid is that having a 0 represent true value?
> -           i just spent few hours on this shit */
> +        /* Bah, how stupid is that having a 0 represent true value? */
>          AUD_log ("es1370: warning", "non looping mode\n");
>      } else {
>          d->frame_cnt = size;
> --
> 2.41.0

We could be more usefully clear here anyway:

/*
 * loop_sel tells us which bit in the SCTL register to look at
 * (either P1_LOOP_SEL, P2_LOOP_SEL or R1_LOOP_SEL). The sense
 * of these bits is 0 for loop mode (set interrupt and keep recording
 * when the sample count reaches zero) or 1 for stop mode (set
 * interrupt and stop recording).
 */

PS: while we are cleaning up comments in this source file,
how about the bit marked /* Start blatant GPL violation */ ?

I think what that comment is trying to say is "most of this
source file is under the MIT license per the comment at the top
of the file, but these register constants are from the Linux
kernel sources and so they are GPL2". Both these licenses are
fine for QEMU, but we should have the commentary at the top
say "file is this license except for the GPL2-only macros etc"
rather than misleadingly claiming this is a GPL violation...

thanks
-- PMM

Re: [PATCH] hw/audio/es1370: Avoid four-letter word

[Thomas Huth]

On 09/11/2023 19.01, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 17:36, Thomas Huth <thuth@redhat.com> wrote:
>>
>> Using certain four-letter words is not good style in source code,
>> so let's avoid that.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/audio/es1370.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
>> index 91c47330ad..bd460c810e 100644
>> --- a/hw/audio/es1370.c
>> +++ b/hw/audio/es1370.c
>> @@ -670,8 +670,7 @@ static void es1370_transfer_audio (ES1370State *s, struct chan *d, int loop_sel,
>>       cnt += (transferred + d->leftover) >> 2;
>>
>>       if (s->sctl & loop_sel) {
>> -        /* Bah, how stupid is that having a 0 represent true value?
>> -           i just spent few hours on this shit */
>> +        /* Bah, how stupid is that having a 0 represent true value? */
>>           AUD_log ("es1370: warning", "non looping mode\n");
>>       } else {
>>           d->frame_cnt = size;
>> --
>> 2.41.0
> 
> We could be more usefully clear here anyway:
> 
> /*
>   * loop_sel tells us which bit in the SCTL register to look at
>   * (either P1_LOOP_SEL, P2_LOOP_SEL or R1_LOOP_SEL). The sense
>   * of these bits is 0 for loop mode (set interrupt and keep recording
>   * when the sample count reaches zero) or 1 for stop mode (set
>   * interrupt and stop recording).
>   */

Since you already formulated that, could you please send it as a proper 
patch? I'll drop my trivial patch here then.

  Thomas

Re: [PATCH] hw/audio/es1370: Avoid four-letter word

[Daniel P. Berrangé]

On Thu, Nov 09, 2023 at 06:01:13PM +0000, Peter Maydell wrote:
> On Thu, 9 Nov 2023 at 17:36, Thomas Huth <thuth@redhat.com> wrote:
> >
> > Using certain four-letter words is not good style in source code,
> > so let's avoid that.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  hw/audio/es1370.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
> > index 91c47330ad..bd460c810e 100644
> > --- a/hw/audio/es1370.c
> > +++ b/hw/audio/es1370.c
> > @@ -670,8 +670,7 @@ static void es1370_transfer_audio (ES1370State *s, struct chan *d, int loop_sel,
> >      cnt += (transferred + d->leftover) >> 2;
> >
> >      if (s->sctl & loop_sel) {
> > -        /* Bah, how stupid is that having a 0 represent true value?
> > -           i just spent few hours on this shit */
> > +        /* Bah, how stupid is that having a 0 represent true value? */
> >          AUD_log ("es1370: warning", "non looping mode\n");
> >      } else {
> >          d->frame_cnt = size;
> > --
> > 2.41.0
> 
> We could be more usefully clear here anyway:
> 
> /*
>  * loop_sel tells us which bit in the SCTL register to look at
>  * (either P1_LOOP_SEL, P2_LOOP_SEL or R1_LOOP_SEL). The sense
>  * of these bits is 0 for loop mode (set interrupt and keep recording
>  * when the sample count reaches zero) or 1 for stop mode (set
>  * interrupt and stop recording).
>  */
> 
> PS: while we are cleaning up comments in this source file,
> how about the bit marked /* Start blatant GPL violation */ ?
> 
> I think what that comment is trying to say is "most of this
> source file is under the MIT license per the comment at the top
> of the file, but these register constants are from the Linux
> kernel sources and so they are GPL2". Both these licenses are
> fine for QEMU, but we should have the commentary at the top
> say "file is this license except for the GPL2-only macros etc"
> rather than misleadingly claiming this is a GPL violation...

Mere definitions of constants aren't generally considered to be
copyrightable material. They're simply data needed for the purposes
of any implementation. The accompanying text could potentially
become copyrightable if it had non-trivial prose, but the stuff
there is essentially just more specification of values.

IOW, I would likley not consider this copying to be a GPL
violation, even if GPL were not otherwise fine for QEMU.

It is still poor etiquette to not acknowledge the source
of the constants though. So in addition to simply mentioning
the GPL soruce as you suggest,  I would suggest we replace
the "GPL violation" comment with:

  /*
   * These constants were copied from the linux kernel
   * sources at /the/file/path
   */

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|