Re: [U-Boot] [PATCH] usb: f_mass_storage: Fix compile on x86

2018-04-26 Thread Lukasz Majewski 
Hi,

> On 04/26/2018 06:58 PM, Bryan O'Donoghue wrote:
> > On 26/04/18 16:14, Marek Vasut wrote:  
> >> On 04/26/2018 04:41 PM, Bryan O'Donoghue wrote:  
> >>> 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
> >>>
> >>> Fix that now by only compiling up the local definition of set_bit
> >>> and clear_bit only if not already provided by the environment.
> >>>
> >>> Signed-off-by: Bryan O'Donoghue 
> >>> Cc: Lukasz Majewski 
> >>> Cc: Marek Vasut 
> >>> ---
> >>>   drivers/usb/gadget/f_mass_storage.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/gadget/f_mass_storage.c
> >>> b/drivers/usb/gadget/f_mass_storage.c
> >>> index 1ecb92ac6b..2e856af6ed 100644
> >>> --- a/drivers/usb/gadget/f_mass_storage.c
> >>> +++ b/drivers/usb/gadget/f_mass_storage.c
> >>> @@ -283,6 +283,7 @@ static const char fsg_string_interface[] =
> >>> "Mass Storage";
> >>>   struct kref {int x; };
> >>>   struct completion {int x; };
> >>>   +#ifndef _I386_BITOPS_H
> >>>   inline void set_bit(int nr, volatile void *addr)
> >>>   {
> >>>   int    mask;
> >>> @@ -302,6 +303,7 @@ inline void clear_bit(int nr, volatile void
> >>> *addr) mask = 1 << (nr & 0x1f);
> >>>   *a &= ~mask;
> >>>   }
> >>> +#endif /* _I386_BITOPTS_H */  
> >>
> >> This doesn't look right, generic driver shouldn't contain
> >> arch-specific fixup or ifdef. Can this be somehow abstracted out?
> >>  
> > 
> > Hmm.
> > 
> > On a second look - the name of these functions should change not to
> > conflict with bitops.h - there's some funny bit-shifting going on
> > there..  
> 
> Better :)
> 

The best option would be to replace those inlines with functions
already provided in bitops.h - delete local instances and then re-use
ones from the generic header.


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: w...@denx.de


pgpyRYLar28oa.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: f_mass_storage: Fix compile on x86

2018-04-26 Thread Marek Vasut 
On 04/26/2018 06:58 PM, Bryan O'Donoghue wrote:
> On 26/04/18 16:14, Marek Vasut wrote:
>> On 04/26/2018 04:41 PM, Bryan O'Donoghue wrote:
>>> 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
>>>
>>> Fix that now by only compiling up the local definition of set_bit and
>>> clear_bit only if not already provided by the environment.
>>>
>>> Signed-off-by: Bryan O'Donoghue 
>>> Cc: Lukasz Majewski 
>>> Cc: Marek Vasut 
>>> ---
>>>   drivers/usb/gadget/f_mass_storage.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/f_mass_storage.c
>>> b/drivers/usb/gadget/f_mass_storage.c
>>> index 1ecb92ac6b..2e856af6ed 100644
>>> --- a/drivers/usb/gadget/f_mass_storage.c
>>> +++ b/drivers/usb/gadget/f_mass_storage.c
>>> @@ -283,6 +283,7 @@ static const char fsg_string_interface[] = "Mass
>>> Storage";
>>>   struct kref {int x; };
>>>   struct completion {int x; };
>>>   +#ifndef _I386_BITOPS_H
>>>   inline void set_bit(int nr, volatile void *addr)
>>>   {
>>>   int    mask;
>>> @@ -302,6 +303,7 @@ inline void clear_bit(int nr, volatile void *addr)
>>>   mask = 1 << (nr & 0x1f);
>>>   *a &= ~mask;
>>>   }
>>> +#endif /* _I386_BITOPTS_H */
>>
>> This doesn't look right, generic driver shouldn't contain arch-specific
>> fixup or ifdef. Can this be somehow abstracted out?
>>
> 
> Hmm.
> 
> On a second look - the name of these functions should change not to
> conflict with bitops.h - there's some funny bit-shifting going on there..

Better :)

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: f_mass_storage: Fix compile on x86

2018-04-26 Thread Bryan O'Donoghue 

On 26/04/18 16:14, Marek Vasut wrote:

On 04/26/2018 04:41 PM, Bryan O'Donoghue wrote:

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

Fix that now by only compiling up the local definition of set_bit and
clear_bit only if not already provided by the environment.

Signed-off-by: Bryan O'Donoghue 
Cc: Lukasz Majewski 
Cc: Marek Vasut 
---
  drivers/usb/gadget/f_mass_storage.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/f_mass_storage.c 
b/drivers/usb/gadget/f_mass_storage.c
index 1ecb92ac6b..2e856af6ed 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -283,6 +283,7 @@ static const char fsg_string_interface[] = "Mass Storage";
  struct kref {int x; };
  struct completion {int x; };
  
+#ifndef _I386_BITOPS_H

  inline void set_bit(int nr, volatile void *addr)
  {
int mask;
@@ -302,6 +303,7 @@ inline void clear_bit(int nr, volatile void *addr)
mask = 1 << (nr & 0x1f);
*a &= ~mask;
  }
+#endif /* _I386_BITOPTS_H */


This doesn't look right, generic driver shouldn't contain arch-specific
fixup or ifdef. Can this be somehow abstracted out?



Hmm.

On a second look - the name of these functions should change not to 
conflict with bitops.h - there's some funny bit-shifting going on there..


---
bod
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] usb: f_mass_storage: Fix compile on x86

2018-04-26 Thread Marek Vasut 
On 04/26/2018 04:41 PM, Bryan O'Donoghue wrote:
> 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
> 
> Fix that now by only compiling up the local definition of set_bit and
> clear_bit only if not already provided by the environment.
> 
> Signed-off-by: Bryan O'Donoghue 
> Cc: Lukasz Majewski 
> Cc: Marek Vasut 
> ---
>  drivers/usb/gadget/f_mass_storage.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/f_mass_storage.c 
> b/drivers/usb/gadget/f_mass_storage.c
> index 1ecb92ac6b..2e856af6ed 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -283,6 +283,7 @@ static const char fsg_string_interface[] = "Mass Storage";
>  struct kref {int x; };
>  struct completion {int x; };
>  
> +#ifndef _I386_BITOPS_H
>  inline void set_bit(int nr, volatile void *addr)
>  {
>   int mask;
> @@ -302,6 +303,7 @@ inline void clear_bit(int nr, volatile void *addr)
>   mask = 1 << (nr & 0x1f);
>   *a &= ~mask;
>  }
> +#endif /* _I386_BITOPTS_H */

This doesn't look right, generic driver shouldn't contain arch-specific
fixup or ifdef. Can this be somehow abstracted out?

>  struct fsg_dev;
>  struct fsg_common;
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot