| Message ID | 20231109173544.375129-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/audio/es1370: Avoid four-letter word | expand |
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
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
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
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
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;
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(-)