| Message ID | 20180427150848.12075-9-pure.logic@nexus-software.ie |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fixup set_bit/clear_bit definition and usage | expand |
Hi Bryan, > Compiling the f_mass_storage driver for an x86 target results in a > compilation error as set_bit and clear_bit are provided by bitops.h > > Looking at the provenance of the current u-boot code and the git > change history in the kernel, it looks like we have a local copy of > set_bit and clear_bit as a hold-over from porting the Linux driver > into u-boot. > > These days __set_bit and __clear_bit are optionally provided by an > arch and can be used as inputs to generic_bit_set and > generic_bit_clear. > > This patch switches over to generic_set_bit and generic_clear_bit to > accommodate. > > Tested on i.MX WaRP7 and Intel Edison Just I'm a bit puzzled: This is v3 8/9, followed by 9/9 (v1)? And after some time you sent v2 8/9 and v2 9/9. I suppose that the latter ones are correct? Also it is very handy to have some kind of change log in patches - i.e.: Changes for v2: - Switching to generic_set|clear bit functions - ...... The patch seems OK, but I need to test them on my machines. > > Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > Cc: Lukasz Majewski <lukma@denx.de> > Cc: Marek Vasut <marex@denx.de> > --- > drivers/usb/gadget/f_mass_storage.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > > diff --git a/drivers/usb/gadget/f_mass_storage.c > b/drivers/usb/gadget/f_mass_storage.c index 1ecb92ac6b..af42136e1d > 100644 --- a/drivers/usb/gadget/f_mass_storage.c > +++ b/drivers/usb/gadget/f_mass_storage.c > @@ -252,6 +252,7 @@ > #include <usb_mass_storage.h> > > #include <asm/unaligned.h> > +#include <asm/bitops.h> > #include <linux/usb/gadget.h> > #include <linux/usb/gadget.h> > #include <linux/usb/composite.h> > @@ -283,26 +284,6 @@ static const char fsg_string_interface[] = "Mass > Storage"; struct kref {int x; }; > struct completion {int x; }; > > -inline void set_bit(int nr, volatile void *addr) > -{ > - int mask; > - unsigned int *a = (unsigned int *) addr; > - > - a += nr >> 5; > - mask = 1 << (nr & 0x1f); > - *a |= mask; > -} > - > -inline void clear_bit(int nr, volatile void *addr) > -{ > - int mask; > - unsigned int *a = (unsigned int *) addr; > - > - a += nr >> 5; > - mask = 1 << (nr & 0x1f); > - *a &= ~mask; > -} > - > struct fsg_dev; > struct fsg_common; > > @@ -2086,7 +2067,7 @@ static int received_cbw(struct fsg_dev *fsg, > struct fsg_buffhd *bh) > * we can simply accept and discard any data received > * until the next reset. */ > wedge_bulk_in_endpoint(fsg); > - set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); > + generic_set_bit(IGNORE_BULK_OUT, > &fsg->atomic_bitflags); return -EINVAL; > } > > @@ -2250,7 +2231,7 @@ reset: > fsg->bulk_out_enabled = 1; > common->bulk_out_maxpacket = > le16_to_cpu(get_unaligned(&d->wMaxPacketSize)); > - clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); > + generic_clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); > > /* Allocate the requests */ > for (i = 0; i < FSG_NUM_BUFFERS; ++i) { Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On 27/04/18 17:20, Lukasz Majewski wrote: > Hi Bryan, > >> Compiling the f_mass_storage driver for an x86 target results in a >> compilation error as set_bit and clear_bit are provided by bitops.h >> >> Looking at the provenance of the current u-boot code and the git >> change history in the kernel, it looks like we have a local copy of >> set_bit and clear_bit as a hold-over from porting the Linux driver >> into u-boot. >> >> These days __set_bit and __clear_bit are optionally provided by an >> arch and can be used as inputs to generic_bit_set and >> generic_bit_clear. >> >> This patch switches over to generic_set_bit and generic_clear_bit to >> accommodate. >> >> Tested on i.MX WaRP7 and Intel Edison > > Just I'm a bit puzzled: > > This is v3 8/9, followed by 9/9 (v1)? I sent out two previous versions of this patch as a standalone, if you recall. > And after some time you sent v2 8/9 and v2 9/9. > > I suppose that the latter ones are correct? > > Also it is very handy to have some kind of change log in patches - i.e.: Cover letter contains, and I believe best practice is to keep the changelog in the cover-letter - not the patch itself. ## V2: - Fix commit log nds2 -> nds32 - Fix copy/paste error resulting in double colon "arch : : text" V1: Following on from a discussion with Marek and Lukasz re: a namespace collision with set_bit and clear_bit in f_mass_storage, I noticed some inconsistencies in the definition and usage of PLATFORM__SET_BIT and PLATFORM__CLEAR_BIT as well as a similar use of __set_bit in the composite USB gadget driver. __set_bit is lock-prefixed on x86 whereas set_bit is not and the analog driver in upstream Linux does set_bit() not __set_bit(). This series addresses all of those inconsistencies. There are some usages of __set_bit() but those are in SoC specific GPIO code-paths and therefore don't really need to change IMO. ## Anyway I'll resend this set as a v3 since Bin Meng needs a change to the log text anyway. --- bod
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 1ecb92ac6b..af42136e1d 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -252,6 +252,7 @@ #include <usb_mass_storage.h> #include <asm/unaligned.h> +#include <asm/bitops.h> #include <linux/usb/gadget.h> #include <linux/usb/gadget.h> #include <linux/usb/composite.h> @@ -283,26 +284,6 @@ static const char fsg_string_interface[] = "Mass Storage"; struct kref {int x; }; struct completion {int x; }; -inline void set_bit(int nr, volatile void *addr) -{ - int mask; - unsigned int *a = (unsigned int *) addr; - - a += nr >> 5; - mask = 1 << (nr & 0x1f); - *a |= mask; -} - -inline void clear_bit(int nr, volatile void *addr) -{ - int mask; - unsigned int *a = (unsigned int *) addr; - - a += nr >> 5; - mask = 1 << (nr & 0x1f); - *a &= ~mask; -} - struct fsg_dev; struct fsg_common; @@ -2086,7 +2067,7 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh) * we can simply accept and discard any data received * until the next reset. */ wedge_bulk_in_endpoint(fsg); - set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); + generic_set_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); return -EINVAL; } @@ -2250,7 +2231,7 @@ reset: fsg->bulk_out_enabled = 1; common->bulk_out_maxpacket = le16_to_cpu(get_unaligned(&d->wMaxPacketSize)); - clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); + generic_clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags); /* Allocate the requests */ for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
Compiling the f_mass_storage driver for an x86 target results in a compilation error as set_bit and clear_bit are provided by bitops.h Looking at the provenance of the current u-boot code and the git change history in the kernel, it looks like we have a local copy of set_bit and clear_bit as a hold-over from porting the Linux driver into u-boot. These days __set_bit and __clear_bit are optionally provided by an arch and can be used as inputs to generic_bit_set and generic_bit_clear. This patch switches over to generic_set_bit and generic_clear_bit to accommodate. Tested on i.MX WaRP7 and Intel Edison Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> Cc: Lukasz Majewski <lukma@denx.de> Cc: Marek Vasut <marex@denx.de> --- drivers/usb/gadget/f_mass_storage.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-)