Re: [OSSNA] Intro to kernel hacking tutorial

[Tobin C. Harding]

On Mon, Sep 02, 2019 at 10:08:54AM -0400, Valdis Klētnieks wrote:
> On Mon, 02 Sep 2019 15:42:19 +0300, Anatoly Pugachev said:
> 
> > is it intentionally that you use
> >
> > yes "" | make oldconfig
> >
> > instead of
> >
> > make olddefconfig
> 
> They do something different.  'olddefconfig' just takes the platform or
> architecture defconfig and updates it for any new CONFIG_* variables added
> since the last time the defconfig was updated in the tree.
> 
> yes "" | make oldconfig  does the same updating for new CONFIG_* variables, 
> but
> starts with the most recent .config - which produces wildly different results
> if the .config had previously been minimized by 'make localmodconfig' or other
> similar techniques.

Thanks Valdis, you're the man!


   Tobin




signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [OSSNA] Intro to kernel hacking tutorial

[Tobin C. Harding]

On Sun, Sep 01, 2019 at 05:30:23AM +0530, Amit Kumar wrote:
> Hi,
> I think now your tutorial should be ready.

I do  not understand what this means sorry.  Is it a request for action?
The tutorial was a couple of weeks ago now, here is a link to the
material if that is what you were asking

https://github.com/tcharding/kernel/tree/master/workshop

Cheers,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging/erofs: Balanced braces around a few conditional statements.

[Tobin C. Harding]

On Wed, Aug 21, 2019 at 10:31:22AM +0800, Gao Xiang wrote:
> On Tue, Aug 20, 2019 at 07:26:46PM -0700, Joe Perches wrote:
> > On Tue, 2019-08-20 at 20:18 -0400, Caitlyn wrote:
> > > Balanced braces to fix some checkpath warnings in inode.c and
> > > unzip_vle.c
> > []
> > > diff --git a/drivers/staging/erofs/unzip_vle.c 
> > > b/drivers/staging/erofs/unzip_vle.c
> > []
> > > @@ -915,21 +915,21 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> > >   mutex_lock(&work->lock);
> > >   nr_pages = work->nr_pages;
> > >  
> > > - if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES))
> > > + if (likely(nr_pages <= Z_EROFS_VLE_VMAP_ONSTACK_PAGES)) {
> > >   pages = pages_onstack;
> > > - else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > -  mutex_trylock(&z_pagemap_global_lock))
> > > + } else if (nr_pages <= Z_EROFS_VLE_VMAP_GLOBAL_PAGES &&
> > > +  mutex_trylock(&z_pagemap_global_lock)) {
> > 
> > Extra space after tab
> 
> There is actually balanced braces in linux-next.
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/erofs/zdata.c#n762

Which tree did these changes go in through please Gao?  I believe
Caitlyn was working off of the staging-next branch of Greg's staging
tree.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] comedi: remove camelcase

[Tobin C. Harding]

On Wed, Aug 21, 2019 at 02:26:18AM -0700, Greg KH wrote:
> On Tue, Aug 20, 2019 at 09:12:51PM -0700, Edmund Huber wrote:
> > My apologies. Is it possible that you are replying to a different thread
> > than intended? I don't think I have an email addressed to me from the
> > patchbot.
> 
> I got a bunch of patches from a lot of different people all at once
> (Tobin must have run a class),

lols, I'm glad you have a new patchbot Greg :)  I hope it is cheap to
run, some of those messages it had to send were caused by omissions on
my behalf.

   Tobin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [OSSNA] Intro to kernel hacking tutorial

[Tobin C. Harding]

On Fri, Jul 19, 2019 at 12:36:58PM +0300, Dan Carpenter wrote:
> On Fri, Jul 05, 2019 at 12:50:55PM +1000, Tobin C. Harding wrote:
> > Outcome will (hopefully) be a small patch set into drivers/staging/.
> > (Don't worry Greg only one group got to this stage last time, you
> > won't get flooded with patches :)
> 
> We're good at reviewing floods of patches.  Send away.
> 
> In the end what we want is people who will take over a driver and
> understand it completely and become the maintainer.  We've had a few
> people that did appointed themselves to become the maintainer of a
> random driver and move it out of staging.  But even if people don't make
> it all the way to become a maintainer, it's nice when they start down
> that path by focusing on one driver and trying to understand it as much
> as possible.
> 
> Most of the time when you look at a new staging driver, then you do want
> to clean up the white space just because it's hard to look at
> non-standard code.  So that's the first step.  But then maybe start at
> the probe and release functions and clean it up.  Keep your eyes open
> to any other mistakes or bugs you see.  Write them down.  Then the
> ioctls.  Etc.  Look at the TODO too.
> 
> The other thing I wish people knew was about the relationship with
> maintainers.  When you start out, you're virtually anonymous for the
> first couple patchsets.  We get so many and they blend together so we
> don't remember your name.  So don't think that we mean anything
> personally if we don't apply your patch.  We have forgotten about the
> patch as soon as we reply to it.  Don't panic and resend quickly.  You
> will be too stressed.  Wait until the next day.
> 
> In staging we really want to apply patches (unless it's in staging
> because we're going to remove the code).  I get annoyed with other
> staging reviewers who NAK patches because "I don't like churn" or
> whatever.
> 
> On the other hand, patches just "silencing checkpatch.pl" is not a valid
> justification for sending a patch.  Patches should make the code more
> readable.
> 
> Anyway, maintainers are not monsters.  Very few people have made me
> annoyed to the point where I refuse to review their code.  And everyone
> else is in my good books so that's fine.

Cool, points noted.  Thanks Dan


Tobin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [OSSNA] Intro to kernel hacking tutorial

[Tobin C. Harding]

On Fri, Jul 05, 2019 at 10:40:43AM +0530, Amit Kumar wrote:
> On Fri, Jul 5, 2019 at 9:02 AM Amit Kumar  wrote:
> >
> > On Fri, Jul 5, 2019 at 8:21 AM Tobin C. Harding  wrote:
> > >
> > > Hi,
> > >
> > > I am doing a tutorial at OSSNA in San Diego on getting into kernel
> > > hacking.  I'm only a couple of years deep into kernel hacking so I
> > > wanted to reach out to those more experienced than myself (and those
> > > less experienced).
> > >
> > > Is there any thing that you would really like to see covered in this
> > > tutorial?
> > >
> > > If you are a grey beard is there anything that you have been lamenting
> > > us newbies not knowing/doing?
> > >
> > > If you are a newbie is there anything that you are struggling with that
> > > you really want to learn?
> > Thank you.
> > Where can I find your tutorial?

It's not written yet :)

> I forget to tell, merely creating and sending patches is not important.
> Also I would like to know how to manage patches, using git, mutt, quilt
> and so on.
> Sending patch through git-email is good. But different versions of patch.
> Applying patch from mutt. Replying to patch recipients.

Nice suggestions thanks, will work this in.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[OSSNA] Intro to kernel hacking tutorial

[Tobin C. Harding]

Hi,

I am doing a tutorial at OSSNA in San Diego on getting into kernel
hacking.  I'm only a couple of years deep into kernel hacking so I
wanted to reach out to those more experienced than myself (and those
less experienced).

Is there any thing that you would really like to see covered in this
tutorial?

If you are a grey beard is there anything that you have been lamenting
us newbies not knowing/doing?

If you are a newbie is there anything that you are struggling with that
you really want to learn?

Current format/content: the tutorial will attempt to bridge the gap in
the learning process between the 'first patch' page on kernelnewbies.org
wiki and being 'comfortable' patching the kernel via LKML.  Outcome will
(hopefully) be a small patch set into drivers/staging/.  (Don't worry
Greg only one group got to this stage last time, you won't get flooded
with patches :)

Thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 04/11] staging: vchiq_arm: Clear VLA warning

[Tobin C . Harding]

On Sat, Mar 31, 2018 at 10:09:40PM +0200, Stefan Wahren wrote:
> The kernel would like to have all stack VLA usage removed[1]. The array
> here is fixed (declared with a const variable) but it appears like a VLA
> to the compiler. Also, currently we are putting 768 bytes on the
> stack. So save stack space and removes the VLA build warning by
> making it static.
> 
> [1]: https://lkml.org/lkml/2018/3/7/621
> 
> CC: Tobin C. Harding 
> Signed-off-by: Stefan Wahren 
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c| 20 
> ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c 
> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 24d456b..f276437 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -3414,13 +3414,18 @@ vchiq_release_service(VCHIQ_SERVICE_HANDLE_T handle)
>   return ret;
>  }
>  
> +struct service_data_struct {
> + int fourcc;
> + int clientid;
> + int use_count;
> +};
> +
>  void
>  vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>  {
>   VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
> + static struct service_data_struct service_data[64];
>   int i, j = 0;
> - /* Only dump 64 services */
> - static const int local_max_services = 64;
>   /* If there's more than 64 services, only dump ones with
>* non-zero counts */
>   int only_nonzero = 0;
> @@ -3431,11 +3436,6 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>   int peer_count;
>   int vc_use_count;
>   int active_services;
> - struct service_data_struct {
> - int fourcc;
> - int clientid;
> - int use_count;
> - } service_data[local_max_services];
>  
>   if (!arm_state)
>   return;
> @@ -3446,10 +3446,10 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>   peer_count = arm_state->peer_use_count;
>   vc_use_count = arm_state->videocore_use_count;
>   active_services = state->unused_service;
> - if (active_services > local_max_services)
> + if (active_services > ARRAY_SIZE(service_data))
>   only_nonzero = 1;
>  
> - for (i = 0; (i < active_services) && (j < local_max_services); i++) {
> + for (i = 0; (i < active_services) && (j < ARRAY_SIZE(service_data)); 
> i++) {
>   VCHIQ_SERVICE_T *service_ptr = state->services[i];
>  
>   if (!service_ptr)
> @@ -3479,7 +3479,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
>   vchiq_log_warning(vchiq_susp_log_level, "Too many active "
>   "services (%d).  Only dumping up to first %d services "
>   "with non-zero use-count", active_services,
> - local_max_services);
> + ARRAY_SIZE(service_data));
>  
>   for (i = 0; i < j; i++) {
>   vchiq_log_warning(vchiq_susp_log_level,
> -- 
> 2.7.4
> 

On a previous attempt to fix this VLA Linus commented that 64 was too
big (it equates to 768 bytes on the stack).  Here is that email for your
reference

https://lkml.org/lkml/2018/3/9/744

I didn't understand what he meant by 'just do the stack size limitation'


Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: vchiq_arm: Clear VLA warning

[Tobin C. Harding]

On Mon, Mar 12, 2018 at 06:58:04AM +0100, Stefan Wahren wrote:
> Hi Tobin,
> 
> > "Tobin C. Harding"  hat am 12. März 2018 um 06:46 
> > geschrieben:
> > 
> > 
> > On Mon, Mar 12, 2018 at 12:37:53PM +1100, Tobin C. Harding wrote:
> > > The kernel would like to have all stack VLA usage removed[1].  The array
> > > here is fixed (declared with a const variable) but it appears like a VLA
> > > to the compiler.  Also, currently we are putting 768 bytes on the
> > > stack.  This function is only called on the error path so performance is
> > > not critical, let's just allocate the memory instead of using the
> > > stack.  This saves stack space and removes the VLA build warning.
> > > 
> > > kmalloc a buffer for dumping state instead of using the stack.
> > > 
> > > [1]: https://lkml.org/lkml/2018/3/7/621
> > > 
> > > Signed-off-by: Tobin C. Harding 
> > > ---
> > 
> > Drop this please, leaks memory.
> 
> except from the leak, did you test this patch on a RPi?

No I didn't, but I can have a go at it.  Will try before doing v3.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: vchiq_arm: Clear VLA warning

[Tobin C. Harding]

On Mon, Mar 12, 2018 at 12:37:53PM +1100, Tobin C. Harding wrote:
> The kernel would like to have all stack VLA usage removed[1].  The array
> here is fixed (declared with a const variable) but it appears like a VLA
> to the compiler.  Also, currently we are putting 768 bytes on the
> stack.  This function is only called on the error path so performance is
> not critical, let's just allocate the memory instead of using the
> stack.  This saves stack space and removes the VLA build warning.
> 
> kmalloc a buffer for dumping state instead of using the stack.
> 
> [1]: https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Tobin C. Harding 
> ---

Drop this please, leaks memory.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: vchiq_arm: Clear VLA warning

[Tobin C. Harding]

The kernel would like to have all stack VLA usage removed[1].  The array
here is fixed (declared with a const variable) but it appears like a VLA
to the compiler.  Also, currently we are putting 768 bytes on the
stack.  This function is only called on the error path so performance is
not critical, let's just allocate the memory instead of using the
stack.  This saves stack space and removes the VLA build warning.

kmalloc a buffer for dumping state instead of using the stack.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tobin C. Harding 
---

v1 of this patch already merged into staging-testing branch of Greg's
staging tree.  This patch depends on v1 being removed, can re-do this
one on top of tip of staging-testing if required.

v2:
 - Use kmalloc() instead of the stack

Patch is untested.  

 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f5cefda49b22..408ea73f8da7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3455,11 +3455,15 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
int fourcc;
int clientid;
int use_count;
-   } service_data[local_max_services];
+   } *service_data;
 
if (!arm_state)
return;
 
+   service_data = kmalloc_array(local_max_services, sizeof(*service_data), 
GFP_KERNEL);
+   if (!service_data)
+   return;
+
read_lock_bh(&arm_state->susp_res_lock);
vc_suspend_state = arm_state->vc_suspend_state;
vc_resume_state  = arm_state->vc_resume_state;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] video: Remove stack VLA usage

[Tobin C. Harding]

On Fri, Mar 09, 2018 at 12:16:21AM -0600, Gustavo A. R. Silva wrote:
> 
> I sent a patch for this six hours ago:
> 
> https://patchwork.kernel.org/patch/10268591/
> 
> --
> Gustavo

lol, I knew there would be a race on to fix these :)  And you got it
right, bonus points. Let's drop this one then.


thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: vchiq_arm: Clear VLA warning

[Tobin C. Harding]

The kernel would like to have all stack VLA usage removed[1].  The
array here is fixed (declared with a const variable) but it appears
like VLA to the compiler.  We can use a pre-processor define to quiet
the compiler.

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tobin C. Harding 
---

The name of this constant may need changing, there is already a
pre-processor constant VCHIQ_MAX_SERVICES

 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c  | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index f5cefda49b22..c972869a0333 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -3434,13 +3434,15 @@ vchiq_release_service(VCHIQ_SERVICE_HANDLE_T handle)
return ret;
 }
 
+/* Only dump 64 services */
+#define VCHIQ_LOCAL_MAX_SERVICES 64
+
 void
 vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
 {
VCHIQ_ARM_STATE_T *arm_state = vchiq_platform_get_arm_state(state);
int i, j = 0;
-   /* Only dump 64 services */
-   static const int local_max_services = 64;
+
/* If there's more than 64 services, only dump ones with
 * non-zero counts */
int only_nonzero = 0;
@@ -3455,7 +3457,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
int fourcc;
int clientid;
int use_count;
-   } service_data[local_max_services];
+   } service_data[VCHIQ_LOCAL_MAX_SERVICES];
 
if (!arm_state)
return;
@@ -3466,10 +3468,10 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
peer_count = arm_state->peer_use_count;
vc_use_count = arm_state->videocore_use_count;
active_services = state->unused_service;
-   if (active_services > local_max_services)
+   if (active_services > VCHIQ_LOCAL_MAX_SERVICES)
only_nonzero = 1;
 
-   for (i = 0; (i < active_services) && (j < local_max_services); i++) {
+   for (i = 0; (i < active_services) && (j < VCHIQ_LOCAL_MAX_SERVICES); 
i++) {
VCHIQ_SERVICE_T *service_ptr = state->services[i];
 
if (!service_ptr)
@@ -3499,7 +3501,7 @@ vchiq_dump_service_use_state(VCHIQ_STATE_T *state)
vchiq_log_warning(vchiq_susp_log_level, "Too many active "
"services (%d).  Only dumping up to first %d services "
"with non-zero use-count", active_services,
-   local_max_services);
+   VCHIQ_LOCAL_MAX_SERVICES);
 
for (i = 0; i < j; i++) {
vchiq_log_warning(vchiq_susp_log_level,
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/4] video: Remove stack VLA usage

[Tobin C. Harding]

On Thu, Mar 08, 2018 at 10:01:07PM -0800, Joe Perches wrote:
> On Fri, 2018-03-09 at 16:50 +1100, Tobin C. Harding wrote:
> > The kernel would like to have all stack VLA usage removed[1].  The
> > arrays are fixed here (declared with a const variable) but they appear
> > like VLAs to the compiler.  We can use a pre-processor define to fix the
> > warning.  
> []
> > diff --git a/drivers/video/fbdev/via/via_aux_sii164.c 
> > b/drivers/video/fbdev/via/via_aux_sii164.c
> []
> > @@ -27,6 +27,9 @@
> >  
> >  static const char *name = "SiI 164 PanelLink Transmitter";
> >  
> > +/* check vendor id and device id */
> > +const u8 id[] = {0x01, 0x00, 0x06, 0x00};
> 
> It seems id is now global in multiple places.
> Perhaps these should be static.

woops, thanks Joe.  Will fix and re-spin.

> 
> > diff --git a/drivers/video/fbdev/via/via_aux_vt1631.c 
> > b/drivers/video/fbdev/via/via_aux_vt1631.c
> []
> > @@ -27,16 +27,19 @@
> >  
> >  static const char *name = "VT1631 LVDS Transmitter";
> >  
> > +/* check vendor id and device id */
> > +const u8 id[] = {0x06, 0x11, 0x91, 0x31}, len = ARRAY_SIZE(id);
> 
> etc...

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] video: Remove stack VLA usage

[Tobin C. Harding]

The kernel would like to have all stack VLA usage removed[1].  The
arrays are fixed here (declared with a const variable) but they appear
like VLAs to the compiler.  We can use a pre-processor define to fix the
warning.  

[1]: https://lkml.org/lkml/2018/3/7/621

Signed-off-by: Tobin C. Harding 
---
 drivers/video/fbdev/via/via_aux_sii164.c |  8 +---
 drivers/video/fbdev/via/via_aux_vt1631.c | 11 +++
 drivers/video/fbdev/via/via_aux_vt1632.c | 11 +++
 drivers/video/fbdev/via/via_aux_vt1636.c | 11 +++
 4 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/video/fbdev/via/via_aux_sii164.c 
b/drivers/video/fbdev/via/via_aux_sii164.c
index ca1b35f033b1..f715ea4f466c 100644
--- a/drivers/video/fbdev/via/via_aux_sii164.c
+++ b/drivers/video/fbdev/via/via_aux_sii164.c
@@ -27,6 +27,9 @@
 
 static const char *name = "SiI 164 PanelLink Transmitter";
 
+/* check vendor id and device id */
+const u8 id[] = {0x01, 0x00, 0x06, 0x00};
+#define VIA_SII164_LEN ARRAY_SIZE(id)
 
 static void probe(struct via_aux_bus *bus, u8 addr)
 {
@@ -34,9 +37,8 @@ static void probe(struct via_aux_bus *bus, u8 addr)
.bus=   bus,
.addr   =   addr,
.name   =   name};
-   /* check vendor id and device id */
-   const u8 id[] = {0x01, 0x00, 0x06, 0x00}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   u8 tmp[VIA_SII164_LEN];
+   int len = VIA_SII164_LEN;
 
if (!via_aux_read(&drv, 0x00, tmp, len) || memcmp(id, tmp, len))
return;
diff --git a/drivers/video/fbdev/via/via_aux_vt1631.c 
b/drivers/video/fbdev/via/via_aux_vt1631.c
index 06e742f1f723..5bfaa20ec5a8 100644
--- a/drivers/video/fbdev/via/via_aux_vt1631.c
+++ b/drivers/video/fbdev/via/via_aux_vt1631.c
@@ -27,16 +27,19 @@
 
 static const char *name = "VT1631 LVDS Transmitter";
 
+/* check vendor id and device id */
+const u8 id[] = {0x06, 0x11, 0x91, 0x31}, len = ARRAY_SIZE(id);
+#define VIA_VT1631_LEN ARRAY_SIZE(id)
 
 void via_aux_vt1631_probe(struct via_aux_bus *bus)
 {
struct via_aux_drv drv = {
.bus=   bus,
.addr   =   0x38,
-   .name   =   name};
-   /* check vendor id and device id */
-   const u8 id[] = {0x06, 0x11, 0x91, 0x31}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   .name   =   name
+   };
+   u8 tmp[VIA_VT1631_LEN];
+   int len = VIA_VT1631_LEN;
 
if (!via_aux_read(&drv, 0x00, tmp, len) || memcmp(id, tmp, len))
return;
diff --git a/drivers/video/fbdev/via/via_aux_vt1632.c 
b/drivers/video/fbdev/via/via_aux_vt1632.c
index d24f4cd97401..fcddd761d4a4 100644
--- a/drivers/video/fbdev/via/via_aux_vt1632.c
+++ b/drivers/video/fbdev/via/via_aux_vt1632.c
@@ -27,16 +27,19 @@
 
 static const char *name = "VT1632 DVI Transmitter";
 
+/* check vendor id and device id */
+const u8 id[] = {0x06, 0x11, 0x92, 0x31};
+#define VIA_VT1632_LEN ARRAY_SIZE(id)
 
 static void probe(struct via_aux_bus *bus, u8 addr)
 {
struct via_aux_drv drv = {
.bus=   bus,
.addr   =   addr,
-   .name   =   name};
-   /* check vendor id and device id */
-   const u8 id[] = {0x06, 0x11, 0x92, 0x31}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   .name   =   name
+   };
+   u8 tmp[VIA_VT1632_LEN];
+   int len = VIA_VT1632_LEN;
 
if (!via_aux_read(&drv, 0x00, tmp, len) || memcmp(id, tmp, len))
return;
diff --git a/drivers/video/fbdev/via/via_aux_vt1636.c 
b/drivers/video/fbdev/via/via_aux_vt1636.c
index 9e015c101d4d..49c9269c7f81 100644
--- a/drivers/video/fbdev/via/via_aux_vt1636.c
+++ b/drivers/video/fbdev/via/via_aux_vt1636.c
@@ -27,16 +27,19 @@
 
 static const char *name = "VT1636 LVDS Transmitter";
 
+/* check vendor id and device id */
+const u8 id[] = {0x06, 0x11, 0x45, 0x33};
+#define VIA_VT1636_LEN ARRAY_SIZE(id)
 
 void via_aux_vt1636_probe(struct via_aux_bus *bus)
 {
struct via_aux_drv drv = {
.bus=   bus,
.addr   =   0x40,
-   .name   =   name};
-   /* check vendor id and device id */
-   const u8 id[] = {0x06, 0x11, 0x45, 0x33}, len = ARRAY_SIZE(id);
-   u8 tmp[len];
+   .name   =   name
+   };
+   u8 tmp[VIA_VT1636_LEN];
+   int len = VIA_VT1636_LEN;
 
if (!via_aux_read(&drv, 0x00, tmp, len) || memcmp(id, tmp, len))
return;
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Remove VLA usage

[Tobin C. Harding]

On Tue, Mar 06, 2018 at 09:46:08PM -0800, Kees Cook wrote:
> The kernel would like to remove all VLA usage. This switches to a
> simple kasprintf() instead.
> 
> Signed-off-by: Kees Cook 
> ---
>  drivers/staging/lustre/lustre/llite/xattr.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c 
> b/drivers/staging/lustre/lustre/llite/xattr.c
> index 532384c91447..aab4eab64289 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -87,7 +87,7 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>   const char *name, const void *value, size_t size,
>   int flags)
>  {
> - char fullname[strlen(handler->prefix) + strlen(name) + 1];
> + char *fullname;
>   struct ll_sb_info *sbi = ll_i2sbi(inode);
>   struct ptlrpc_request *req = NULL;
>   const char *pv = value;
> @@ -141,10 +141,13 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>   return -EPERM;
>   }
>  
> - sprintf(fullname, "%s%s\n", handler->prefix, name);
> + fullname = kasprintf(GFP_KERNEL, "%s%s\n", handler->prefix, name);
> + if (!fullname)
> + return -ENOMEM;
>   rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
>valid, fullname, pv, size, 0, flags,
>ll_i2suppgid(inode), &req);
> + kfree(fullname);

This is cool.  We've had kasprintf() since 2007, who knew?!

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Fri, Mar 02, 2018 at 12:05:03PM +0300, Dan Carpenter wrote:
> There are so many rules for kernel developers to deal with.  Is it worse
> to go over the 80 character limit or align the parameters properly?  Is
> it OK to start the subject with a lower case letter?  I get in trouble
> for using the wrong prefix but I'm the first person to patch a driver so
> how on earth am I supposed to read into your @#$R@#$@#$@#$@ mind to see
> what prefix you are going to want?
> 
> The imperitive thing is a good suggestion for people to think about but
> it's not something that you should suggest to other people out of the
> blue for no reason.  I've seen people do it for OPW and I'm like, "Eh...
> I guess in that case maybe the intern will have to deal with some super
> anal maintainer so they should know all the rules".  But generally,
> unless the person asked you to tutor them about every single unpleasant
> thing/maintainer they might have to deal then just let it be.  Otherwise
> you risk becoming that unpleasant thing they have to deal with.
> 
> Don't lose track of the bigger picture which is "Can you understand the
> changelog?"  There is no such thing as a perfect patch.  This patch has
> a style violation which I overlooked because it just doesn't matter.
> 
> It's discouraging to be nagged at.
> 
> regards,
> dan carpenter

