[PATCH 1/2] staging: rtl8712: throw away custom hex_to_bin()

[PATCH 1/2] staging: rtl8712: throw away custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 1529239..eac049b 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -62,17 +62,6 @@ static const char * const iw_operation_mode[] = {
 	 "Monitor"
 };
 
-static int hex2num_i(char c)
-{
-	if (c >= '0' && c <= '9')
-		return c - '0';
-	if (c >= 'a' && c <= 'f')
-		return c - 'a' + 10;
-	if (c >= 'A' && c <= 'F')
-		return c - 'A' + 10;
-	return -1;
-}
-
 /**
  * hwaddr_aton - Convert ASCII string to MAC address
  * @txt: MAC address as a string (e.g., "00:11:22:33:44:55")
@@ -86,10 +75,10 @@ static int hwaddr_aton_i(const char *txt, u8 *addr)
 	for (i = 0; i < 6; i++) {
 		int a, b;
 
-		a = hex2num_i(*txt++);
+		a = hex_to_bin(*txt++);
 		if (a < 0)
 			return -1;
-		b = hex2num_i(*txt++);
+		b = hex_to_bin(*txt++);
 		if (b < 0)
 			return -1;
 		*addr++ = (a << 4) | b;
-- 
1.7.2.2

[PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Andy Shevchenko]

There are already implemented methods such hex_to_bin() or isxdigit() in
the kernel. Let's use them.

Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 .../staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c   |   31 ++++++-------------
 drivers/staging/ath6kl/os/linux/eeprom.c           |   32 +++++++++----------
 2 files changed, 25 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
index 5eed8ac..8dce054 100644
--- a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
+++ b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
@@ -27,6 +27,9 @@
 
 #include "ar3kpsparser.h"
 
+#include <linux/ctype.h>
+#include <linux/kernel.h>
+
 #define BD_ADDR_SIZE            6
 #define WRITE_PATCH             8
 #define ENABLE_PATCH            11
@@ -61,20 +64,6 @@
 #define MAX_BYTE_LENGTH                   244
 
 #define SKIP_BLANKS(str) while (*str == ' ') str++
-#define MIN(x, y) (((x) <= (y))? (x):(y))
-#define MAX(x, y) (((x) >= (y))? (x):(y))
-
-#define UNUSED(x) (x=x)
-
-#define IS_BETWEEN(x, lower, upper) (((lower) <= (x)) && ((x) <= (upper)))
-#define IS_DIGIT(c) (IS_BETWEEN((c), '0', '9'))
-#define IS_HEX(c)   (IS_BETWEEN((c), '0', '9') || IS_BETWEEN((c), 'a', 'f') || IS_BETWEEN((c), 'A', 'F'))
-#define TO_LOWER(c) (IS_BETWEEN((c), 'A', 'Z') ? ((c) - 'A' + 'a') : (c))
-#define IS_BLANK(c) ((c) == ' ')
-#define CONV_DEC_DIGIT_TO_VALUE(c) ((c) - '0')
-#define CONV_HEX_DIGIT_TO_VALUE(c) (IS_DIGIT(c) ? ((c) - '0') : (IS_BETWEEN((c), 'A', 'Z') ? ((c) - 'A' + 10) : ((c) - 'a' + 10))) 
-#define CONV_VALUE_TO_HEX(v) ((A_UINT8)( ((v & 0x0F) <= 9) ? ((v & 0x0F) + '0') : ((v & 0x0F) - 10 + 'A') ) )
-
 
 enum MinBootFileFormatE
 {
@@ -483,12 +472,12 @@ A_STATUS AthParseFilesUnified(A_UCHAR *srcbuffer,A_UINT32 srclen, int FileFormat
                     if((stPS_DataFormat.eDataType == eHex) && stPS_DataFormat.bIsArray == true) {
                        while(uReadCount > 0) {
                            PsTagEntry[TagCount].TagData[stReadStatus.uByteCount] =
-                                                     (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount]) << 4)
-                                                     | (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount + 1]));
+                                                     (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount]) << 4)
+                                                     | (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 1]));
 
                            PsTagEntry[TagCount].TagData[stReadStatus.uByteCount+1] =
-                                                     (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount + 3]) << 4)
-                                                     | (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount + 4]));
+                                                     (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 3]) << 4)
+                                                     | (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 4]));
 
                            stReadStatus.uCharCount += 6; // read two bytes, plus a space;
                            stReadStatus.uByteCount += 2;
@@ -574,14 +563,14 @@ A_STATUS GetNextTwoChar(A_UCHAR *srcbuffer,A_UINT32 len, A_UINT32 *pos, char * b
     unsigned char ch;
 
     ch = AthReadChar(srcbuffer,len,pos);
-    if(ch != '\0' && IS_HEX(ch)) {
+    if(ch != '\0' && isxdigit(ch)) {
         buffer[0] =  ch;
     } else 
     {
         return A_ERROR;
     }
     ch = AthReadChar(srcbuffer,len,pos);
-    if(ch != '\0' && IS_HEX(ch)) {
+    if(ch != '\0' && isxdigit(ch)) {
         buffer[1] =  ch;
     } else 
     {
@@ -606,7 +595,7 @@ A_STATUS AthDoParsePatch(A_UCHAR *patchbuffer, A_UINT32 patchlen)
     Patch_Count = 0;
 
     while(NULL != AthGetLine(Line,MAX_BYTE_LENGTH,patchbuffer,patchlen,&filepos)) {
-        if(strlen(Line) <= 1 || !IS_HEX(Line[0])) {
+        if(strlen(Line) <= 1 || !isxdigit(Line[0])) {
             continue;
         } else {
             break;
diff --git a/drivers/staging/ath6kl/os/linux/eeprom.c b/drivers/staging/ath6kl/os/linux/eeprom.c
index 8dd130a..be77fb8 100644
--- a/drivers/staging/ath6kl/os/linux/eeprom.c
+++ b/drivers/staging/ath6kl/os/linux/eeprom.c
@@ -71,24 +71,22 @@ wmic_ether_aton(const char *orig, A_UINT8 *eth)
   i = 0;
   for(bufp = orig; *bufp != '\0'; ++bufp) {
     unsigned int val;
-    unsigned char c = *bufp++;
-    if (c >= '0' && c <= '9') val = c - '0';
-    else if (c >= 'a' && c <= 'f') val = c - 'a' + 10;
-    else if (c >= 'A' && c <= 'F') val = c - 'A' + 10;
-    else {
-        printk("%s: MAC value is invalid\n", __FUNCTION__);
-        break;
-    }
+	int h, l;
 
-    val <<= 4;
-    c = *bufp++;
-    if (c >= '0' && c <= '9') val |= c - '0';
-    else if (c >= 'a' && c <= 'f') val |= c - 'a' + 10;
-    else if (c >= 'A' && c <= 'F') val |= c - 'A' + 10;
-    else {
-        printk("%s: MAC value is invalid\n", __FUNCTION__);
-        break;
-    }
+	h = hex_to_bin(*bufp++);
+
+	if (h < 0) {
+		printk("%s: MAC value is invalid\n", __FUNCTION__);
+		break;
+	}
+
+	l = hex_to_bin(*bufp++);
+	if (l < 0) {
+		printk("%s: MAC value is invalid\n", __FUNCTION__);
+		break;
+	}
+
+	val = (h << 4) | l;
 
     eth[i] = (unsigned char) (val & 0377);
     if(++i == ATH_MAC_LEN) {
-- 
1.7.2.2

RE: [PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Vipin Mehta]

Andy,
   Thanks for the patch. Although the driver supports Linux but it has been architected to support other OSes if desired. The code is, therefore, organized into generic and OS specific components with the latter part of the code captured into the os/linux directory. As such, the patch will be more useful if we can move out the inclusion of linux specific header files <linux/kernel.h> and <linux/ctype.h> into a header file in os/linux/include directory. You can refer to os/linux/include/osapi_linux.h for example. A macro could be defined which would use linux specific APIs for Linux and keep the original definitions for any other OS.

I'll update the documentation on the ath6kl wiki to make it more informative.

Regards,
Vipin


> -----Original Message-----
> From: devel-bounces@linuxdriverproject.org [mailto:devel-
> bounces@linuxdriverproject.org] On Behalf Of Andy Shevchenko
> Sent: Tuesday, September 07, 2010 8:20 AM
> To: linux-kernel@vger.kernel.org; Greg Kroah-Hartman;
> devel@driverdev.osuosl.org
> Cc: Andy Shevchenko
> Subject: [PATCH 2/2] staging: ath6kl: use native methods from kernel
> library
> 
> There are already implemented methods such hex_to_bin() or isxdigit() in
> the kernel. Let's use them.
> 
> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  .../staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c   |   31 ++++++---------
> ----
>  drivers/staging/ath6kl/os/linux/eeprom.c           |   32 +++++++++------
> ----
>  2 files changed, 25 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
> b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
> index 5eed8ac..8dce054 100644
> --- a/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
> +++ b/drivers/staging/ath6kl/miscdrv/ar3kps/ar3kpsparser.c
> @@ -27,6 +27,9 @@
> 
>  #include "ar3kpsparser.h"
> 
> +#include <linux/ctype.h>
> +#include <linux/kernel.h>
> +
>  #define BD_ADDR_SIZE            6
>  #define WRITE_PATCH             8
>  #define ENABLE_PATCH            11
> @@ -61,20 +64,6 @@
>  #define MAX_BYTE_LENGTH                   244
> 
>  #define SKIP_BLANKS(str) while (*str == ' ') str++
> -#define MIN(x, y) (((x) <= (y))? (x):(y))
> -#define MAX(x, y) (((x) >= (y))? (x):(y))
> -
> -#define UNUSED(x) (x=x)
> -
> -#define IS_BETWEEN(x, lower, upper) (((lower) <= (x)) && ((x) <=
> (upper)))
> -#define IS_DIGIT(c) (IS_BETWEEN((c), '0', '9'))
> -#define IS_HEX(c)   (IS_BETWEEN((c), '0', '9') || IS_BETWEEN((c), 'a',
> 'f') || IS_BETWEEN((c), 'A', 'F'))
> -#define TO_LOWER(c) (IS_BETWEEN((c), 'A', 'Z') ? ((c) - 'A' + 'a') : (c))
> -#define IS_BLANK(c) ((c) == ' ')
> -#define CONV_DEC_DIGIT_TO_VALUE(c) ((c) - '0')
> -#define CONV_HEX_DIGIT_TO_VALUE(c) (IS_DIGIT(c) ? ((c) - '0') :
> (IS_BETWEEN((c), 'A', 'Z') ? ((c) - 'A' + 10) : ((c) - 'a' + 10)))
> -#define CONV_VALUE_TO_HEX(v) ((A_UINT8)( ((v & 0x0F) <= 9) ? ((v & 0x0F)
> + '0') : ((v & 0x0F) - 10 + 'A') ) )
> -
> 
>  enum MinBootFileFormatE
>  {
> @@ -483,12 +472,12 @@ A_STATUS AthParseFilesUnified(A_UCHAR
> *srcbuffer,A_UINT32 srclen, int FileFormat
>                      if((stPS_DataFormat.eDataType == eHex) &&
> stPS_DataFormat.bIsArray == true) {
>                         while(uReadCount > 0) {
> 
> PsTagEntry[TagCount].TagData[stReadStatus.uByteCount] =
> -
> (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount]) <<
> 4)
> -                                                     |
> (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount +
> 1]));
> +
> (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount]) << 4)
> +                                                     |
> (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 1]));
> 
> 
> PsTagEntry[TagCount].TagData[stReadStatus.uByteCount+1] =
> -
> (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount + 3])
> << 4)
> -                                                     |
> (A_UINT8)(CONV_HEX_DIGIT_TO_VALUE(pCharLine[stReadStatus.uCharCount +
> 4]));
> +
> (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 3]) << 4)
> +                                                     |
> (A_UINT8)(hex_to_bin(pCharLine[stReadStatus.uCharCount + 4]));
> 
>                             stReadStatus.uCharCount += 6; // read two
> bytes, plus a space;
>                             stReadStatus.uByteCount += 2;
> @@ -574,14 +563,14 @@ A_STATUS GetNextTwoChar(A_UCHAR *srcbuffer,A_UINT32
> len, A_UINT32 *pos, char * b
>      unsigned char ch;
> 
>      ch = AthReadChar(srcbuffer,len,pos);
> -    if(ch != '\0' && IS_HEX(ch)) {
> +    if(ch != '\0' && isxdigit(ch)) {
>          buffer[0] =  ch;
>      } else
>      {
>          return A_ERROR;
>      }
>      ch = AthReadChar(srcbuffer,len,pos);
> -    if(ch != '\0' && IS_HEX(ch)) {
> +    if(ch != '\0' && isxdigit(ch)) {
>          buffer[1] =  ch;
>      } else
>      {
> @@ -606,7 +595,7 @@ A_STATUS AthDoParsePatch(A_UCHAR *patchbuffer,
> A_UINT32 patchlen)
>      Patch_Count = 0;
> 
>      while(NULL !=
> AthGetLine(Line,MAX_BYTE_LENGTH,patchbuffer,patchlen,&filepos)) {
> -        if(strlen(Line) <= 1 || !IS_HEX(Line[0])) {
> +        if(strlen(Line) <= 1 || !isxdigit(Line[0])) {
>              continue;
>          } else {
>              break;
> diff --git a/drivers/staging/ath6kl/os/linux/eeprom.c
> b/drivers/staging/ath6kl/os/linux/eeprom.c
> index 8dd130a..be77fb8 100644
> --- a/drivers/staging/ath6kl/os/linux/eeprom.c
> +++ b/drivers/staging/ath6kl/os/linux/eeprom.c
> @@ -71,24 +71,22 @@ wmic_ether_aton(const char *orig, A_UINT8 *eth)
>    i = 0;
>    for(bufp = orig; *bufp != '\0'; ++bufp) {
>      unsigned int val;
> -    unsigned char c = *bufp++;
> -    if (c >= '0' && c <= '9') val = c - '0';
> -    else if (c >= 'a' && c <= 'f') val = c - 'a' + 10;
> -    else if (c >= 'A' && c <= 'F') val = c - 'A' + 10;
> -    else {
> -        printk("%s: MAC value is invalid\n", __FUNCTION__);
> -        break;
> -    }
> +	int h, l;
> 
> -    val <<= 4;
> -    c = *bufp++;
> -    if (c >= '0' && c <= '9') val |= c - '0';
> -    else if (c >= 'a' && c <= 'f') val |= c - 'a' + 10;
> -    else if (c >= 'A' && c <= 'F') val |= c - 'A' + 10;
> -    else {
> -        printk("%s: MAC value is invalid\n", __FUNCTION__);
> -        break;
> -    }
> +	h = hex_to_bin(*bufp++);
> +
> +	if (h < 0) {
> +		printk("%s: MAC value is invalid\n", __FUNCTION__);
> +		break;
> +	}
> +
> +	l = hex_to_bin(*bufp++);
> +	if (l < 0) {
> +		printk("%s: MAC value is invalid\n", __FUNCTION__);
> +		break;
> +	}
> +
> +	val = (h << 4) | l;
> 
>      eth[i] = (unsigned char) (val & 0377);
>      if(++i == ATH_MAC_LEN) {
> --
> 1.7.2.2
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel

Re: [PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Andy Shevchenko]

On Wed, Sep 8, 2010 at 12:18 AM, Vipin Mehta <Vipin.Mehta@atheros.com> wrote:
> Andy,
>   Thanks for the patch. Although the driver supports Linux but it has been architected to support other OSes if desired. The code is, therefore, organized into generic and OS specific components with the latter part of the code captured into the os/linux directory. As such, the patch will be more useful if we can move out the inclusion of linux specific header files <linux/kernel.h> and <linux/ctype.h> into a header file in os/linux/include directory. You can refer to os/linux/include/osapi_linux.h for example. A macro could be defined which would use linux specific APIs for Linux and keep the original definitions for any other OS.

The file ar3kpsparser.h, which is directly included by patched one,
contains few includes like <linux/XXX.h>. Due to this I consider this
part of code as OS-dependent. That's why I decide to put necessary
headers inside c-file.
Thus I think patch could be applied w/o modifications. However, If you
want to do me some odd stuff (at least until I have not got the clear
explanation), I could provide you 3 patches: clean up file under
os/linux, remove unused macros, and fix in ar3kpsparser in way when
necessary os-dependent headers will be located at some common header.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Greg KH]

On Tue, Sep 07, 2010 at 02:18:02PM -0700, Vipin Mehta wrote:
> Andy,
> Thanks for the patch. Although the driver supports Linux but it has
> been architected to support other OSes if desired. The code is,
> therefore, organized into generic and OS specific components with the
> latter part of the code captured into the os/linux directory. As such,
> the patch will be more useful if we can move out the inclusion of
> linux specific header files <linux/kernel.h> and <linux/ctype.h> into
> a header file in os/linux/include directory.

Ick, no.

The driver is now in the main kernel tree, the "compatiblity" wrappers
are to now be removed as the kernel code does not need to run on any
other operating systems.

That whole subdir needs to be deleted entirely.

> You can refer to
> os/linux/include/osapi_linux.h for example. A macro could be defined
> which would use linux specific APIs for Linux and keep the original
> definitions for any other OS.

Again, no, this is not ok at all.  If you are going to insist on trying
to do this for the driver, I will just delete it from the tree right now
as that is now how kernel development happens, sorry.

thanks,

greg k-h

RE: [PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Vipin Mehta]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Tuesday, September 07, 2010 5:04 PM
> To: Vipin Mehta
> Cc: Andy Shevchenko; linux-kernel@vger.kernel.org;
> devel@driverdev.osuosl.org
> Subject: Re: [PATCH 2/2] staging: ath6kl: use native methods from kernel
> library
> 
> On Tue, Sep 07, 2010 at 02:18:02PM -0700, Vipin Mehta wrote:
> > Andy,
> > Thanks for the patch. Although the driver supports Linux but it has
> > been architected to support other OSes if desired. The code is,
> > therefore, organized into generic and OS specific components with the
> > latter part of the code captured into the os/linux directory. As such,
> > the patch will be more useful if we can move out the inclusion of
> > linux specific header files <linux/kernel.h> and <linux/ctype.h> into
> > a header file in os/linux/include directory.
> 
> Ick, no.
> 
> The driver is now in the main kernel tree, the "compatiblity" wrappers
> are to now be removed as the kernel code does not need to run on any
> other operating systems.
> 
> That whole subdir needs to be deleted entirely.
> 
> > You can refer to
> > os/linux/include/osapi_linux.h for example. A macro could be defined
> > which would use linux specific APIs for Linux and keep the original
> > definitions for any other OS.
> 
> Again, no, this is not ok at all.  If you are going to insist on trying
> to do this for the driver, I will just delete it from the tree right now
> as that is now how kernel development happens, sorry.

Got it. I think we can discard my suggestion in that case. The patch looks good otherwise. It is ok from my side and could be pushed in to the driver.
 
> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/2] staging: ath6kl: use native methods from kernel library

[Greg KH]

On Tue, Sep 07, 2010 at 07:29:30PM -0700, Vipin Mehta wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@suse.de]
> > Sent: Tuesday, September 07, 2010 5:04 PM
> > To: Vipin Mehta
> > Cc: Andy Shevchenko; linux-kernel@vger.kernel.org;
> > devel@driverdev.osuosl.org
> > Subject: Re: [PATCH 2/2] staging: ath6kl: use native methods from kernel
> > library
> > 
> > On Tue, Sep 07, 2010 at 02:18:02PM -0700, Vipin Mehta wrote:
> > > Andy,
> > > Thanks for the patch. Although the driver supports Linux but it has
> > > been architected to support other OSes if desired. The code is,
> > > therefore, organized into generic and OS specific components with the
> > > latter part of the code captured into the os/linux directory. As such,
> > > the patch will be more useful if we can move out the inclusion of
> > > linux specific header files <linux/kernel.h> and <linux/ctype.h> into
> > > a header file in os/linux/include directory.
> > 
> > Ick, no.
> > 
> > The driver is now in the main kernel tree, the "compatiblity" wrappers
> > are to now be removed as the kernel code does not need to run on any
> > other operating systems.
> > 
> > That whole subdir needs to be deleted entirely.
> > 
> > > You can refer to
> > > os/linux/include/osapi_linux.h for example. A macro could be defined
> > > which would use linux specific APIs for Linux and keep the original
> > > definitions for any other OS.
> > 
> > Again, no, this is not ok at all.  If you are going to insist on trying
> > to do this for the driver, I will just delete it from the tree right now
> > as that is now how kernel development happens, sorry.
> 
> Got it. I think we can discard my suggestion in that case. The patch
> looks good otherwise. It is ok from my side and could be pushed in to
> the driver.

Thanks, will do.

greg k-h