* Re: [PATCH] hw/audio/es1370: Avoid four-letter word
2023-11-09 18:01 ` Peter Maydell
@ 2023-11-09 18:04 ` Thomas Huth
2023-11-09 18:21 ` Daniel P. Berrangé
1 sibling, 0 replies; 5+ messages in thread
From: Thomas Huth @ 2023-11-09 18:04 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, Gerd Hoffmann, qemu-trivial, Daniel P. Berrange
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/audio/es1370: Avoid four-letter word
2023-11-09 18:01 ` Peter Maydell
2023-11-09 18:04 ` Thomas Huth
@ 2023-11-09 18:21 ` Daniel P. Berrangé
1 sibling, 0 replies; 5+ messages in thread
From: Daniel P. Berrangé @ 2023-11-09 18:21 UTC (permalink / raw)
To: Peter Maydell; +Cc: Thomas Huth, qemu-devel, Gerd Hoffmann, qemu-trivial
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 :|
^ permalink raw reply [flat|nested] 5+ messages in thread