No worries Dan, thanks for pointing this out.  I actually don't enjoy
reviewing staging patches.  I guessed no one really likes reviewing them
but I feel that we should all give back a little and since I don't have
all that much to give I figured reviewing staging patches was a good way
to do something that others probably don't like doing.  I'd like to be
able to do the reviews in a way that is encouraging to new devs but like
most I only have my own likes/dislikes to go on.  I liked having every
detail of my first X patches picked apart because it meant I was
confident then to go on and try to do patches outside of staging -
others may vary and I definitely agree with you that no one likes to be
nagged so it's a fine line.  If you have any suggestions for me as to
how to be more useful in reviewing staging patches please do share
them.  So far I only have one clue as to how to go about it and that was
this from Greg KH 'Just be polite'.  There certainly is a lot to learn
getting started with kernel work but for me anyways staging is doing
really well at smoothing the process and that is mainly thanks to Greg
and yourself in doing the reviews.  If I'm totally wrong and you two
really love doing it then I will happily bow out and not touch another
staging patch (please say so if this is the case).  If I can help to
carry the load then I'm happy to do so because you two helped me a lot.
Please continue to pick me up if I do wrongs - I'm super happy when you
do because then I've got a chance of doing it right next time :)


thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 05:28:26PM -0800, Quytelda Kahja wrote:
> Tobin,
> I understand your point, and I've read submitting-patches.rst.  I made
> that wording choice because I was looking at some older commits that
> were worded like that.  I'm fairly new to the kernel workflow, so I
> was just trying to emulate something established, and it sounded less
> stilted than my imperative attempts.  However, I will word my change
> logs in the imperative as best I can in the future.

If you master the art of writing commit logs let me know - I'm chasing
that elusive goal myself.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 02:15:00PM +0300, Dan Carpenter wrote:
> On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
> > On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> > > The code that generates a WLAN capability mask is repeated in five
> > > functions.  This change refactors that code into a new function, which is
> > > called now in each of those functions.
> > 
> > Perhaps in future something like:
> > 
> > Code to generate the WLAN capability mask is duplicated five times
> > 
> > Add helper function to generate WLAN capability mask, refactor code to
> > use newly defined function.
> > 
> 
> I honestly don't see the difference between that and what Quytelda
> wrote?  I understood the original changelog just fine.
> 
> regards,
> dan carpenter

I had a feeling that the sentiment of the suggestion I was trying to get
at didn't come across, thanks for pointing it out.  I was intending to
suggest not using sentences like this

> This change refactors that code into a new function, which is
> called now in each of those functions.

And instead use, as suggested in submitting-patches.rst, imperative mood

Refactor code into new function ...

FTR I find the English bits of kernel dev (and programming in general)
the most difficult even though English is my first language.  I would
like to write it better.


Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> The code that generates a WLAN capability mask is repeated in five
> functions.  This change refactors that code into a new function, which is
> called now in each of those functions.

Perhaps in future something like:

Code to generate the WLAN capability mask is duplicated five times

Add helper function to generate WLAN capability mask, refactor code to
use newly defined function.


See Documentation/process/submitting-patches.rst section 2 for more info.

2) Describe your changes

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
...

I would normally respond to the cover letter but here goes.

Reviewed-by: Tobin C. Harding 

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
> The case statement in get_ap_information() should not use literal integers
> to parse information element IDs when these values are provided by name
> in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Nice.  Magic number removal, I like it.

