| Message ID | 20250409154356.423512-3-visitorckw@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce parity_odd() and refactor redundant parity code | expand |
On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote: > On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote: > > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote: > > > Refactor parity calculations to use the standard parity_odd() helper. > > > This change eliminates redundant implementations. > > > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > > --- > > > drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > index 70a4024d461e..5e1b7b1742e4 100644 > > > --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > @@ -5,6 +5,7 @@ > > > * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved. > > > */ > > > > > > +#include <linux/bitops.h> > > > #include <linux/errno.h> > > > #include <linux/kernel.h> > > > #include <linux/ktime.h> > > > @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = { > > > > > > static u8 calc_parity(u8 val) > > > { > > > - unsigned i; > > > - unsigned tot = 0; > > > - > > > - for (i = 0; i < 7; i++) > > > - tot += (val & (1 << i)) ? 1 : 0; > > > - return val | ((tot & 1) ? 0 : 0x80); > > > + return val | (parity_odd(val) ? 0 : 0x80); > > > > So, if val == 0 than parity_odd(val) is also 0, and this can be > > simplified just to: > > return parity(val) ? 0 : 0x80; > > Or I miss something? > > > If val == 0x01, the return value of calc_parity() will remain 0x01. > If changed to return parity_odd(val) ? 0 : 0x80;, the return value will > be changed to 0x00. Sorry, I meant return val ? 0 : 0x80; This 'val | (parity_odd(val)' is only false when val == 0, right? When val != 0, compiler will return true immediately, not even calling parity(). I think we need a comment from authors.
On Wed, Apr 09, 2025 at 02:41:03PM -0400, Yury Norov wrote: > On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote: > > On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote: > > > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote: > > > > Refactor parity calculations to use the standard parity_odd() helper. > > > > This change eliminates redundant implementations. > > > > > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > > > --- > > > > drivers/media/test-drivers/vivid/vivid-vbi-gen.c | 8 ++------ > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > > index 70a4024d461e..5e1b7b1742e4 100644 > > > > --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > > +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c > > > > @@ -5,6 +5,7 @@ > > > > * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved. > > > > */ > > > > > > > > +#include <linux/bitops.h> > > > > #include <linux/errno.h> > > > > #include <linux/kernel.h> > > > > #include <linux/ktime.h> > > > > @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = { > > > > > > > > static u8 calc_parity(u8 val) > > > > { > > > > - unsigned i; > > > > - unsigned tot = 0; > > > > - > > > > - for (i = 0; i < 7; i++) > > > > - tot += (val & (1 << i)) ? 1 : 0; > > > > - return val | ((tot & 1) ? 0 : 0x80); > > > > + return val | (parity_odd(val) ? 0 : 0x80); > > > > > > So, if val == 0 than parity_odd(val) is also 0, and this can be > > > simplified just to: > > > return parity(val) ? 0 : 0x80; > > > Or I miss something? > > > > > If val == 0x01, the return value of calc_parity() will remain 0x01. > > If changed to return parity_odd(val) ? 0 : 0x80;, the return value will > > be changed to 0x00. > > Sorry, I meant > return val ? 0 : 0x80; > > This 'val | (parity_odd(val)' is only false when val == 0, right? > When val != 0, compiler will return true immediately, not even calling > parity(). > I'm still confused. Maybe you're interpreting the code as: (val | parity(val)) ? 0 : 0x80 But what we're trying to do is: val | (parity(val) ? 0 : 0x80) So, for example, if val == 0x06, the return value will be 0x86. Only returning 0 or 0x80 seems wrong to me. Or did I misunderstand something? Regards, Kuan-Wei > I think we need a comment from authors.
On Thu, Apr 10, 2025 at 02:56:41AM +0800, Kuan-Wei Chiu wrote: > On Wed, Apr 09, 2025 at 02:41:03PM -0400, Yury Norov wrote: > > On Thu, Apr 10, 2025 at 02:23:09AM +0800, Kuan-Wei Chiu wrote: > > > On Wed, Apr 09, 2025 at 01:03:42PM -0400, Yury Norov wrote: > > > > On Wed, Apr 09, 2025 at 11:43:45PM +0800, Kuan-Wei Chiu wrote: > > > > So, if val == 0 than parity_odd(val) is also 0, and this can be > > > > simplified just to: > > > > return parity(val) ? 0 : 0x80; > > > > Or I miss something? > > > > > > > If val == 0x01, the return value of calc_parity() will remain 0x01. > > > If changed to return parity_odd(val) ? 0 : 0x80;, the return value will > > > be changed to 0x00. > > > > Sorry, I meant > > return val ? 0 : 0x80; > > > > This 'val | (parity_odd(val)' is only false when val == 0, right? > > When val != 0, compiler will return true immediately, not even calling > > parity(). > > > I'm still confused. > > Maybe you're interpreting the code as: > > (val | parity(val)) ? 0 : 0x80 > > But what we're trying to do is: > > val | (parity(val) ? 0 : 0x80) > > So, for example, if val == 0x06, the return value will be 0x86. > Only returning 0 or 0x80 seems wrong to me. > Or did I misunderstand something? I misread the whole construction. Sorry, you're right. Scratch this.
diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c index 70a4024d461e..5e1b7b1742e4 100644 --- a/drivers/media/test-drivers/vivid/vivid-vbi-gen.c +++ b/drivers/media/test-drivers/vivid/vivid-vbi-gen.c @@ -5,6 +5,7 @@ * Copyright 2014 Cisco Systems, Inc. and/or its affiliates. All rights reserved. */ +#include <linux/bitops.h> #include <linux/errno.h> #include <linux/kernel.h> #include <linux/ktime.h> @@ -165,12 +166,7 @@ static const u8 vivid_cc_sequence2[30] = { static u8 calc_parity(u8 val) { - unsigned i; - unsigned tot = 0; - - for (i = 0; i < 7; i++) - tot += (val & (1 << i)) ? 1 : 0; - return val | ((tot & 1) ? 0 : 0x80); + return val | (parity_odd(val) ? 0 : 0x80); } static void vivid_vbi_gen_set_time_of_day(u8 *packet)