> Signed-off-by: Quytelda Kahja 
> ---
>  drivers/staging/ks7010/ks_hostif.c | 31 +++
>  drivers/staging/ks7010/ks_hostif.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c 
> b/drivers/staging/ks7010/ks_hostif.c
> index 74a08417bd0b..67cf32433023 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   offset = 0;
>  
>   while (bsize > offset) {
> - /* DPRINTK(4, "Element ID=%d\n",*bp); */

nit: Probably shouldn't remove debugging output in the same patch as
doing magic number removal.  (super nit-picky though.)

> - switch (*bp) {
> - case 0: /* ssid */
> + switch (*bp) { /* Information Element ID */
> + case WLAN_EID_SSID:
>   if (*(bp + 1) <= SSID_MAX_SIZE) {
>   ap->ssid.size = *(bp + 1);
>   } else {
> @@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
>   break;
> - case 1: /* rate */
> - case 50:/* ext rate */
> + case WLAN_EID_SUPP_RATES:
> + case WLAN_EID_EXT_SUPP_RATES:
>   if ((*(bp + 1) + ap->rate_set.size) <=
>   RATE_SET_MAX_SIZE) {
>   memcpy(&ap->rate_set.body[ap->rate_set.size],
> @@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   (RATE_SET_MAX_SIZE - ap->rate_set.size);
>   }
>   break;
> - case 3: /* DS parameter */
> + case WLAN_EID_DS_PARAMS:
>   break;
> - case 48:/* RSN(WPA2) */
> + case WLAN_EID_RSN:
>   ap->rsn_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->rsn_ie.size = *(bp + 1);
> @@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
>   break;
> - case 221:   /* WPA */
> - if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {   
> /* WPA OUI check */
> + case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
> + if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* 
> WPA OUI check */

Shouldn't have this line (unnecessary white space change)

>   ap->wpa_ie.id = *bp;
>   if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>   ap->wpa_ie.size = *(bp + 1);
> @@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, 
> struct ap_info_t *ap_info,
>   }
>   break;
>  
> - case 2: /* FH parameter */
> - case 4: /* CF parameter */
> - case 5: /* TIM */
> - case 6: /* IBSS parameter */
> - case 7: /* Country */
> - case 42:/* ERP information */
> - case 47:/* Reserve ID 47 Broadcom AP */
> + case WLAN_EID_FH_PARAMS:
> + case WLAN_EID_CF_PARAMS:
> + case WLAN_EID_TIM:
> + case WLAN_EID_IBSS_PARAMS:
> + case WLAN_EID_COUNTRY:
> + case WLAN_EID_ERP_INFO:
>   break;
>   default:
>   DPRINTK(4, "unknown Element ID=%d\n", *bp);
>   break;
>   }
> +

FTR try not to include unrelated whitespace changes.  I see you changed
the case statement but the whitespace is after it and not really
related.  Again quite nit-picky but I'm guessing you are in staging to
learn so might as well learn it now than when you are patching the core
kernel :)


Good work,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] drivers/fbtft: Fix indentation

[Tobin C. Harding]

On Wed, Jan 10, 2018 at 06:30:35PM +0100, Jonny Schaefer wrote:
> From: Luis Gerhorst 
> 
> This fixes the checkpatch message:
> 
> CHECK: Alignment should match open parenthesis
> #1380: FILE: drivers/staging/fbtft/fbtft-core.c:1380:
> + dev_warn(dev,
> + "no default functions for regwidth=%d and 
> buswidth=%d\n",
> 

Perhaps:

Checkpatch emits CHECK: Alignment should match open parenthesis

Align code to open parenthesis.

Reasoning:

Patch description should describe the problem then describe what the
patch does (in imperative mood).  Better advice is given in
Documentation/process/submitting-patches.rst section 2

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Fri, Nov 24, 2017 at 04:02:09PM +0300, Dan Carpenter wrote:
> On Fri, Nov 24, 2017 at 09:20:20AM +1100, Tobin C. Harding wrote:
> > My current favourite review of all time was done by you on a what was
> > at the time a pretty hard patch for me. It was
> > 
> > Nope
> > 
> 
> Wow...  I know I've said that to other people but I can't believe I sent
> that to *you*...  Sorry.  You write good patches.

Not always :) No apology needed, I was genuinely encouraged to tighten
my game.

thanks,
Tobin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Thu, Nov 23, 2017 at 02:51:38PM +0300, Dan Carpenter wrote:
> On Thu, Nov 23, 2017 at 03:59:26PM +1100, Tobin C. Harding wrote:
> > On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> > > Fixed some signedness warnings from sparse on lustre.
> > > 
> > > Stefano Manni (4):
> > >   staging: lustre: fixed signedness of some socklnd params
> > >   staging: lustre: fixed signedness of llite
> > >   staging: lustre: fixed signedness of lov
> > >   staging: lustre: fixed signedness of obdclass
> > 
> > You may like to use imperative mood for your git log brief descriptions
> > Stefano.  
> > 
> > s/fixed/fix/
> > 
> 
> Someone once chewed me a second butt hole for not using the imperative
> mood so I know some people care intensely about this but I think so long
> as you can understand the description it's fine.  I will never send a
> patch for that maintainer's subsystem again, btw, which is probably
> grateful for and now I can poop twice as fast so we're both winners.

I try to only make these suggestions to people doing clean up patches to
staging, with the reasoning that if we are learning we might as well
learn the correct method from the start.

I try to be polite and helpful, I am not very long past learning these
things myself. No one likes being told they are wrong, better to learn
how to take it while in staging, that's what it's there for right.

> Especially in the 0/4 patch which is going to be discarded.  Who cares?

Definitely agree for the 0/4 patch, doesn't _need_ imperative mood.

For the record, I love your short snappy reviews Dan. My current
favourite review of all time was done by you on a what was at the time a
pretty hard patch for me. It was

Nope

I burst out laughing when I got that one. It spurred me on to greater
things (though I'm not sure everyone would be equally encouraged :)

All the best,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: lustre: fixed some signedness warns from sparse

[Tobin C. Harding]

On Wed, Nov 22, 2017 at 08:38:27PM +0100, Stefano Manni wrote:
> Fixed some signedness warnings from sparse on lustre.
> 
> Stefano Manni (4):
>   staging: lustre: fixed signedness of some socklnd params
>   staging: lustre: fixed signedness of llite
>   staging: lustre: fixed signedness of lov
>   staging: lustre: fixed signedness of obdclass

You may like to use imperative mood for your git log brief descriptions
Stefano.  

s/fixed/fix/

For justification see Documentation/process/submitting-patches.rst.

Specifically section 2 of that document.

Hope this helps,
Tobin.


>  drivers/staging/lustre/lnet/klnds/socklnd/socklnd.h|  4 ++--
>  .../staging/lustre/lnet/klnds/socklnd/socklnd_modparams.c  |  2 +-
>  drivers/staging/lustre/lustre/llite/dir.c  |  3 ++-
>  drivers/staging/lustre/lustre/llite/llite_lib.c|  9 ++---
>  drivers/staging/lustre/lustre/llite/lproc_llite.c  | 14 
> ++
>  drivers/staging/lustre/lustre/lov/lov_obd.c|  2 +-
>  drivers/staging/lustre/lustre/lov/lov_offset.c | 11 +++
>  drivers/staging/lustre/lustre/obdclass/obd_mount.c |  2 +-
>  8 files changed, 26 insertions(+), 21 deletions(-)
> 
> -- 
> 2.5.5
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Fix style issues in olpc_dcon

[Tobin C. Harding]

Missing subsystem in subject line. Please use the same git brief
description format that has been used previously for files you want to
patch. You can view previous commits for a file using

git log --pretty=oneline --abbrev --reverse

On Mon, Nov 20, 2017 at 03:14:21PM -0600, zebmccor...@disr.it wrote:
> From: Zebulon McCorkle 
> 
> The olpc_dcon driver had some slight style issues, mostly pertaining to
> indentation in function calls and definitions. I've solved those, and
> plan to work on the issues in the TODO.

Please read Documentation/process/submitting-patches.rst for tips on how to
write git commit log messages. Especially section '2) Describe your changes'.

> Signed-off-by: Zebulon McCorkle 
> ---
>  drivers/staging/olpc_dcon/olpc_dcon.c  | 30 
> --
>  drivers/staging/olpc_dcon/olpc_dcon.h  | 30 
> +++---
>  drivers/staging/olpc_dcon/olpc_dcon_xo_1.c |  2 +-
>  3 files changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/olpc_dcon/olpc_dcon.c 
> b/drivers/staging/olpc_dcon/olpc_dcon.c
> index 82bffd911435..2744c9f0920e 100644
> --- a/drivers/staging/olpc_dcon/olpc_dcon.c
> +++ b/drivers/staging/olpc_dcon/olpc_dcon.c
> @@ -393,7 +393,8 @@ static void dcon_set_source_sync(struct dcon_priv *dcon, 
> int arg)
>  }
>  
>  static ssize_t dcon_mode_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +   struct device_attribute *attr,
> +   char *buf)
>  {
>   struct dcon_priv *dcon = dev_get_drvdata(dev);
>  
> @@ -401,7 +402,8 @@ static ssize_t dcon_mode_show(struct device *dev,
>  }
>  
>  static ssize_t dcon_sleep_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +struct device_attribute *attr,
> +char *buf)
>  {
>   struct dcon_priv *dcon = dev_get_drvdata(dev);
>  
> @@ -409,7 +411,8 @@ static ssize_t dcon_sleep_show(struct device *dev,
>  }
>  
>  static ssize_t dcon_freeze_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> + struct device_attribute *attr,
> + char *buf)
>  {
>   struct dcon_priv *dcon = dev_get_drvdata(dev);
>  
> @@ -417,7 +420,8 @@ static ssize_t dcon_freeze_show(struct device *dev,
>  }
>  
>  static ssize_t dcon_mono_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> +   struct device_attribute *attr,
> +   char *buf)
>  {
>   struct dcon_priv *dcon = dev_get_drvdata(dev);
>  
> @@ -425,13 +429,15 @@ static ssize_t dcon_mono_show(struct device *dev,
>  }
>  
>  static ssize_t dcon_resumeline_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> + struct device_attribute *attr,
> + char *buf)
>  {
>   return sprintf(buf, "%d\n", resumeline);
>  }
>  
>  static ssize_t dcon_mono_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t count)
> +struct device_attribute *attr,
> +const char *buf, size_t count)
>  {
>   unsigned long enable_mono;
>   int rc;
> @@ -446,7 +452,8 @@ static ssize_t dcon_mono_store(struct device *dev,
>  }
>  
>  static ssize_t dcon_freeze_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t count)
> +  struct device_attribute *attr,
> +  const char *buf, size_t count)
>  {
>   struct dcon_priv *dcon = dev_get_drvdata(dev);
>   unsigned long output;
> @@ -474,7 +481,8 @@ static ssize_t dcon_freeze_store(struct device *dev,
>  }
>  
>  static ssize_t dcon_resumeline_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t count)
> +  struct device_attribute *attr,
> +  const char *buf, size_t count)
>  {
>   unsigned short rl;
>   int rc;
> @@ -490,7 +498,8 @@ static ssize_t dcon_resumeline_store(struct device *dev,
>  }
>  
>  static ssize_t dcon_sleep_store(struct device *dev,
> - struct device_attribute *attr, const char *buf, size_t count)
> + struct device_attribute *attr,
> + const char *buf, size_t count)
>  {
>   unsigned long output;
>   int ret;
> @@ -641,7 +650,8 @@ static int dcon_probe(struct i2c_client *client, const 
> struct i2c_device_id *id)
>   /* Add the backlight device for the DCON */
>   dcon_bl_props.brightness = dcon->bl_val;
>   dcon->bl_dev = backlight_device_register("dcon-bl", &dcon_device->dev,
> - dcon, &dcon_bl_ops, &dcon_bl_props);
> +  dcon, &dcon_bl_ops,
> +   

Re: [PATCH] Staging: comedi: adl_pci9118: fixed some parentheses coding style issue

[Tobin C. Harding]

You may like to limit the git log brief description to 50 characters
(this is going to be hard with such a long pre-fix though :)

Brief description should be in imperative mood i.e 'Fix foo' instead of
'fixed foo'.

On Tue, Nov 21, 2017 at 05:17:53PM -0200, Guilherme Tadashi Maeoka wrote:
> Fixed some code style issues.

This is not descriptive enough. You may like to read

Documentation/process/submitting-patches.rst

for tips on writing git log messages. 

> Signed-off-by: Guilherme Tadashi Maeoka 
> ---
>  drivers/staging/comedi/drivers/adl_pci9118.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c 
> b/drivers/staging/comedi/drivers/adl_pci9118.c
> index 1cc9b7ef1ff9..53f13994ac94 100644
> --- a/drivers/staging/comedi/drivers/adl_pci9118.c
> +++ b/drivers/staging/comedi/drivers/adl_pci9118.c
> @@ -947,7 +947,7 @@ static int pci9118_ai_cmd(struct comedi_device *dev, 
> struct comedi_subdevice *s)
>   if (devpriv->master) {
>   devpriv->usedma = 1;
>   if ((cmd->flags & CMDF_WAKE_EOS) &&
> - (cmd->scan_end_arg == 1)) {
> + cmd->scan_end_arg == 1) {

The code was easier to read before this change IMO.

>   if (cmd->convert_src == TRIG_NOW)
>   devpriv->ai_add_back = 1;
>   if (cmd->convert_src == TRIG_TIMER) {
> @@ -960,7 +960,7 @@ static int pci9118_ai_cmd(struct comedi_device *dev, 
> struct comedi_subdevice *s)
>   }
>   if ((cmd->flags & CMDF_WAKE_EOS) &&
>   (cmd->scan_end_arg & 1) &&
> - (cmd->scan_end_arg > 1)) {
> + cmd->scan_end_arg > 1) {
>   if (cmd->scan_begin_src == TRIG_FOLLOW) {
>   devpriv->usedma = 0;
>   /*
> @@ -983,7 +983,7 @@ static int pci9118_ai_cmd(struct comedi_device *dev, 
> struct comedi_subdevice *s)
>*/
>   if (cmd->convert_src == TRIG_NOW && devpriv->softsshdelay) {
>   devpriv->ai_add_front = 2;
> - if ((devpriv->usedma == 1) && (devpriv->ai_add_back == 1)) {
> + if (devpriv->usedma == 1 && devpriv->ai_add_back == 1) {

Likewise, this is not making the code easier to read.

>   /* move it to front */
>   devpriv->ai_add_front++;
>   devpriv->ai_add_back = 0;
> @@ -1185,7 +1185,7 @@ static int pci9118_ai_cmdtest(struct comedi_device *dev,
>   (!(cmd->convert_src & (TRIG_TIMER | TRIG_NOW
>   err |= -EINVAL;
>  
> - if ((cmd->scan_begin_src == TRIG_FOLLOW) &&
> + if (cmd->scan_begin_src == TRIG_FOLLOW &&
>   (!(cmd->convert_src & (TRIG_TIMER | TRIG_EXT
>   err |= -EINVAL;
>  
> @@ -1210,8 +1210,8 @@ static int pci9118_ai_cmdtest(struct comedi_device *dev,
>   if (cmd->scan_begin_src & (TRIG_FOLLOW | TRIG_EXT))
>   err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>  
> - if ((cmd->scan_begin_src == TRIG_TIMER) &&
> - (cmd->convert_src == TRIG_TIMER) && (cmd->scan_end_arg == 1)) {
> + if (cmd->scan_begin_src == TRIG_TIMER &&
> + cmd->convert_src == TRIG_TIMER && cmd->scan_end_arg == 1) {
>   cmd->scan_begin_src = TRIG_FOLLOW;
>   cmd->convert_arg = cmd->scan_begin_arg;
>   cmd->scan_begin_arg = 0;

Remember, we are writing code for developers to read. It should be as
easy to parse as possible.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: fsl-mc: fix mc-portal to use u32 type

[Tobin C. Harding]

On Tue, Nov 21, 2017 at 10:35:23AM +0530, Bharat Bhushan wrote:
> According to MC APIs, size of mc-portal in 32bit.
> Also fsl_create_mc_io() storing 32 bit mc-portal size.
>" mc_io->portal_size = mc_portal_size;"
>While "mc_io->portal_size" is u16 type and
>"mc_portal_size" is u32 type.
> 
> This patches changes mc_io->portal_size from u16 to u32

You may like to read Documentation/process/submitting-patches.rst for
tips or writing the git commit log.

> Signed-off-by: Bharat Bhushan 
> ---

Include here what changed from v1 -> v2

>  drivers/staging/fsl-mc/include/mc-sys.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fsl-mc/include/mc-sys.h 
> b/drivers/staging/fsl-mc/include/mc-sys.h
> index dca7f90..11d4367 100644
> --- a/drivers/staging/fsl-mc/include/mc-sys.h
> +++ b/drivers/staging/fsl-mc/include/mc-sys.h
> @@ -75,7 +75,7 @@
>  struct fsl_mc_io {
>   struct device *dev;
>   u16 flags;
> - u16 portal_size;
> + u32 portal_size;
>   phys_addr_t portal_phys_addr;
>   void __iomem *portal_virt_addr;
>   struct fsl_mc_device *dpmcp_dev;

I was not able to apply this patch to either Greg's staging tree or
Linus' mainline.

Does this change clear any Sparse warnings? If so you may like to note
that in the commit log.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:56:40PM +0300, Dan Carpenter wrote:
> On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > > > *channel, u32 queue,
> > > >   void *msg)
> > > >  {
> > > > int rc;
> > > > -   unsigned long flags;
> > > >  
> > > > if (channel->needs_lock) {
> > > > +   unsigned long flags;
> > > > +
> > > 
> > > Same here, the original code is fine.
> > > 
> > > No idea why the tool wants you to move these around, you should ignore
> > > stuff like that :(
> > 
> > Oh? I'm surprise at this comment. I have always thought limiting scope
> > of local variables was seen as a good thing. Is this a style thing that
> > is on case by case basis or do you never like to declare local variables
> > within blocks?
> > 
> 
> Greg has answered for himself but here are my thoughts...

thanks for taking the time.

> If you look at Colin King's patches, he's using CPPcheck and he's pretty
> religiously moving variables to the localest scope and no one complains.
> It makes sense when he does it from what I have seen.  But mostly he's
> definitely cleaning up the code and fixing bugs and no one cares too
> much about the scope one way or the other.
> 
> But here, I agreed with Greg that the original code was better.  The
> chdr_size and copy_size variables are closely related and are better
> declared together.  The flags declaration was also slightly cleaner at
> the start instead of mixing it up with the code.  Sometimes in a long
> function it definitely makes sense to use a local declaration.  So it's
> a case by case thing.

Cool, got it.

> But mostly no one cares, and I don't want to see hundreds of patches
> mechanically moving declarations around.

Oh bother, scrap that 400 patch series I queued up ... just kidding. 

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:46:54PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Nov 06, 2017 at 10:59:47AM +0200, Gilad Ben-Yossef wrote:
> > On Mon, Nov 6, 2017 at 10:37 AM, Tobin C. Harding  wrote:
> > > On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> > >> Registers ioread/iowrite operations were done via macros,
> > >> sometime using a "magical" implicit parameter.
> > >>
> > >> Replace all register access with simple inline macros.
> > >>
> > >> Signed-off-by: Gilad Ben-Yossef 
> > >
> > > Hi,
> > >
> > > Nice work. I had a little trouble following this one. Perhaps you are
> > > doing more than one thing per patch, feel free to ignore me if I am
> > > wrong but it seems you are moving the macro definition of CC_REG to a
> > > different header, adding the new inline functions and doing some other
> > > change that I can't grok (commented on below).
> > >
> > > Perhaps this patch could be broken up.
> > 
> > Thank you Tobin.
> > 
> > The original macro that I am replacing had an assumption of a variable void 
> > *
> > cc_base being defined in the context of the macro being called, even though
> > it was not listed in the explicit parameter list of the macro.
> > 
> > The inline function that replace it instead takes an explicit
> > parameter a pointer to
> > struct ssi_drive data * , who has said cc_base as one of the fields.
> > 
> > As a result several function that took a void * cc_base parameter
> > (which is than only
> > used implicitly via the macro without ever being visibly referenced), now 
> > take
> > struct ssi_drive data * parameter instead which is passed explicitly
> > to the inline
> > function.
> > 
> > These seems to be the places you are referring to. They are cascading 
> > changes
> > resulting from the change in API between the macro and the inline
> > function that replaces it.
> > 
> > I imagine I can try to break that change to two patches but at least
> > in my mind this is artificial
> > and it is a single logical change.
> > 
> > Having said that, if you think otherwise and consider this
> > none-reviewable even after this
> > explanation let me know and  I'd be happy to break it down.
> 
> Nah, this is fine, I'll take it as-is.  Tobin, thanks for the review.

No worries. Greg make sure you yell at me if I start causing you more
work than I'm saving. It's a fine line reviewing patches when you are
not super experienced.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: ccree: simplify ioread/iowrite

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 06:55:52AM +, Gilad Ben-Yossef wrote:
> Registers ioread/iowrite operations were done via macros,
> sometime using a "magical" implicit parameter.
> 
> Replace all register access with simple inline macros.
> 
> Signed-off-by: Gilad Ben-Yossef 

Hi,

Nice work. I had a little trouble following this one. Perhaps you are
doing more than one thing per patch, feel free to ignore me if I am
wrong but it seems you are moving the macro definition of CC_REG to a
different header, adding the new inline functions and doing some other
change that I can't grok (commented on below).

Perhaps this patch could be broken up.

> ---
>  drivers/staging/ccree/cc_hal.h   | 33 --
>  drivers/staging/ccree/cc_regs.h  | 35 
>  drivers/staging/ccree/dx_reg_base_host.h | 25 --
>  drivers/staging/ccree/ssi_driver.c   | 47 --
>  drivers/staging/ccree/ssi_driver.h   | 20 +--
>  drivers/staging/ccree/ssi_fips.c | 12 +++
>  drivers/staging/ccree/ssi_pm.c   |  4 +--
>  drivers/staging/ccree/ssi_request_mgr.c  | 57 
> 
>  drivers/staging/ccree/ssi_sysfs.c| 11 +++---
>  9 files changed, 78 insertions(+), 166 deletions(-)
>  delete mode 100644 drivers/staging/ccree/cc_hal.h
>  delete mode 100644 drivers/staging/ccree/cc_regs.h
>  delete mode 100644 drivers/staging/ccree/dx_reg_base_host.h
> 
> diff --git a/drivers/staging/ccree/cc_hal.h b/drivers/staging/ccree/cc_hal.h
> deleted file mode 100644
> index eecc866..000
> --- a/drivers/staging/ccree/cc_hal.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/* pseudo cc_hal.h for cc7x_perf_test_driver (to be able to include code from
> - * CC drivers).
> - */
> -
> -#ifndef __CC_HAL_H__
> -#define __CC_HAL_H__
> -
> -#include 
> -
> -#define READ_REGISTER(_addr) ioread32((_addr))
> -#define WRITE_REGISTER(_addr, _data)  iowrite32((_data), (_addr))
> -
> -#define CC_HAL_WRITE_REGISTER(offset, val) \
> - WRITE_REGISTER(cc_base + (offset), val)
> -#define CC_HAL_READ_REGISTER(offset) READ_REGISTER(cc_base + (offset))
> -
> -#endif
> diff --git a/drivers/staging/ccree/cc_regs.h b/drivers/staging/ccree/cc_regs.h
> deleted file mode 100644
> index 2a8fc73..000
> --- a/drivers/staging/ccree/cc_regs.h
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, see .
> - */
> -
> -/*!
> - * @file
> - * @brief This file contains macro definitions for accessing ARM TrustZone
> - *CryptoCell register space.
> - */
> -
> -#ifndef _CC_REGS_H_
> -#define _CC_REGS_H_
> -
> -#include 
> -
> -#define AXIM_MON_COMP_VALUE GENMASK(DX_AXIM_MON_COMP_VALUE_BIT_SIZE + \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT, \
> - DX_AXIM_MON_COMP_VALUE_BIT_SHIFT)
> -
> -/* Register name mangling macro */
> -#define CC_REG(reg_name) DX_ ## reg_name ## _REG_OFFSET
> -
> -#endif /*_CC_REGS_H_*/
> diff --git a/drivers/staging/ccree/dx_reg_base_host.h 
> b/drivers/staging/ccree/dx_reg_base_host.h
> deleted file mode 100644
> index 47bbadb..000
> --- a/drivers/staging/ccree/dx_reg_base_host.h
> +++ /dev/null
> @@ -1,25 +0,0 @@
> -/*
> - * Copyright (C) 2012-2017 ARM Limited or its affiliates.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILIT

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:02:21AM +0100, gre...@linuxfoundation.org wrote:
> On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> > On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@linuxfoundation.org wrote:
> > > On Mon, Oct 02, 2017 at 03:41:42PM +, Kershner, David A wrote:
> > > > Hey Greg, we think the code for visorbus is ready to be moved out
> > > > of staging, can you review it to see if we have missed anything?
> > > 
> > > I think your html email got rejected by the list :(
> > > 
> > > > The files include:
> > > > /drivers/staging/unisys/visorbus/
> > > > /drivers/staging/unisys/include/visorchannel.h
> > > > /drivers/staging/unisys/include/visorbus.h
> > > > 
> > > > The directories staging/drivers/unisys/visornic, visorhba, and 
> > > > visorinput
> > > > are not part of the review as well as the file
> > > > drivers/staging/unisys/include/iochannel.h.
> > > > 
> > > > We currently have 5 checkpatch.pl warnings that I know about:
> > > >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed a 
> > > > FIELD
> > > > instead of just a variable
> > > > - 2 WARNINGS dealing with char * becoming static const
> > > > 
> > > >  
> > > > 
> > > > Dan Carpenter found some extra parenthesis errors that I will address as
> > > > well as look through the code to fix, but I would like to ask for the 
> > > > review
> > > > while we are working on that.
> > > 
> > > Sure, I'll look at doing it in a week or so when I catch up with other
> > > patches in my queue.
> > > 
> > > Everyone else is also welcome to do review :)
> > 
> > cppcheck emits a few style warnings, nothing super important but FWIW
> > here is a patch
> > 
> > ---
> >  drivers/staging/unisys/visorbus/visorchannel.c | 9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/visorbus/visorchannel.c 
> > b/drivers/staging/unisys/visorbus/visorchannel.c
> > index aae16073ba03..2c1beddfee29 100644
> > --- a/drivers/staging/unisys/visorbus/visorchannel.c
> > +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> > @@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel *channel, 
> > ulong offset, void *dest,
> >ulong nbytes)
> >  {
> > size_t chdr_size = sizeof(struct channel_header);
> > -   size_t copy_size;
> >  
> > if (offset + nbytes > channel->nbytes)
> > return -EIO;
> >  
> > if (offset < chdr_size) {
> > +   size_t copy_size;
> > +
> 
> Ick, no, the original code here is fine.
> 
> > copy_size = min(chdr_size - offset, nbytes);
> > memcpy(((char *)(&channel->chan_hdr)) + offset,
> >dest, copy_size);
> > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
> > *channel, u32 queue,
> >   void *msg)
> >  {
> > int rc;
> > -   unsigned long flags;
> >  
> > if (channel->needs_lock) {
> > +   unsigned long flags;
> > +
> 
> Same here, the original code is fine.
> 
> No idea why the tool wants you to move these around, you should ignore
> stuff like that :(

Oh? I'm surprise at this comment. I have always thought limiting scope
of local variables was seen as a good thing. Is this a style thing that
is on case by case basis or do you never like to declare local variables
within blocks?

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [REVIEW REQUEST] staging: unisys: review request for visorbus

[Tobin C. Harding]

On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@linuxfoundation.org wrote:
> On Mon, Oct 02, 2017 at 03:41:42PM +, Kershner, David A wrote:
> > Hey Greg, we think the code for visorbus is ready to be moved out
> > of staging, can you review it to see if we have missed anything?
> 
> I think your html email got rejected by the list :(
> 
> > The files include:
> > /drivers/staging/unisys/visorbus/
> > /drivers/staging/unisys/include/visorchannel.h
> > /drivers/staging/unisys/include/visorbus.h
> > 
> > The directories staging/drivers/unisys/visornic, visorhba, and visorinput
> > are not part of the review as well as the file
> > drivers/staging/unisys/include/iochannel.h.
> > 
> > We currently have 5 checkpatch.pl warnings that I know about:
> >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed a FIELD
> > instead of just a variable
> > - 2 WARNINGS dealing with char * becoming static const
> > 
> >  
> > 
> > Dan Carpenter found some extra parenthesis errors that I will address as
> > well as look through the code to fix, but I would like to ask for the review
> > while we are working on that.
> 
> Sure, I'll look at doing it in a week or so when I catch up with other
> patches in my queue.
> 
> Everyone else is also welcome to do review :)

cppcheck emits a few style warnings, nothing super important but FWIW
here is a patch

---
 drivers/staging/unisys/visorbus/visorchannel.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/unisys/visorbus/visorchannel.c 
b/drivers/staging/unisys/visorbus/visorchannel.c
index aae16073ba03..2c1beddfee29 100644
--- a/drivers/staging/unisys/visorbus/visorchannel.c
+++ b/drivers/staging/unisys/visorbus/visorchannel.c
@@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel *channel, 
ulong offset, void *dest,
   ulong nbytes)
 {
size_t chdr_size = sizeof(struct channel_header);
-   size_t copy_size;
 
if (offset + nbytes > channel->nbytes)
return -EIO;
 
if (offset < chdr_size) {
+   size_t copy_size;
+
copy_size = min(chdr_size - offset, nbytes);
memcpy(((char *)(&channel->chan_hdr)) + offset,
   dest, copy_size);
@@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel 
*channel, u32 queue,
  void *msg)
 {
int rc;
-   unsigned long flags;
 
if (channel->needs_lock) {
+   unsigned long flags;
+
spin_lock_irqsave(&channel->remove_lock, flags);
rc = signalremove_inner(channel, queue, msg);
spin_unlock_irqrestore(&channel->remove_lock, flags);
@@ -428,9 +430,10 @@ int visorchannel_signalinsert(struct visorchannel 
*channel, u32 queue,
  void *msg)
 {
int rc;
-   unsigned long flags;
 
if (channel->needs_lock) {
+   unsigned long flags;
+
spin_lock_irqsave(&channel->insert_lock, flags);
rc = signalinsert_inner(channel, queue, msg);
spin_unlock_irqrestore(&channel->insert_lock, flags);
-- 
2.7.4

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ccree: Fix lines longer than 80 characters

[Tobin C. Harding]

On Tue, Oct 24, 2017 at 11:52:58PM -0700, Stephen Brennan wrote:
> Hi Gilad & Tobin,
> 
> > > Perhaps, if Stephen is willing, re-write the code to be more readable
> > > by, for example, using a temp
> > > variable for the register address, and in doing so both making the
> > > code more readable as well as
> > > treating the symptom?
> 
> I'm definitely willing; however, I don't know the hardware, so I'm already
> off to a bad start with questions like "which integer type is appropriate
> for the register address?"
> 
> > Oh, refactoring, that's a whole 'nother story. I'm a huge fan. And I agree 
> > with you that that is the
> > correct way to deal with 'line over 80' warnings. For me, it helped to get 
> > a few _trivial_
> > checkpatch fixes in before I moved onto refactoring. 'line over 80' 
> > warnings are great to fix if
> > viewed as an opportunity to refactor but not so great if viewed as a 
> > trivial white space fix to get
> > started with.
> 
> I think this makes the most sense right now. A pure trivial change is what
> I'd like, to help get some experience with the development process and
> working with the community. I'll look for something else (hopefully within
> the same staging driver, since you've all been super welcoming) and submit
> a patch for it. Hopefully I'll circle back for some slightly less trivial
> refactors after that!

When you want to link a few patches together into a series I wrote a
blog post a while back, check it out if you want some further guidance

http://tobin.cc/blog/kernel-dev-2/

Stick at it. There ain't nothin' to it but to do it

Good luck,
Tobin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ccree: Fix lines longer than 80 characters

[Tobin C. Harding]

Removing from CC list:

- de...@driverdev.osuosl.org (this is the old address, it forwards to
  driverdev-devel@linuxdriverproject.org now I believe). 

- Linux Crypto Mailing List , should we be 
spamming these guys with
  checkpatch fixing discussions? Please do correct me if this is not the 
correct etiquette, I am
  still learning also.

On Tue, Oct 24, 2017 at 11:49:41AM +0300, Gilad Ben-Yossef wrote:
> Hi Tobin,
> 
> On Tue, Oct 24, 2017 at 6:02 AM, Tobin C. Harding  wrote:
> > On Mon, Oct 23, 2017 at 07:53:18AM -0700, Stephen Brennan wrote:
> >> Simply break down some long lines and tab-indent them.
> >
> > Hi Stephen,
> >
> > Welcome to the Linux kernel. Great that you have put in a patch, you are, 
> > however, unlikely to see
> > success fixing 'line over 80' warnings. There are a bunch of arguments for 
> > and against the line over
> > 80 limit, a web search will surely show them to you. The TL;DR is that it 
> > these changes do not
> > _really_ improve the readability of the code, they are just changes to 
> > quiet the static analysis
> > tool.
> 
> I completely agree with you that the end target is more readable code
> and that lines over 80 char is
> only a symptom but I do think in this case there is something useful to do.
> 
> Perhaps, if Stephen is willing, re-write the code to be more readable

Oh, refactoring, that's a whole 'nother story. I'm a huge fan. And I agree with 
you that that is the
correct way to deal with 'line over 80' warnings. For me, it helped to get a 
few _trivial_
checkpatch fixes in before I moved onto refactoring. 'line over 80' warnings 
are great to fix if
viewed as an opportunity to refactor but not so great if viewed as a trivial 
white space fix to get
started with.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ccree: Fix lines longer than 80 characters

[Tobin C. Harding]

On Mon, Oct 23, 2017 at 07:53:18AM -0700, Stephen Brennan wrote:
> Simply break down some long lines and tab-indent them.

Hi Stephen,

Welcome to the Linux kernel. Great that you have put in a patch, you are, 
however, unlikely to see
success fixing 'line over 80' warnings. There are a bunch of arguments for and 
against the line over
80 limit, a web search will surely show them to you. The TL;DR is that it these 
changes do not
_really_ improve the readability of the code, they are just changes to quiet 
the static analysis
tool.

There are a bunch of other white space fixes that are more beneficial, perhaps 
you could wet your
feet with these (incorrect placement of parenthesis for example).

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: wlan-ng: Remove unnecessary parentheses

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 06:50:52PM -0400, Frank A. Cancio Bello wrote:
> On Thu, Oct 19, 2017 at 08:40:08AM +1100, Tobin C. Harding wrote:
> > On Wed, Oct 18, 2017 at 11:48:21AM -0400, Frank A. Cancio Bello wrote:
> > > --- a/drivers/staging/wlan-ng/p80211req.c
> > > +++ b/drivers/staging/wlan-ng/p80211req.c
> > > @@ -124,7 +124,7 @@ int p80211req_dorequest(struct wlandevice *wlandev, 
> > > u8 *msgbuf)
> > >  
> > >   /* Check Permissions */
> > >   if (!capable(CAP_NET_ADMIN) &&
> > > - (msg->msgcode != DIDmsg_dot11req_mibget)) {
> > > + msg->msgcode != DIDmsg_dot11req_mibget) {
> > 
> > While this is not making the code _harder_ to read, it is not making it any 
> > easier either. So all
> > the change is really doing is quieting checkpatch. Usually it is not a good 
> > idea to make code
> > changes _just_ to quieten a static analysis tool. It's just a tool 
> > remember, there to help us write
> > better code.
> > 
> 
> For me is easy to read without parentheses given the fact that I tend to jump 
> to the closing parentheses and then read from the opening parentheses up to 
> the mental mark that I did at the closing parentheses. But that is me, and 
> given the fact that I'm a newbie that is still learning I will stop sending 
> this kind of patches if you consider it wise.
> 
> > On top of that CHECKS are just that, things that should be CHECK'ed, not 
> > necessarily fixed.
> > 
> 
> Agreed.
> 
> > Hope this helps,
> 
> A lot! I really appreciate any input at this stage.

Glad to help, stick at it. You will get there.

> 
> thanks,
> frank
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: wlan-ng: Remove unnecessary parentheses

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 11:48:21AM -0400, Frank A. Cancio Bello wrote:
> Remove unnecessary parentheses to comply with preferred coding style for
> the linux kernel and avoid the following checkpatch's messages:
> 'CHECK: Unnecessary parentheses around'
> 'CHECK: Logical continuations should be on the previous line'
> 
> Credits to checkpatch.
> 
> Signed-off-by: Frank A. Cancio Bello 
> ---
> Changes in v3:
>   * Exclude any parentheses removal that makes unclear the order of the 
> operations.
> 
> Changes in v2:
>   * I rewrote the log message to improve the style taking in 
> consideration Julia's suggestions.
>   * I merged in this patch similars changes that initially were in theirs 
> own patch. I will reply that other patch email thread, saying to discard it, 
> to avoid confussion.
> 
>  drivers/staging/wlan-ng/p80211req.c  |  2 +-
>  drivers/staging/wlan-ng/prism2fw.c   | 21 ++---
>  drivers/staging/wlan-ng/prism2mgmt.c | 23 +++
>  drivers/staging/wlan-ng/prism2sta.c  |  4 ++--
>  4 files changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/p80211req.c 
> b/drivers/staging/wlan-ng/p80211req.c
> index afe8477..0d1556c 100644
> --- a/drivers/staging/wlan-ng/p80211req.c
> +++ b/drivers/staging/wlan-ng/p80211req.c
> @@ -124,7 +124,7 @@ int p80211req_dorequest(struct wlandevice *wlandev, u8 
> *msgbuf)
>  
>   /* Check Permissions */
>   if (!capable(CAP_NET_ADMIN) &&
> - (msg->msgcode != DIDmsg_dot11req_mibget)) {
> + msg->msgcode != DIDmsg_dot11req_mibget) {

While this is not making the code _harder_ to read, it is not making it any 
easier either. So all
the change is really doing is quieting checkpatch. Usually it is not a good 
idea to make code
changes _just_ to quieten a static analysis tool. It's just a tool remember, 
there to help us write
better code.

On top of that CHECKS are just that, things that should be CHECK'ed, not 
necessarily fixed.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

Hi Suniel,

Well done with you continued versions. I am being particularly nit picky here 
but since we are
striving for perfection I'm sure will humour me. If English is not your first 
language please
forgive me for picking you up on language subtleties.

On Wed, Oct 18, 2017 at 12:11:55PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

This should be a description of the problem, so saying _why_ there is a problem 
or _what_ is wrong
with the code currently that warrants a patch. Sometimes while describing the 
problem you may
include descriptions of the solution especially it is not immediately obvious 
why your proposed
solution fixes the issue being explained. As an extra we shouldn't ever say 
'This patch ...' or
'This does xyz'.

> return "false" instead of 0.

Perfect, this is in imperative mood. Spot on!

> Signed-off-by: Suniel Mahesh 
> ---
> Changes for v3:
> - Changed the commit log even more to give an accurate
>   description of the changeset as suggested by Toby C.Harding.

My name is Tobin :)

> ---
> Changes for v2:
> - Changed the commit log to give a more accurate description
>   of the changeset as suggested by Toby C.Harding.
> ---
> Note:
> - Patch was built(ARCH=arm) on latest linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }
>  
>  #endif /* CRYPTO_TFM_REQ_HW_KEY */
> -- 
> 1.9.1
> 

For what it's worth, Reviewed-by: Tobin C. Harding 

As stated I am being particularly 'nit picky', the commit log is _probably_ 
good enough to be
merged, I am not a maintainer though so it's not really anything to do with me. 
I do know however
that sometimes patches go to the bottom of Greg's list if they have 
comments/suggestions. I mention
this only so you learn more about the process and to help you with successfully 
getting you patches
merged. Keep up the work!

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:42:53AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
>
> Return "false" instead of 0.
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

So close! The order of problem description and fix is inverted. What about

```
coccinelle emits: WARNING: return of 0/1 in function 'ssi_is_hw_key' with
return type bool.

Return "false" instead of 0.
```


Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Wed, Oct 18, 2017 at 07:40:14AM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Comparision operator "equal to" not required on a variable
> "foo" of type "bool". Bool has only two values, can be used
> directly or with logical not.
> 
> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 

Nice.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ccree: fix boolreturn.cocci warning

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:39:57PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> This fixes the following coccinelle warning:
> WARNING: return of 0/1 in function 'ssi_is_hw_key' with return type bool.

Perhaps

Coccinelle emits WARNING: return of 0/1 in function 'ssi_is_hw_key' with return 
type bool.

Return 'false' instead of 0.

> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.
> - No build issues reported, however it was not
>   tested on real hardware.
> - Please discard this changeset, if this is not
>   helping the code look better.
> ---
>  drivers/staging/ccree/ssi_cipher.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index c9a83df..f499962 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -75,7 +75,7 @@ struct arm_hw_key_info {
>  
>  static inline bool ssi_is_hw_key(struct crypto_tfm *tfm)
>  {
> - return 0;
> + return false;
>  }

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ccree: Fix bool comparison

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 03:38:11PM +0530, suni...@techveda.org wrote:
> From: Suniel Mahesh 
> 
> Bool tests don't need comparisons.

This commit log could be a bit longer. You may like to read
Documentation/process/submitting-patches.rst (section 2).

> This fixes the following coccinelle warning:
> WARNING: Comparison of bool to 0/1
> 
> Signed-off-by: Suniel Mahesh 
> ---
> Note:
> - Patch was tested and built(ARCH=arm) on latest
>   linux-next.

Building is _not_ testing.

> - No build issues reported, however it was not
>   tested on real hardware.

This is implicit if you state 'builds on ARM'

> - Please discard this changeset, if this is not
>   helping the code look better.

This is implicit also ;)

> ---
>  drivers/staging/ccree/ssi_request_mgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
> b/drivers/staging/ccree/ssi_request_mgr.c
> index 2e0df57..942afe2 100644
> --- a/drivers/staging/ccree/ssi_request_mgr.c
> +++ b/drivers/staging/ccree/ssi_request_mgr.c
> @@ -272,7 +272,7 @@ int send_request(
>   unsigned int max_required_seq_len = (total_seq_len +
>   ((ssi_req->ivgen_dma_addr_len == 0) ? 0 
> :
>   SSI_IVPOOL_SEQ_LEN) +
> - ((is_dout == 0) ? 1 : 0));
> + (!is_dout ? 1 : 0));

Perhaps

-   ((is_dout == 0) ? 1 : 0));
+   (is_dout ? 0 : 1)

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] stagin: atomisp: Fix oops by unbalanced clk enable/disable call

[Tobin C. Harding]

On Mon, Oct 16, 2017 at 02:34:48PM +0200, Hans de Goede wrote:
> The common-clk core expects clk consumers to always call enable/disable
> in a balanced manner. The atomisp driver does not call gmin_flisclk_ctrl()
> in a balanced manner, so add a clock_on bool and skip redundant calls.
> 
> This fixes kernel oops like this one:
> 
> [   19.811613] gc0310_s_config S
> [   19.811655] [ cut here ]
> [   19.811664] WARNING: CPU: 1 PID: 720 at drivers/clk/clk.c:594 
> clk_core_disabl
> [   19.811666] Modules linked in: tpm_crb(+) snd_soc_sst_atom_hifi2_platform 
> tpm
> [   19.811744] CPU: 1 PID: 720 Comm: systemd-udevd Tainted: G C OE   
> 4.1
> [   19.811746] Hardware name: Insyde T701/T701, BIOS BYT70A.YNCHENG.WIN.007 
> 08/2
> [   19.811749] task: 988df7ab2500 task.stack: ac1400474000
> [   19.811752] RIP: 0010:clk_core_disable+0xc0/0x130
> ...
> [   19.811775] Call Trace:
> [   19.811783]  clk_core_disable_lock+0x1f/0x30
> [   19.811788]  clk_disable+0x1f/0x30
> [   19.811794]  gmin_flisclk_ctrl+0x87/0xf0
> [   19.811801]  0xc0528512
> [   19.811805]  0xc05295e2
> [   19.811811]  ? acpi_device_wakeup_disable+0x50/0x60
> [   19.811815]  ? acpi_dev_pm_attach+0x8e/0xd0
> [   19.811818]  ? 0xc05294d0
> [   19.811823]  i2c_device_probe+0x1cd/0x280
> [   19.811828]  driver_probe_device+0x2ff/0x450
> 
> Fixes: "staging: atomisp: use clock framework for camera clocks"
> Signed-off-by: Hans de Goede 
> ---
>  .../media/atomisp/platform/intel-mid/atomisp_gmin_platform.c   | 7 
> +++
>  1 file changed, 7 insertions(+)
> 
> diff --git 
> a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c 
> b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> index 828fe5abd832..6671ebe4ecc9 100644
> --- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> +++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
> @@ -29,6 +29,7 @@ struct gmin_subdev {
>   struct v4l2_subdev *subdev;
>   int clock_num;
>   int clock_src;
> + bool clock_on;
>   struct clk *pmc_clk;
>   struct gpio_desc *gpio0;
>   struct gpio_desc *gpio1;
> @@ -583,6 +584,9 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, 
> int on)
>   struct gmin_subdev *gs = find_gmin_subdev(subdev);
>   struct i2c_client *client = v4l2_get_subdevdata(subdev);
>  
> + if (gs->clock_on == !!on)
> + return 0;
> +
>   if (on) {
>   ret = clk_set_rate(gs->pmc_clk, gs->clock_src);

Which tree [and branch] are you working off please? In the staging-next branch 
of Greg's staging
tree this function does not appear as it is in this patch.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: rtlwifi: Remove NULL pointer dereference

[Tobin C. Harding]

On Wed, Oct 11, 2017 at 06:02:47PM +0530, Shreeya Patel wrote:
> On Tue, 2017-10-10 at 11:06 +1100, Tobin C. Harding wrote:
> > On Tue, Oct 10, 2017 at 02:48:58AM +0530, Shreeya Patel wrote:
> > > 
> > > Remove NULL pointer dereference as it results in undefined
> > > behaviour, and will usually lead to a runtime error.
> > The diff does not show any pointer dereference so it is hard to
> > understand what you are trying to do
> > with this patch.
> > 
> > > 
> > > Signed-off-by: Shreeya Patel 
> > > ---
> > >  drivers/staging/rtlwifi/base.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/rtlwifi/base.c
> > > b/drivers/staging/rtlwifi/base.c
> > > index b88b0e8..5bb8f98 100644
> > > --- a/drivers/staging/rtlwifi/base.c
> > > +++ b/drivers/staging/rtlwifi/base.c
> > > @@ -781,7 +781,7 @@ static void _rtl_txrate_selectmode(struct
> > > ieee80211_hw *hw,
> > >  
> > >   struct rtl_priv *rtlpriv = rtl_priv(hw);
> > >   struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
> > > - struct rtl_sta_info *sta_entry = NULL;
> > > + struct rtl_sta_info *sta_entry;
> > Now the pointer just has garbage in it instead of the testable value
> > of NULL. If you are concerned
> > with the dereference perhaps you could add a NULL check, again it's
> > hard to say without seeing the
> > code.
> 
> Hello, 
> 
> Thanks for making me understand. 
> 
> Here is the code after declaration and initialization of sta_entry. 
> Will it be good to add a NULL check in this case? 
> 
> struct rtl_sta_info *sta_entry = NULL;
>   u8 ratr_index = SET_RATE_ID(RATR_INX_WIRELESS_MC);
> 
>   if (sta) {
>   sta_entry = (struct rtl_sta_info *)sta->drv_priv;
>   ratr_index = sta_entry->ratr_index;
>   }

Later in this function the macro SET_RATE_ID() is called, it relies on 
sta_entry being NULL if it
was not explicitly set.

Here is the macro;

#define SET_RATE_ID(rate_id)\
((rtlpriv->cfg->spec_ver & RTL_SPEC_NEW_RATEID) ?   \
rtl_mrate_idx_to_arfr_id(hw, rate_id,   \
(sta_entry ? sta_entry->wireless_mode : \
 WIRELESS_MODE_G)) :\
rate_id)

> If we are making a pointer point to NULL then what if any other
> variable is already pointing to NULL for some other purpose.
> Instead, removing initialization will be good right?

A pointer does not _point_ to NULL as such. A NULL pointer has a value of all 
zero bytes. Have you
read (and completed all the exercises) in KnR

https://en.wikipedia.org/wiki/The_C_Programming_Language

It is, in my opinion, one of the best tech books ever written. If you have any 
holes in your C
knowledge, this is the place to start.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] Staging: bcm2048 fix bare use of 'unsigned' in radio-bcm2048.c

[Tobin C. Harding]

Hi Branislav,

On Tue, Oct 10, 2017 at 03:29:19PM +0200, Branislav Radocaj wrote:
> This is a patch to the radio-bcm2048.c file that fixes up
> a warning found by the checkpatch.pl tool.
> 
> Signed-off-by: Branislav Radocaj 

Nice work, a few git log nit picks for you to ensure your future kernel 
development success.

You can read all this in Documentaton/process/submitting-patches.rst (section 
2).

- You can use imperative mood, i.e 'Fix foo by doing bar' instead of 'This 
patch ...'
- We don't need to mention the file (either in the summary or in the body), 
people can see this from
  the diff.

This is one way of writing the git log message for checkpatch fixes

checkpatch emits WARNING: EXPORT_SYMBOL(foo); should immediately follow
its function/variable.

Move EXPORT_SYMBOL macro call to immediately follow function definition.


Good work, hope this helps.

Tobin
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: rtlwifi: Remove NULL pointer dereference

[Tobin C. Harding]

On Tue, Oct 10, 2017 at 02:48:58AM +0530, Shreeya Patel wrote:
> Remove NULL pointer dereference as it results in undefined
> behaviour, and will usually lead to a runtime error.

The diff does not show any pointer dereference so it is hard to understand what 
you are trying to do
with this patch.

> Signed-off-by: Shreeya Patel 
> ---
>  drivers/staging/rtlwifi/base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtlwifi/base.c b/drivers/staging/rtlwifi/base.c
> index b88b0e8..5bb8f98 100644
> --- a/drivers/staging/rtlwifi/base.c
> +++ b/drivers/staging/rtlwifi/base.c
> @@ -781,7 +781,7 @@ static void _rtl_txrate_selectmode(struct ieee80211_hw 
> *hw,
>  
>   struct rtl_priv *rtlpriv = rtl_priv(hw);
>   struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
> - struct rtl_sta_info *sta_entry = NULL;
> + struct rtl_sta_info *sta_entry;

Now the pointer just has garbage in it instead of the testable value of NULL. 
If you are concerned
with the dereference perhaps you could add a NULL check, again it's hard to say 
without seeing the
code.

It is hard to see how this patch is correct though.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/18] use ARRAY_SIZE macro

[Tobin C. Harding]

On Sun, Oct 01, 2017 at 03:30:38PM -0400, Jérémy Lefaure wrote:
> Hi everyone,
> Using ARRAY_SIZE improves the code readability. I used coccinelle (I
> made a change to the array_size.cocci file [1]) to find several places
> where ARRAY_SIZE could be used instead of other macros or sizeof
> division.
> 
> I tried to divide the changes into a patch per subsystem (excepted for
> staging). If one of the patch should be split into several patches, let
> me know.
> 
> In order to reduce the size of the To: and Cc: lines, each patch of the
> series is sent only to the maintainers and lists concerned by the patch.
> This cover letter is sent to every list concerned by this series.

Why don't you just send individual patches for each subsystem? I'm not a 
maintainer but I don't see
how any one person is going to be able to apply this whole series, it is making 
it hard for
maintainers if they have to pick patches out from among the series (if indeed 
any will bother
doing that).

I get that this will be more work for you but AFAIK we are optimizing for 
maintainers.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] Staging: rtl8723bs: Remove unnecessary comments.

[Tobin C. Harding]

On Sat, Sep 30, 2017 at 07:41:11PM +0530, Shreeya Patel wrote:
> Remove unnecessary comments which are there
> to explain why call to memset is in comments. Both of the
> comments are not needed as they are not very useful.
> 
> 
> Signed-off-by: Shreeya Patel 
> ---
> Changes in v2:
>   -Remove some more unnecessary comments and make the
>commit message more appropriate.
> 
> Changes in v3:
>   -Make the commit message in imperative form.

Well done. You forgot the period on the commit subject. Here is a blog post you 
might like (it is
not kernel specific but useful still IMO).

Good luck,
Tobin.

>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 3 ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 ---
>  drivers/staging/rtl8723bs/core/rtw_pwrctrl.c  | 2 --
>  drivers/staging/rtl8723bs/core/rtw_recv.c | 4 
>  drivers/staging/rtl8723bs/core/rtw_xmit.c | 3 ---
>  5 files changed, 15 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 6b77820..5b583f7 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -28,9 +28,6 @@ sint_rtw_init_mlme_priv(struct adapter *padapter)
>   struct mlme_priv*pmlmepriv = &padapter->mlmepriv;
>   sintres = _SUCCESS;
>  
> - /*  We don't need to memset padapter->XXX to zero, because adapter is 
> allocated by vzalloc(). */
> - /* memset((u8 *)pmlmepriv, 0, sizeof(struct mlme_priv)); */
> -
>   pmlmepriv->nic_hdl = (u8 *)padapter;
>  
>   pmlmepriv->pscanned = NULL;
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index b6d137f..ca35c1c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -474,9 +474,6 @@ int   init_mlme_ext_priv(struct adapter *padapter)
>   struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
>   struct mlme_ext_info *pmlmeinfo = &(pmlmeext->mlmext_info);
>  
> - /*  We don't need to memset padapter->XXX to zero, because adapter is 
> allocated by vzalloc(). */
> - /* memset((u8 *)pmlmeext, 0, sizeof(struct mlme_ext_priv)); */
> -
>   pmlmeext->padapter = padapter;
>  
>   /* fill_fwpriv(padapter, &(pmlmeext->fwpriv)); */
> diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c 
> b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> index aabdaaf..820a061 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
> @@ -1193,8 +1193,6 @@ void rtw_init_pwrctrl_priv(struct adapter *padapter)
>  
>  void rtw_free_pwrctrl_priv(struct adapter *adapter)
>  {
> - /* memset((unsigned char *)pwrctrlpriv, 0, sizeof(struct 
> pwrctrl_priv)); */
> -
>  #ifdef CONFIG_PNO_SUPPORT
>   if (pwrctrlpriv->pnlo_info != NULL)
>   printk("** pnlo_info memory leak\n");
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c 
> b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index 68a6303..73e6e41 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -46,9 +46,6 @@ sint _rtw_init_recv_priv(struct recv_priv *precvpriv, 
> struct adapter *padapter)
>   union recv_frame *precvframe;
>   sintres = _SUCCESS;
>  
> - /*  We don't need to memset padapter->XXX to zero, because adapter is 
> allocated by vzalloc(). */
> - /* memset((unsigned char *)precvpriv, 0, sizeof (struct  recv_priv)); */
> -
>   spin_lock_init(&precvpriv->lock);
>  
>   _rtw_init_queue(&precvpriv->free_recv_queue);
> @@ -65,7 +62,6 @@ sint _rtw_init_recv_priv(struct recv_priv *precvpriv, 
> struct adapter *padapter)
>   res = _FAIL;
>   goto exit;
>   }
> - /* memset(precvpriv->pallocated_frame_buf, 0, NR_RECVFRAME * 
> sizeof(union recv_frame) + RXFRAME_ALIGN_SZ); */
>  
>   precvpriv->precv_frame_buf = (u8 
> *)N_BYTE_ALIGMENT((SIZE_PTR)(precvpriv->pallocated_frame_buf), 
> RXFRAME_ALIGN_SZ);
>   /* precvpriv->precv_frame_buf = precvpriv->pallocated_frame_buf + 
> RXFRAME_ALIGN_SZ - */
> diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
> b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> index 022f654..8cd05f8 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
> @@ -51,9 +51,6 @@ s32 _rtw_init_xmit_priv(struct xmit_priv *pxmitpriv, struct 
> adapter *padapter)
>   struct xmit_frame *pxframe;
>   sintres = _SUCCESS;
>  
> - /*  We don't need to memset padapter->XXX to zero, because adapter is 
> allocated by vzalloc(). */
> - /* memset((unsigned char *)pxmitpriv, 0, sizeof(struct xmit_priv)); */
> -
>   spin_lock_init(&pxmitpriv->lock);
>   spin_lock_init(&pxmitpriv->lock_sctx);
>   sema_init(&pxmitpriv->xmit_sema, 0);
> -- 
> 2.7.4
> 
> 

Re: [PATCH 0/6] Replace container_of with list_entry

[Tobin C. Harding]

On Sat, Sep 30, 2017 at 12:49:00PM +0530, Srishti Sharma wrote:
> Replaces instances of container_of with list_entry to 
> access current list element.
> 
> Srishti Sharma (6):
>   Staging: rtl8188eu: core: Use list_entry instead of container_of
>   Staging: rtl8188eu: core: Use list_entry instead of container_of
>   Staging: rtl8188eu: core: Use list_entry instead of container_of
>   Staging: rtl8188eu: core: Use list_entry instead of container_of
>   Staging: rtl8188eu: core: Use list_entry instead of container_of
>   Staging: rtl8188eu: core: Use list_entry instead of container_of

You may have trouble getting patches merged with duplicate commit logs like 
this. The reason is that
the git index should be grep'able. You may like to squash all of these commits 
into a single patch
since they all do the same thing. The mantra is 'one thing per patch' so this 
makes sense in this
case.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] Staging: rtl8723bs: Remove unnecessary comments.

[Tobin C. Harding]

Hi Shreeya,

We don't usually add a period to the subject line for kernel patches. (reason: 
we only have about
52 characters for the commit brief description so best not to waste any).

On Sat, Sep 30, 2017 at 01:30:34PM +0530, Shreeya Patel wrote:
> This patch removes unnecessary comments which are there
> to explain why call to memset is in comments. Both of the
> comments are not needed as they are not very useful.

You may like to read Documentation/process/submitting-patches.rst (specifically
section 2) for tips on writing your git log.

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1] staging: rtl8188eu: Fix spelling

[Tobin C. Harding]

Hi Valentine,

I can't quite work out the email threading of this patch. My guess is that if I 
cannot work it out
it might get missed by Greg.

Is this a new patch that you made by squashing the three patches previously 
submitted into one? If
so, my suggestion would be to respond to this patch yourself with 'please drop 
this patch' (this lets
maintainers know to not worry further with it). Then submit the patch again 
without the
'In-Reply-To' header i.e send the patch with `git send-email`. You don't need 
v1 in the subject for
version 1, that is implicit.

On Thu, Sep 14, 2017 at 06:34:20PM -0700, Valentine Sinitsyn wrote:
> rtl8188eu contains some spelling errors in comment lines as well as in
> constants. Harmless as they are, they still make the code feel a bit
> unclean, which is not something we want in the kernel.

Nice description.

> Improve this by fixing typos so they won't catch eyes of future driver
> developers anymore.

This would be better in imperative mood i.e "Fix typos so they won't catch the 
eyes of future
developers."

> Signed-off-by: Wolfgang Hartmann 
> Signed-off-by: Manish Shrestha 
> Signed-off-by: Valentine Sinitsyn 

 Reviewed-by: Tobin C. Harding 

> ---
>  drivers/staging/rtl8188eu/core/rtw_efuse.c| 2 +-
>  drivers/staging/rtl8188eu/core/rtw_mlme.c | 2 +-
>  drivers/staging/rtl8188eu/hal/odm_HWConfig.c  | 4 ++--
>  drivers/staging/rtl8188eu/include/odm.h   | 2 +-
>  drivers/staging/rtl8188eu/include/rtl8188e_spec.h | 4 ++--
>  5 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_efuse.c 
> b/drivers/staging/rtl8188eu/core/rtw_efuse.c
> index b9bdff0..2c4c8c4 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_efuse.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_efuse.c
> @@ -48,7 +48,7 @@ void Efuse_PowerSwitch(
>   if (PwrState) {
>   usb_write8(pAdapter, REG_EFUSE_ACCESS, EFUSE_ACCESS_ON);
>  
> - /*  1.2V Power: From VDDON with Power Cut(0xh[15]), defualt 
> valid */
> + /*  1.2V Power: From VDDON with Power Cut(0xh[15]), default 
> valid */
>   tmpV16 = usb_read16(pAdapter, REG_SYS_ISO_CTRL);
>   if (!(tmpV16 & PWC_EV12V)) {
>   tmpV16 |= PWC_EV12V;
> diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme.c 
> b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> index f663e6c..0d2381d 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_mlme.c
> @@ -1329,7 +1329,7 @@ void rtw_cpwm_event_callback(struct adapter *padapter, 
> u8 *pbuf)
>  }
>  
>  /*
> - * _rtw_join_timeout_handler - Timeout/faliure handler for CMD JoinBss
> + * _rtw_join_timeout_handler - Timeout/failure handler for CMD JoinBss
>   * @adapter: pointer to struct adapter structure
>   */
>  void _rtw_join_timeout_handler (unsigned long data)
> diff --git a/drivers/staging/rtl8188eu/hal/odm_HWConfig.c 
> b/drivers/staging/rtl8188eu/hal/odm_HWConfig.c
> index 0555e42..5fcbe56 100644
> --- a/drivers/staging/rtl8188eu/hal/odm_HWConfig.c
> +++ b/drivers/staging/rtl8188eu/hal/odm_HWConfig.c
> @@ -109,7 +109,7 @@ static void odm_RxPhyStatus92CSeries_Parsing(struct 
> odm_dm_struct *dm_odm,
>  
>   dm_odm->PhyDbgInfo.NumQryPhyStatusCCK++;
>   /*  (1)Hardware does not provide RSSI for CCK */
> - /*  (2)PWDB, Average PWDB cacluated by hardware (for rate 
> adaptive) */
> + /*  (2)PWDB, Average PWDB calculated by hardware (for rate 
> adaptive) */
>  
>   cck_highpwr = dm_odm->bCckHighPower;
>  
> @@ -223,7 +223,7 @@ static void odm_RxPhyStatus92CSeries_Parsing(struct 
> odm_dm_struct *dm_odm,
>   pPhyInfo->RxSNR[i] = (s32)(pPhyStaRpt->path_rxsnr[i]/2);
>   dm_odm->PhyDbgInfo.RxSNRdB[i] = 
> (s32)(pPhyStaRpt->path_rxsnr[i]/2);
>   }
> - /*  (2)PWDB, Average PWDB cacluated by hardware (for rate 
> adaptive) */
> + /*  (2)PWDB, Average PWDB calculated by hardware (for rate 
> adaptive) */
>   rx_pwr_all = (((pPhyStaRpt->cck_sig_qual_ofdm_pwdb_all) >> 1) & 
> 0x7f) - 110;
>  
>   PWDB_ALL = odm_QueryRxPwrPercentage(rx_pwr_all);
> diff --git a/drivers/staging/rtl8188eu/include/odm.h 
> b/drivers/staging/rtl8188eu/include/odm.h
> index 4fb3bb0..50e2673 100644
> --- a/drivers/staging/rtl8188eu/include/odm.h
> +++ b/drivers/staging/rtl8188eu/include/odm.h
> @@ -478,7 +478,7 @@ enum odm_operation_mode {
>  
>  /*  ODM_CMNINFO_WM_MODE */
>  enum odm_wireless_mode {
> - ODM_WM_UNKNOW   = 0x0,
> + ODM_WM_UNKNOWN  

Re: [PATCH 0/3] Spelling fixes for rtl8188eu

[Tobin C. Harding]

On Thu, Sep 14, 2017 at 12:05:17PM -0700, Valentine Sinitsyn wrote:
> There are some fixes for typos we discovered in rtl8188eu staging driver
> as a part of our Open Source Summit North America 2017 tutorial session.
> 
> Valentine Sinitsyn (3):
>   staging: rtl8188eu: Fix a typo ("deflaut")
>   staging: rtl8188eu: Fix a typo ("faliure")
>   taging: rtl8188eu: Fix a typo ("cacluated")

All three of these changes could have been done in a single patch. The saying 
goes 'one thing per
patch', since all three patches in this set fix typos they could all be in a 
single patch.

Good luck,
Tobin.

>  drivers/staging/rtl8188eu/core/rtw_efuse.c   | 2 +-
>  drivers/staging/rtl8188eu/core/rtw_mlme.c| 2 +-
>  drivers/staging/rtl8188eu/hal/odm_HWConfig.c | 4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.7.4
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] staging: rtl8188eu: Fix a typo ("deflaut")

[Tobin C. Harding]

Good work getting this set submitted.

On Thu, Sep 14, 2017 at 12:05:18PM -0700, Valentine Sinitsyn wrote:
> This improves spelling in the comment line to make things easier to get
> for future rtl8188eu developers.
> 
> Fixes: ee5f8a431ea (staging: rtl8188eu: Move all efuse related code to
> rtw_efuse.c)

You may like to re-read Documentation/process/submitting-patches.rst to check 
the format for commit
logs. Preferred style is

1. what is wrong with the code i.e why the patch is necessary.
2. what the patch does to fix the above described problem.

Also, prefer imperative mood i.e 'Do XYZ' instead of 'This patch does XYZ'

> Signed-off-by: Wolfgang Hartmann 
> Signed-off-by: Manish Shrestha 
> Signed-off-by: Valentine Sinitsyn 
> ---
>  drivers/staging/rtl8188eu/core/rtw_efuse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8188eu/core/rtw_efuse.c 
> b/drivers/staging/rtl8188eu/core/rtw_efuse.c
> index b9bdff0..2c4c8c4 100644
> --- a/drivers/staging/rtl8188eu/core/rtw_efuse.c
> +++ b/drivers/staging/rtl8188eu/core/rtw_efuse.c
> @@ -48,7 +48,7 @@ void Efuse_PowerSwitch(
>   if (PwrState) {
>   usb_write8(pAdapter, REG_EFUSE_ACCESS, EFUSE_ACCESS_ON);
>  
> - /*  1.2V Power: From VDDON with Power Cut(0xh[15]), defualt 
> valid */
> + /*  1.2V Power: From VDDON with Power Cut(0xh[15]), default 
> valid */
>   tmpV16 = usb_read16(pAdapter, REG_SYS_ISO_CTRL);
>   if (!(tmpV16 & PWC_EV12V)) {
>   tmpV16 |= PWC_EV12V;
> -- 
> 2.7.4
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Fix unbalanced braces around else statement

[Tobin C. Harding]

On Wed, Sep 13, 2017 at 04:14:29PM +0100, Liam Ryan wrote:
> On Wed, Sep 13, 2017 at 08:47:39AM +0200, Frans Klaver wrote:
> > On Tue, Sep 12, 2017 at 2:40 AM, Liam Ryan  wrote:
> > > Fix checkpath-reported unbalanced braces in the following areas
> > >
> > > 221: FILE: drivers/staging/rtl8712/hal_init.c:221:
> > > 392: FILE: drivers/staging/rtl8712/os_intfs.c:392:
> > > 363: FILE: drivers/staging/rtl8712/rtl8712_cmd.c:363:
> > > 889: FILE: drivers/staging/rtl8712/rtl8712_recv.c:889:
> > > 902: FILE: drivers/staging/rtl8712/rtl871x_cmd.c:902:
> > > 84: FILE: drivers/staging/rtl8712/rtl871x_ioctl_set.c:84:
> > > 580: FILE: drivers/staging/rtl8712/rtl871x_mlme.c:580:
> > > 593: FILE: drivers/staging/rtl8712/usb_intf.c:593:
> > >
> > > Signed-off-by: Liam Ryan 
> > > ---
> > > This is my first patch and I have several doubts about style choices
> > >
> > > At line 216 of hal_init.c should opening brace follow comment instead?
> > >
> > > At line 577 of rtl871x_mlme.c should I bring logic to one line instead of
> > > opening the brace on the continued line?
> > >
> > > At line 353 of rtl8712_cmd.c the if/else is technically only 1 line each.
> > > Should the braces still have been added per checkpath for readability?
> > >
> > >  drivers/staging/rtl8712/hal_init.c  | 4 ++--
> > >  drivers/staging/rtl8712/os_intfs.c  | 4 ++--
> > >  drivers/staging/rtl8712/rtl8712_cmd.c   | 4 ++--
> > >  drivers/staging/rtl8712/rtl8712_recv.c  | 4 ++--
> > >  drivers/staging/rtl8712/rtl871x_cmd.c   | 3 ++-
> > >  drivers/staging/rtl8712/rtl871x_ioctl_set.c | 4 ++--
> > >  drivers/staging/rtl8712/rtl871x_mlme.c  | 4 ++--
> > >  drivers/staging/rtl8712/usb_intf.c  | 3 ++-
> > >  8 files changed, 16 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8712/hal_init.c 
> > > b/drivers/staging/rtl8712/hal_init.c
> > > index c83d7eb..de832b0b5 100644
> > > --- a/drivers/staging/rtl8712/hal_init.c
> > > +++ b/drivers/staging/rtl8712/hal_init.c
> > > @@ -216,9 +216,9 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
> > > emem_sz = fwhdr.img_SRAM_size;
> > > do {
> > > memset(ptx_desc, 0, TXDESC_SIZE);
> > > -   if (emem_sz >  MAX_DUMP_FWSZ) /* max=48k */
> > > +   if (emem_sz >  MAX_DUMP_FWSZ) { /* max=48k */
> > 
> > I would not have the comment there in the first place. It doesn't add
> > any new information and just messes up the style.
> I'm happy to remove the comment, the patch was accepted so I'm not sure
> of procedure, should it be a new patch or a revision to this patch? 
> 
> In general I don't have the knowledge to decide which comments are worth 
> keeping in the source 
> code.

While you are getting started, if you are unsure of things I tend to lean 
towards making the minimal
change to improve the code. A patch will be rejected if there are obvious extra 
fixes that need
doing. Review will point these out, reviewers generally don't mind doing this 
because that is how you
learn. Please be sure to carefully study these suggestions and learn from them 
so review does not
have to point them out again unnecessarily. 

> is there a rule of thumb for brace placement near inline comments such as 
> this?

Like Frans said; If there is more than one line of code (irrelevant to whether 
it is comments or a
single statement over two lines) braces make the code cleaner.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: rtlwifi: remove unused functions

[Tobin C. Harding]

On Tue, Sep 05, 2017 at 09:46:55AM -0500, Larry Finger wrote:
> On 09/05/2017 01:53 AM, Tobin C. Harding wrote:
> >Functions rtl_rfreg_delay() and rtl_bb_delay() are unused within the
> >driver. Both functions call rtl_addr_delay(), this function is unused
> >outside of these call sites.The driver (and kernel) code base is cleaner
> >if unused functions are removed.
> >
> >Remove unused functions.
> 
> While it is true that those routines are unused in the staging instance of
> rtlwifi, they are needed in the copy in drivers/net/wireless/... I would
> prefer to keep the differences between these two versions as small as
> possible; therefore, NACK
> 
> Larry

No worries Larry, thanks for the response. Sorry to waste your time.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/3] staging: rtlwifi: remove unused functions

[Tobin C. Harding]

Functions rtl_rfreg_delay() and rtl_bb_delay() are unused within the
driver. Both functions call rtl_addr_delay(), this function is unused
outside of these call sites.The driver (and kernel) code base is cleaner
if unused functions are removed.

Remove unused functions.
---
 drivers/staging/rtlwifi/core.c | 37 -
 drivers/staging/rtlwifi/core.h |  4 
 2 files changed, 41 deletions(-)

diff --git a/drivers/staging/rtlwifi/core.c b/drivers/staging/rtlwifi/core.c
index 6ddf7e8..43b8b9ef 100644
--- a/drivers/staging/rtlwifi/core.c
+++ b/drivers/staging/rtlwifi/core.c
@@ -49,43 +49,6 @@ u8 channel5g_80m[CHANNEL_MAX_NUMBER_5G_80M] = {
42, 58, 106, 122, 138, 155, 171
 };
 
-void rtl_addr_delay(u32 addr)
-{
-   if (addr == 0xfe)
-   mdelay(50);
-   else if (addr == 0xfd)
-   msleep(5);
-   else if (addr == 0xfc)
-   msleep(1);
-   else if (addr == 0xfb)
-   usleep_range(50, 100);
-   else if (addr == 0xfa)
-   usleep_range(5, 10);
-   else if (addr == 0xf9)
-   usleep_range(1, 2);
-}
-
-void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
-u32 mask, u32 data)
-{
-   if (addr >= 0xf9 && addr <= 0xfe) {
-   rtl_addr_delay(addr);
-   } else {
-   rtl_set_rfreg(hw, rfpath, addr, mask, data);
-   udelay(1);
-   }
-}
-
-void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
-{
-   if (addr >= 0xf9 && addr <= 0xfe) {
-   rtl_addr_delay(addr);
-   } else {
-   rtl_set_bbreg(hw, addr, MASKDWORD, data);
-   udelay(1);
-   }
-}
-
 static void rtl_fw_do_work(const struct firmware *firmware, void *context,
   bool is_wow)
 {
diff --git a/drivers/staging/rtlwifi/core.h b/drivers/staging/rtlwifi/core.h
index 782ac2f..4c2b694 100644
--- a/drivers/staging/rtlwifi/core.h
+++ b/drivers/staging/rtlwifi/core.h
@@ -75,10 +75,6 @@ enum dm_dig_connect_e {
 extern const struct ieee80211_ops rtl_ops;
 void rtl_fw_cb(const struct firmware *firmware, void *context);
 void rtl_wowlan_fw_cb(const struct firmware *firmware, void *context);
-void rtl_addr_delay(u32 addr);
-void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 addr,
-u32 mask, u32 data);
-void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data);
 bool rtl_cmd_send_packet(struct ieee80211_hw *hw, struct sk_buff *skb);
 bool rtl_btc_status_false(void);
 void rtl_dm_diginit(struct ieee80211_hw *hw, u32 cur_igval);
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: rtlwifi: use kcalloc instead of multiply

[Tobin C. Harding]

checkpatch emits multiple warnings of type

WARNING:ALLOC_WITH_MULTIPLY: Prefer kcalloc over kzalloc with multiply

Replace two calls to kzalloc() with calls to kcalloc().
---
 drivers/staging/rtlwifi/efuse.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtlwifi/efuse.c b/drivers/staging/rtlwifi/efuse.c
index 6d5e657..d74c80d 100644
--- a/drivers/staging/rtlwifi/efuse.c
+++ b/drivers/staging/rtlwifi/efuse.c
@@ -252,12 +252,11 @@ void read_efuse(struct ieee80211_hw *hw, u16 _offset, u16 
_size_byte, u8 *pbuf)
sizeof(u8), GFP_ATOMIC);
if (!efuse_tbl)
return;
-   efuse_word = kzalloc(EFUSE_MAX_WORD_UNIT * sizeof(u16 *), GFP_ATOMIC);
+   efuse_word = kcalloc(EFUSE_MAX_WORD_UNIT, sizeof(u16 *), GFP_ATOMIC);
if (!efuse_word)
goto out;
for (i = 0; i < EFUSE_MAX_WORD_UNIT; i++) {
-   efuse_word[i] = kzalloc(efuse_max_section * sizeof(u16),
-   GFP_ATOMIC);
+   efuse_word[i] = kcalloc(efuse_max_section, sizeof(u16), 
GFP_ATOMIC);
if (!efuse_word[i])
goto done;
}
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/3] staging: rtlwifi: checkpatch fixes

[Tobin C. Harding]

Fix checkpatch warnings.

Patch 1: Fix CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open 
parenthesis.
Patch 2: Fix WARNING:ALLOC_WITH_MULTIPLY: Prefer kcalloc over kzalloc with 
multiply.
Patch 3: Remove unused functions (clears checkpatch warnings).

Code is untested. Builds on x86_64.

Tobin C. Harding (3):
  staging: rtlwifi: fix parenthesis alignment
  staging: rtlwifi: use kcalloc instead of multiply
  staging: rtlwifi: remove unused functions

 drivers/staging/rtlwifi/core.c  | 43 +++--
 drivers/staging/rtlwifi/core.h  |  4 
 drivers/staging/rtlwifi/efuse.c |  5 ++---
 3 files changed, 5 insertions(+), 47 deletions(-)

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: rtlwifi: fix parenthesis alignment

[Tobin C. Harding]

Checkpatch emits multiple warnings of type

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis

Fix parenthesis alignment in line with checkpatch suggestion.
---
 drivers/staging/rtlwifi/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtlwifi/core.c b/drivers/staging/rtlwifi/core.c
index d33847d..6ddf7e8 100644
--- a/drivers/staging/rtlwifi/core.c
+++ b/drivers/staging/rtlwifi/core.c
@@ -1160,7 +1160,7 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw 
*hw,
if (rtlpriv->dm.supp_phymode_switch) {
if (sta->ht_cap.ht_supported)
rtl_send_smps_action(hw, sta,
-   IEEE80211_SMPS_STATIC);
+
IEEE80211_SMPS_STATIC);
}
 
if (rtlhal->current_bandtype == BAND_ON_5G) {
@@ -1224,7 +1224,7 @@ static void rtl_op_bss_info_changed(struct ieee80211_hw 
*hw,
cfg80211_unlink_bss(hw->wiphy, bss);
cfg80211_put_bss(hw->wiphy, bss);
RT_TRACE(rtlpriv, COMP_MAC80211, DBG_DMESG,
-   "cfg80211_unlink !!\n");
+"cfg80211_unlink !!\n");
}
 
eth_zero_addr(mac->bssid);
@@ -1885,7 +1885,7 @@ bool rtl_hal_pwrseqcmdparsing(struct rtl_priv *rtlpriv, 
u8 cut_version,
break;
case PWR_CMD_WRITE:
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
-   "%s(): PWR_CMD_WRITE\n", __func__);
+"%s(): PWR_CMD_WRITE\n", __func__);
offset = GET_PWR_CFG_OFFSET(cfg_cmd);
 
/*Read the value from system register*/
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/4] staging: ks7010: cfg80211 conversion

[Tobin C. Harding]

On Wed, Jun 14, 2017 at 04:30:34PM +1000, Tobin C. Harding wrote:
[snip]

Please drop this series.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH V2] staging: unisys: visorhba - style fix

[Tobin C. Harding]

On Mon, Jun 19, 2017 at 03:28:19PM +, Kershner, David A wrote:
> 
> > -Original Message-
> > From: Derek Robson [mailto:robso...@gmail.com]
> > Sent: Friday, June 16, 2017 11:13 PM
> > To: Kershner, David A ;
> > gre...@linuxfoundation.org; Sell, Timothy C ;
> > Binder, David Anthony ; Wadgaonkar, Sameer
> > Laxmikant ;
> > marcos.souza@gmail.com; robso...@gmail.com
> > Cc: *S-Par-Maintainer ;
> > de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> > Subject: [PATCH V2] staging: unisys: visorhba - style fix
> > 
> > Fixed style of permissions to octal.
> > Found using checkpatch
> > 
> > Signed-off-by: Derek Robson 
> 
> I applied it and tested it. Though when I applied it, the V1 comment was 
> included in the patch commit. 

Derek, for clarification what David means is that the version notes
should go below the dotted line so as not to be included in the patch
commit. You may also like to read
Documentation/process/submitting-patches.rst for tips on writing your
commit log (specifically section 2).

> 
> 
> > 
> > V1 has vauge subject
> > ---

v1 -> v2
 - fix vague subject

> >  drivers/staging/unisys/visorhba/visorhba_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> > b/drivers/staging/unisys/visorhba/visorhba_main.c
> > index 2fd31c9762c6..a6e7a6bbc428 100644
> > --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> > +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> > @@ -1090,7 +1090,7 @@ static int visorhba_probe(struct visor_device *dev)
> > goto err_scsi_remove_host;
> > }
> > devdata->debugfs_info =
> > -   debugfs_create_file("info", S_IRUSR | S_IRGRP,
> > +   debugfs_create_file("info", 0440,
> > devdata->debugfs_dir, devdata,
> > &info_debugfs_fops);
> > if (!devdata->debugfs_info) {
> > --
> > 2.13.0
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/4] staging: ks7010: add initial cfg80211 implementation

[Tobin C. Harding]

On Thu, Jun 15, 2017 at 09:59:06AM +1000, Tobin C. Harding wrote:
> On Wed, Jun 14, 2017 at 12:24:21PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jun 14, 2017 at 04:30:37PM +1000, Tobin C. Harding wrote:
> > > Currently we are in the process of replacing the WEXT interface with
> > > the cfg80211 API. WEXT code is currently within a sub directory and
> > > not included in the module build.
> > > 
> > > The driver is designed with various layers of abstraction;
> > > 
> > >   - The main layer contains the net_device_ops callbacks. Also included
> > > in this layer is the rx/tx code.
> > >   - The SDIO layer contains code specific to the SDIO hardware.
> > >   - The cfg80211 layer contains the implementation of the cfg80211
> > > configuration API.
> > >   - The HIF (Host InterFace) layer provides driver policy and interfaces
> > > between the cfg80211 layer and the FIL.
> > >   - The FIL (Firmware Interface Layer) provides mechanism for
> > > interfacing with the firmware.
> > > 
> > > The firmware interface is derived from the WEXT driver, if we
> > > write code to interface with the firmware as a separate abstraction
> > > layer then the cfg80211 code and the rest of the driver functionality
> > > can be written cleanly from scratch.
> > > 
> > > The separate layers are restricted to calls as indicated by the
> > > following diagram, A --> B indicates layer A calls functions in layer B.
> > > 
> > > >  main (tx/rx)  <--->  SDIO
> > > |
> > > | |
> > > | |
> > > v v
> > > 
> > > cfg80211  <--->  HIF
> > > 
> > >   |
> > >   |
> > >   v
> > > 
> > >  FIL
> > > 
> > > Implementation status is outlined in README.rst. A todo list is
> > > included in TODO.rst.
> > > 
> > > Add initial cfg80211 implementation.
> > > 
> > > Signed-off-by: Tobin C. Harding 
> > > ---
> > >  drivers/staging/ks7010/Kconfig |2 +
> > >  drivers/staging/ks7010/Makefile|   28 +-
> > >  drivers/staging/ks7010/README.rst  |   91 +++
> > >  drivers/staging/ks7010/TODO.rst|   30 +
> > >  drivers/staging/ks7010/cfg80211.c  |  981 +++
> > >  drivers/staging/ks7010/cfg80211.h  |   40 ++
> > >  drivers/staging/ks7010/common.h|   33 +
> > >  drivers/staging/ks7010/eap.h   |   73 ++
> > >  drivers/staging/ks7010/fil.c   | 1294 
> > > 
> > >  drivers/staging/ks7010/fil.h   |  559 
> > >  drivers/staging/ks7010/fil_types.h |  851 
> > >  drivers/staging/ks7010/hif.c   |  505 ++
> > >  drivers/staging/ks7010/hif.h   |  202 ++
> > >  drivers/staging/ks7010/ks7010.h|  309 +
> > >  drivers/staging/ks7010/main.c  |  337 ++
> > >  drivers/staging/ks7010/rx.c|  130 
> > >  drivers/staging/ks7010/sdio.c  |  691 +++
> > >  drivers/staging/ks7010/sdio.h  |   37 ++
> > >  drivers/staging/ks7010/tx.c|  170 +
> > >  19 files changed, 6362 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/staging/ks7010/README.rst
> > >  create mode 100644 drivers/staging/ks7010/TODO.rst
> > >  create mode 100644 drivers/staging/ks7010/cfg80211.c
> > >  create mode 100644 drivers/staging/ks7010/cfg80211.h
> > >  create mode 100644 drivers/staging/ks7010/common.h
> > >  create mode 100644 drivers/staging/ks7010/eap.h
> > >  create mode 100644 drivers/staging/ks7010/fil.c
> > >  create mode 100644 drivers/staging/ks7010/fil.h
> > >  create mode 100644 drivers/staging/ks7010/fil_types.h
> > >  create mode 100644 drivers/staging/ks7010/hif.c
> > >  create mode 100644 drivers/staging/ks7010/hif.h
> > >  create mode 100644 drivers/staging/ks7010/ks7010.h
> > >  create mode 100644 drivers/staging/ks7010/main.c
> > >  create mode 100644 drivers/staging/ks7010/rx.c
> > >  create mode 100644 drivers/staging/ks7010/sdio.c
> > >  create mode 100644 drivers/staging/ks7010/sdio.h
> > >  create mode 100644 drivers/staging/ks7010/tx.c
> > > 
> > > diff --git a/drivers/staging/ks7010/Kconfig 
> > > b/drive

Re: [PATCH 3/4] staging: ks7010: add initial cfg80211 implementation

[Tobin C. Harding]

On Wed, Jun 14, 2017 at 12:24:21PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 14, 2017 at 04:30:37PM +1000, Tobin C. Harding wrote:
> > Currently we are in the process of replacing the WEXT interface with
> > the cfg80211 API. WEXT code is currently within a sub directory and
> > not included in the module build.
> > 
> > The driver is designed with various layers of abstraction;
> > 
> >   - The main layer contains the net_device_ops callbacks. Also included
> > in this layer is the rx/tx code.
> >   - The SDIO layer contains code specific to the SDIO hardware.
> >   - The cfg80211 layer contains the implementation of the cfg80211
> > configuration API.
> >   - The HIF (Host InterFace) layer provides driver policy and interfaces
> > between the cfg80211 layer and the FIL.
> >   - The FIL (Firmware Interface Layer) provides mechanism for
> > interfacing with the firmware.
> > 
> > The firmware interface is derived from the WEXT driver, if we
> > write code to interface with the firmware as a separate abstraction
> > layer then the cfg80211 code and the rest of the driver functionality
> > can be written cleanly from scratch.
> > 
> > The separate layers are restricted to calls as indicated by the
> > following diagram, A --> B indicates layer A calls functions in layer B.
> > 
> > >  main (tx/rx)  <--->  SDIO
> > |
> > | |
> > | |
> > v v
> > 
> > cfg80211  <--->  HIF
> > 
> >   |
> >   |
> >   v
> > 
> >  FIL
> > 
> > Implementation status is outlined in README.rst. A todo list is
> > included in TODO.rst.
> > 
> > Add initial cfg80211 implementation.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> >  drivers/staging/ks7010/Kconfig |2 +
> >  drivers/staging/ks7010/Makefile|   28 +-
> >  drivers/staging/ks7010/README.rst  |   91 +++
> >  drivers/staging/ks7010/TODO.rst|   30 +
> >  drivers/staging/ks7010/cfg80211.c  |  981 +++
> >  drivers/staging/ks7010/cfg80211.h  |   40 ++
> >  drivers/staging/ks7010/common.h|   33 +
> >  drivers/staging/ks7010/eap.h   |   73 ++
> >  drivers/staging/ks7010/fil.c   | 1294 
> > 
> >  drivers/staging/ks7010/fil.h   |  559 
> >  drivers/staging/ks7010/fil_types.h |  851 
> >  drivers/staging/ks7010/hif.c   |  505 ++
> >  drivers/staging/ks7010/hif.h   |  202 ++
> >  drivers/staging/ks7010/ks7010.h|  309 +
> >  drivers/staging/ks7010/main.c  |  337 ++
> >  drivers/staging/ks7010/rx.c|  130 
> >  drivers/staging/ks7010/sdio.c  |  691 +++
> >  drivers/staging/ks7010/sdio.h  |   37 ++
> >  drivers/staging/ks7010/tx.c|  170 +
> >  19 files changed, 6362 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/staging/ks7010/README.rst
> >  create mode 100644 drivers/staging/ks7010/TODO.rst
> >  create mode 100644 drivers/staging/ks7010/cfg80211.c
> >  create mode 100644 drivers/staging/ks7010/cfg80211.h
> >  create mode 100644 drivers/staging/ks7010/common.h
> >  create mode 100644 drivers/staging/ks7010/eap.h
> >  create mode 100644 drivers/staging/ks7010/fil.c
> >  create mode 100644 drivers/staging/ks7010/fil.h
> >  create mode 100644 drivers/staging/ks7010/fil_types.h
> >  create mode 100644 drivers/staging/ks7010/hif.c
> >  create mode 100644 drivers/staging/ks7010/hif.h
> >  create mode 100644 drivers/staging/ks7010/ks7010.h
> >  create mode 100644 drivers/staging/ks7010/main.c
> >  create mode 100644 drivers/staging/ks7010/rx.c
> >  create mode 100644 drivers/staging/ks7010/sdio.c
> >  create mode 100644 drivers/staging/ks7010/sdio.h
> >  create mode 100644 drivers/staging/ks7010/tx.c
> > 
> > diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
> > index 437b928..71f2026 100644
> > --- a/drivers/staging/ks7010/Kconfig
> > +++ b/drivers/staging/ks7010/Kconfig
> > @@ -1,5 +1,7 @@
> >  config KS7010
> > tristate "KeyStream KS7010 SDIO support"
> > +   depends on CFG80211
> > +depends on MMC
> 
> tabs vs. spaces???

Face palm.

> > ---help---
> >   This is a driver for KeyStream KS7010 based SDIO WIFI cards. It is
> >   found on at least later Specte

Re: [PATCH 1/4] staging: ks7010: move WEXT files to sub directory

[Tobin C. Harding]

On Wed, Jun 14, 2017 at 12:22:31PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 14, 2017 at 04:30:35PM +1000, Tobin C. Harding wrote:
> > Current driver implements the WEXT interface. WEXT is in maintenance
> > mode, we need to re-write the driver using the cfg80211 API. The current
> > driver is handy as a reference for the new implementation, we can keep
> > it in tree for now.
> > 
> > Move WEXT driver to sub directory, add dummy Makefile and Kconfig so
> > build completes successfully but does not process any files from the
> > WEXT directory.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> >  drivers/staging/ks7010/Kconfig| 6 +-
> >  drivers/staging/ks7010/Makefile   | 5 +
> >  drivers/staging/ks7010/{ => wext}/TODO| 0
> >  drivers/staging/ks7010/{ => wext}/eap_packet.h| 0
> >  drivers/staging/ks7010/{ => wext}/ks7010_sdio.c   | 0
> >  drivers/staging/ks7010/{ => wext}/ks7010_sdio.h   | 0
> >  drivers/staging/ks7010/{ => wext}/ks_hostif.c | 0
> >  drivers/staging/ks7010/{ => wext}/ks_hostif.h | 0
> >  drivers/staging/ks7010/{ => wext}/ks_wlan.h   | 0
> >  drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h | 0
> >  drivers/staging/ks7010/{ => wext}/ks_wlan_net.c   | 0
> >  drivers/staging/ks7010/{ => wext}/michael_mic.c   | 0
> >  drivers/staging/ks7010/{ => wext}/michael_mic.h   | 0
> >  13 files changed, 2 insertions(+), 9 deletions(-)
> >  rename drivers/staging/ks7010/{ => wext}/TODO (100%)
> >  rename drivers/staging/ks7010/{ => wext}/eap_packet.h (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.c (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.h (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks_hostif.c (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks_hostif.h (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks_wlan.h (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h (100%)
> >  rename drivers/staging/ks7010/{ => wext}/ks_wlan_net.c (100%)
> >  rename drivers/staging/ks7010/{ => wext}/michael_mic.c (100%)
> >  rename drivers/staging/ks7010/{ => wext}/michael_mic.h (100%)
> > 
> > diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
> > index 0b92176..437b928 100644
> > --- a/drivers/staging/ks7010/Kconfig
> > +++ b/drivers/staging/ks7010/Kconfig
> > @@ -1,10 +1,6 @@
> >  config KS7010
> > tristate "KeyStream KS7010 SDIO support"
> > -   depends on MMC && WIRELESS
> > -   select WIRELESS_EXT
> > -   select WEXT_PRIV
> > -   select FW_LOADER
> > -   help
> 
> With this patch, doesn't the code now break as it does depend on
> WIRELESS and MMC still?

The module builds successfully because the build does not include any
source files once this patch is applied.

> Why not keep those depends?

Since there is no code being built there are no dependencies.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/4] staging: ks7010: add note regarding patching WEXT

[Tobin C. Harding]

WEXT code has been moved to a sub directory and is no longer included
in the module build. We should make a note of this for future
developers.

Add note to WEXT code TODO file informing developers that code is not
included in module build.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/wext/TODO | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ks7010/wext/TODO b/drivers/staging/ks7010/wext/TODO
index d393ca5..8c0e309 100644
--- a/drivers/staging/ks7010/wext/TODO
+++ b/drivers/staging/ks7010/wext/TODO
@@ -1,6 +1,17 @@
 KS7010 Linux driver
 ===
 
+--
+** NOTE **
+--
+
+Code in this directory is not included in the kernel build. Please do
+not submit patches to this code unless you first make [local]
+modifications to the build process in order to verify that your
+patches build.
+
+--
+
 This driver is based on source code from the Ben Nanonote extra repository [1]
 which is based on the original v007 release from Renesas [2]. Some more
 background info about the chipset can be found here [3] and here [4]. Thank
@@ -21,9 +32,6 @@ First a few words what not to do (at least not blindly):
 
 Now the TODOs:
 
-- fix codechecker warnings (checkpatch, sparse, smatch). But PLEASE make sure
-  that you are not only silencing the warning but really fixing code. You
-  should understand the change you submit.
 - fix the 'card removal' event when card is inserted when booting
 - check what other upstream wireless mechanisms can be used instead of the
   custom ones here
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/4] staging: ks7010: cfg80211 conversion

[Tobin C. Harding]

Current ks7010 driver uses the WEXT interface. This series is the
initial stage of re-writing the driver to use cfg80211.

This patch set includes the code that was submitted as an RFC

 Subject: [RFC 0/3] staging: ks7010: cfg80211 conversion, add FIL

The RFC has been in flight for two weeks. All feed back has been seen
to.

In addition, this set includes the initial cfg80211 configuration API
implementation. Also includes rx and tx paths.

Patch 01 moves WEXT driver code into sub directory, includes empty
Makefile so build still passes.

Patch 02 adds a note to the WEXT todo file.

Patch 03 adds the new cfg80211 driver code.

Patch 04 adds an entry to the MAINTAINERS database.

Code is untested. Builds on x86_64 and PowerPC. Code is clear of any
warnings from checkpatch.pl, Sparse, and Spatch (kchecker).

Tobin C. Harding (4):
  staging: ks7010: move WEXT files to sub directory
  staging: ks7010: add note regarding patching WEXT
  staging: ks7010: add initial cfg80211 implementation
  MAINTAINERS: add maintainer entry for ks7010

 MAINTAINERS   |7 +
 drivers/staging/ks7010/Kconfig|8 +-
 drivers/staging/ks7010/Makefile   |   29 +-
 drivers/staging/ks7010/README.rst |   91 ++
 drivers/staging/ks7010/TODO.rst   |   30 +
 drivers/staging/ks7010/cfg80211.c |  981 
 drivers/staging/ks7010/cfg80211.h |   40 +
 drivers/staging/ks7010/common.h   |   33 +
 drivers/staging/ks7010/eap.h  |   73 ++
 drivers/staging/ks7010/fil.c  | 1294 +
 drivers/staging/ks7010/fil.h  |  559 +
 drivers/staging/ks7010/fil_types.h|  851 ++
 drivers/staging/ks7010/hif.c  |  505 
 drivers/staging/ks7010/hif.h  |  202 
 drivers/staging/ks7010/ks7010.h   |  309 +
 drivers/staging/ks7010/main.c |  337 ++
 drivers/staging/ks7010/rx.c   |  130 +++
 drivers/staging/ks7010/sdio.c |  691 +++
 drivers/staging/ks7010/sdio.h |   37 +
 drivers/staging/ks7010/tx.c   |  170 +++
 drivers/staging/ks7010/{ => wext}/TODO|   14 +-
 drivers/staging/ks7010/{ => wext}/eap_packet.h|0
 drivers/staging/ks7010/{ => wext}/ks7010_sdio.c   |0
 drivers/staging/ks7010/{ => wext}/ks7010_sdio.h   |0
 drivers/staging/ks7010/{ => wext}/ks_hostif.c |0
 drivers/staging/ks7010/{ => wext}/ks_hostif.h |0
 drivers/staging/ks7010/{ => wext}/ks_wlan.h   |0
 drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h |0
 drivers/staging/ks7010/{ => wext}/ks_wlan_net.c   |0
 drivers/staging/ks7010/{ => wext}/michael_mic.c   |0
 drivers/staging/ks7010/{ => wext}/michael_mic.h   |0
 31 files changed, 6380 insertions(+), 11 deletions(-)
 create mode 100644 drivers/staging/ks7010/README.rst
 create mode 100644 drivers/staging/ks7010/TODO.rst
 create mode 100644 drivers/staging/ks7010/cfg80211.c
 create mode 100644 drivers/staging/ks7010/cfg80211.h
 create mode 100644 drivers/staging/ks7010/common.h
 create mode 100644 drivers/staging/ks7010/eap.h
 create mode 100644 drivers/staging/ks7010/fil.c
 create mode 100644 drivers/staging/ks7010/fil.h
 create mode 100644 drivers/staging/ks7010/fil_types.h
 create mode 100644 drivers/staging/ks7010/hif.c
 create mode 100644 drivers/staging/ks7010/hif.h
 create mode 100644 drivers/staging/ks7010/ks7010.h
 create mode 100644 drivers/staging/ks7010/main.c
 create mode 100644 drivers/staging/ks7010/rx.c
 create mode 100644 drivers/staging/ks7010/sdio.c
 create mode 100644 drivers/staging/ks7010/sdio.h
 create mode 100644 drivers/staging/ks7010/tx.c
 rename drivers/staging/ks7010/{ => wext}/TODO (83%)
 rename drivers/staging/ks7010/{ => wext}/eap_packet.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.c (100%)
 rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_hostif.c (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_hostif.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan_net.c (100%)
 rename drivers/staging/ks7010/{ => wext}/michael_mic.c (100%)
 rename drivers/staging/ks7010/{ => wext}/michael_mic.h (100%)

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/4] staging: ks7010: move WEXT files to sub directory

[Tobin C. Harding]

Current driver implements the WEXT interface. WEXT is in maintenance
mode, we need to re-write the driver using the cfg80211 API. The current
driver is handy as a reference for the new implementation, we can keep
it in tree for now.

Move WEXT driver to sub directory, add dummy Makefile and Kconfig so
build completes successfully but does not process any files from the
WEXT directory.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/Kconfig| 6 +-
 drivers/staging/ks7010/Makefile   | 5 +
 drivers/staging/ks7010/{ => wext}/TODO| 0
 drivers/staging/ks7010/{ => wext}/eap_packet.h| 0
 drivers/staging/ks7010/{ => wext}/ks7010_sdio.c   | 0
 drivers/staging/ks7010/{ => wext}/ks7010_sdio.h   | 0
 drivers/staging/ks7010/{ => wext}/ks_hostif.c | 0
 drivers/staging/ks7010/{ => wext}/ks_hostif.h | 0
 drivers/staging/ks7010/{ => wext}/ks_wlan.h   | 0
 drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h | 0
 drivers/staging/ks7010/{ => wext}/ks_wlan_net.c   | 0
 drivers/staging/ks7010/{ => wext}/michael_mic.c   | 0
 drivers/staging/ks7010/{ => wext}/michael_mic.h   | 0
 13 files changed, 2 insertions(+), 9 deletions(-)
 rename drivers/staging/ks7010/{ => wext}/TODO (100%)
 rename drivers/staging/ks7010/{ => wext}/eap_packet.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.c (100%)
 rename drivers/staging/ks7010/{ => wext}/ks7010_sdio.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_hostif.c (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_hostif.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan_ioctl.h (100%)
 rename drivers/staging/ks7010/{ => wext}/ks_wlan_net.c (100%)
 rename drivers/staging/ks7010/{ => wext}/michael_mic.c (100%)
 rename drivers/staging/ks7010/{ => wext}/michael_mic.h (100%)

diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
index 0b92176..437b928 100644
--- a/drivers/staging/ks7010/Kconfig
+++ b/drivers/staging/ks7010/Kconfig
@@ -1,10 +1,6 @@
 config KS7010
tristate "KeyStream KS7010 SDIO support"
-   depends on MMC && WIRELESS
-   select WIRELESS_EXT
-   select WEXT_PRIV
-   select FW_LOADER
-   help
+   ---help---
  This is a driver for KeyStream KS7010 based SDIO WIFI cards. It is
  found on at least later Spectec SDW-821 (FCC-ID "S2Y-WLAN-11G-K" only,
  sadly not FCC-ID "S2Y-WLAN-11B-G") and Spectec SDW-823 microSD cards.
diff --git a/drivers/staging/ks7010/Makefile b/drivers/staging/ks7010/Makefile
index 69fcf8d..9444885 100644
--- a/drivers/staging/ks7010/Makefile
+++ b/drivers/staging/ks7010/Makefile
@@ -1,4 +1 @@
-obj-$(CONFIG_KS7010) += ks7010.o
-
-ccflags-y   += -DKS_WLAN_DEBUG=0
-ks7010-y:= michael_mic.o ks_hostif.o ks_wlan_net.o ks7010_sdio.o
+# Makefile intentionally left blank
diff --git a/drivers/staging/ks7010/TODO b/drivers/staging/ks7010/wext/TODO
similarity index 100%
rename from drivers/staging/ks7010/TODO
rename to drivers/staging/ks7010/wext/TODO
diff --git a/drivers/staging/ks7010/eap_packet.h 
b/drivers/staging/ks7010/wext/eap_packet.h
similarity index 100%
rename from drivers/staging/ks7010/eap_packet.h
rename to drivers/staging/ks7010/wext/eap_packet.h
diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/wext/ks7010_sdio.c
similarity index 100%
rename from drivers/staging/ks7010/ks7010_sdio.c
rename to drivers/staging/ks7010/wext/ks7010_sdio.c
diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/wext/ks7010_sdio.h
similarity index 100%
rename from drivers/staging/ks7010/ks7010_sdio.h
rename to drivers/staging/ks7010/wext/ks7010_sdio.h
diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/wext/ks_hostif.c
similarity index 100%
rename from drivers/staging/ks7010/ks_hostif.c
rename to drivers/staging/ks7010/wext/ks_hostif.c
diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/wext/ks_hostif.h
similarity index 100%
rename from drivers/staging/ks7010/ks_hostif.h
rename to drivers/staging/ks7010/wext/ks_hostif.h
diff --git a/drivers/staging/ks7010/ks_wlan.h 
b/drivers/staging/ks7010/wext/ks_wlan.h
similarity index 100%
rename from drivers/staging/ks7010/ks_wlan.h
rename to drivers/staging/ks7010/wext/ks_wlan.h
diff --git a/drivers/staging/ks7010/ks_wlan_ioctl.h 
b/drivers/staging/ks7010/wext/ks_wlan_ioctl.h
similarity index 100%
rename from drivers/staging/ks7010/ks_wlan_ioctl.h
rename to drivers/staging/ks7010/wext/ks_wlan_ioctl.h
diff --git a/drivers/staging/ks7010/ks_wlan_net.c 
b/drivers/staging/ks7010/wext/ks_wlan_net.c
similarity index 100%
rename from drivers/staging/ks7010/ks_wlan_net.c
rename to drivers/staging/ks7010/wext/ks_wlan_net.c
diff --git a/drivers/staging/ks7010/michael_mic.c 
b/drivers/staging/ks

[PATCH 4/4] MAINTAINERS: add maintainer entry for ks7010

[Tobin C. Harding]

Driver ks7010 does not currently have a maintainer.

Take ownership of the driver and add an entry for the ks7010 driver to
the maintainers database.

Signed-off-by: Tobin C. Harding 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f6ef3f3..d5f856c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7428,6 +7428,13 @@ F:   Documentation/auxdisplay/ks0108
 F: drivers/auxdisplay/ks0108.c
 F: include/linux/ks0108.h
 
+KS7010 KEYSTREAM DRIVER
+M: Tobin C. Harding 
+L: driverdev-devel@linuxdriverproject.org
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
+S: Maintained
+F: drivers/staging/ks7010/
+
 L3MDEV
 M: David Ahern 
 L: net...@vger.kernel.org
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: use little-endian types

[Tobin C. Harding]

On Thu, Jun 08, 2017 at 11:06:54PM -0600, Perry Hooker wrote:
> This patch fixes a number of sparse warnings of the form:
> drivers/staging/ks7010/ks_hostif.c:2187:29:
>   warning: incorrect type in assignment (different base types)
> generated when storing little-endian data in variables
> that do not have a specified endianness.
> 
> Signed-off-by: Perry Hooker 

For what it's worth

 Reviewed-By: Tobin C. Harding 

> ---
>  drivers/staging/ks7010/ks_hostif.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c 
> b/drivers/staging/ks7010/ks_hostif.c
> index 697347b..db01a48 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -1821,7 +1821,7 @@ void hostif_receive(struct ks_wlan_private *priv, 
> unsigned char *p,
>  static
>  void hostif_sme_set_wep(struct ks_wlan_private *priv, int type)
>  {
> - u32 val;
> + __le32 val;
>  
>   switch (type) {
>   case SME_WEP_INDEX_REQUEST:
> @@ -1870,13 +1870,13 @@ void hostif_sme_set_wep(struct ks_wlan_private *priv, 
> int type)
>  }
>  
>  struct wpa_suite_t {
> - unsigned short size;
> + __le16 size;
>   unsigned char suite[4][CIPHER_ID_LEN];
>  } __packed;
>  
>  struct rsn_mode_t {
> - u32 rsn_mode;
> - u16 rsn_capability;
> + __le32 rsn_mode;
> + __le16 rsn_capability;
>  } __packed;
>  
>  static
> @@ -1884,7 +1884,7 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, 
> int type)
>  {
>   struct wpa_suite_t wpa_suite;
>   struct rsn_mode_t rsn_mode;
> - u32 val;
> + __le32 val;
>  
>   memset(&wpa_suite, 0, sizeof(wpa_suite));
>  
> @@ -1936,7 +1936,8 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, 
> int type)
>  
>   hostif_mib_set_request(priv, DOT11_RSN_CONFIG_UNICAST_CIPHER,
>  sizeof(wpa_suite.size) +
> -CIPHER_ID_LEN * wpa_suite.size,
> +CIPHER_ID_LEN *
> +le16_to_cpu(wpa_suite.size),
>  MIB_VALUE_TYPE_OSTRING, &wpa_suite);
>   break;
>   case SME_RSN_MCAST_REQUEST:
> @@ -2028,7 +2029,8 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, 
> int type)
>  
>   hostif_mib_set_request(priv, DOT11_RSN_CONFIG_AUTH_SUITE,
>  sizeof(wpa_suite.size) +
> -KEY_MGMT_ID_LEN * wpa_suite.size,
> +KEY_MGMT_ID_LEN *
> +le16_to_cpu(wpa_suite.size),
>  MIB_VALUE_TYPE_OSTRING, &wpa_suite);
>   break;
>   case SME_RSN_ENABLED_REQUEST:
> @@ -2165,7 +2167,7 @@ void hostif_sme_multicast_set(struct ks_wlan_private 
> *priv)
>   int mc_count;
>   struct netdev_hw_addr *ha;
>   char set_address[NIC_MAX_MCAST_LIST * ETH_ALEN];
> - unsigned long filter_type;
> + __le32 filter_type;
>   int i = 0;
>  
>   DPRINTK(3, "\n");
> @@ -2276,7 +2278,7 @@ void hostif_sme_sleep_set(struct ks_wlan_private *priv)
>  static
>  void hostif_sme_set_key(struct ks_wlan_private *priv, int type)
>  {
> - u32 val;
> + __le32 val;
>  
>   switch (type) {
>   case SME_SET_FLAG:
> @@ -2335,7 +2337,7 @@ static
>  void hostif_sme_set_pmksa(struct ks_wlan_private *priv)
>  {
>   struct pmk_cache_t {
> - u16 size;
> + __le16 size;
>   struct {
>   u8 bssid[ETH_ALEN];
>   u8 pmkid[IW_PMKID_LEN];
> @@ -2366,7 +2368,7 @@ void hostif_sme_set_pmksa(struct ks_wlan_private *priv)
>  static
>  void hostif_sme_execute(struct ks_wlan_private *priv, int event)
>  {
> - u32 val;
> + __le32 val;
>  
>   DPRINTK(3, "event=%d\n", event);
>   switch (event) {
> -- 
> 2.4.11
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ks7010 firmware upload fail

[Tobin C. Harding]

On Mon, Jun 05, 2017 at 08:07:40PM -0400, Brian Masney wrote:
> On Mon, Jun 05, 2017 at 08:24:44PM +1000, Tobin C. Harding wrote:
> > On Mon, Jun 05, 2017 at 09:22:12AM +0200, Wolfram Sang wrote:
> > > Tobin,
> > > 
> > > > My question is should I be digging further into the MMC code or be doing
> > > > something else with the driver code?
> > > 
> > > So, you haven't found any branch that worked? No plain v4.9 or the
> > > gen3-sdio branch from my tree?
> > 
> > I have not tried cloning your tree. I am using the Rasperry Pi kernel
> > source
> > 
> > https://github.com/raspberrypi/linux
> > 
> > which is v4.9.29
> 
> Hi Tobin,
> 
> Here are some instructions on how to compile an upstream kernel for a
> Raspberry Pi: http://elinux.org/RPi_Upstream_Kernel_Compilation.
> It describes setting up u-boot as the boot loader instead of the
> boot loader that the Raspberry Pi kernel uses. One gotcha to be aware of
> is that the config.txt file on the sd card won't be used.

Thanks Brian.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: ks7010 firmware upload fail

[Tobin C. Harding]

On Mon, Jun 05, 2017 at 09:22:12AM +0200, Wolfram Sang wrote:
> Tobin,
> 
> > My question is should I be digging further into the MMC code or be doing
> > something else with the driver code?
> 
> So, you haven't found any branch that worked? No plain v4.9 or the
> gen3-sdio branch from my tree?

I have not tried cloning your tree. I am using the Rasperry Pi kernel
source

https://github.com/raspberrypi/linux

which is v4.9.29

I checked out the last commit you made to the mainline then cross
compiled the driver from within the Rpi tree, copied the module over
to the Pi and insmod'd it.

To debug I was making code changes on my desktop, re cross compiling
the module and scp'ing it over (then rmmod'ing and insmod'ing the new module).

I'll try using a vanilla 4.9 kernel, and if that fails I'll try cloning your 
tree.

Thanks for getting back to me.

Will report my progress.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

ks7010 firmware upload fail

[Tobin C. Harding]

Hi,

I am attempting to test the ks7010 SIDO Wi-Fi driver (drivers/staging/ks7010/).

Currently probing the driver fails because of a firmware upload error.

I am seeking ideas on where to continue troubleshooting this issue.

Test setup:
- Spectec SDW-823 WIFI card (micro SD).
- Raspberry Pi B 1 (kernel 4.9.29) breakout board connected via GPIO pins.
- Driver code from current mainline (commit 
453e102db531ac1ffa55f3e03c4907c063125859)
  (with additional debugging output calls).

Debugging thus far:

- define DEBUG in ks7010 and mmc makefiles (core, card, host).

kernel messages from time of fail:

[ 3746.903972] mmc1: starting CMD53 arg 94002004 flags 01b5
[ 3746.903985] mmc1: blksz 4 blocks 1 flags 0100 tsac 1000 ms nsac 0
[ 3746.904077] mmc1: req done (CMD53): 0: 1000   
[ 3746.904090] mmc1: 4 bytes transferred: 0
[ 3746.904114] mmc1: starting CMD53 arg 94000804 flags 01b5
[ 3746.904126] mmc1: blksz 4 blocks 1 flags 0100 tsac 1000 ms nsac 0
[ 3746.904180] mmc1: req done (CMD53): 0: 1000   
[ 3746.904189] mmc1: 4 bytes transferred: 0
[ 3746.912784] ks7010_upload_firmware: updated index to: 600
[ 3746.912816] mmc1: starting CMD53 arg 9e80 flags 01b5
[ 3746.912831] mmc1: blksz 512 blocks 128 flags 0100 tsac 1000 ms nsac 0
[ 3746.916336] mmc1: req done (CMD53): 0: 1000   
[ 3746.916353] mmc1: 65536 bytes transferred: 0
[ 3746.916420] ks7010_upload_firmware: wrote to address 1 (DATA_WINDOW) 
65536 bytes
[ 3746.916429] ks7010_sdio_data_compare: read 65536 bytes from address 1
[ 3746.916436] debug messages from mmc/core are enabled
[ 3746.916459] mmc1: starting CMD53 arg 1e80 flags 01b5
[ 3746.916472] mmc1: blksz 512 blocks 128 flags 0200 tsac 1000 ms nsac 0
[ 3746.916575] mmc1: req done (CMD53): 0: 1000   
[ 3746.916588] mmc1: 0 bytes transferred: -84
[ 3746.920029] ks7010_sdio_read: sdio_memcpy_fromio() failed: -84
[ 3746.920100] ks7010: ERROR firmware load failed: 9

Briefly, firmware is uploaded to the card in chunks. Each chunk is
read back from the card to verify the transfer.

CMD53 can be seen to succeed when writing the first chunk to the
card. ks7010_sdio_data_compare() is then called which calls
ks7010_sdio_read() to read back the data written to the card. It is
this call that fails.

(CMD52 appears to be successful as well as CMD53 write).

All function calls (including memory addresses on the card, and kernel
buffer addresses) appear to be correct.

Removing the call to ks7010_sdio_data_compare() leads to an MMC read
error for each successive read (via CMD53) by the driver.

My question is should I be digging further into the MMC code or be doing
something else with the driver code?

Any suggestions, no matter how simple, most appreciated. This is my
first time digging into MMC and my first time really trying to trouble
shoot a kernel issue.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC 3/3] MAINTAINERS: add maintainer entry for ks7010

[Tobin C. Harding]

On Thu, Jun 01, 2017 at 12:46:30PM +0900, Greg KH wrote:
> On Thu, Jun 01, 2017 at 01:27:08PM +1000, Tobin C. Harding wrote:
> > Driver ks7010 does not currently have a maintainer.
> > 
> > Add maintainers entry for ks7010.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> >  MAINTAINERS | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9e98464..75250ee 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7429,6 +7429,13 @@ F:   Documentation/auxdisplay/ks0108
> >  F: drivers/auxdisplay/ks0108.c
> >  F: include/linux/ks0108.h
> >  
> > +KS7010 KEYSTREAM DRIVER
> > +M:  Tobin Harding 
> > +L:  driverdev-devel@linuxdriverproject.org
> > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> > +S:  Maintained
> > +F:  drivers/staging/ks7010/
> 
> All tabs please :)

Will fix in v2.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC 1/3] staging: ks7010: move WEXT files to sub directory

[Tobin C. Harding]

On Thu, Jun 01, 2017 at 12:45:37PM +0900, Greg KH wrote:
> On Thu, Jun 01, 2017 at 01:27:06PM +1000, Tobin C. Harding wrote:
> > Current driver implements the WEXT interface. WEXT is in maintenance
> > mode, we need to re-write the driver using cfg80211. The current
> > driver is handy as a reference for the new implementation, we can keep
> > it in tree for now.
> > 
> > Move WEXT driver to sub directory, add dummy Makefile and Kconfig so
> > build completes successfully but does not process any files from the
> > WEXT directory.
> > 
> > Signed-off-by: Tobin C. Harding 
> > ---
> >  drivers/staging/ks7010/Kconfig  |6 +-
> >  drivers/staging/ks7010/Makefile |3 -
> >  drivers/staging/ks7010/TODO |   36 -
> >  drivers/staging/ks7010/eap_packet.h |  144 --
> >  drivers/staging/ks7010/ks7010_sdio.c| 1079 --
> >  drivers/staging/ks7010/ks7010_sdio.h|  164 --
> >  drivers/staging/ks7010/ks_hostif.c  | 2638 ---
> >  drivers/staging/ks7010/ks_hostif.h  |  685 --
> >  drivers/staging/ks7010/ks_wlan.h|  514 -
> >  drivers/staging/ks7010/ks_wlan_ioctl.h  |   67 -
> >  drivers/staging/ks7010/ks_wlan_net.c| 2999 
> > ---
> >  drivers/staging/ks7010/michael_mic.c|  148 --
> >  drivers/staging/ks7010/michael_mic.h|   25 -
> >  drivers/staging/ks7010/wext/Kconfig |   10 +
> >  drivers/staging/ks7010/wext/Makefile|4 +
> >  drivers/staging/ks7010/wext/TODO|   36 +
> >  drivers/staging/ks7010/wext/eap_packet.h|  144 ++
> >  drivers/staging/ks7010/wext/ks7010_sdio.c   | 1079 ++
> >  drivers/staging/ks7010/wext/ks7010_sdio.h   |  164 ++
> >  drivers/staging/ks7010/wext/ks_hostif.c | 2638 +++
> >  drivers/staging/ks7010/wext/ks_hostif.h |  685 ++
> >  drivers/staging/ks7010/wext/ks_wlan.h   |  514 +
> >  drivers/staging/ks7010/wext/ks_wlan_ioctl.h |   67 +
> >  drivers/staging/ks7010/wext/ks_wlan_net.c   | 2999 
> > +++
> >  drivers/staging/ks7010/wext/michael_mic.c   |  148 ++
> >  drivers/staging/ks7010/wext/michael_mic.h   |   25 +
> >  26 files changed, 8514 insertions(+), 8507 deletions(-)
> >  delete mode 100644 drivers/staging/ks7010/TODO
> >  delete mode 100644 drivers/staging/ks7010/eap_packet.h
> >  delete mode 100644 drivers/staging/ks7010/ks7010_sdio.c
> >  delete mode 100644 drivers/staging/ks7010/ks7010_sdio.h
> >  delete mode 100644 drivers/staging/ks7010/ks_hostif.c
> >  delete mode 100644 drivers/staging/ks7010/ks_hostif.h
> >  delete mode 100644 drivers/staging/ks7010/ks_wlan.h
> >  delete mode 100644 drivers/staging/ks7010/ks_wlan_ioctl.h
> >  delete mode 100644 drivers/staging/ks7010/ks_wlan_net.c
> >  delete mode 100644 drivers/staging/ks7010/michael_mic.c
> >  delete mode 100644 drivers/staging/ks7010/michael_mic.h
> >  create mode 100644 drivers/staging/ks7010/wext/Kconfig
> >  create mode 100644 drivers/staging/ks7010/wext/Makefile
> >  create mode 100644 drivers/staging/ks7010/wext/TODO
> >  create mode 100644 drivers/staging/ks7010/wext/eap_packet.h
> >  create mode 100644 drivers/staging/ks7010/wext/ks7010_sdio.c
> >  create mode 100644 drivers/staging/ks7010/wext/ks7010_sdio.h
> >  create mode 100644 drivers/staging/ks7010/wext/ks_hostif.c
> >  create mode 100644 drivers/staging/ks7010/wext/ks_hostif.h
> >  create mode 100644 drivers/staging/ks7010/wext/ks_wlan.h
> >  create mode 100644 drivers/staging/ks7010/wext/ks_wlan_ioctl.h
> >  create mode 100644 drivers/staging/ks7010/wext/ks_wlan_net.c
> >  create mode 100644 drivers/staging/ks7010/wext/michael_mic.c
> >  create mode 100644 drivers/staging/ks7010/wext/michael_mic.h
> 
> Please use the -M option to 'git format-patch" so that we can just see
> file moves, not delete/add like you have here.

Point noted, will submit v2 using -M option. This is the second time
you have told me this, that means I have to buy you a beer doesn't it?

Will follow the 'wait a day before submitting next version' rule.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFC 2/3] staging: ks7010: add cfg80211 files

[Tobin C. Harding]

We are in the process of re-writing the current WEXT driver to use the
cfg80211 configuration API. Currently driver root directory is
empty. First step is to implement all the firmware interface in a
single layer of abstraction, Firmware Interface Layer (FIL). We can
add a skeleton implementation for most of the rest of the driver at
the same time.

Add cfg80211 driver skeleton. Implement FIL.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/Makefile|6 +
 drivers/staging/ks7010/README.rst  |   73 ++
 drivers/staging/ks7010/TODO.rst|   17 +
 drivers/staging/ks7010/cfg80211.c  |   45 ++
 drivers/staging/ks7010/cfg80211.h  |9 +
 drivers/staging/ks7010/common.h|   10 +
 drivers/staging/ks7010/eap.h   |   36 +
 drivers/staging/ks7010/fil.c   | 1294 
 drivers/staging/ks7010/fil.h   |  527 +++
 drivers/staging/ks7010/fil_types.h |  845 +++
 drivers/staging/ks7010/hif.c   |  104 +++
 drivers/staging/ks7010/hif.h   |   23 +
 drivers/staging/ks7010/ks7010.h|   94 +++
 drivers/staging/ks7010/main.c  |  122 
 drivers/staging/ks7010/sdio.c  |  399 +++
 drivers/staging/ks7010/sdio.h  |   86 +++
 drivers/staging/ks7010/tx.c|   29 +
 17 files changed, 3719 insertions(+)
 create mode 100644 drivers/staging/ks7010/README.rst
 create mode 100644 drivers/staging/ks7010/TODO.rst
 create mode 100644 drivers/staging/ks7010/cfg80211.c
 create mode 100644 drivers/staging/ks7010/cfg80211.h
 create mode 100644 drivers/staging/ks7010/common.h
 create mode 100644 drivers/staging/ks7010/eap.h
 create mode 100644 drivers/staging/ks7010/fil.c
 create mode 100644 drivers/staging/ks7010/fil.h
 create mode 100644 drivers/staging/ks7010/fil_types.h
 create mode 100644 drivers/staging/ks7010/hif.c
 create mode 100644 drivers/staging/ks7010/hif.h
 create mode 100644 drivers/staging/ks7010/ks7010.h
 create mode 100644 drivers/staging/ks7010/main.c
 create mode 100644 drivers/staging/ks7010/sdio.c
 create mode 100644 drivers/staging/ks7010/sdio.h
 create mode 100644 drivers/staging/ks7010/tx.c

diff --git a/drivers/staging/ks7010/Makefile b/drivers/staging/ks7010/Makefile
index f58cf9a..29c46db 100644
--- a/drivers/staging/ks7010/Makefile
+++ b/drivers/staging/ks7010/Makefile
@@ -1 +1,7 @@
 obj-$(CONFIG_KS7010) += ks7010.o
+ks7010-y+= main.o
+ks7010-y+= tx.o
+ks7010-y+= sdio.o
+ks7010-y+= cfg80211.o
+ks7010-y+= fil.o
+ks7010-y+= hif.o
diff --git a/drivers/staging/ks7010/README.rst 
b/drivers/staging/ks7010/README.rst
new file mode 100644
index 000..5ce54f9
--- /dev/null
+++ b/drivers/staging/ks7010/README.rst
@@ -0,0 +1,73 @@
+=
+Key Stream SDIO Device Driver
+=
+
+Current Status
+--
+
+Firmware Interface Layer only.
+Skeleton implementation in all other files.
+
+Description
+---
+
+Driver conversion from WEXT interface to cfg80211 API.
+
+The current KeyStream SDIO wireless driver (drivers/staging/ks7010)
+implements the WEXT interface.
+
+This driver is based on source code from the Ben Nanonote extra repository [1]
+which is based on the original v007 release from Renesas [2].
+
+[1] 
http://projects.qi-hardware.com/index.php/p/openwrt-packages/source/tree/master/ks7010/src
+[2] http://downloads.qi-hardware.com/software/ks7010_sdio_v007.tar.bz2
+
+Extensive refactoring has been done to the driver whilst in staging
+and the current mainline tip is untested.
+
+WEXT driver files :-
+ - ks7010_sdio.[ch]- SDIO code.
+ - ks_hostif.[ch]  - Device interface.
+ - ks_wlan_net.c   - WEXT interface.
+ - mic.[ch]- Custom Michael MIC implementation.
+ - eap_packet.h- EAP headers.
+ - ks_wlan_ioctl.h - WEXT IOCTL.
+
+cfg80211 driver files :-
+ - main.c  - Main driver file (net_device_ops etc).
+ - ks7010.h- Main driver header file.
+ - common.h- Constant definitions and forward declarations.
+ - eap.h   - EAPOL structure descriptions.
+ - sdio.[ch]   - SDIO code.
+ - fil.[ch]- Firmware Interface Layer.
+ - fil_types.h - Internal FIL types.
+ - hif.[ch]- Host Interface Layer.
+ - cfg80211.c  - cfg80211 API implementation.
+ - tx.c- Transmit path functions.
+
+cfg80211 driver files to do :-
+ - mic.[ch]- Interface to the kernel Michael MIC implementation.
+ - rx.c- Recive path functions.
+
+Other Information
+=
+
+Hardware
+
+https://wikidevi.com/wiki/Spectec_SDW-821_(KeyStream)
+https://wikidevi.com/wiki/Spectec_SDW-823
+
+Kernel Config
+-
+http://cateee.net/lkddb/web-lkddb/KS7010.html
+
+also enable
+ - MMC_DEBUG
+
+Testing
+---
+http://elinux.org/Tests:SDIO-KS7010
+
+Writing SDIO Linux Drivers
+--
+http

[RFC 3/3] MAINTAINERS: add maintainer entry for ks7010

[Tobin C. Harding]

Driver ks7010 does not currently have a maintainer.

Add maintainers entry for ks7010.

Signed-off-by: Tobin C. Harding 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e98464..75250ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7429,6 +7429,13 @@ F:   Documentation/auxdisplay/ks0108
 F: drivers/auxdisplay/ks0108.c
 F: include/linux/ks0108.h
 
+KS7010 KEYSTREAM DRIVER
+M:  Tobin Harding 
+L:  driverdev-devel@linuxdriverproject.org
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
+S:  Maintained
+F:  drivers/staging/ks7010/
+
 L3MDEV
 M: David Ahern 
 L: net...@vger.kernel.org
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFC 0/3] staging: ks7010: cfg80211 conversion, add FIL

[Tobin C. Harding]

Current ks7010 driver uses the WEXT interface. This series is the
initial stage of re-writing the driver to use cfg80211.

Patch set applies on Linus' mainline.

5ed02dbb497422bf225783f46e6eadd237d23d6b Linux 4.12-rc3

Throws 4 compiler warnings for functions defined and not used.

As a first step I have implemented a Firmware Interface Layer (FIL)
based on how the current driver interfaces with the firmware.

This is my first interaction with wireless networking and device
driver development. Please be as pedantic as you like, I am here to
learn.

I have attempted to document the driver functionality thoroughly,
including explanation of wireless networking features. My
understanding is far from complete, if I have made mistakes please do
point them out. My aim is for this driver to be approachable by
developers new to Wi-Fi drivers (as well as shamelessly educating
myself).

I am unsure of the etiquette when attempting such a conversion so
elected to keep the original code in a sub directory but exclude it
from the build process.

Also the code does not include a licence comment in each file. It does
include the MODULE_LICENSE("GPL") macro. I am unsure of the correct
handling of the licensing, in particular which exact form of license
comment to use and what to do about copyright. I do not wish to
violate, or upset, the previous developers in any way. I am not employed
by a company, all work is my own and is based on the current kernel driver.

Thank you for taking the time to read this and thank you in advance
for any time that you spend on this RFC. All feedback very much
appreciated.

Regards,
Tobin.

Tobin C. Harding (3):
  staging: ks7010: move WEXT files to sub directory
  staging: ks7010: Add cfg80211 files
  MAINTAINERS: add maintainer entry for ks7010

 MAINTAINERS |7 +
 drivers/staging/ks7010/Kconfig  |6 +-
 drivers/staging/ks7010/Makefile |9 +-
 drivers/staging/ks7010/README.rst   |   73 +
 drivers/staging/ks7010/TODO |   36 -
 drivers/staging/ks7010/TODO.rst |   17 +
 drivers/staging/ks7010/cfg80211.c   |   45 +
 drivers/staging/ks7010/cfg80211.h   |9 +
 drivers/staging/ks7010/common.h |   10 +
 drivers/staging/ks7010/eap.h|   36 +
 drivers/staging/ks7010/eap_packet.h |  144 --
 drivers/staging/ks7010/fil.c| 1294 
 drivers/staging/ks7010/fil.h|  527 +
 drivers/staging/ks7010/fil_types.h  |  845 
 drivers/staging/ks7010/hif.c|  104 +
 drivers/staging/ks7010/hif.h|   23 +
 drivers/staging/ks7010/ks7010.h |   94 +
 drivers/staging/ks7010/ks7010_sdio.c| 1079 --
 drivers/staging/ks7010/ks7010_sdio.h|  164 --
 drivers/staging/ks7010/ks_hostif.c  | 2638 ---
 drivers/staging/ks7010/ks_hostif.h  |  685 --
 drivers/staging/ks7010/ks_wlan.h|  514 -
 drivers/staging/ks7010/ks_wlan_ioctl.h  |   67 -
 drivers/staging/ks7010/ks_wlan_net.c| 2999 ---
 drivers/staging/ks7010/main.c   |  122 ++
 drivers/staging/ks7010/michael_mic.c|  148 --
 drivers/staging/ks7010/michael_mic.h|   25 -
 drivers/staging/ks7010/sdio.c   |  399 
 drivers/staging/ks7010/sdio.h   |   86 +
 drivers/staging/ks7010/tx.c |   29 +
 drivers/staging/ks7010/wext/Kconfig |   10 +
 drivers/staging/ks7010/wext/Makefile|4 +
 drivers/staging/ks7010/wext/TODO|   36 +
 drivers/staging/ks7010/wext/eap_packet.h|  144 ++
 drivers/staging/ks7010/wext/ks7010_sdio.c   | 1079 ++
 drivers/staging/ks7010/wext/ks7010_sdio.h   |  164 ++
 drivers/staging/ks7010/wext/ks_hostif.c | 2638 +++
 drivers/staging/ks7010/wext/ks_hostif.h |  685 ++
 drivers/staging/ks7010/wext/ks_wlan.h   |  514 +
 drivers/staging/ks7010/wext/ks_wlan_ioctl.h |   67 +
 drivers/staging/ks7010/wext/ks_wlan_net.c   | 2999 +++
 drivers/staging/ks7010/wext/michael_mic.c   |  148 ++
 drivers/staging/ks7010/wext/michael_mic.h   |   25 +
 43 files changed, 12240 insertions(+), 8507 deletions(-)
 create mode 100644 drivers/staging/ks7010/README.rst
 delete mode 100644 drivers/staging/ks7010/TODO
 create mode 100644 drivers/staging/ks7010/TODO.rst
 create mode 100644 drivers/staging/ks7010/cfg80211.c
 create mode 100644 drivers/staging/ks7010/cfg80211.h
 create mode 100644 drivers/staging/ks7010/common.h
 create mode 100644 drivers/staging/ks7010/eap.h
 delete mode 100644 drivers/staging/ks7010/eap_packet.h
 create mode 100644 drivers/staging/ks7010/fil.c
 create mode 100644 drivers/staging/ks7010/fil.h
 create mode 100644 drivers/staging/ks7010/fil_types.h
 create mode 100644 drivers/staging/ks7010/hif.c
 create mode 100644 

Re: [PATCH V2 00/27] Drivers: ccree - align block comments

[Tobin C. Harding]

On Tue, May 30, 2017 at 08:49:41AM +0200, Antoine Tenart wrote:
> Hello Derek,
> 
> On Tue, May 30, 2017 at 06:09:37PM +1200, Derek Robson wrote:
> > Fixed block comments across whole ccree driver
> 
> Since all these commits are doing the same logical change across a
> single driver, you could probably squash them all into a single one.

I suggested that when v1 was submitted ;)

> 
> Thanks!
> Antoine

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] fix multiple blank lines coding style problem

[Tobin C. Harding]

On Thu, May 25, 2017 at 12:22:15PM +0300, Aliza Minkov wrote:
> Signed-off-by: Aliza Minkov 
> ---
>  drivers/staging/dgnc/dgnc_driver.c | 2 --
>  1 file changed, 2 deletions(-)

Hi Aliza,

Well done getting your patch together. I think you might get an
automated message from Greg's patch robot so at the risk of repeating
the message here goes.

You will need to add a commit log in order for your patch to have a
chance of being picked up. Your commit summary is good, you may like
to read Documentation/process/submitting-patches.rst (Section 2
Describe your changes). Apart from that the patch looks good to me.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

drivers/staging/ks7010 hardware test

[Tobin C. Harding]

Hi Wolfram,

I began testing the card you sent today. I'm getting a firmware load
error like you mentioned.

I checked out code from when you originally merged into staging,
however I built the module in a 4.9 kernel.

To help me track down the issue could you please tell me what testing
you managed to do successfully in the past, and if you can,
approximately the date you were doing the tests or the git commit of
the code you tested.

Thanks. Oh, also could you please indicate your level of interest my
continued efforts. I don't want to send you copious emails if you
would rather me not.

For anyone on the lists reading this, the hardware is

Spectec SDW-823 microSD (SDIO) Wi-Fi card

The error code is -EILSEQ (Illegal byte sequence). It is returned by
sdio_memcpy_fromio(). I do not know what it signifies?

thanks,
Tobin.


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: bcm2835-audio: bcm2835-ctl.c: Fixed a comment coding style issue.

[Tobin C. Harding]

On Wed, May 24, 2017 at 08:03:14PM +0530, srishti sharma wrote:

This driver is not in Greg KH's staging tree. You may like to work off
of that tree when doing staging patches.

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/

To aid you future patches here are a couple of minor comments for you.

> fixed a trailing */ issue

You may like to be more explicit here using the format described in
Documentation/process/submitting-patches.rst (Section 2 Describe your
changes). This goes for your subject line as well.

> Signed-off-by: srishti sharma 
> ---
>  drivers/staging/bcm2835-audio/bcm2835-ctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/bcm2835-audio/bcm2835-ctl.c 
> b/drivers/staging/bcm2835-audio/bcm2835-ctl.c
> index a4ffa1b..38fabab 100644
> --- a/drivers/staging/bcm2835-audio/bcm2835-ctl.c
> +++ b/drivers/staging/bcm2835-audio/bcm2835-ctl.c
> @@ -247,8 +247,8 @@ static int snd_bcm2835_spdif_mask_get(struct snd_kcontrol 
> *kcontrol,
>   struct snd_ctl_elem_value *ucontrol)
>  {
>   /* bcm2835 supports only consumer mode and sets all other format flags
> -  * automatically. So the only thing left is signalling non-audio
> -  * content */
> +  * automatically. So the only thing left is signalling non-audio content
> +  */

And for completeness this is the block comment style in networking
code, the rest of the kernel uses the format

/*
 * block comment blah blah
 * on multiple lines
 */

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: fix macros with multiple statements in rtl8723bs/core/rtw_security.c

[Tobin C. Harding]

On Wed, May 24, 2017 at 10:38:57PM +0800, Jamie Huang wrote:

Comment on your patch subject line. Patches to staging typically
include the driver in the subject. You can view previous commits to
get an idea using

$ git log --pretty=oneline drivers/staging/rtl8723bs/core/rtw_security.c

And from Documentation/process/submitting-patches.rst

   The ``subsystem`` in the email's Subject should identify which
   area or subsystem of the kernel is being patched.
   
   The ``summary phrase`` in the email's Subject should concisely
   describe the patch which that email contains.  The ``summary
   phrase`` should not be a filename.  Do not use the same ``summary
   phrase`` for every patch in a whole patch series (where a ``patch
   series`` is an ordered sequence of multiple, related patches).

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging: sm750fb: removed hungarian prfx and replace CamelCase variables

[Tobin C. Harding]

On Wed, May 24, 2017 at 01:24:14PM +0530, Richa Jha wrote:
[snip]

Nice work so far. I'm having trouble with your patches. Here are a few
nitpicks to help you on your way.

There are 4 emails in the last few days with similar subjects but they
are not linked together. Also your email subject does not start with '[PATCH]',
I doubt they will be picked up without it. You should read
Documentation/process/submitting-patches.rst.

There are some other issues with your subject line and commit-log, if
you would like further comments please ask.

So for initial patch the subject is something like

[PATCH] staging: foo: Fix identifiers, hungarian notation and camel case

If you submit version 2, it will then be

[PATCH v2] staging: foo: Fix identifiers, hungarian notation and camel case

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/27] Drivers: ccree: ssi_sysfs.h - align block comments

[Tobin C. Harding]

On Wed, May 24, 2017 at 04:40:32PM +1200, Derek Robson wrote:
> Fixed block comment alignment, Style fix only
> Found using checkpatch

It's 'one thing per patch', this whole set does one thing. You may
like to submit it as a single patch. You won't need the file name in
the commit summary then also :)

Good luck,
Tobin.

> 
> Signed-off-by: Derek Robson 
> ---
>  drivers/staging/ccree/ssi_sysfs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/ccree/ssi_sysfs.h 
> b/drivers/staging/ccree/ssi_sysfs.h
> index cd456c5dccc4..4893e014adf7 100644
> --- a/drivers/staging/ccree/ssi_sysfs.h
> +++ b/drivers/staging/ccree/ssi_sysfs.h
> @@ -15,7 +15,7 @@
>   */
>  
>  /* \file ssi_sysfs.h
> -   ARM CryptoCell sysfs APIs
> + * ARM CryptoCell sysfs APIs
>   */
>  
>  #ifndef __SSI_SYSFS_H__
> -- 
> 2.12.2
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: staging: sm750fb: removed hungarian prfx and replace CamelCase variables

[Tobin C. Harding]

On Tue, May 23, 2017 at 06:37:39PM +0530, Richa Jha wrote:
> Replace CamelCase variable names with underscores and remove
>  hungarian prefixes to comply with the standard kernel coding style

Hi Richa,

Nice work. You may like to do all the camel case at the same
time. You could limit yourself to just this file and structures it
touches if it is easier. That way when reviewers are reviewing
your patch they will see nice clean lines added (instead of lines with
camel case in them still).

> 
> Signed-off-by: Richa Jha 
> ---
>  drivers/staging/sm750fb/ddk750_chip.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
> b/drivers/staging/sm750fb/ddk750_chip.c
> index 8e51f60..56b8d66 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -55,7 +55,7 @@ static unsigned int get_mxclk_freq(void)
>  static void set_chip_clock(unsigned int frequency)
>  {
>   struct pll_value pll;
> - unsigned int ulActualMxClk;
> + unsigned int actual_max_clk;
>  
>   /* Cheok_0509: For SM750LE, the chip clock is fixed. Nothing to set. */
>   if (sm750_get_chip_type() == SM750LE)
> @@ -75,7 +75,7 @@ static void set_chip_clock(unsigned int frequency)
>* Return value of sm750_calc_pll_value gives the actual
>* possible clock.
>*/
> - ulActualMxClk = sm750_calc_pll_value(frequency, &pll);
> + actual_max_clk = sm750_calc_pll_value(frequency, &pll);
>  
>   /* Master Clock Control: MXCLK_PLL */
>   poke32(MXCLK_PLL_CTRL, sm750_format_pll_reg(&pll));
> @@ -210,13 +210,13 @@ unsigned int ddk750_get_vm_size(void)
>   return data;
>  }
>  
> -int ddk750_init_hw(struct initchip_param *pInitParam)
> +int ddk750_init_hw(struct initchip_param *init_param)
>  {
>   unsigned int reg;
>  
> - if (pInitParam->powerMode != 0)
> - pInitParam->powerMode = 0;
> - sm750_set_power_mode(pInitParam->powerMode);
> + if (init_param->powerMode != 0)
> + init_param->powerMode = 0;
> + sm750_set_power_mode(init_param->powerMode);
>  
>   /* Enable display power gate & LOCALMEM power gate*/
>   reg = peek32(CURRENT_GATE);
> @@ -237,13 +237,13 @@ int ddk750_init_hw(struct initchip_param *pInitParam)
>   }
>  
>   /* Set the Main Chip Clock */
> - set_chip_clock(MHz((unsigned int)pInitParam->chipClock));
> + set_chip_clock(MHz((unsigned int)init_param->chipClock));
>  
>   /* Set up memory clock. */
> - set_memory_clock(MHz(pInitParam->memClock));
> + set_memory_clock(MHz(init_param->memClock));
>  
>   /* Set up master clock */
> - set_master_clock(MHz(pInitParam->masterClock));
> + set_master_clock(MHz(init_param->masterClock));
>  
>   /*
>* Reset the memory controller.
> @@ -251,7 +251,7 @@ int ddk750_init_hw(struct initchip_param *pInitParam)
>* the system might hang when sw accesses the memory.
>* The memory should be resetted after changing the MXCLK.
>*/
> - if (pInitParam->resetMemory == 1) {
> + if (init_param->resetMemory == 1) {
>   reg = peek32(MISC_CTRL);
>   reg &= ~MISC_CTRL_LOCALMEM_RESET;
>   poke32(MISC_CTRL, reg);
> @@ -260,7 +260,7 @@ int ddk750_init_hw(struct initchip_param *pInitParam)
>   poke32(MISC_CTRL, reg);
>   }
>  
> - if (pInitParam->setAllEngOff == 1) {
> + if (init_param->setAllEngOff == 1) {

This would be nicer like this;

-   if (pInitParam->setAllEngOff == 1) {
+   if (init_param->set_all_eng_off == 1) {

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/3] staging: ks7010: hostif, u16 data types to __le16

[Tobin C. Harding]

Target device is little endian. Host interface data structures used
for building frames to pass to target device should use little endian
data types. All u16 structure members in ks_hostif.h need to be
changed to __le16, Sparse can then be used to make sure we update all
code that touches these data.

Change all u16 data types in host interface structures to be
__le16. Update all code that touches modified data types. Check using
Sparse.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c |  14 ++--
 drivers/staging/ks7010/ks_hostif.h | 130 ++---
 2 files changed, 72 insertions(+), 72 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index caf2551..cf9b22f 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -147,7 +147,7 @@ int get_current_ap(struct ks_wlan_private *priv, struct 
link_ap_info_t *ap_info)
/* noise */
ap->noise = ap_info->noise;
/* capability */
-   ap->capability = ap_info->capability;
+   ap->capability = le16_to_cpu(ap_info->capability);
/* rsn */
if ((ap_info->rsn_mode & RSN_MODE_WPA2) &&
(priv->wpa.version == IW_AUTH_WPA_VERSION_WPA2)) {
@@ -233,12 +233,12 @@ int get_ap_information(struct ks_wlan_private *priv, 
struct ap_info_t *ap_info,
/* noise */
ap->noise = ap_info->noise;
/* capability */
-   ap->capability = ap_info->capability;
+   ap->capability = le16_to_cpu(ap_info->capability);
/* channel */
ap->channel = ap_info->ch_info;
 
bp = ap_info->body;
-   bsize = ap_info->body_size;
+   bsize = le16_to_cpu(ap_info->body_size);
offset = 0;
 
while (bsize > offset) {
@@ -948,18 +948,18 @@ void hostif_associate_indication(struct ks_wlan_private 
*priv)
wrqu.data.length += sizeof(associnfo_leader0) - 1;
pbuf += sizeof(associnfo_leader0) - 1;
 
-   for (i = 0; i < assoc_req->req_ies_size; i++)
+   for (i = 0; i < le16_to_cpu(assoc_req->req_ies_size); i++)
pbuf += sprintf(pbuf, "%02x", *(pb + i));
-   wrqu.data.length += (assoc_req->req_ies_size) * 2;
+   wrqu.data.length += (le16_to_cpu(assoc_req->req_ies_size)) * 2;
 
memcpy(pbuf, associnfo_leader1, sizeof(associnfo_leader1) - 1);
wrqu.data.length += sizeof(associnfo_leader1) - 1;
pbuf += sizeof(associnfo_leader1) - 1;
 
pb += assoc_req->req_ies_size;
-   for (i = 0; i < assoc_resp->resp_ies_size; i++)
+   for (i = 0; i < le16_to_cpu(assoc_resp->resp_ies_size); i++)
pbuf += sprintf(pbuf, "%02x", *(pb + i));
-   wrqu.data.length += (assoc_resp->resp_ies_size) * 2;
+   wrqu.data.length += (le16_to_cpu(assoc_resp->resp_ies_size)) * 2;
 
pbuf += sprintf(pbuf, ")");
wrqu.data.length += 1;
diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/ks_hostif.h
index 538600f..35d51fe 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -68,21 +68,21 @@ struct hostif_hdr {
 
 struct hostif_data_request_t {
struct hostif_hdr header;
-   u16 auth_type;
+   __le16 auth_type;
 #define TYPE_DATA 0x
 #define TYPE_AUTH 0x0001
-   u16 reserved;
+   __le16 reserved;
u8 data[0];
 } __packed;
 
 struct hostif_data_indication_t {
struct hostif_hdr header;
-   u16 auth_type;
+   __le16 auth_type;
 /* #define TYPE_DATA 0x */
 #define TYPE_PMK1 0x0001
 #define TYPE_GMK1 0x0002
 #define TYPE_GMK2 0x0003
-   u16 reserved;
+   __le16 reserved;
u8 data[0];
 } __packed;
 
@@ -147,8 +147,8 @@ struct hostif_mib_get_request_t {
 } __packed;
 
 struct hostif_mib_value_t {
-   u16 size;
-   u16 type;
+   __le16 size;
+   __le16 type;
 #define MIB_VALUE_TYPE_NULL 0
 #define MIB_VALUE_TYPE_INT  1
 #define MIB_VALUE_TYPE_BOOL 2
@@ -207,12 +207,12 @@ enum power_mgmt_mode_type {
 
 struct hostif_power_mgmt_confirm_t {
struct hostif_hdr header;
-   u16 result_code;
+   __le16 result_code;
 } __packed;
 
 struct hostif_start_request_t {
struct hostif_hdr header;
-   u16 mode;
+   __le16 mode;
 #define MODE_PSEUDO_ADHOC   0
 #define MODE_INFRASTRUCTURE 1
 #define MODE_AP 2  /* not used */
@@ -221,7 +221,7 @@ struct hostif_start_request_t {
 
 struct hostif_start_confirm_t {
struct hostif_hdr header;
-   u16 result_code;
+   __le16 result_code;
 } __packed;
 
 #define SSID_MAX_SIZE 32
@@ -239,7 +239,7 @@ struct rate_set8_t {
 } __packed;
 
 struct fh_parms_t {
-   u16 dwell_time;
+   __le16 dwell_time;
u8 hop_set;
u8 hop_pattern;
u8 hop_index;
@@ -252,12 +252,12 @@ struct ds_parms_t

[PATCH 3/3] staging: ks7010: hostif, u32 data types to __le32

[Tobin C. Harding]

Target device is little endian. Host interface data structures used
for building frames to pass to target device should use little endian
data types. All u32 structure members in ks_hostif.h need to be
changed to __le32.

Change all u16 data types in host interface structures to be
__le32.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.h | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/ks_hostif.h
index 35d51fe..45e3a01 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -143,7 +143,7 @@ struct channel_list_t {
 
 struct hostif_mib_get_request_t {
struct hostif_hdr header;
-   u32 mib_attribute;
+   __le32 mib_attribute;
 } __packed;
 
 struct hostif_mib_value_t {
@@ -159,36 +159,36 @@ struct hostif_mib_value_t {
 
 struct hostif_mib_get_confirm_t {
struct hostif_hdr header;
-   u32 mib_status;
+   __le32 mib_status;
 #define MIB_SUCCESS0
 #define MIB_INVALID1
 #define MIB_READ_ONLY  2
 #define MIB_WRITE_ONLY 3
-   u32 mib_attribute;
+   __le32 mib_attribute;
struct hostif_mib_value_t mib_value;
 } __packed;
 
 struct hostif_mib_set_request_t {
struct hostif_hdr header;
-   u32 mib_attribute;
+   __le32 mib_attribute;
struct hostif_mib_value_t mib_value;
 } __packed;
 
 struct hostif_mib_set_confirm_t {
struct hostif_hdr header;
-   u32 mib_status;
-   u32 mib_attribute;
+   __le32 mib_status;
+   __le32 mib_attribute;
 } __packed;
 
 struct hostif_power_mgmt_request_t {
struct hostif_hdr header;
-   u32 mode;
+   __le32 mode;
 #define POWER_ACTIVE  1
 #define POWER_SAVE2
-   u32 wake_up;
+   __le32 wake_up;
 #define SLEEP_FALSE 0
 #define SLEEP_TRUE  1  /* not used */
-   u32 receiveDTIMs;
+   __le32 receiveDTIMs;
 #define DTIM_FALSE 0
 #define DTIM_TRUE  1
 } __packed;
@@ -509,8 +509,8 @@ struct hostif_bss_scan_request_t {
 #define ACTIVE_SCAN  0
 #define PASSIVE_SCAN 1
u8 pad[3];
-   u32 ch_time_min;
-   u32 ch_time_max;
+   __le32 ch_time_min;
+   __le32 ch_time_max;
struct channel_list_t channel_list;
struct ssid_t ssid;
 } __packed;
@@ -535,10 +535,10 @@ struct hostif_phy_information_confirm_t {
u8 sq;
u8 noise;
u8 link_speed;
-   u32 tx_frame;
-   u32 rx_frame;
-   u32 tx_error;
-   u32 rx_error;
+   __le32 tx_frame;
+   __le32 rx_frame;
+   __le32 tx_error;
+   __le32 rx_error;
 } __packed;
 
 enum sleep_mode_type {
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: ks7010: eap, change unsigned short to __be16

[Tobin C. Harding]

Sparse emits warning: cast to restricted __be16. EAP header uses
network byte order. The structures used to describe it should use
__beXX data types.

Change data type unsigned short -> __be16.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/eap_packet.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/eap_packet.h 
b/drivers/staging/ks7010/eap_packet.h
index b2d25ef..ae03f74 100644
--- a/drivers/staging/ks7010/eap_packet.h
+++ b/drivers/staging/ks7010/eap_packet.h
@@ -18,7 +18,7 @@ struct ether_hdr {
unsigned char h_source_snap;
unsigned char h_command;
unsigned char h_vendor_id[3];
-   unsigned short h_proto; /* packet type ID field */
+   __be16 h_proto; /* packet type ID field */
 #define ETHER_PROTOCOL_TYPE_EAP0x888e
 #define ETHER_PROTOCOL_TYPE_IP 0x0800
 #define ETHER_PROTOCOL_TYPE_ARP0x0806
@@ -91,7 +91,7 @@ struct ieee802_1x_eapol_key {
 
 struct wpa_eapol_key {
unsigned char type;
-   unsigned short key_info;
+   __be16 key_info;
unsigned short key_length;
unsigned char replay_counter[WPA_REPLAY_COUNTER_LEN];
unsigned char key_nonce[WPA_NONCE_LEN];
-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 0/3] staging: ks7010: fix endian issues

[Tobin C. Harding]

This patch set fixes various endian issues. Sparse was used to discover these
issues and also to check the fixes. 

Patch 01 fixes the EAP header structure descriptions.

Patches 2 and 3 fix descriptions for SDIO structures used as headers
when building frames to pass to the SDIO device.00

Code is untested. Builds on x86_64 and PowerPC.

Tobin C. Harding (3):
  staging: ks7010: eap, change unsigned short to __be16
  staging: ks7010: hostif, u16 data types to __le16
  staging: ks7010: hostif, u32 data types to __le32

 drivers/staging/ks7010/eap_packet.h |   4 +-
 drivers/staging/ks7010/ks_hostif.c  |  14 ++--
 drivers/staging/ks7010/ks_hostif.h  | 160 ++--
 3 files changed, 89 insertions(+), 89 deletions(-)

-- 
2.7.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: wilc1000: Last line is empty

[Tobin C. Harding]

On Tue, May 02, 2017 at 04:18:39PM +0200, Vincent Siles wrote:
> Removing empty line at the end of the file

Hi Vincent,

Small comment on your patch series. You may have more success if your
summary phrase is in imperative mood i.e sounds like you are
commanding changes to be made to the code base

Recent examples from LKML

fpga manager: Add Altera CvP driver
sched/fair: fix load balancer behavior when cgroup is in use

Aslo the changelog should describe the reason for the patch and then
describe the changes made in the patch to rectify the previously
described problem. See Documentation/process/submitting-patches.rst
for complete description of how to write a changelog.

Good luck,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/ks7010: Fix type assignment for struct hostif_hdr

[Tobin C. Harding]

On Mon, May 01, 2017 at 04:11:30PM +0200, gapalinux wrote:
> On Mon, 1 May 2017 19:59:44 +1000
> "Tobin C. Harding"  wrote:
> 
> > On Sat, Apr 29, 2017 at 07:54:30PM +0200, Cezary Gapinski wrote:
> > > Sparse spits out a warnings about __le16 and unsigned short
> > > assignment. Change the type of size and event members of struct
> > > hostif_hdr to __le16 and correct conversion to the proper cpu type.
> > > 
> > > Signed-off-by: Cezary Gapinski   
> > 
> > After discussion on kernelnewbies mailing list and thinking about this
> > for a couple of days I am now of the opinion that this patch is
> > merge-able. I am probably not fully qualified to say so but no one
> > else appears to be super active on this driver. If I am wrong, I'll be
> > here to fix it up :)
> > 
> >  Reviewed-by: Tobin C. Harding 
> > 
> > Good work Cezsary, apologies for the wish-washy replies. I'm learning
> > also ;)
> > 
> > Good luck,
> > Tobin.
> 
> Hi Tobin,
> 
> nice to know that you also thinking about this fixes. I think that is
> just a first step because I noticed much more warning with
> big/little-endian issues and perhaps no one else want to touch this.

I did up a patch set yesterday that fixes most of the Sparse
warnings. It was simply a matter of changing all struct members in
ks_hostif.h to be __leXX and adding a few cpu_to_leXX()/leXX_to_cpu()
calls. Exactly as you did. I'm going to wait for your patch, and
another that is in flight on ks7010, to be merged before I submit
it. There will still be a few Sparse warnings that are a little more
tricky, so there is still some fun to be had.

> I'm also here to help if it is something will be incorrect ;)."

Cool

> Sorry for my previous messages. Sometimes I forget about crtl+s will
> sending my e-mails, so if you received other messages just ignore it.

Didn't receive any extra messages :)

Cheers,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/ks7010: Fix type assignment for struct hostif_hdr

[Tobin C. Harding]

On Sat, Apr 29, 2017 at 07:54:30PM +0200, Cezary Gapinski wrote:
> Sparse spits out a warnings about __le16 and unsigned short assignment.
> Change the type of size and event members of struct hostif_hdr
> to __le16 and correct conversion to the proper cpu type.
> 
> Signed-off-by: Cezary Gapinski 

After discussion on kernelnewbies mailing list and thinking about this
for a couple of days I am now of the opinion that this patch is
merge-able. I am probably not fully qualified to say so but no one
else appears to be super active on this driver. If I am wrong, I'll be
here to fix it up :)

 Reviewed-by: Tobin C. Harding 

Good work Cezsary, apologies for the wish-washy replies. I'm learning
also ;)

Good luck,
Tobin.

> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 10 ++
>  drivers/staging/ks7010/ks_hostif.h   |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index ec11799..e3a134d 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -269,7 +269,8 @@ static int write_to_device(struct ks_wlan_private *priv, 
> unsigned char *buffer,
>   hdr = (struct hostif_hdr *)buffer;
>  
>   DPRINTK(4, "size=%d\n", hdr->size);
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + le16_to_cpu(hdr->event) > HIF_REQ_MAX) {
>   DPRINTK(1, "unknown event=%04X\n", hdr->event);
>   return 0;
>   }
> @@ -327,13 +328,14 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void 
> *p, unsigned long size,
>  
>   hdr = (struct hostif_hdr *)p;
>  
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + le16_to_cpu(hdr->event) > HIF_REQ_MAX) {
>   DPRINTK(1, "unknown event=%04X\n", hdr->event);
>   return 0;
>   }
>  
>   /* add event to hostt buffer */
> - priv->hostt.buff[priv->hostt.qtail] = hdr->event;
> + priv->hostt.buff[priv->hostt.qtail] = le16_to_cpu(hdr->event);
>   priv->hostt.qtail = (priv->hostt.qtail + 1) % SME_EVENT_BUFF_SIZE;
>  
>   DPRINTK(4, "event=%04X\n", hdr->event);
> @@ -403,7 +405,7 @@ static void ks_wlan_hw_rx(struct ks_wlan_private *priv, 
> uint16_t size)
>  
>   hdr = (struct hostif_hdr *)&rx_buffer->data[0];
>   rx_buffer->size = le16_to_cpu(hdr->size) + sizeof(hdr->size);
> - event = hdr->event;
> + event = le16_to_cpu(hdr->event);
>   inc_rxqtail(priv);
>  
>   ret = ks7010_sdio_writeb(priv, READ_STATUS, REG_STATUS_IDLE);
> diff --git a/drivers/staging/ks7010/ks_hostif.h 
> b/drivers/staging/ks7010/ks_hostif.h
> index d773432..7e4d1aa 100644
> --- a/drivers/staging/ks7010/ks_hostif.h
> +++ b/drivers/staging/ks7010/ks_hostif.h
> @@ -62,8 +62,8 @@
>   */
>  
>  struct hostif_hdr {
> - u16 size;
> - u16 event;
> + __le16 size;
> + __le16 event;
>  } __packed;
>  
>  struct hostif_data_request_t {
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] staging: ks7010: fix block comment style

[Tobin C. Harding]

On Fri, Apr 28, 2017 at 03:01:32PM +0200, Ilia Sergachev wrote:
> On Fri, 28 Apr 2017 14:20:21 +0200
> Greg Kroah-Hartman  wrote:
> 
> > 
> > Nope, the whole thing still doesn't apply:
> > 
> > checking file drivers/staging/ks7010/ks_wlan_net.c
> > Hunk #1 FAILED at 230.
> > Hunk #2 FAILED at 343.
> > Hunk #3 FAILED at 1137.
> > Hunk #4 FAILED at 1189.
> > Hunk #5 FAILED at 1225.
> > Hunk #6 FAILED at 1497.
> > Hunk #7 FAILED at 1569.
> > Hunk #8 FAILED at 1596.
> > Hunk #9 FAILED at 1970.
> > Hunk #10 FAILED at 2105.
> > Hunk #11 FAILED at 3351.
> > 11 out of 11 hunks FAILED
> > 
> > What tree/branch are you making this patch against?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Ok, I was using torvalds/linux instead of next/linux-next.
> In fact, the style fixes on this file were already done in 'next'.
> So, thank you all for your patience, and sorry about lost time,
> these were the necessary lessons for me - I'll find now something else
> to fix :) 

If you are attempting fixes to staging drivers you may like to base
your patches off Greg's staging tree

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/

The branch you want is either staging-next or staging-testing.

Hope this helps,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ks7010: remove line continuations in quoted strings

[Tobin C. Harding]

On Fri, Apr 28, 2017 at 05:26:17PM +0300, Dan Carpenter wrote:
> On Fri, Apr 28, 2017 at 04:19:28PM +0200, Ilia Sergachev wrote:
> > Checkpatch emits WARNING: Avoid line continuations in quoted strings.
> > 
> > Remove line continuations - split strings using quotes.
> > 
> > Signed-off-by: Ilia Sergachev 
> > ---
> >  drivers/staging/ks7010/ks_hostif.c | 20 ++--
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/ks7010/ks_hostif.c 
> > b/drivers/staging/ks7010/ks_hostif.c
> > index 7151f16e2f9c..a0c632a52f48 100644
> > --- a/drivers/staging/ks7010/ks_hostif.c
> > +++ b/drivers/staging/ks7010/ks_hostif.c
> > @@ -191,9 +191,15 @@ int get_current_ap(struct ks_wlan_private *priv, 
> > struct link_ap_info_t *ap_info)
> > wireless_send_event(netdev, SIOCGIWAP, &wrqu, NULL);
> > }
> > DPRINTK(4, "\nLink AP\n");
> > -   DPRINTK(4, "bssid=%02X:%02X:%02X:%02X:%02X:%02X\n \
> > -   essid=%s\nrate_set=%02X,%02X,%02X,%02X,%02X,%02X,%02X,%02X\n
> > channel=%d\n \
> > -   rssi=%d\nsq=%d\ncapability=%04X\n", ap->bssid[0], ap->bssid[1], 
> > ap->bssid[2], ap->bssid[3], ap->bssid[4], ap->bssid[5], 
> > &(ap->ssid.body[0]), ap->rate_set.body[0], ap->rate_set.body[1], 
> > ap->rate_set.body[2], ap->rate_set.body[3], ap->rate_set.body[4], 
> > ap->rate_set.body[5], ap->rate_set.body[6], ap->rate_set.body[7], 
> > ap->channel, ap->rssi, ap->sq, ap->capability);
> > +   DPRINTK(4, "bssid=%02X:%02X:%02X:%02X:%02X:%02X\n"
> > +   "essid=%s\nrate_set=%02X,%02X,%02X,%02X,%02X,%02X,%02X,%02X\n"
> > +   "channel=%d\nrssi=%d\nsq=%d\ncapability=%04X\n",
> > +   ap->bssid[0], ap->bssid[1], ap->bssid[2],
> > +   ap->bssid[3], ap->bssid[4], ap->bssid[5],
> > +   &(ap->ssid.body[0]), ap->rate_set.body[0], ap->rate_set.body[1],
> > +   ap->rate_set.body[2], ap->rate_set.body[3], ap->rate_set.body[4],
> > +   ap->rate_set.body[5], ap->rate_set.body[6], ap->rate_set.body[7],
> > +   ap->channel, ap->rssi, ap->sq, ap->capability);
> 
> Could you indent it like this:
> 
>   DPRINTK(4, "bssid=%02X:%02X:%02X:%02X:%02X:%02X\n"
>  "essid=%s\n
> rate_set=%02X,%02X,%02X,%02X,%02X,%02X,%02X,%02X\n"
>  "channel=%d\nrssi=%d\nsq=%d\n
> capability=%04X\n",
>   ap->bssid[0], ap->bssid[1], ap->bssid[2],
>   ap->bssid[3], ap->bssid[4], ap->bssid[5],
>   &(ap->ssid.body[0]), ap->rate_set.body[0], ap->rate_set.body[1],
>   ap->rate_set.body[2], ap->rate_set.body[3], 
> ap->rate_set.body[4],
>   ap->rate_set.body[5], ap->rate_set.body[6], 
> ap->rate_set.body[7],
>   ap->channel, ap->rssi, ap->sq, ap->capability);
> 
> And actually it might be more readable to break it up more:
> 
>   DPRINTK(4, "bssid=%02X:%02X:%02X:%02X:%02X:%02X\n"
>  "essid=%s\n"
>  "rate_set=%02X,%02X,%02X,%02X,%02X,%02X,%02X,%02X\n"
>  "channel=%d\n"
>  "rssi=%d\n"
>  "sq=%d\n"
>  "capability=%04X\n",
>   ap->bssid[0], ap->bssid[1], ap->bssid[2],
>   ap->bssid[3], ap->bssid[4], ap->bssid[5],
>   &(ap->ssid.body[0]), ap->rate_set.body[0], ap->rate_set.body[1],
>   ap->rate_set.body[2], ap->rate_set.body[3], 
> ap->rate_set.body[4],
>   ap->rate_set.body[5], ap->rate_set.body[6], 
> ap->rate_set.body[7],
>   ap->channel, ap->rssi, ap->sq, ap->capability);
> 
> Except that's not really how newlines are supposed to work in dmesg...
> Whatever.

This driver is full of these DPRINTK's, I would like to see them gone
once some more testing is done. I don't see any nice way to keep them
without upsetting checkpatch. It may not be worth bothering with them
for now.

thanks,
Tobin.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/8] Avoid CamelCases in ks7010 driver

[Tobin C. Harding]

On Sat, Apr 29, 2017 at 10:58:40PM +0200, Janusz Lisiecki wrote:
> This patchset fix CamelCases in ks7010 driver except michael_mic
> which will be replaced by kernel implementation in the future.

Series looks good to me. For what its worth;
 
 Reviewed-by: Tobin C. Harding 

> Janusz Lisiecki (8):
>   staging: ks7010: avoid CamelCase in fields of struct local_gain_t
>   staging: ks7010: avoid CamelCase: receiveDTIMs
>   staging: ks7010: avoid CamelCase: FhParms_t fields
>   staging: ks7010: avoid CamelCase: link_ap_info_t fields
>   staging: ks7010: avoid CamelCase: CfParms_t fields
>   staging: ks7010: avoid CamelCase: atimWindow
>   staging: ks7010: avoid CamelCase: reqIEs_size and respIEs_size
>   staging: ks7010: avoid CamelCase: local variables in ks_hostif.c
> 
>  drivers/staging/ks7010/ks_hostif.c   | 65 
> ++--
>  drivers/staging/ks7010/ks_hostif.h   | 42 +++
>  drivers/staging/ks7010/ks_wlan.h |  8 ++---
>  drivers/staging/ks7010/ks_wlan_net.c | 20 +--
>  4 files changed, 68 insertions(+), 67 deletions(-)
> 
> -- 
> 1.9.1
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 4/8] staging: ks7010: avoid CamelCase: link_ap_info_t fields

[Tobin C. Harding]

On Sat, Apr 29, 2017 at 10:58:44PM +0200, Janusz Lisiecki wrote:
> Replace CamelCase struct field names with underscores to comply
> with the standard kernel coding style.
> Changed:
> - FhParms_t
> - DsParms_t
> - CfParms_t
> - IbssParms_t
> - ErpParams_t
> 
> Signed-off-by: Janusz Lisiecki 
> ---
>  drivers/staging/ks7010/ks_hostif.h | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.h 
> b/drivers/staging/ks7010/ks_hostif.h
> index ba0bd92..769dbe8 100644
> --- a/drivers/staging/ks7010/ks_hostif.h
> +++ b/drivers/staging/ks7010/ks_hostif.h
> @@ -239,25 +239,25 @@ struct rate_set8_t {
>   u8 rate_pad;
>  } __packed;
>  
> -struct FhParms_t {
> +struct fh_parms_t {
>   u16 dwell_time;
>   u8 hop_set;
>   u8 hop_pattern;
>   u8 hop_index;
>  } __packed;
>  
> -struct DsParms_t {
> +struct ds_parms_t {
>   u8 channel;
>  } __packed;
>  
> -struct CfParms_t {
> +struct cf_parms_t {
>   u8 count;
>   u8 period;
>   u16 maxDuration;
>   u16 durRemaining;
>  } __packed;
>  
> -struct IbssParms_t {
> +struct ibss_parms_t {
>   u16 atimWindow;
>  } __packed;
>  
> @@ -267,7 +267,7 @@ struct rsn_t {
>   u8 body[RSN_BODY_SIZE];
>  } __packed;
>  
> -struct ErpParams_t {
> +struct erp_params_t {
>   u8 erp_info;
>  } __packed;
>  
> @@ -313,11 +313,11 @@ struct link_ap_info_t {
>   u16 beacon_period;  /* +10 */
>   u16 capability; /* +12 */
>   struct rate_set8_t rate_set;/* +14 */
> - struct FhParms_t fh_parameter;  /* +24 */
> - struct DsParms_t ds_parameter;  /* +29 */
> - struct CfParms_t cf_parameter;  /* +30 */
> - struct IbssParms_t ibss_parameter;  /* +36 */
> - struct ErpParams_t erp_parameter;   /* +38 */
> + struct fh_parms_t fh_parameter; /* +24 */
> + struct ds_parms_t ds_parameter; /* +29 */
> + struct cf_parms_t cf_parameter; /* +30 */
> + struct ibss_parms_t ibss_parameter; /* +36 */
> + struct erp_params_t erp_parameter;  /* +38 */

4 out of 5 of these struct members appear to be unused in the
driver. I think the patch is good as it is though because I don't
think we should be chopping up struct descriptions until we are
testing on hardware.

Only my opinion, based more on a hunch than on any real experience.

thanks,
Tobin.

>   u8 pad1;/* +39 */
>   struct rate_set8_t ext_rate_set;/* +40 */
>   u8 DTIM_period; /* +50 */
> -- 
> 1.9.1
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: ks7010: add driver from Nanonote extra-repository

[Tobin C. Harding]

On Fri, Apr 28, 2017 at 04:27:29PM +0200, Wolfram Sang wrote:
> On Fri, Apr 28, 2017 at 04:41:33PM +0300, Dan Carpenter wrote:
> 
> Adding Tobin to CC, he is way more into the driver than I am these
> days...

I've mocked up a patch set to address this issue (along with some
further refactoring of the containing function). I'm going to sit on
it for a couple days before submitting. I was not able to decipher the
reason for the line that caused the checker warning. FYI the patch removes
the if statement.

thanks,
Tobin.

> > Hello Wolfram Sang,
> > 
> > The patch 13a9930d15b4: "staging: ks7010: add driver from Nanonote
> > extra-repository" from May 31, 2016, leads to the following static
> > checker warning:
> > 
> > drivers/staging/ks7010/ks_wlan_net.c:1141 ks_wlan_get_range()
> > warn: dead code because of 'i > 2' and 'i < 13'
> > 
> > drivers/staging/ks7010/ks_wlan_net.c
> >   1062  static int ks_wlan_get_range(struct net_device *dev,
> >   1063   struct iw_request_info *info,
> >   1064   struct iw_point *dwrq, char *extra)
> >   1065  {
> >   1066  struct ks_wlan_private *priv =
> >   1067  (struct ks_wlan_private *)netdev_priv(dev);
> >   1068  struct iw_range *range = (struct iw_range *)extra;
> >   1069  int i, k;
> >   1070  
> >   1071  DPRINTK(2, "\n");
> >   1072  
> >   1073  if (priv->sleep_mode == SLP_SLEEP)
> >   1074  return -EPERM;
> >   1075  
> >   1076  /* for SLEEP MODE */
> >   1077  dwrq->length = sizeof(struct iw_range);
> >   1078  memset(range, 0, sizeof(*range));
> >   1079  range->min_nwid = 0x;
> >   1080  range->max_nwid = 0x;
> >   1081  range->num_channels = 14;
> >   1082  /* Should be based on cap_rid.country to give only
> >   1083   * what the current card support
> >   1084   */
> >   1085  k = 0;
> >   1086  for (i = 0; i < 13; i++) {  /* channel 1 -- 13 */
> > ^^
> >   1087  range->freq[k].i = i + 1;   /* List index */
> >   1088  range->freq[k].m = frequency_list[i] * 10;
> >   1089  range->freq[k++].e = 1; /* Values in table in MHz 
> > -> * 10^5 * 10 */
> >   1090  }
> > 
> > i is always 13 after the loop.
> > 
> >   1091  range->num_frequency = k;
> >   1092  if (priv->reg.phy_type == D_11B_ONLY_MODE || 
> > priv->reg.phy_type == D_11BG_COMPATIBLE_MODE) {/* channel 14 */
> >   1093  range->freq[13].i = 14; /* List index */
> >   1094  range->freq[13].m = frequency_list[13] * 10;
> >   1095  range->freq[13].e = 1;  /* Values in table in MHz 
> > -> * 10^5 * 10 */
> >   1096  range->num_frequency = 14;
> >   1097  }
> >   1098  
> >   1099  /* Hum... Should put the right values there */
> >   1100  range->max_qual.qual = 100;
> >   1101  range->max_qual.level = 256 - 128;  /* 0 dBm? */
> >   1102  range->max_qual.noise = 256 - 128;
> >   1103  range->sensitivity = 1;
> >   1104  
> >   1105  if (priv->reg.phy_type == D_11B_ONLY_MODE) {
> >   1106  range->bitrate[0] = 1e6;
> >   1107  range->bitrate[1] = 2e6;
> >   1108  range->bitrate[2] = 5.5e6;
> >   1109  range->bitrate[3] = 11e6;
> >   1110  range->num_bitrates = 4;
> >     } else {/* D_11G_ONLY_MODE or 
> > D_11BG_COMPATIBLE_MODE */
> >   1112  range->bitrate[0] = 1e6;
> >   1113  range->bitrate[1] = 2e6;
> >   1114  range->bitrate[2] = 5.5e6;
> >   1115  range->bitrate[3] = 11e6;
> >   1116  
> >   1117  range->bitrate[4] = 6e6;
> >   1118  range->bitrate[5] = 9e6;
> >   1119  range->bitrate[6] = 12e6;
> >   1120  if (IW_MAX_BITRATES < 9) {
> >   1121  range->bitrate[7] = 54e6;
> >   1122  range->num_bitrates = 8;
> >   1123  } else {
> >   1124  range->bitrate[7] = 18e6;
> >   1125  range->bitrate[8] = 24e6;
> >   1126  range->bitrate[9] = 36e6;
> >   1127  range->bitrate[10] = 48e6;
> >   1128  range->bitrate[11] = 54e6;
> >   1129  
> >   1130  range->num_bitrates = 12;
> >   1131  }
> >   1132  }
> >   1133  
> >   1134  /* Set an indication of the max TCP throughput
> >   1135   * in bit/s that we can expect using this interface.
> >   1136   * May be use for QoS stuff... Jean II
> >   1137   */
> >   1138  if (i > 2)
> > ^

Re: [PATCH] staging/ks7010: Fix type assignment for struct hostif_hdr

[Tobin C. Harding]

On Sat, Apr 29, 2017 at 07:54:30PM +0200, Cezary Gapinski wrote:
> Sparse spits out a warnings about __le16 and unsigned short assignment.
> Change the type of size and event members of struct hostif_hdr
> to __le16 and correct conversion to the proper cpu type.

I believe that this patch is correct. I am of the opinion though that
endian changes to this driver require prior testing on hardware. There
are some anomalies that I have not figured out yet (for example the
eap header appears to be in network byte order).

I may, however, very well be wrong. It may be perfectly safe to merge
this as is.

Good luck,
Tobin.

> Signed-off-by: Cezary Gapinski 
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 10 ++
>  drivers/staging/ks7010/ks_hostif.h   |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
> b/drivers/staging/ks7010/ks7010_sdio.c
> index ec11799..e3a134d 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -269,7 +269,8 @@ static int write_to_device(struct ks_wlan_private *priv, 
> unsigned char *buffer,
>   hdr = (struct hostif_hdr *)buffer;
>  
>   DPRINTK(4, "size=%d\n", hdr->size);
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + le16_to_cpu(hdr->event) > HIF_REQ_MAX) {
>   DPRINTK(1, "unknown event=%04X\n", hdr->event);
>   return 0;
>   }
> @@ -327,13 +328,14 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void 
> *p, unsigned long size,
>  
>   hdr = (struct hostif_hdr *)p;
>  
> - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) {
> + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ ||
> + le16_to_cpu(hdr->event) > HIF_REQ_MAX) {
>   DPRINTK(1, "unknown event=%04X\n", hdr->event);
>   return 0;
>   }
>  
>   /* add event to hostt buffer */
> - priv->hostt.buff[priv->hostt.qtail] = hdr->event;
> + priv->hostt.buff[priv->hostt.qtail] = le16_to_cpu(hdr->event);
>   priv->hostt.qtail = (priv->hostt.qtail + 1) % SME_EVENT_BUFF_SIZE;
>  
>   DPRINTK(4, "event=%04X\n", hdr->event);
> @@ -403,7 +405,7 @@ static void ks_wlan_hw_rx(struct ks_wlan_private *priv, 
> uint16_t size)
>  
>   hdr = (struct hostif_hdr *)&rx_buffer->data[0];
>   rx_buffer->size = le16_to_cpu(hdr->size) + sizeof(hdr->size);
> - event = hdr->event;
> + event = le16_to_cpu(hdr->event);
>   inc_rxqtail(priv);
>  
>   ret = ks7010_sdio_writeb(priv, READ_STATUS, REG_STATUS_IDLE);
> diff --git a/drivers/staging/ks7010/ks_hostif.h 
> b/drivers/staging/ks7010/ks_hostif.h
> index d773432..7e4d1aa 100644
> --- a/drivers/staging/ks7010/ks_hostif.h
> +++ b/drivers/staging/ks7010/ks_hostif.h
> @@ -62,8 +62,8 @@
>   */
>  
>  struct hostif_hdr {
> - u16 size;
> - u16 event;
> + __le16 size;
> + __le16 event;
>  } __packed;
>  
>  struct hostif_data_request_t {
> -- 
> 2.7.4
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel