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: [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: [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: [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

[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: [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

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: [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 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 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 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 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 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 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 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] 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 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 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] 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: 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] 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 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 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 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 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: 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] 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] 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 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: [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: [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: [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: [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: [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 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: [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] 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 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 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 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

[PATCH] greybus: fw-management: Replace strncpy with strlcpy

[Tobin C. Harding]

Greybus currently uses strncpy() coupled with a check for '\0' on the
last byte of various buffers. strncpy() is passed size parameter equal
to the size of the buffer in all instances. If the source string is
larger than the destination buffer the check catches this and, after
logging the error, returns an error value. In one instance the
immediate return is not required. Using strncpy() with the manual check
adds code that could be removed by the use of strlcpy(), although truncation
then needs to be checked.

Replace calls to strncpy() with calls to strlcpy(). Replace null
termination checks  with checks for truncated string. Add log message
if string is truncated but do not return an error code.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/greybus/fw-management.c | 59 +++--
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/greybus/fw-management.c 
b/drivers/staging/greybus/fw-management.c
index 3cd6cf0..1cd5a45 100644
--- a/drivers/staging/greybus/fw-management.c
+++ b/drivers/staging/greybus/fw-management.c
@@ -108,6 +108,7 @@ static int fw_mgmt_interface_fw_version_operation(struct 
fw_mgmt *fw_mgmt,
struct gb_connection *connection = fw_mgmt->connection;
struct gb_fw_mgmt_interface_fw_version_response response;
int ret;
+   size_t len;
 
ret = gb_operation_sync(connection,
GB_FW_MGMT_TYPE_INTERFACE_FW_VERSION, NULL, 0,
@@ -121,18 +122,11 @@ static int fw_mgmt_interface_fw_version_operation(struct 
fw_mgmt *fw_mgmt,
fw_info->major = le16_to_cpu(response.major);
fw_info->minor = le16_to_cpu(response.minor);
 
-   strncpy(fw_info->firmware_tag, response.firmware_tag,
+   len = strlcpy(fw_info->firmware_tag, response.firmware_tag,
GB_FIRMWARE_TAG_MAX_SIZE);
-
-   /*
-* The firmware-tag should be NULL terminated, otherwise throw error but
-* don't fail.
-*/
-   if (fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
+   if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
dev_err(fw_mgmt->parent,
-   "fw-version: firmware-tag is not NULL terminated\n");
-   fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] = '\0';
-   }
+   "fw-version: firmware-tag has been truncated\n");
 
return 0;
 }
@@ -142,6 +136,7 @@ static int fw_mgmt_load_and_validate_operation(struct 
fw_mgmt *fw_mgmt,
 {
struct gb_fw_mgmt_load_and_validate_fw_request request;
int ret;
+   size_t len;
 
if (load_method != GB_FW_LOAD_METHOD_UNIPRO &&
load_method != GB_FW_LOAD_METHOD_INTERNAL) {
@@ -151,16 +146,10 @@ static int fw_mgmt_load_and_validate_operation(struct 
fw_mgmt *fw_mgmt,
}
 
request.load_method = load_method;
-   strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
-
-   /*
-* The firmware-tag should be NULL terminated, otherwise throw error and
-* fail.
-*/
-   if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
-   dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is 
not NULL terminated\n");
-   return -EINVAL;
-   }
+   len = strlcpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
+   if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
+   dev_err(fw_mgmt->parent,
+   "load-and-validate: firmware-tag has been truncated\n");
 
/* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
ret = ida_simple_get(&fw_mgmt->id_map, 1, 256, GFP_KERNEL);
@@ -247,18 +236,13 @@ static int fw_mgmt_backend_fw_version_operation(struct 
fw_mgmt *fw_mgmt,
struct gb_fw_mgmt_backend_fw_version_request request;
struct gb_fw_mgmt_backend_fw_version_response response;
int ret;
+   size_t len;
 
-   strncpy(request.firmware_tag, fw_info->firmware_tag,
+   len = strlcpy(request.firmware_tag, fw_info->firmware_tag,
GB_FIRMWARE_TAG_MAX_SIZE);
-
-   /*
-* The firmware-tag should be NULL terminated, otherwise throw error and
-* fail.
-*/
-   if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
-   dev_err(fw_mgmt->parent, "backend-version: firmware-tag is not 
NULL terminated\n");
-   return -EINVAL;
-   }
+   if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
+   dev_err(fw_mgmt->parent,
+   "backend-version: firmware-tag has been truncated\n");
 
ret = gb_operation_sync(connection,
GB_FW_MGMT_TYPE_BACKEND_FW_VERSION, &request,
@@ -301,17 +285,12 @@ static int fw_mgmt_backend_fw_update_operation(struct 
fw_

[PATCH] staging: fbtft: Fix buffer overflow vulnerability

[Tobin C. Harding]

Module copies a user supplied string (module parameter) into a buffer
using strncpy() and does not check that the buffer is null terminated.

Replace call to strncpy() with call to strlcpy() ensuring that the
buffer is null terminated.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/fbtft/fbtft_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c 
b/drivers/staging/fbtft/fbtft_device.c
index de46f8d..7b7223b 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1483,7 +1483,7 @@ static int __init fbtft_device_init(void)
displays[i].pdev->name = name;
displays[i].spi = NULL;
} else {
-   strncpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
+   strlcpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
displays[i].pdev = NULL;
}
}
-- 
2.7.4

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

[PATCH v2 0/3] staging: fbtft: Fix buffer overflow vulnerability

[Tobin C. Harding]

Module copies a user supplied string (module parameter) into a buffer
using strncpy() and does not check that the buffer is null terminated.

Replace call to strncpy() with call to strlcpy() ensuring that the
buffer is null terminated.

Replace magic number with pre-existing compile time constant.

Check return value of call to strlcpy() and throw warning if source
string is truncated.

v1 was a single patch. v2 adds 2 extra patches while retaining the
original v1 patch as the first of the series.

v2:
 - Replace magic number
 - Check return value of call to strlcpy()

Tobin C. Harding (3):
  staging: fbtft: Fix buffer overflow vulnerability
  staging: fbtft: Replace magic number with constant
  staging: fbtft: Add check on strlcpy() return value

 drivers/staging/fbtft/fbtft_device.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[PATCH v2 3/3] staging: fbtft: Add check on strlcpy() return value

[Tobin C. Harding]

Return value of strlcpy() is not checked. Name string is silently
truncated if longer that SPI_NAME_SIZE, whilst not detrimental to
the program logic it would be nice to notify the user. Module is
currently quite verbose, adding extra pr_warn() calls will not overly
impact this verbosity.

Check return value from call to strlcpy(). If source string is
truncated call pr_warn() to notify user.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/fbtft/fbtft_device.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c 
b/drivers/staging/fbtft/fbtft_device.c
index 5fbdd37..78baf46 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1483,7 +1483,13 @@ static int __init fbtft_device_init(void)
displays[i].pdev->name = name;
displays[i].spi = NULL;
} else {
-   strlcpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
+   size_t len;
+
+   len = strlcpy(displays[i].spi->modalias, name,
+   SPI_NAME_SIZE);
+   if (len >= SPI_NAME_SIZE)
+   pr_warn("modalias (name) truncated to: %s\n",
+   displays[i].spi->modalias);
displays[i].pdev = NULL;
}
}
-- 
2.7.4

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

[PATCH v2 2/3] staging: fbtft: Replace magic number with constant

[Tobin C. Harding]

Current call to strncmp() uses a magic number. There is a compile
time constant defined for this buffer, included and used already at
other sites in the file.

Remove magic number. Replace with pre-existing compile time constant.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/fbtft/fbtft_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c 
b/drivers/staging/fbtft/fbtft_device.c
index 7b7223b..5fbdd37 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1489,7 +1489,7 @@ static int __init fbtft_device_init(void)
}
 
for (i = 0; i < ARRAY_SIZE(displays); i++) {
-   if (strncmp(name, displays[i].name, 32) == 0) {
+   if (strncmp(name, displays[i].name, SPI_NAME_SIZE) == 0) {
if (displays[i].spi) {
spi = displays[i].spi;
spi->chip_select = cs;
-- 
2.7.4

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

[PATCH v2 1/3] staging: fbtft: Fix buffer overflow vulnerability

[Tobin C. Harding]

Module copies a user supplied string (module parameter) into a buffer
using strncpy() and does not check that the buffer is null terminated.

Replace call to strncpy() with call to strlcpy() ensuring that the
buffer is null terminated.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/fbtft/fbtft_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c 
b/drivers/staging/fbtft/fbtft_device.c
index de46f8d..7b7223b 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1483,7 +1483,7 @@ static int __init fbtft_device_init(void)
displays[i].pdev->name = name;
displays[i].spi = NULL;
} else {
-   strncpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
+   strlcpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
displays[i].pdev = NULL;
}
}
-- 
2.7.4

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

[PATCH 0/9] staging: ks7010: Checkpatch fixes

[Tobin C. Harding]

Checkpatch emits numerous warnings and checks. Line over 80 warnings
are not touched. Each patch addresses an individual checkpatch warning
(or check). Code is refactored over a series of patches in an effort
to modularize the changes. When a patch reduces the indentation no
other changes are made in the same patch. Decision to bundle
refactoring along with checkpatch fixes was made because the
motivating force is checkpatch.

Refactoring is limited to the functions surrounding checkpatch
warnings, if these changes are deemed satisfactory the rest of the
file could be refactored in a similar manner, perhaps that should be
done in this series also? 9 patches for the series seemed like it was
enough.

After application of this series 3 types of checkpatch message remain
(line over 80, macro argument reuse, and code compiled out with
ifdef).

Tobin C. Harding (9):
  staging: ks7010: Fix checkpatch warnings
  staging: ks7010: Remove dead code
  staging: ks7010: Fix checkpatch warning
  staging: ks7010: Fix checkpatch warning
  staging: ks7010: refactor, change retval to ret
  staging: ks7010: Refactor code
  staging: ks7010: Remove level of indentation
  staging: ks7010: Refactor code
  staging: ks7010: Fix checkpatch whitespace checks

 drivers/staging/ks7010/ks7010_sdio.c | 274 ---
 1 file changed, 123 insertions(+), 151 deletions(-)

-- 
2.7.4

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

[PATCH 2/9] staging: ks7010: Remove dead code

[Tobin C. Harding]

There are blocks of code that are commented out. 

Remove commented out code. Fix two small spelling typo's.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 129702c..488213e 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -482,7 +482,7 @@ static void ks7010_rw_function(struct work_struct *work)
 
DPRINTK(4, "\n");
 
-   /* wiat after DOZE */
+   /* wait after DOZE */
if (time_after(priv->last_doze + ((30 * HZ) / 1000), jiffies)) {
DPRINTK(4, "wait after DOZE\n");
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
@@ -490,11 +490,9 @@ static void ks7010_rw_function(struct work_struct *work)
return;
}
 
-   /* wiat after WAKEUP */
+   /* wait after WAKEUP */
while (time_after(priv->last_wakeup + ((30 * HZ) / 1000), jiffies)) {
DPRINTK(4, "wait after WAKEUP\n");
-/* 
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,&priv->ks_wlan_hw.rw_wq,
-   (priv->last_wakeup + ((30*HZ)/1000) - jiffies));*/
dev_info(&priv->ks_wlan_hw.sdio_card->func->dev,
 "wake: %lu %lu\n",
 priv->last_wakeup + (30 * HZ) / 1000,
@@ -634,7 +632,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)

ks_wlan_hw_wakeup_request(priv);
queue_delayed_work

(priv->ks_wlan_hw.ks7010sdio_wq,
-
&priv->ks_wlan_hw.rw_wq, 1);
+   
&priv->ks_wlan_hw.rw_wq, 1);
return;
}
} else {
-- 
2.7.4

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

[PATCH 1/9] staging: ks7010: Fix checkpatch warnings

[Tobin C. Harding]

There are multiple line dereferences. This leaves a '.' as the
last character of the line and makes the code hard to read.

Move variable dereference onto single line. Even if this causes line
over 80 warning.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index a604c83..129702c 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -223,8 +223,8 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
DPRINTK(4,
"PMG SET!! : 
GCR_B=%02X\n",
rw_data);
-   atomic_set(&priv->psstatus.
-  status, PS_SNOOZE);
+   
atomic_set(&priv->psstatus.status,
+   PS_SNOOZE);
DPRINTK(3,

"psstatus.status=PS_SNOOZE\n");
} else {
@@ -232,8 +232,7 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
   
&priv->ks_wlan_hw.rw_wq, 1);
}
} else {
-   queue_delayed_work(priv->ks_wlan_hw.
-  ks7010sdio_wq,
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
   
&priv->ks_wlan_hw.rw_wq,
   0);
}
@@ -334,8 +333,7 @@ static void tx_device_task(void *dev)
if (rc) {
DPRINTK(1, "write_to_device error !!(%d)\n",
rc);
-   queue_delayed_work(priv->ks_wlan_hw.
-  ks7010sdio_wq,
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
   &priv->ks_wlan_hw.rw_wq, 1);
return;
}
@@ -635,10 +633,8 @@ static void ks_sdio_interrupt(struct sdio_func *func)
if (cnt_txqbody(priv)) {

ks_wlan_hw_wakeup_request(priv);
queue_delayed_work
-   (priv->ks_wlan_hw.
-ks7010sdio_wq,
-&priv->ks_wlan_hw.
-rw_wq, 1);
+   
(priv->ks_wlan_hw.ks7010sdio_wq,
+
&priv->ks_wlan_hw.rw_wq, 1);
return;
}
} else {
-- 
2.7.4

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

[PATCH 5/9] staging: ks7010: refactor, change retval to ret

[Tobin C. Harding]

Code can be refactored to take advantage of indentation removal in
previous commit. Code is still nested quite deeply. Variable name
retval takes up six characters, this is significant because 80
character line limit is being hit frequently.

Change variable name retval to ret to get an extra three characters to
fit on each line.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 06d1037..29d332d 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -171,7 +171,7 @@ void ks_wlan_hw_wakeup_request(struct ks_wlan_private *priv)
 static int _ks_wlan_hw_power_save(struct ks_wlan_private *priv)
 {
unsigned char rw_data;
-   int retval;
+   int ret;
 
if (priv->reg.powermgt == POWMGT_ACTIVE_MODE)
return 0;
@@ -196,11 +196,11 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (!atomic_read(&priv->psstatus.confirm_wait)
&& !atomic_read(&priv->psstatus.snooze_guard)
&& !cnt_txqbody(priv)) {
-   retval =
+   ret =
ks7010_sdio_read(priv, INT_PENDING,
&rw_data,
sizeof(rw_data));
-   if (retval) {
+   if (ret) {
DPRINTK(1,
" error : INT_PENDING=%02X\n",
rw_data);
@@ -210,12 +210,12 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
}
if (!rw_data) {
rw_data = GCR_B_DOZE;
-   retval =
+   ret =
ks7010_sdio_write(priv,
GCR_B,
&rw_data,
sizeof(rw_data));
-   if (retval) {
+   if (ret) {
DPRINTK(1,
" error : GCR_B=%02X\n",
rw_data);
-- 
2.7.4

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

[PATCH 4/9] staging: ks7010: Fix checkpatch warning

[Tobin C. Harding]

Checkpatch emits WARNING: Too many leading tabs - consider code
refactoring. Code in question is nested under two if statements.
Outside of if statements there is the return statement. If statement
conditionals can be inverted and function made to return immediately
without changing program logic.

Invert conditionals. Also change && to ||. Return immediately if new
conditionals are true. Do not touch remaining code except to remove
indentation.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 122 +--
 1 file changed, 61 insertions(+), 61 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 2992eca..06d1037 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -176,71 +176,71 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (priv->reg.powermgt == POWMGT_ACTIVE_MODE)
return 0;
 
-   if (priv->reg.operation_mode == MODE_INFRASTRUCTURE &&
-   (priv->connect_status & CONNECT_STATUS_MASK) == CONNECT_STATUS) {
-   if (priv->dev_state == DEVICE_STATE_SLEEP) {
-   switch (atomic_read(&priv->psstatus.status)) {
-   case PS_SNOOZE: /* 4 */
+   if (priv->reg.operation_mode != MODE_INFRASTRUCTURE ||
+   (priv->connect_status & CONNECT_STATUS_MASK) != CONNECT_STATUS)
+   return 0;
+
+   if (priv->dev_state != DEVICE_STATE_SLEEP)
+   return 0;
+
+   switch (atomic_read(&priv->psstatus.status)) {
+   case PS_SNOOZE: /* 4 */
+   break;
+   default:
+   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
+   atomic_read(&priv->psstatus.status),
+   atomic_read(&priv->psstatus.confirm_wait),
+   atomic_read(&priv->psstatus.snooze_guard),
+   cnt_txqbody(priv));
+
+   if (!atomic_read(&priv->psstatus.confirm_wait)
+   && !atomic_read(&priv->psstatus.snooze_guard)
+   && !cnt_txqbody(priv)) {
+   retval =
+   ks7010_sdio_read(priv, INT_PENDING,
+   &rw_data,
+   sizeof(rw_data));
+   if (retval) {
+   DPRINTK(1,
+   " error : INT_PENDING=%02X\n",
+   rw_data);
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+   &priv->ks_wlan_hw.rw_wq, 1);
break;
-   default:
-   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
-   atomic_read(&priv->psstatus.status),
-   
atomic_read(&priv->psstatus.confirm_wait),
-   
atomic_read(&priv->psstatus.snooze_guard),
-   cnt_txqbody(priv));
-
-   if (!atomic_read(&priv->psstatus.confirm_wait)
-   && 
!atomic_read(&priv->psstatus.snooze_guard)
-   && !cnt_txqbody(priv)) {
-   retval =
-   ks7010_sdio_read(priv, INT_PENDING,
-&rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1,
-   " error : 
INT_PENDING=%02X\n",
-   rw_data);
-   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-  
&priv->ks_wlan_hw.rw_wq, 1);
-   break;
-   }
-   if (!rw_data) {
-   rw_data = GCR_B_DOZE;
-   retval =
-   ks7010_sdio_write(priv,
- GCR_B

[PATCH 3/9] staging: ks7010: Fix checkpatch warning

[Tobin C. Harding]

Checkpatch emits warning because string is split over two lines.

Concatenate string onto single line.

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

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 488213e..2992eca 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -996,8 +996,8 @@ static int ks7010_sdio_probe(struct sdio_func *func,
 
sdio_set_drvdata(func, card);
 
-   DPRINTK(5, "class = 0x%X, vendor = 0x%X, "
-   "device = 0x%X\n", func->class, func->vendor, func->device);
+   DPRINTK(5, "class = 0x%X, vendor = 0x%X, device = 0x%X\n",
+   func->class, func->vendor, func->device);
 
/* private memory allocate */
netdev = alloc_etherdev(sizeof(*priv));
-- 
2.7.4

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

[PATCH 6/9] staging: ks7010: Refactor code

[Tobin C. Harding]

Code may be refactored to take advantage of previous commit.

Refactor code. Introduce four new line over 80 warnings. Make code
more legible.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 31 +--
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 29d332d..4842b2d 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -196,40 +196,27 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (!atomic_read(&priv->psstatus.confirm_wait)
&& !atomic_read(&priv->psstatus.snooze_guard)
&& !cnt_txqbody(priv)) {
-   ret =
-   ks7010_sdio_read(priv, INT_PENDING,
-   &rw_data,
-   sizeof(rw_data));
+   ret = ks7010_sdio_read(priv, INT_PENDING, &rw_data,
+  sizeof(rw_data));
if (ret) {
-   DPRINTK(1,
-   " error : INT_PENDING=%02X\n",
-   rw_data);
+   DPRINTK(1, " error : INT_PENDING=%02X\n", 
rw_data);

queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
&priv->ks_wlan_hw.rw_wq, 1);
break;
}
if (!rw_data) {
rw_data = GCR_B_DOZE;
-   ret =
-   ks7010_sdio_write(priv,
-   GCR_B,
-   &rw_data,
+   ret = ks7010_sdio_write(priv, GCR_B, &rw_data,
sizeof(rw_data));
if (ret) {
-   DPRINTK(1,
-   " error : GCR_B=%02X\n",
-   rw_data);
-   queue_delayed_work
-   (priv->ks_wlan_hw.ks7010sdio_wq,
-   
&priv->ks_wlan_hw.rw_wq, 1);
+   DPRINTK(1, " error : GCR_B=%02X\n", 
rw_data);
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+  
&priv->ks_wlan_hw.rw_wq, 1);
break;
}
-   DPRINTK(4,
-   "PMG SET!! : GCR_B=%02X\n",
-   rw_data);
+   DPRINTK(4, "PMG SET!! : GCR_B=%02X\n", rw_data);
atomic_set(&priv->psstatus.status, PS_SNOOZE);
-   DPRINTK(3,
-   "psstatus.status=PS_SNOOZE\n");
+   DPRINTK(3, "psstatus.status=PS_SNOOZE\n");
} else {

queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
&priv->ks_wlan_hw.rw_wq, 1);
-- 
2.7.4

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

[PATCH 7/9] staging: ks7010: Remove level of indentation

[Tobin C. Harding]

Checkpatch emits warning that code is too deeply nested.

Remove one level of nesting by inverting if statement conditional
and using goto to maintain program logic.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 151 ++-
 1 file changed, 76 insertions(+), 75 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 4842b2d..5a69468 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -544,93 +544,94 @@ static void ks_sdio_interrupt(struct sdio_func *func)
priv = card->priv;
DPRINTK(4, "\n");
 
-   if (priv->dev_state >= DEVICE_STATE_BOOT) {
+   if (priv->dev_state < DEVICE_STATE_BOOT)
+   goto intr_out;
+
+   retval =
+   ks7010_sdio_read(priv, INT_PENDING, &status,
+   sizeof(status));
+   if (retval) {
+   DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
+   goto intr_out;
+   }
+   DPRINTK(4, "INT_PENDING=%02X\n", rw_data);
+
+   /* schedule task for interrupt status */
+   /* bit7 -> Write General Communication B register */
+   /* read (General Communication B register) */
+   /* bit5 -> Write Status Idle */
+   /* bit2 -> Read Status Busy  */
+   if (status & INT_GCR_B
+   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
retval =
-   ks7010_sdio_read(priv, INT_PENDING, &status,
-sizeof(status));
+   ks7010_sdio_read(priv, GCR_B, &rw_data,
+   sizeof(rw_data));
if (retval) {
-   DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
+   DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
goto intr_out;
}
-   DPRINTK(4, "INT_PENDING=%02X\n", rw_data);
-
-   /* schedule task for interrupt status */
-   /* bit7 -> Write General Communication B register */
-   /* read (General Communication B register) */
-   /* bit5 -> Write Status Idle */
-   /* bit2 -> Read Status Busy  */
-   if (status & INT_GCR_B
-   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
-   retval =
-   ks7010_sdio_read(priv, GCR_B, &rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
-   goto intr_out;
-   }
-   /* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
-   if (rw_data == GCR_B_ACTIVE) {
-   if (atomic_read(&priv->psstatus.status) ==
-   PS_SNOOZE) {
-   atomic_set(&priv->psstatus.status,
-  PS_WAKEUP);
-   priv->wakeup_count = 0;
-   }
-   complete(&priv->psstatus.wakeup_wait);
+   /* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
+   if (rw_data == GCR_B_ACTIVE) {
+   if (atomic_read(&priv->psstatus.status) ==
+   PS_SNOOZE) {
+   atomic_set(&priv->psstatus.status,
+   PS_WAKEUP);
+   priv->wakeup_count = 0;
}
+   complete(&priv->psstatus.wakeup_wait);
}
+   }
 
-   do {
-   /* read (WriteStatus/ReadDataSize FN1:00_0014) */
-   retval =
-   ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n",
-   rw_data);
-   goto intr_out;
-   }
-   DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
-   rsize = rw_data & RSIZE_MASK;
-   if (rsize) {/* Read schedule */
-   ks_wlan_hw_rx((void *)priv,
- (uint16_t)(rsize << 4));
-   }
-   if (rw_data & WSTATUS_MASK) {
+

[PATCH 8/9] staging: ks7010: Refactor code

[Tobin C. Harding]

Code is able to be refactored. Various assignments have the assigning
value on a separate line.

Refactor code bring it closer to kernel standards.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 29 +
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 5a69468..3119ee2 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -547,9 +547,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
if (priv->dev_state < DEVICE_STATE_BOOT)
goto intr_out;
 
-   retval =
-   ks7010_sdio_read(priv, INT_PENDING, &status,
-   sizeof(status));
+   retval = ks7010_sdio_read(priv, INT_PENDING, &status, sizeof(status));
if (retval) {
DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
goto intr_out;
@@ -563,19 +561,15 @@ static void ks_sdio_interrupt(struct sdio_func *func)
/* bit2 -> Read Status Busy  */
if (status & INT_GCR_B
|| atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
-   retval =
-   ks7010_sdio_read(priv, GCR_B, &rw_data,
-   sizeof(rw_data));
+   retval = ks7010_sdio_read(priv, GCR_B, &rw_data, 
sizeof(rw_data));
if (retval) {
DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
goto intr_out;
}
/* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
if (rw_data == GCR_B_ACTIVE) {
-   if (atomic_read(&priv->psstatus.status) ==
-   PS_SNOOZE) {
-   atomic_set(&priv->psstatus.status,
-   PS_WAKEUP);
+   if (atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
+   atomic_set(&priv->psstatus.status, PS_WAKEUP);
priv->wakeup_count = 0;
}
complete(&priv->psstatus.wakeup_wait);
@@ -584,28 +578,23 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 
do {
/* read (WriteStatus/ReadDataSize FN1:00_0014) */
-   retval =
-   ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
+   retval = ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
sizeof(rw_data));
if (retval) {
-   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n",
-   rw_data);
+   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n", rw_data);
goto intr_out;
}
DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
rsize = rw_data & RSIZE_MASK;
if (rsize) {/* Read schedule */
-   ks_wlan_hw_rx((void *)priv,
-   (uint16_t)(rsize << 4));
+   ks_wlan_hw_rx((void *)priv, (uint16_t)(rsize << 4));
}
if (rw_data & WSTATUS_MASK) {
 #if 0
-   if (status & INT_WRITE_STATUS
-   && !cnt_txqbody(priv)) {
+   if (status & INT_WRITE_STATUS && !cnt_txqbody(priv)) {
/* dummy write for interrupt clear */
rw_data = 0;
-   retval =
-   ks7010_sdio_write(priv, DATA_WINDOW,
+   retval = ks7010_sdio_write(priv, DATA_WINDOW,
&rw_data,
sizeof(rw_data));
if (retval) {
-- 
2.7.4

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

[PATCH 9/9] staging: ks7010: Fix checkpatch whitespace checks

[Tobin C. Harding]

Checkpatch emits two CHECK messages;
CHECK: Logical continuations should be on the previous line
CHECK: Alignment should match open parenthesis

Move logical continuations onto the previous line. Add whitespace to
align code with parenthesis.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 3119ee2..a1e863f 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -193,15 +193,15 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
atomic_read(&priv->psstatus.snooze_guard),
cnt_txqbody(priv));
 
-   if (!atomic_read(&priv->psstatus.confirm_wait)
-   && !atomic_read(&priv->psstatus.snooze_guard)
-   && !cnt_txqbody(priv)) {
+   if (!atomic_read(&priv->psstatus.confirm_wait) &&
+   !atomic_read(&priv->psstatus.snooze_guard) &&
+   !cnt_txqbody(priv)) {
ret = ks7010_sdio_read(priv, INT_PENDING, &rw_data,
   sizeof(rw_data));
if (ret) {
DPRINTK(1, " error : INT_PENDING=%02X\n", 
rw_data);

queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
+  &priv->ks_wlan_hw.rw_wq, 1);
break;
}
if (!rw_data) {
@@ -219,12 +219,12 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
DPRINTK(3, "psstatus.status=PS_SNOOZE\n");
} else {

queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
+  &priv->ks_wlan_hw.rw_wq, 1);
}
} else {
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq,
-   0);
+  &priv->ks_wlan_hw.rw_wq,
+  0);
}
break;
}
@@ -312,8 +312,8 @@ static void tx_device_task(void *dev)
int rc = 0;
 
DPRINTK(4, "\n");
-   if (cnt_txqbody(priv) > 0
-   && atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
+   if (cnt_txqbody(priv) > 0 &&
+   atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
if (priv->dev_state >= DEVICE_STATE_BOOT) {
rc = write_to_device(priv, sp->sendp, sp->size);
@@ -559,9 +559,11 @@ static void ks_sdio_interrupt(struct sdio_func *func)
/* read (General Communication B register) */
/* bit5 -> Write Status Idle */
/* bit2 -> Read Status Busy  */
-   if (status & INT_GCR_B
-   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
-   retval = ks7010_sdio_read(priv, GCR_B, &rw_data, 
sizeof(rw_data));
+   if (status & INT_GCR_B ||
+   atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
+   retval = ks7010_sdio_read(priv, GCR_B, &rw_data,
+ sizeof(rw_data));
+
if (retval) {
DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
goto intr_out;
@@ -579,7 +581,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
do {
/* read (WriteStatus/ReadDataSize FN1:00_0014) */
retval = ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
-   sizeof(rw_data));
+ sizeof(rw_data));
if (retval) {
DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n", rw_data);
goto intr_out;
@@ -595,8 +597,8 @@ static void ks_sdio_interrupt(struct sdio_func *func)
/* dummy write for interrupt clear */
rw_data = 0;
retval = ks7010_sdio_write(priv, DATA_WINDOW,
-   &a

staging: ks7010: Checkpatch fixes

[Tobin C. Harding]

Checkpatch emits various messages. Struct ks_stdio_card contains a
spinlock. It is never used. The data structure contains two fields,
both of which are accessed in ways that don't seem to allow use of a
spinlock. I am not fully qualified however to analyse this.

Spinlock is either not correctly used or is defined and not needed.

Fix whitespace errors/warnings/checks. Fix macro definition to clear
checkpatch check and error.

Kernelnewbie note: I am unsure how to broach issues like this when
discovered. In a patch cover letter like this or some other way?

Tobin C. Harding (2):
  staging: ks7010: Whitespace checkpatch fixes
  staging: ks7010: Whitespace checkpatch fixes

 drivers/staging/ks7010/ks7010_sdio.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[PATCH 1/2] staging: ks7010: Whitespace checkpatch fixes

[Tobin C. Harding]

Checkpatch emits various whitespace messages;

ERROR: trailing whitespace
WARNING: Unnecessary space before function pointer arguments
CHECK: Please use a blank line after function/struct/union/enum

Remove trailing whitespace. Remove unnecessary space. Add blank line
after struct definition.

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

diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/ks7010_sdio.h
index 0f5fd84..9103522 100644
--- a/drivers/staging/ks7010/ks7010_sdio.h
+++ b/drivers/staging/ks7010/ks7010_sdio.h
@@ -85,7 +85,7 @@
 
 #define KS7010_IRAM_ADDRESS0x0600
 
-/* 
+/*
  * struct define
  */
 struct hw_info_t {
@@ -115,7 +115,7 @@ struct ks_sdio_card {
 struct tx_device_buffer {
unsigned char *sendp;   /* pointer of send req data */
unsigned int size;
-   void (*complete_handler) (void *arg1, void *arg2);
+   void (*complete_handler)(void *arg1, void *arg2);
void *arg1;
void *arg2;
 };
@@ -142,6 +142,7 @@ struct rx_device {
unsigned int qtail; /* rx buffer queue last pointer */
spinlock_t rx_dev_lock;
 };
+
 #defineROM_FILE "ks7010sd.rom"
 
 #endif /* _KS7010_SDIO_H */
-- 
2.7.4

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

[PATCH 2/2] staging: ks7010: Checkpatch macro fixes

[Tobin C. Harding]

Checkpatch emits messages relating to macro definition
CHECK: spaces preferred around that '*' (ctx:VxV)
ERROR: Macros with complex values should be enclosed in parentheses

Add space around '*'. Add parentheses to macro definition.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.h 
b/drivers/staging/ks7010/ks7010_sdio.h
index 9103522..a1c7551 100644
--- a/drivers/staging/ks7010/ks7010_sdio.h
+++ b/drivers/staging/ks7010/ks7010_sdio.h
@@ -81,7 +81,7 @@
 
 /* AHB Data Window  0x01-0x01 */
 #define DATA_WINDOW0x01
-#define WINDOW_SIZE64*1024
+#define WINDOW_SIZE(64 * 1024)
 
 #define KS7010_IRAM_ADDRESS0x0600
 
-- 
2.7.4

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

[PATCH 0/8] staging: ks7010: Refactor, fix checkpatch

[Tobin C. Harding]

This is the second patch series to staging ks7010. The two series
are on different files.

Checkpatch emits various warnings. Warnings are an indication that the
code needs refactoring.

Clear trivial checkpatch warnings and errors first. Split large
function into smaller functions. Refactor new functions to make code
cleaner. 

This code has not been tested other than to be built. Needs hardware
to test.

Tobin C. Harding (8):
  staging: ks7010: Fix checkpatch errors
  staging: ks7010: Fix checkpatch warning
  staging: ks7010: Refactor hostif_data_indication
  whitespace refactor
  staging: ks7010: Remove level of indentation
  staging: ks7010: Remove level of indentation
  staging: ks7010: Refactor after indentation removal
  rename refactored function

 drivers/staging/ks7010/ks_hostif.c | 282 -
 drivers/staging/ks7010/ks_hostif.h |  60 
 2 files changed, 179 insertions(+), 163 deletions(-)

-- 
2.7.4

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

[PATCH 2/8] staging: ks7010: Fix checkpatch warning

[Tobin C. Harding]

Checkpatch emits WARNING: Comparisons should place the constant on
the right side of the test.

Move constant to the rights side of the test.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index 1fbd495..a976060 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -2608,13 +2608,12 @@ void hostif_sme_task(unsigned long dev)
DPRINTK(3, "\n");
 
if (priv->dev_state >= DEVICE_STATE_BOOT) {
-   if (0 < cnt_smeqbody(priv)
-   && priv->dev_state >= DEVICE_STATE_BOOT) {
+   if (cnt_smeqbody(priv) > 0 && priv->dev_state >= 
DEVICE_STATE_BOOT) {
hostif_sme_execute(priv,
   priv->sme_i.event_buff[priv->sme_i.
  qhead]);
inc_smeqhead(priv);
-   if (0 < cnt_smeqbody(priv))
+   if (cnt_smeqbody(priv) > 0)
tasklet_schedule(&priv->sme_task);
}
}
-- 
2.7.4

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

[PATCH 3/8] staging: ks7010: Refactor hostif_data_indicationyes

[Tobin C. Harding]

Function hostif_data_indication has 14 local variables an is over 200
lines long. Contains code nested deeply. Function could be refactored.

Start to refactor function. Split out three separate functions. Don't
modify code except to make it compile, add/remove correct local
variables and add code to call new functions.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c | 267 +
 1 file changed, 149 insertions(+), 118 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index a976060..f4beb86 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -307,42 +307,9 @@ int get_ap_information(struct ks_wlan_private *priv, 
struct ap_info_t *ap_info,
return 0;
 }
 
-static
-void hostif_data_indication(struct ks_wlan_private *priv)
+static int source_address_is_valid(struct ks_wlan_private *priv,
+  struct ether_hdr *eth_hdr)
 {
-   unsigned int rx_ind_size;   /* indicate data size */
-   struct sk_buff *skb;
-   unsigned short auth_type;
-   unsigned char temp[256];
-
-   unsigned char RecvMIC[8];
-   char buf[128];
-   struct ether_hdr *eth_hdr;
-   unsigned short eth_proto;
-   unsigned long now;
-   struct mic_failure_t *mic_failure;
-   struct ieee802_1x_hdr *aa1x_hdr;
-   struct wpa_eapol_key *eap_key;
-   struct michel_mic_t michel_mic;
-   union iwreq_data wrqu;
-
-   DPRINTK(3, "\n");
-
-   /* min length check */
-   if (priv->rx_size <= ETH_HLEN) {
-   DPRINTK(3, "rx_size = %d\n", priv->rx_size);
-   priv->nstats.rx_errors++;
-   return;
-   }
-
-   auth_type = get_WORD(priv); /* AuthType */
-   get_WORD(priv); /* Reserve Area */
-
-   eth_hdr = (struct ether_hdr *)(priv->rxp);
-   eth_proto = ntohs(eth_hdr->h_proto);
-   DPRINTK(3, "ether protocol = %04X\n", eth_proto);
-
-   /* source address check */
if (!memcmp(&priv->eth_addr[0], eth_hdr->h_source, ETH_ALEN)) {
DPRINTK(1, "invalid : source is own mac address !!\n");
DPRINTK(1,
@@ -351,92 +318,19 @@ void hostif_data_indication(struct ks_wlan_private *priv)
eth_hdr->h_source[2], eth_hdr->h_source[3],
eth_hdr->h_source[4], eth_hdr->h_source[5]);
priv->nstats.rx_errors++;
-   return;
-   }
-
-   /*  for WPA */
-   if (auth_type != TYPE_DATA && priv->wpa.rsn_enabled) {
-   if (memcmp(ð_hdr->h_source[0], &priv->eth_addr[0], 
ETH_ALEN)) {  /* source address check */
-   if (eth_hdr->h_dest_snap != eth_hdr->h_source_snap) {
-   DPRINTK(1, "invalid data format\n");
-   priv->nstats.rx_errors++;
-   return;
-   }
-   if (((auth_type == TYPE_PMK1
- && priv->wpa.pairwise_suite ==
- IW_AUTH_CIPHER_TKIP) || (auth_type == TYPE_GMK1
-  && priv->wpa.
-  group_suite ==
-  IW_AUTH_CIPHER_TKIP)
-|| (auth_type == TYPE_GMK2
-&& priv->wpa.group_suite ==
-IW_AUTH_CIPHER_TKIP))
-   && priv->wpa.key[auth_type - 1].key_len) {
-   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
-   eth_proto, priv->rx_size);
-   /* MIC save */
-   memcpy(&RecvMIC[0],
-  (priv->rxp) + ((priv->rx_size) - 8), 8);
-   priv->rx_size = priv->rx_size - 8;
-   if (auth_type > 0 && auth_type < 4) {   /* 
auth_type check */
-   MichaelMICFunction(&michel_mic, 
(uint8_t *) priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
-  (uint8_t *)
-  michel_mic.Result);
-   }
-   if (memcmp(michel_mic.Result, RecvMIC, 8)) {
-   now = jiffies;
-   mic_failure = &priv->wpa.mic_failure;
-  

[PATCH 5/8] staging: ks7010: Remove level of indentation

[Tobin C. Harding]

'if' statement conditional guards large block of code. Conditional can
be inverted and function made to return. This allows following code to
have one level of indentation removed.

Invert conditional and return directly if new conditioal is true.
Remove one level of indentation.

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

Correct error constant to return?

 drivers/staging/ks7010/ks_hostif.c | 143 +++--
 1 file changed, 72 insertions(+), 71 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index 565f051..d28e9af 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -414,81 +414,82 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
struct mic_failure_t *mic_failure;
union iwreq_data wrqu;
 
-   if (memcmp(ð_hdr->h_source[0], &priv->eth_addr[0], ETH_ALEN)) {  
/* source address check */
-   if (eth_hdr->h_dest_snap != eth_hdr->h_source_snap) {
-   DPRINTK(1, "invalid data format\n");
-   priv->nstats.rx_errors++;
-   return -ERROR;
-   }
+   if (memcmp(ð_hdr->h_source[0], &priv->eth_addr[0], ETH_ALEN) != 0)
+   return 0;
+
+   if (eth_hdr->h_dest_snap != eth_hdr->h_source_snap) {
+   DPRINTK(1, "invalid data format\n");
+   priv->nstats.rx_errors++;
+   return -ERROR;
+   }
+
+   auth_type = get_WORD(priv); /* AuthType */
+   eth_proto = ntohs(eth_hdr->h_proto);
 
-   auth_type = get_WORD(priv); /* AuthType */
-   eth_proto = ntohs(eth_hdr->h_proto);
-
-   if (((auth_type == TYPE_PMK1
-   && priv->wpa.pairwise_suite ==
-   IW_AUTH_CIPHER_TKIP) || (auth_type == 
TYPE_GMK1
-   && priv->wpa.
-   group_suite ==
-   
IW_AUTH_CIPHER_TKIP)
-   || (auth_type == TYPE_GMK2
-   && priv->wpa.group_suite ==
-   IW_AUTH_CIPHER_TKIP))
-   && priv->wpa.key[auth_type - 1].key_len) {
-   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
-   eth_proto, priv->rx_size);
-   /* MIC save */
-   memcpy(&RecvMIC[0],
-   (priv->rxp) + ((priv->rx_size) - 8), 8);
-   priv->rx_size = priv->rx_size - 8;
-   if (auth_type > 0 && auth_type < 4) {   /* auth_type 
check */
-   MichaelMICFunction(&michel_mic, (uint8_t *) 
priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
-   (uint8_t *)
-   michel_mic.Result);
+   if (((auth_type == TYPE_PMK1
+   && priv->wpa.pairwise_suite ==
+   IW_AUTH_CIPHER_TKIP) || (auth_type == TYPE_GMK1
+   && priv->wpa.
+   group_suite ==
+   IW_AUTH_CIPHER_TKIP)
+   || (auth_type == TYPE_GMK2
+   && priv->wpa.group_suite ==
+   IW_AUTH_CIPHER_TKIP))
+   && priv->wpa.key[auth_type - 1].key_len) {
+   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
+   eth_proto, priv->rx_size);
+   /* MIC save */
+   memcpy(&RecvMIC[0],
+   (priv->rxp) + ((priv->rx_size) - 8), 8);
+   priv->rx_size = priv->rx_size - 8;
+   if (auth_type > 0 && auth_type < 4) {   /* auth_type check */
+   MichaelMICFunction(&michel_mic, (uint8_t *) 
priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
+   (uint8_t *)
+   michel_mic.Result);
+   }
+   if (memcmp(michel_mic.Result, RecvMIC, 8)) {
+   now = jiffies;
+   mic_failure = &priv->wpa.mic_failure;
+   /* MIC FAILU

[PATCH 8/8] staging: ks7010: Rename refactored function

[Tobin C. Harding]

Function was not named during initial refactoring.

Give function new meaningful name.

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

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index ca1051ff..e58ce42 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -402,7 +402,7 @@ static void check_13th_byte_of_rx_data(struct 
ks_wlan_private *priv)
 }
 
 #define ERROR -1   /* FIXME */
-static int function_needs_naming(struct ks_wlan_private *priv,
+static int check_mic(struct ks_wlan_private *priv,
struct ether_hdr *eth_hdr)
 {
unsigned short auth_type;
@@ -508,7 +508,7 @@ void hostif_data_indication(struct ks_wlan_private *priv)
 
/*  for WPA */
if (auth_type != TYPE_DATA && priv->wpa.rsn_enabled) {
-   err = function_needs_naming(priv, eth_hdr);
+   err = check_mic(priv, eth_hdr);
if (err)
return;
}
-- 
2.7.4

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

[PATCH 6/8] staging: ks7010: Remove level of indentation

[Tobin C. Harding]

'if' statement conditional guards large block of code. Conditional can
be inverted and function return. This allows following code to have
one level of indentation removed. However the conditional is complicated,
it can be moved into a separate variable to ease reading.

Move complicated conditional to separate variable. Invert conditional
and remove level of indentation as per previous commit.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c | 121 ++---
 1 file changed, 59 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index d28e9af..d479a88 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -413,6 +413,7 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
char buf[128];
struct mic_failure_t *mic_failure;
union iwreq_data wrqu;
+   int cond;
 
if (memcmp(ð_hdr->h_source[0], &priv->eth_addr[0], ETH_ALEN) != 0)
return 0;
@@ -426,71 +427,67 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
auth_type = get_WORD(priv); /* AuthType */
eth_proto = ntohs(eth_hdr->h_proto);
 
-   if (((auth_type == TYPE_PMK1
-   && priv->wpa.pairwise_suite ==
-   IW_AUTH_CIPHER_TKIP) || (auth_type == TYPE_GMK1
-   && priv->wpa.
-   group_suite ==
-   IW_AUTH_CIPHER_TKIP)
-   || (auth_type == TYPE_GMK2
-   && priv->wpa.group_suite ==
-   IW_AUTH_CIPHER_TKIP))
-   && priv->wpa.key[auth_type - 1].key_len) {
-   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
-   eth_proto, priv->rx_size);
-   /* MIC save */
-   memcpy(&RecvMIC[0],
-   (priv->rxp) + ((priv->rx_size) - 8), 8);
-   priv->rx_size = priv->rx_size - 8;
-   if (auth_type > 0 && auth_type < 4) {   /* auth_type check */
-   MichaelMICFunction(&michel_mic, (uint8_t *) 
priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
-   (uint8_t *)
-   michel_mic.Result);
+   cond = ((auth_type == TYPE_PMK1 && priv->wpa.pairwise_suite == 
IW_AUTH_CIPHER_TKIP) ||
+   (auth_type == TYPE_GMK1 && priv->wpa.group_suite == 
IW_AUTH_CIPHER_TKIP) ||
+   (auth_type == TYPE_GMK2 && priv->wpa.group_suite == 
IW_AUTH_CIPHER_TKIP));
+
+   if (!cond || !priv->wpa.key[auth_type - 1].key_len)
+   return 0;
+   
+   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
+   eth_proto, priv->rx_size);
+   /* MIC save */
+   memcpy(&RecvMIC[0],
+   (priv->rxp) + ((priv->rx_size) - 8), 8);
+   priv->rx_size = priv->rx_size - 8;
+   if (auth_type > 0 && auth_type < 4) {   /* auth_type check */
+   MichaelMICFunction(&michel_mic, (uint8_t *) 
priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
+   (uint8_t *)
+   michel_mic.Result);
+   }
+   if (memcmp(michel_mic.Result, RecvMIC, 8)) {
+   now = jiffies;
+   mic_failure = &priv->wpa.mic_failure;
+   /* MIC FAILURE */
+   if (mic_failure->last_failure_time &&
+   (now -
+   mic_failure->last_failure_time) /
+   HZ >= 60) {
+   mic_failure->failure = 0;
}
-   if (memcmp(michel_mic.Result, RecvMIC, 8)) {
-   now = jiffies;
-   mic_failure = &priv->wpa.mic_failure;
-   /* MIC FAILURE */
-   if (mic_failure->last_failure_time &&
-   (now -
-   mic_failure->last_failure_time) /
-   HZ >= 60) {
-   mic_failure->failure = 0;
-   }
-   DPRINTK(4, "MIC FAILURE\n");
-   if (mic_failure->failure == 0) {
-   mic_failure->failure = 1;
-   mic_failure->counter = 0;
- 

[PATCH 7/8] staging: ks7010: Refactor after indentation removal

[Tobin C. Harding]

Code is able to be refactored after previous removal of levels of
indentation.

Refactor code to bring it more inline with kernel standards.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c | 53 ++
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index d479a88..ca1051ff 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -424,7 +424,7 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
return -ERROR;
}
 
-   auth_type = get_WORD(priv); /* AuthType */
+   auth_type = get_WORD(priv); /* AuthType */
eth_proto = ntohs(eth_hdr->h_proto);
 
cond = ((auth_type == TYPE_PMK1 && priv->wpa.pairwise_suite == 
IW_AUTH_CIPHER_TKIP) ||
@@ -433,26 +433,25 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
 
if (!cond || !priv->wpa.key[auth_type - 1].key_len)
return 0;
-   
-   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n",
-   eth_proto, priv->rx_size);
+
+   DPRINTK(4, "TKIP: protocol=%04X: size=%u\n", eth_proto, priv->rx_size);
+
/* MIC save */
-   memcpy(&RecvMIC[0],
-   (priv->rxp) + ((priv->rx_size) - 8), 8);
+   memcpy(&RecvMIC[0], (priv->rxp) + ((priv->rx_size) - 8), 8);
priv->rx_size = priv->rx_size - 8;
-   if (auth_type > 0 && auth_type < 4) {   /* auth_type check */
-   MichaelMICFunction(&michel_mic, (uint8_t *) 
priv->wpa.key[auth_type - 1].rx_mic_key, (uint8_t *) priv->rxp, 
(int)priv->rx_size, (uint8_t) 0,/* priority */
-   (uint8_t *)
-   michel_mic.Result);
+   if (auth_type > 0 && auth_type < 4) { /* auth_type check */
+   MichaelMICFunction(&michel_mic,
+ (uint8_t *) priv->wpa.key[auth_type - 
1].rx_mic_key,
+ (uint8_t *) priv->rxp, (int)priv->rx_size,
+ (uint8_t) 0, /* priority */
+ (uint8_t *)michel_mic.Result);
}
if (memcmp(michel_mic.Result, RecvMIC, 8)) {
now = jiffies;
mic_failure = &priv->wpa.mic_failure;
/* MIC FAILURE */
if (mic_failure->last_failure_time &&
-   (now -
-   mic_failure->last_failure_time) /
-   HZ >= 60) {
+  (now - mic_failure->last_failure_time) / HZ >= 60) {
mic_failure->failure = 0;
}
DPRINTK(4, "MIC FAILURE\n");
@@ -461,32 +460,20 @@ static int function_needs_naming(struct ks_wlan_private 
*priv,
mic_failure->counter = 0;
} else if (mic_failure->failure == 1) {
mic_failure->failure = 2;
-   mic_failure->counter =
-   (uint16_t) ((now -
-   mic_failure->
-   last_failure_time)
-   / HZ);
+   mic_failure->counter = (uint16_t)((now - 
mic_failure->last_failure_time) / HZ);
if (!mic_failure->counter)  /* mic_failure counter 
value range 1-60 */
-   mic_failure->counter =
-   1;
+   mic_failure->counter = 1;
}
-   priv->wpa.mic_failure.
-   last_failure_time = now;
+   priv->wpa.mic_failure.last_failure_time = now;
+
/*  needed parameters: count, keyid, key type, TSC */
-   sprintf(buf,
-   "MLME-MICHAELMICFAILURE.indication(keyid=%d %scast 
addr="
-   "%pM)",
+   sprintf(buf, "MLME-MICHAELMICFAILURE.indication(keyid=%d %scast 
addr=%pM)",
auth_type - 1,
-   eth_hdr->
-   h_dest[0] & 0x01 ? "broad" :
-   "uni", eth_hdr->h_source);
+   eth_hdr->h_dest[0] & 0x01 ? "broad" : "uni", 
eth_hdr->h_source);
memset(&wrqu, 0, sizeof(wrqu));
wrqu.data.length = strlen(buf);
-   DPRINTK(4,
-   "IWEVENT:MICHAELMICFAILURE\n");
-   wireless_send_event(priv->net_dev,
-   IWEVCUSTOM, &wrqu,

[PATCH 1/8] staging: ks7010: Fix checkpatch errors

[Tobin C. Harding]

Checkpatch emits various whitespace errors and warnings.

Add whitespace around binary operators and logical operators. Remove
whitespace where checkpatch complains.

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

Perhaps I do not know how to parse a diff visually but these changes
do not look like an improvement. The do look aligned in the file when
applied and they do clear the warnings. Please nit pick openly if it
is wrong.

 drivers/staging/ks7010/ks_hostif.h | 60 +++---
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.h 
b/drivers/staging/ks7010/ks_hostif.h
index 743f31e..a9da8e9 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -1,6 +1,6 @@
 /*
  *   Driver for KeyStream wireless LAN
- *   
+ *
  *   Copyright (c) 2005-2008 KeyStream Corp.
  *   Copyright (C) 2009 Renesas Technology Corp.
  *
@@ -128,7 +128,7 @@ struct channel_list_t {
 #define DOT11_PMK_TSC 0x55010100   /* PMK_TSC (W) */
 #define DOT11_GMK1_TSC0x55010101   /* GMK1_TSC (W) */
 #define DOT11_GMK2_TSC0x55010102   /* GMK2_TSC (W) */
-#define DOT11_GMK3_TSC   0x55010103/* GMK3_TSC */
+#define DOT11_GMK3_TSC   0x55010103/* GMK3_TSC */
 #define LOCAL_PMK 0x58010100   /* Pairwise Master Key 
cache (W) */
 
 #define LOCAL_REGION  0xF10A0100   /* Region setting */
@@ -360,7 +360,7 @@ struct hostif_ps_adhoc_set_request_t {
 #define CTS_MODE_TRUE  1
uint16_t channel;
struct rate_set16_t rate_set;
-   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0 
+   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0
 * bit10:ShortSlotTime bit13:DSSS-OFDM 
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
 } __packed;
@@ -376,7 +376,7 @@ struct hostif_infrastructure_set_request_t {
uint16_t cts_mode;
struct rate_set16_t rate_set;
struct ssid_t ssid;
-   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0 
+   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0
 * bit10:ShortSlotTime bit13:DSSS-OFDM 
DSSS-OFDM not supported always 0 */
uint16_t beacon_lost_count;
uint16_t auth_type;
@@ -392,7 +392,7 @@ struct hostif_infrastructure_set2_request_t {
uint16_t cts_mode;
struct rate_set16_t rate_set;
struct ssid_t ssid;
-   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0 
+   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0
 * bit10:ShortSlotTime bit13:DSSS-OFDM 
DSSS-OFDM not supported always 0 */
uint16_t beacon_lost_count;
uint16_t auth_type;
@@ -415,7 +415,7 @@ struct hostif_adhoc_set_request_t {
uint16_t channel;
struct rate_set16_t rate_set;
struct ssid_t ssid;
-   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0 
+   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0
 * bit10:ShortSlotTime bit13:DSSS-OFDM 
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
 } __packed;
@@ -427,7 +427,7 @@ struct hostif_adhoc_set2_request_t {
uint16_t reserved;
struct rate_set16_t rate_set;
struct ssid_t ssid;
-   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0 
+   uint16_t capability;/* bit5:preamble bit6:pbcc pbcc not supported 
always 0
 * bit10:ShortSlotTime bit13:DSSS-OFDM 
DSSS-OFDM not supported always 0 */
uint16_t scan_type;
struct channel_list_t channel_list;
@@ -568,19 +568,19 @@ struct hostif_mic_failure_confirm_t {
 #define TX_RATE_48M(uint8_t)(480/5)
 #define TX_RATE_54M(uint8_t)(540/5)
 
-#define IS_11B_RATE(A) 
(((A&RATE_MASK)==TX_RATE_1M)||((A&RATE_MASK)==TX_RATE_2M)||\
-
((A&RATE_MASK)==TX_RATE_5M)||((A&RATE_MASK)==TX_RATE_11M))
+#define IS_11B_RATE(A) (((A&RATE_MASK) == TX_RATE_1M) || ((A&RATE_MASK) == 
TX_RATE_2M) || \
+  ((A&RATE_MASK) == TX_RATE_5M) || ((A&RATE_MASK) == 
TX_RATE_11M))
 
-#define IS_OFDM_RATE(A) 
(((A&RATE_MASK)==TX_RATE_6M)||((A&RATE_MASK)==TX_RATE_12M)||\
-
((A&RATE_MASK)==TX_RATE_24M)||((A&RATE_MASK)==TX_RATE_9M)||\
-
((A&RATE_MASK)==TX_RATE_18M)||((A&RATE_MASK)==TX_RATE_36M)||\
-
((A&RATE_MASK)==TX_RATE_48M)||((A&RATE_MASK)==TX_RATE_54M))
+#define IS_OFDM_RATE(A) (((A&RATE_MASK) == TX_RATE_6M) || ((A&RATE_MASK) == 
TX_RATE_12M) || \

[PATCH 4/8] staging: ks7010: Refactor whitespace

[Tobin C. Harding]

hostif_data_indication is able to be refactored after previous
commit. 

Refactor function in two separate patches to ease review.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks_hostif.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c 
b/drivers/staging/ks7010/ks_hostif.c
index f4beb86..565f051 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -340,13 +340,14 @@ static void check_13th_byte_of_rx_data(struct 
ks_wlan_private *priv)
if (skb) {
memcpy(skb_put(skb, 12), priv->rxp, 12);/* 
8802/FDDI MAC copy */
/* (SNAP+UI..) skip */
-   memcpy(skb_put(skb, rx_ind_size - 12), priv->rxp + 18, 
rx_ind_size - 12);   /* copy after Type */
+
+   /* copy after Type */
+   memcpy(skb_put(skb, rx_ind_size - 12), priv->rxp + 18, 
rx_ind_size - 12);
 
aa1x_hdr = (struct ieee802_1x_hdr *)(priv->rxp + 20);
-   if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY
-   && priv->wpa.rsn_enabled) {
-   eap_key =
-   (struct wpa_eapol_key *)(aa1x_hdr + 1);
+   if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY &&
+   priv->wpa.rsn_enabled) {
+   eap_key = (struct wpa_eapol_key *)(aa1x_hdr + 
1);
atomic_set(&priv->psstatus.snooze_guard, 1);
}
 
@@ -367,19 +368,19 @@ static void check_13th_byte_of_rx_data(struct 
ks_wlan_private *priv)
DPRINTK(3, "NETBEUI/NetBIOS rx_ind_size=%d\n", rx_ind_size);
 
if (skb) {
-   memcpy(skb_put(skb, 12), priv->rxp, 12);/* 
8802/FDDI MAC copy */
+   memcpy(skb_put(skb, 12), priv->rxp, 12); /* 8802/FDDI 
MAC copy */
 
-   temp[0] = (((rx_ind_size - 12) >> 8) & 0xff);   /* 
NETBEUI size add */
+   temp[0] = (((rx_ind_size - 12) >> 8) & 0xff); /* 
NETBEUI size add */
temp[1] = ((rx_ind_size - 12) & 0xff);
memcpy(skb_put(skb, 2), temp, 2);
 
-   memcpy(skb_put(skb, rx_ind_size - 14), priv->rxp + 12, 
rx_ind_size - 14);   /* copy after Type */
+   /* copy after Type */
+   memcpy(skb_put(skb, rx_ind_size - 14), priv->rxp + 12, 
rx_ind_size - 14);
 
aa1x_hdr = (struct ieee802_1x_hdr *)(priv->rxp + 14);
-   if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY
-   && priv->wpa.rsn_enabled) {
-   eap_key =
-   (struct wpa_eapol_key *)(aa1x_hdr + 1);
+   if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY &&
+  priv->wpa.rsn_enabled) {
+   eap_key = (struct wpa_eapol_key *)(aa1x_hdr + 
1);
atomic_set(&priv->psstatus.snooze_guard, 1);
}
 
-- 
2.7.4

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

[PATCH 0/3] Fix checkpatch warnings

[Tobin C. Harding]

Checkpatch emits various warnings. We can fix CONST_STRUCT trivially.
MULTILINE_DEREFERENCE warnings can be fixed but in most cases the fix
introduces line over 80 warnings.

Quote Documentation/process/coding-style.rst: "Statements longer than
80 columns will be broken into sensible chunks, unless exceeding 80
columns significantly increases readability and does not hide 
information"

Exceeding the 80 columns limit helps readabilty when it means we
don't have to break apart dereferences.

Remove line breaks from the middle of struct variable member
dereferences. Introduce new line over 80 checkpatch warnings.

Tobin C. Harding (3):
  staging: comedi: Fix checkpatch CONST_STRUCT
  staging: comedi: Remove level of indentation
  staging: comedi: Fix checkpatch MULTILINE_DEREFERENCE

 drivers/staging/comedi/drivers/addi_apci_3501.c  |  2 +-
 drivers/staging/comedi/drivers/adl_pci9118.c |  3 +--
 drivers/staging/comedi/drivers/cb_pcidas64.c | 29 +---
 drivers/staging/comedi/drivers/dt3000.c  |  3 +--
 drivers/staging/comedi/drivers/jr3_pci.c |  3 +--
 drivers/staging/comedi/drivers/ni_atmio.c|  4 ++--
 drivers/staging/comedi/drivers/ni_labpc_common.c |  3 +--
 drivers/staging/comedi/drivers/ni_mio_common.c   |  3 +--
 drivers/staging/comedi/drivers/rtd520.c  |  3 +--
 drivers/staging/comedi/drivers/s626.c| 11 -
 10 files changed, 24 insertions(+), 40 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] staging: comedi: Fix checkpatch CONST_STRUCT

[Tobin C. Harding]

Checkpatch emits WARNING: struct comedi_lrange should normally be
const.

Add const keyword to definition of struct.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/addi_apci_3501.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/comedi/drivers/addi_apci_3501.c 
b/drivers/staging/comedi/drivers/addi_apci_3501.c
index 57f0f46..1fdc0f8 100644
--- a/drivers/staging/comedi/drivers/addi_apci_3501.c
+++ b/drivers/staging/comedi/drivers/addi_apci_3501.c
@@ -94,7 +94,7 @@ struct apci3501_private {
unsigned char timer_mode;
 };
 
-static struct comedi_lrange apci3501_ao_range = {
+static const struct comedi_lrange apci3501_ao_range = {
2, {
BIP_RANGE(10),
UNI_RANGE(10)
-- 
2.7.4

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

[PATCH 3/3] staging: comedi: Fix checkpatch MULTILINE_DEREFERENCE

[Tobin C. Harding]

Checkpatch emits multiple WARNING: Avoid multiple line dereference.
Removing these warnings will result in line over 80 warnings being
introduced. However,

Documentation/process/coding-style.rst: "Statements longer than 80
columns will be broken into sensible chunks, unless exceeding 80
columns significantly increases readability and does not hide
information"

Exceeding the 80 columns limit helps readabilty when it means we
don't have to break apart dereferences.

Remove line breaks from the middle of struct variable member
dereferences. Introduce new line over 80 checkpatch warnings.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/adl_pci9118.c |  3 +--
 drivers/staging/comedi/drivers/cb_pcidas64.c | 19 ++-
 drivers/staging/comedi/drivers/dt3000.c  |  3 +--
 drivers/staging/comedi/drivers/jr3_pci.c |  3 +--
 drivers/staging/comedi/drivers/ni_atmio.c|  4 ++--
 drivers/staging/comedi/drivers/ni_labpc_common.c |  3 +--
 drivers/staging/comedi/drivers/ni_mio_common.c   |  3 +--
 drivers/staging/comedi/drivers/rtd520.c  |  3 +--
 drivers/staging/comedi/drivers/s626.c| 11 ---
 9 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/comedi/drivers/adl_pci9118.c 
b/drivers/staging/comedi/drivers/adl_pci9118.c
index 86450c0..29ccb0b 100644
--- a/drivers/staging/comedi/drivers/adl_pci9118.c
+++ b/drivers/staging/comedi/drivers/adl_pci9118.c
@@ -1279,8 +1279,7 @@ static int pci9118_ai_cmdtest(struct comedi_device *dev,
} else {
arg = cmd->convert_arg * cmd->chanlist_len;
}
-   err |= comedi_check_trigger_arg_min(&cmd->
-   scan_begin_arg,
+   err |= 
comedi_check_trigger_arg_min(&cmd->scan_begin_arg,
arg);
}
}
diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 352f754..62438cbf 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1500,11 +1500,9 @@ static int alloc_and_init_dma_members(struct 
comedi_device *dev)
if (!ao_cmd_is_supported(board))
continue;
devpriv->ao_buffer[i] =
-   dma_alloc_coherent(&pcidev->dev,
-   DMA_BUFFER_SIZE,
-   &devpriv->
-   ao_buffer_bus_addr[i],
-   GFP_KERNEL);
+   dma_alloc_coherent(&pcidev->dev, DMA_BUFFER_SIZE,
+  &devpriv->ao_buffer_bus_addr[i],
+  GFP_KERNEL);
if (!devpriv->ao_buffer[i])
return -ENOMEM;
}
@@ -2476,18 +2474,13 @@ static int setup_channel_queue(struct comedi_device 
*dev,
for (i = 0; i < cmd->chanlist_len; i++) {
bits = 0;
/* set channel */
-   bits |= adc_chan_bits(CR_CHAN(cmd->
- chanlist[i]));
+   bits |= 
adc_chan_bits(CR_CHAN(cmd->chanlist[i]));
/* set gain */
bits |= ai_range_bits_6xxx(dev,
-  CR_RANGE(cmd->
-   chanlist
-   [i]));
+  
CR_RANGE(cmd->chanlist[i]));
/* set single-ended / differential */
bits |= se_diff_bit_6xxx(dev,
-CR_AREF(cmd->
-chanlist[i]) ==
-AREF_DIFF);
+
CR_AREF(cmd->chanlist[i]) == AREF_DIFF);
if (CR_AREF(cmd->chanlist[i]) == AREF_COMMON)
bits |= ADC_COMMON_BIT;
/* mark end of queue */
diff --git a/drivers/staging/comedi/drivers/dt3000.c 
b/drivers/staging/comedi/drivers/dt3000.c
index 19e0b7b..38b94d4 100644
--- a/drivers/staging/comedi/drivers/dt3000.c
+++ b/drivers/staging/comedi/drivers/dt3000.c
@@ -448,8 +448,7 @@ static int dt3k_ai_cmdtest(struct comedi_device *dev,
 
 

[PATCH 2/3] staging: comedi: Remove level of indentation

[Tobin C. Harding]

For loop contains only an if conditional (and body of if conditional).
Conditional can be inverted and the loop continued if the new
conditional is true without modifying the program logic. This allows
one level of indentation to be removed.

Invert conditional and continue loop if new conditional evaluates to
true. Remove one level of indentation from subsequent loop body.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/cb_pcidas64.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index cb9c269..352f754 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1497,16 +1497,16 @@ static int alloc_and_init_dma_members(struct 
comedi_device *dev)
return -ENOMEM;
}
for (i = 0; i < AO_DMA_RING_COUNT; i++) {
-   if (ao_cmd_is_supported(board)) {
-   devpriv->ao_buffer[i] =
-   dma_alloc_coherent(&pcidev->dev,
-  DMA_BUFFER_SIZE,
-  &devpriv->
-  ao_buffer_bus_addr[i],
-  GFP_KERNEL);
-   if (!devpriv->ao_buffer[i])
-   return -ENOMEM;
-   }
+   if (!ao_cmd_is_supported(board))
+   continue;
+   devpriv->ao_buffer[i] =
+   dma_alloc_coherent(&pcidev->dev,
+   DMA_BUFFER_SIZE,
+   &devpriv->
+   ao_buffer_bus_addr[i],
+   GFP_KERNEL);
+   if (!devpriv->ao_buffer[i])
+   return -ENOMEM;
}
/* allocate dma descriptors */
devpriv->ai_dma_desc =
-- 
2.7.4

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

Re: [PATCH 2/3] staging: comedi: Remove level of indentation

[Tobin C. Harding]

Re-send and include cc's

On Tue, Feb 21, 2017 at 03:55:18PM +, Ian Abbott wrote:
> On 21/02/17 11:18, Tobin C. Harding wrote:
> 
> For comedi patches affecting a single driver, we prefer the driver name to
> be mentioned in the patch subject, like...
> 
> staging: comedi: cb_pcidas64: blah blah
> 
> >For loop contains only an if conditional (and body of if conditional).
> >Conditional can be inverted and the loop continued if the new
> >conditional is true without modifying the program logic. This allows
> >one level of indentation to be removed.
> >
> >Invert conditional and continue loop if new conditional evaluates to
> >true. Remove one level of indentation from subsequent loop body.
> >
> >Signed-off-by: Tobin C. Harding 
> >---
> > drivers/staging/comedi/drivers/cb_pcidas64.c | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
> >b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >index cb9c269..352f754 100644
> >--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> >+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >@@ -1497,16 +1497,16 @@ static int alloc_and_init_dma_members(struct 
> >comedi_device *dev)
> > return -ENOMEM;
> > }
> > for (i = 0; i < AO_DMA_RING_COUNT; i++) {
> >-if (ao_cmd_is_supported(board)) {
> >-devpriv->ao_buffer[i] =
> >-dma_alloc_coherent(&pcidev->dev,
> >-   DMA_BUFFER_SIZE,
> >-   &devpriv->
> >-   ao_buffer_bus_addr[i],
> >-   GFP_KERNEL);
> >-if (!devpriv->ao_buffer[i])
> >-return -ENOMEM;
> >-}
> >+if (!ao_cmd_is_supported(board))
> >+continue;
> >+devpriv->ao_buffer[i] =
> >+dma_alloc_coherent(&pcidev->dev,
> >+DMA_BUFFER_SIZE,
> >+&devpriv->
> >+ao_buffer_bus_addr[i],
> >+GFP_KERNEL);
> 
> Since you are unindenting the code, you can reformat those function
> parameters at the same time to fix the multiline dereference.

Righto, so for comedi/drivers prefer to group multiple checkpatch fixes to one
driver in a patch as apposed to grouping by individual checkpatch warning
but mixing drivers?

> 
> >+if (!devpriv->ao_buffer[i])
> >+return -ENOMEM;
> > }
> > /* allocate dma descriptors */
> > devpriv->ai_dma_desc =
> >
> 
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.E-mail:  )=-
> -=(  Web: http://www.mev.co.uk/  )=-

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

Re: [PATCH 3/3] staging: comedi: Fix checkpatch MULTILINE_DEREFERENCE

[Tobin C. Harding]

On Tue, Feb 21, 2017 at 04:00:40PM +, Ian Abbott wrote:
> On 21/02/17 11:18, Tobin C. Harding wrote:
> >Checkpatch emits multiple WARNING: Avoid multiple line dereference.
> >Removing these warnings will result in line over 80 warnings being
> >introduced. However,
> >
> >Documentation/process/coding-style.rst: "Statements longer than 80
> >columns will be broken into sensible chunks, unless exceeding 80
> >columns significantly increases readability and does not hide
> >information"
> >
> >Exceeding the 80 columns limit helps readabilty when it means we
> >don't have to break apart dereferences.
> >
> >Remove line breaks from the middle of struct variable member
> >dereferences. Introduce new line over 80 checkpatch warnings.
> >
> >Signed-off-by: Tobin C. Harding 
> >---
> > drivers/staging/comedi/drivers/adl_pci9118.c |  3 +--
> > drivers/staging/comedi/drivers/cb_pcidas64.c | 19 ++-
> > drivers/staging/comedi/drivers/dt3000.c  |  3 +--
> > drivers/staging/comedi/drivers/jr3_pci.c |  3 +--
> > drivers/staging/comedi/drivers/ni_atmio.c|  4 ++--
> > drivers/staging/comedi/drivers/ni_labpc_common.c |  3 +--
> > drivers/staging/comedi/drivers/ni_mio_common.c   |  3 +--
> > drivers/staging/comedi/drivers/rtd520.c  |  3 +--
> > drivers/staging/comedi/drivers/s626.c| 11 ---
> > 9 files changed, 18 insertions(+), 34 deletions(-)
> 
> This patch ought to be split up by driver.  Also, we're bound to get
> follow-up patches from people due to lines exceeding 80 columns.

I wast hesitant to send this patch set in when I completed it because
of this issue you mention. Is this series worth pursuing or is it two
steps forward one step back?

If it is worth doing I'm happy to break it up by driver and re-submit.

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

Comedi use of standard types

[Tobin C. Harding]

Comedi drivers make heavy use of standard types i.e unsigned
short. According to Linux Device Drivers standard C data types are not
the same size of all architectures.

Should we be converting comedi/drivers to use kernel types u8, u16 etc

Follow up question. Some comedi/drivers also use unsigned long for
variables that are not memory addresses. If we are to convert standard
types to kernel types how does one know the intended size of these
variables. I see that the register values come from the manual

http://www.ni.com/pdf/manuals/340934b.pdf

Is the choice (size) of type in a struct (eg struct ni_private,
comedi/drivers/ni_stc.h) a kernel design issue or is it a hardware
issue and should I be reading the manual to find it.

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

Re: Comedi use of standard types

[Tobin C. Harding]

On Wed, Feb 22, 2017 at 08:27:37AM +1100, Tobin C. Harding wrote:
> Comedi drivers make heavy use of standard types i.e unsigned
> short. According to Linux Device Drivers standard C data types are not
> the same size of all architectures.
> 
> Should we be converting comedi/drivers to use kernel types u8, u16 etc
> 
> Follow up question. Some comedi/drivers also use unsigned long for
> variables that are not memory addresses. If we are to convert standard
> types to kernel types how does one know the intended size of these
> variables.

Since comedi was written in 1999/2002 I guess we can safely assume
unsigned long was intended to be 32 bits. And considering that
unsigned char, short, and int are use in the same file.

> I see that the register values come from the manual
> 
> http://www.ni.com/pdf/manuals/340934b.pdf
> 
> Is the choice (size) of type in a struct (eg struct ni_private,
> comedi/drivers/ni_stc.h) a kernel design issue or is it a hardware
> issue and should I be reading the manual to find it.
> 
> thanks,
> Tobin.
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[PATCH] staging: comedi: s626: Kernel doc format comments

[Tobin C. Harding]

Checkpatch emits WARNING: Block comments use a trailing */ on a
separate line. Offending comments are commenting variables within
the main data structure of s626 driver. We can move these comments
to kernel doc format with the benefit of clearing the warning and
improving the documentation for the driver.

Remove comments on structure members. Add original comments to the
head of the structure definition in kernel doc format.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/s626.c | 46 ---
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c 
b/drivers/staging/comedi/drivers/s626.c
index 0dd5fe2..311bf45 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -74,26 +74,34 @@ struct s626_buffer_dma {
void *logical_base;
 };
 
+/**
+ * struct s626_private - Working data for s626 driver.
+ * @ai_cmd_running: non-zero if ai_cmd is running.
+ * @ai_sample_timer: time between samples in units of the timer.
+ * @ai_convert_count: conversion counter.
+ * @ai_convert_timer: time between conversion in units of the timer.
+ * @counter_int_enabs: counter interrupt enable mask for MISC2 register.
+ * @adc_items: number of items in ADC poll list.
+ * @rps_buf: DMA buffer used to hold ADC (RPS1) program.
+ * @ana_buf:  DMA buffer used to receive ADC data and hold DAC data.
+ * @dac_wbuf: pointer to logical adrs of DMA buffer used to hold DAC data.
+ * @dacpol: image of DAC polarity register.
+ * @trim_setpoint: images of TrimDAC setpoints.
+ * @i2c_adrs: I2C device address for onboard EEPROM (board rev dependent)
+ */
 struct s626_private {
-   u8 ai_cmd_running;  /* ai_cmd is running */
-   unsigned int ai_sample_timer;   /* time between samples in
-* units of the timer */
-   int ai_convert_count;   /* conversion counter */
-   unsigned int ai_convert_timer;  /* time between conversion in
-* units of the timer */
-   u16 counter_int_enabs;  /* counter interrupt enable mask
-* for MISC2 register */
-   u8 adc_items;   /* number of items in ADC poll list */
-   struct s626_buffer_dma rps_buf; /* DMA buffer used to hold ADC (RPS1)
-* program */
-   struct s626_buffer_dma ana_buf; /* DMA buffer used to receive ADC data
-* and hold DAC data */
-   u32 *dac_wbuf;  /* pointer to logical adrs of DMA buffer
-* used to hold DAC data */
-   u16 dacpol; /* image of DAC polarity register */
-   u8 trim_setpoint[12];   /* images of TrimDAC setpoints */
-   u32 i2c_adrs;   /* I2C device address for onboard EEPROM
-* (board rev dependent) */
+   u8 ai_cmd_running;
+   unsigned int ai_sample_timer;
+   int ai_convert_count;
+   unsigned int ai_convert_timer;
+   u16 counter_int_enabs;
+   u8 adc_items;
+   struct s626_buffer_dma rps_buf;
+   struct s626_buffer_dma ana_buf;
+   u32 *dac_wbuf;
+   u16 dacpol;
+   u8 trim_setpoint[12];
+   u32 i2c_adrs;
 };
 
 /* Counter overflow/index event flag masks for RDMISC2. */
-- 
2.7.4

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

Re: [PATCH 2/3] staging: comedi: Remove level of indentation

[Tobin C. Harding]

On Wed, Feb 22, 2017 at 11:52:49AM +, Ian Abbott wrote:
> On 21/02/17 20:32, Tobin C. Harding wrote:
> >On Tue, Feb 21, 2017 at 03:55:18PM +, Ian Abbott wrote:
> >>On 21/02/17 11:18, Tobin C. Harding wrote:
> >>
> >>For comedi patches affecting a single driver, we prefer the driver name to
> >>be mentioned in the patch subject, like...
> >>
> >>staging: comedi: cb_pcidas64: blah blah
> >>
> >>>For loop contains only an if conditional (and body of if conditional).
> >>>Conditional can be inverted and the loop continued if the new
> >>>conditional is true without modifying the program logic. This allows
> >>>one level of indentation to be removed.
> >>>
> >>>Invert conditional and continue loop if new conditional evaluates to
> >>>true. Remove one level of indentation from subsequent loop body.
> >>>
> >>>Signed-off-by: Tobin C. Harding 
> >>>---
> >>>drivers/staging/comedi/drivers/cb_pcidas64.c | 20 ++--
> >>>1 file changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>>diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
> >>>b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >>>index cb9c269..352f754 100644
> >>>--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> >>>+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >>>@@ -1497,16 +1497,16 @@ static int alloc_and_init_dma_members(struct 
> >>>comedi_device *dev)
> >>>   return -ENOMEM;
> >>>   }
> >>>   for (i = 0; i < AO_DMA_RING_COUNT; i++) {
> >>>-  if (ao_cmd_is_supported(board)) {
> >>>-  devpriv->ao_buffer[i] =
> >>>-  dma_alloc_coherent(&pcidev->dev,
> >>>- DMA_BUFFER_SIZE,
> >>>- &devpriv->
> >>>- ao_buffer_bus_addr[i],
> >>>- GFP_KERNEL);
> >>>-  if (!devpriv->ao_buffer[i])
> >>>-  return -ENOMEM;
> >>>-  }
> >>>+  if (!ao_cmd_is_supported(board))
> >>>+  continue;
> >>>+  devpriv->ao_buffer[i] =
> >>>+  dma_alloc_coherent(&pcidev->dev,
> >>>+  DMA_BUFFER_SIZE,
> >>>+  &devpriv->
> >>>+  ao_buffer_bus_addr[i],
> >>>+  GFP_KERNEL);
> >>
> >>Since you are unindenting the code, you can reformat those function
> >>parameters at the same time to fix the multiline dereference.
> >
> >Righto, so for comedi/drivers prefer to group multiple checkpatch fixes to 
> >one
> >driver in a patch as apposed to grouping by individual checkpatch warning
> >but mixing drivers?
> 
> It's more a case of there being no harm in using up the spare space at the
> end of the lines that you have created to reduce the the number of lines.
> The fact that it fixes another checkpatch warning is a bonus!

Cool, thanks. I'll wait until the merge window closes before
re-submitting this.

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

Re: Comedi use of standard types

[Tobin C. Harding]

On Wed, Feb 22, 2017 at 04:51:05PM +0300, Dan Carpenter wrote:
> On Wed, Feb 22, 2017 at 11:46:07AM +, Ian Abbott wrote:
> > On 21/02/17 21:27, Tobin C. Harding wrote:
> > >Comedi drivers make heavy use of standard types i.e unsigned
> > >short. According to Linux Device Drivers standard C data types are not
> > >the same size of all architectures.
> > >
> > >Should we be converting comedi/drivers to use kernel types u8, u16 etc
> > 
> > Linux kernel C data types have stricter requirements than the C
> > standard allows.  There is common agreement on the widths of 'char',
> > 'short', 'int', and 'long long', a choice of two widths for 'long'.
> > Signed integers are assumed to use 2's complement representation,
> > and null pointers are assumed to have an "all bits zero"
> > representation. Obviously, the "endianness" of the types will be
> > arch-specific.  (I probably missed some stuff.  For example, I'm not
> > sure if the Linux kernel requires a plain 'char' to be signed.  And
> > I don't know if any of this is officially documented anywhere, so
> > count it as my opinion.)

Thanks for clarifying. So, as I understand it now, as far as kernel
code goes;

u8 <=> unsigned char
u16 <=> unsigned short
u32 <=> unsigned int
u64 <=> unsigned long long

(where <=> means 'is equivalent to')

and

unsigned long is arch dependant.

> char can be signed (x86) or unsigned (s390) depending on the arch, yes.

Point noted.

> 
> > >Follow up question. Some comedi/drivers also use unsigned long for
> > >variables that are not memory addresses. If we are to convert standard
> > >types to kernel types how does one know the intended size of these
> > >variables. I see that the register values come from the manual
> > >
> > >http://www.ni.com/pdf/manuals/340934b.pdf
> > >
> > >Is the choice (size) of type in a struct (eg struct ni_private,
> > >comedi/drivers/ni_stc.h) a kernel design issue or is it a hardware
> > >issue and should I be reading the manual to find it.
> > 
> > It's mostly a design issue.  In general, there is no harm in making
> > local variables wider than they need to be up to 'int' size,
> > although the exact-width types such as 'u16' may be useful
> > sometimes.  In general, structure members and array elements should
> > not be excessively wide.  The use of 'long' or 'unsigned long'
> > should be viewed with suspicion unless there is a specific reason
> > for it.
> 
> Long and unsigned long are fairly normal types used everywhere.

One source said unsigned long are used for memory address, with the benefit
over pointers that they cannot be accidentally dereferenced.

> I don't understand really what Tobin is talking about without a specific
> example.

I won't use up more of you time with a specific example, I think I'm
across it now.

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

Re: [PATCH v3] staging: xgifb: function prototype argument should also have an identifier name

[Tobin C. Harding]

Please use the imperative mood in the subject line i.e the short form
of git commit log.

A couple of example Subjects::

Subject: [PATCH 2/5] ext2: improve scalability of bitmap searching
Subject: [PATCH v2 01/27] x86: fix eflags tracking

Awesome work on you patches!

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

[PATCH 1/5] staging: ks7010: invert conditional, reduce indent

[Tobin C. Harding]

Function _ks_wlan_hw_power_save has 5 levels of indentation. Reducing
the amount of indentation may make code clearer to read. Also a number
of other checkpatch warnings may be removed if we first reduce the
level of indentation in this function.

Invert conditional and return from function if new conditional
evaluates to true. Repeat on subsequent conditional. Reduce
indentation without changing the program logic.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 122 ++-
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 6f9f746..1cf8b12 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -176,70 +176,72 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (priv->reg.powermgt == POWMGT_ACTIVE_MODE)
return 0;
 
-   if (priv->reg.operation_mode == MODE_INFRASTRUCTURE &&
-   (priv->connect_status & CONNECT_STATUS_MASK) == CONNECT_STATUS) {
-   if (priv->dev_state == DEVICE_STATE_SLEEP) {
-   switch (atomic_read(&priv->psstatus.status)) {
-   case PS_SNOOZE: /* 4 */
+   if (priv->reg.operation_mode != MODE_INFRASTRUCTURE ||
+   (priv->connect_status & CONNECT_STATUS_MASK) != CONNECT_STATUS)
+   return 0;
+
+   if (priv->dev_state != DEVICE_STATE_SLEEP)
+   return 0;
+
+   switch (atomic_read(&priv->psstatus.status)) {
+   case PS_SNOOZE: /* 4 */
+   break;
+   default:
+   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
+   atomic_read(&priv->psstatus.status),
+   atomic_read(&priv->psstatus.confirm_wait),
+   atomic_read(&priv->psstatus.snooze_guard),
+   cnt_txqbody(priv));
+
+   if (!atomic_read(&priv->psstatus.confirm_wait)
+   && !atomic_read(&priv->psstatus.snooze_guard)
+   && !cnt_txqbody(priv)) {
+   retval =
+   ks7010_sdio_read(priv, INT_PENDING,
+   &rw_data,
+   sizeof(rw_data));
+   if (retval) {
+   DPRINTK(1,
+   " error : INT_PENDING=%02X\n",
+   rw_data);
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+   &priv->ks_wlan_hw.rw_wq, 1);
break;
-   default:
-   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
-   atomic_read(&priv->psstatus.status),
-   
atomic_read(&priv->psstatus.confirm_wait),
-   
atomic_read(&priv->psstatus.snooze_guard),
-   cnt_txqbody(priv));
-
-   if (!atomic_read(&priv->psstatus.confirm_wait)
-   && 
!atomic_read(&priv->psstatus.snooze_guard)
-   && !cnt_txqbody(priv)) {
-   retval =
-   ks7010_sdio_read(priv, INT_PENDING,
-&rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1,
-   " error : 
INT_PENDING=%02X\n",
-   rw_data);
-   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-  
&priv->ks_wlan_hw.rw_wq, 1);
-   break;
-   }
-   if (!rw_data) {
-   rw_data = GCR_B_DOZE;
-   retval =
-   ks7010_sdio_write(priv,
- GCR_B,
- 

[PATCH 3/5] staging: ks7010: move logic operator to end of line

[Tobin C. Harding]

Logic operator (&&) is place at the start of the line. Kernel
standards suggest that logical operators should be placed at the end
of the line.

Move logical operator to the end of the previous line.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 4ce5867..0e07b83 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -193,9 +193,9 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
atomic_read(&priv->psstatus.snooze_guard),
cnt_txqbody(priv));
 
-   if (!atomic_read(&priv->psstatus.confirm_wait)
-   && !atomic_read(&priv->psstatus.snooze_guard)
-   && !cnt_txqbody(priv)) {
+   if (!atomic_read(&priv->psstatus.confirm_wait) &&
+   !atomic_read(&priv->psstatus.snooze_guard) &&
+   !cnt_txqbody(priv)) {
retval =
ks7010_sdio_read(priv, INT_PENDING,
&rw_data,
-- 
2.7.4

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

[PATCH 0/5] staging: ks7010 refactor _ks_wlan_hw_power_save

[Tobin C. Harding]

Function _ks_wlan_hw_power_save has 5 levels of indentation. Reducing
the amount of indentation may make code clearer to read. Also a number
of other checkpatch warnings may be removed if we first reduce the
level of indentation in this function.

There are a couple of split line warnings and various function call
parameters split over many lines due to the deep nesting.

Reduce level of indentation (by 3), fix checkpatch warnings and clean
up function call parameter line placing.

Tobin C. Harding (5):
  staging: ks7010: invert conditional, reduce indent
  staging: ks7010: fix checkpatch MULTILINE_DEREFERENCE
  staging: ks7010: move logic operator to end of line
  staging: ks7010: remove switch statement
  staging: ks7010: refactor function call parameters

 drivers/staging/ks7010/ks7010_sdio.c | 107 +++
 1 file changed, 45 insertions(+), 62 deletions(-)

-- 
2.7.4

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

[PATCH 2/5] staging: ks7010: fix checkpatch MULTILINE_DEREFERENCE

[Tobin C. Harding]

Checkpatch emits WARNING: Avoid multiple line dereference.

Move dereference onto single line.

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

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 1cf8b12..4ce5867 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -227,8 +227,7 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
DPRINTK(4,
"PMG SET!! : GCR_B=%02X\n",
rw_data);
-   atomic_set(&priv->psstatus.
-   status, PS_SNOOZE);
+   atomic_set(&priv->psstatus.status, PS_SNOOZE);
DPRINTK(3,
"psstatus.status=PS_SNOOZE\n");
} else {
@@ -236,8 +235,7 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
&priv->ks_wlan_hw.rw_wq, 1);
}
} else {
-   queue_delayed_work(priv->ks_wlan_hw.
-   ks7010sdio_wq,
+   queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
&priv->ks_wlan_hw.rw_wq,
0);
}
-- 
2.7.4

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

[PATCH 4/5] staging: ks7010: remove switch statement

[Tobin C. Harding]

Switch statement uses one [trivial] case and a default case holding
bulk of code. We can swap the switch statement with an if/return
statement as replacement for the original switch. This can be done
without changing the program logic.

Remove switch statement. Use original switch parameter as conditional
and return if conditional evaluates to true. Reduce level of
indentation. Do not change the program logic.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 89 +---
 1 file changed, 43 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 0e07b83..04130fb 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -183,63 +183,60 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (priv->dev_state != DEVICE_STATE_SLEEP)
return 0;
 
-   switch (atomic_read(&priv->psstatus.status)) {
-   case PS_SNOOZE: /* 4 */
-   break;
-   default:
-   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
-   atomic_read(&priv->psstatus.status),
-   atomic_read(&priv->psstatus.confirm_wait),
-   atomic_read(&priv->psstatus.snooze_guard),
-   cnt_txqbody(priv));
-
-   if (!atomic_read(&priv->psstatus.confirm_wait) &&
-   !atomic_read(&priv->psstatus.snooze_guard) &&
-   !cnt_txqbody(priv)) {
+   if (atomic_read(&priv->psstatus.status) == PS_SNOOZE)
+   return 0;
+
+   DPRINTK(5, 
"\npsstatus.status=%d\npsstatus.confirm_wait=%d\npsstatus.snooze_guard=%d\ncnt_txqbody=%d\n",
+   atomic_read(&priv->psstatus.status),
+   atomic_read(&priv->psstatus.confirm_wait),
+   atomic_read(&priv->psstatus.snooze_guard),
+   cnt_txqbody(priv));
+
+   if (!atomic_read(&priv->psstatus.confirm_wait) &&
+   !atomic_read(&priv->psstatus.snooze_guard) &&
+   !cnt_txqbody(priv)) {
+   retval =
+   ks7010_sdio_read(priv, INT_PENDING,
+   &rw_data,
+   sizeof(rw_data));
+   if (retval) {
+   DPRINTK(1,
+   " error : INT_PENDING=%02X\n",
+   rw_data);
+   queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+   &priv->ks_wlan_hw.rw_wq, 1);
+   return 0;
+   }
+   if (!rw_data) {
+   rw_data = GCR_B_DOZE;
retval =
-   ks7010_sdio_read(priv, INT_PENDING,
+   ks7010_sdio_write(priv,
+   GCR_B,
&rw_data,
sizeof(rw_data));
if (retval) {
DPRINTK(1,
-   " error : INT_PENDING=%02X\n",
+   " error : GCR_B=%02X\n",
rw_data);
-   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
-   break;
-   }
-   if (!rw_data) {
-   rw_data = GCR_B_DOZE;
-   retval =
-   ks7010_sdio_write(priv,
-   GCR_B,
-   &rw_data,
-   sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1,
-   " error : GCR_B=%02X\n",
-   rw_data);
-   queue_delayed_work
-   (priv->ks_wlan_hw.ks7010sdio_wq,
-   
&priv->ks_wlan_hw.rw_wq, 1);
-   break;
-   }
-   DPRINTK(4,
-   "PMG SET!! : GCR_B=%02X\n",
-   rw_

[PATCH 5/5] staging: ks7010: refactor function call parameters

[Tobin C. Harding]

Function call parameters are split over more lines than
necessary. Also assignment statements are split after the '=' sign.
This adds extra lines to the function and may also reduces
readability. 

Refactor function call parameters and reduce the number of lines
used. Put assignment statements onto single line.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 40 
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 04130fb..14580cb 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -195,48 +195,34 @@ static int _ks_wlan_hw_power_save(struct ks_wlan_private 
*priv)
if (!atomic_read(&priv->psstatus.confirm_wait) &&
!atomic_read(&priv->psstatus.snooze_guard) &&
!cnt_txqbody(priv)) {
-   retval =
-   ks7010_sdio_read(priv, INT_PENDING,
-   &rw_data,
-   sizeof(rw_data));
+   retval = ks7010_sdio_read(priv, INT_PENDING, &rw_data,
+ sizeof(rw_data));
if (retval) {
-   DPRINTK(1,
-   " error : INT_PENDING=%02X\n",
-   rw_data);
+   DPRINTK(1, " error : INT_PENDING=%02X\n", rw_data);
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
+  &priv->ks_wlan_hw.rw_wq, 1);
return 0;
}
if (!rw_data) {
rw_data = GCR_B_DOZE;
-   retval =
-   ks7010_sdio_write(priv,
-   GCR_B,
-   &rw_data,
-   sizeof(rw_data));
+   retval = ks7010_sdio_write(priv, GCR_B, &rw_data,
+  sizeof(rw_data));
if (retval) {
-   DPRINTK(1,
-   " error : GCR_B=%02X\n",
-   rw_data);
-   queue_delayed_work
-   (priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
+   DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
+   
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+  &priv->ks_wlan_hw.rw_wq, 1);
return 0;
}
-   DPRINTK(4,
-   "PMG SET!! : GCR_B=%02X\n",
-   rw_data);
+   DPRINTK(4, "PMG SET!! : GCR_B=%02X\n", rw_data);
atomic_set(&priv->psstatus.status, PS_SNOOZE);
-   DPRINTK(3,
-   "psstatus.status=PS_SNOOZE\n");
+   DPRINTK(3, "psstatus.status=PS_SNOOZE\n");
} else {
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq, 1);
+  &priv->ks_wlan_hw.rw_wq, 1);
}
} else {
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
-   &priv->ks_wlan_hw.rw_wq,
-   0);
+  &priv->ks_wlan_hw.rw_wq, 0);
}
 
return 0;
-- 
2.7.4

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

Re: [PATCHv3 1/4] Encasing macros with complex values (ie: base value plus index) with parentheses.

[Tobin C. Harding]

On Sun, Feb 26, 2017 at 06:00:19PM -0800, Matthew Giassa wrote:
> ---
>  drivers/staging/ks7010/ks_wlan_ioctl.h | 64 
> +-
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_wlan_ioctl.h 
> b/drivers/staging/ks7010/ks_wlan_ioctl.h
> index 8e62b10..47c8015 100644
> --- a/drivers/staging/ks7010/ks_wlan_ioctl.h
> +++ b/drivers/staging/ks7010/ks_wlan_ioctl.h
> @@ -15,43 +15,43 @@
>  #include 
>  /* The low order bit identify a SET (0) or a GET (1) ioctl.  */


Patches without a git log won't be able to be merged. Please see
Documentation/process/submitting-patches.rst

thanks,
Tobin.

>  
> -/*   SIOCIWFIRSTPRIV + 0 */
> -/* former KS_WLAN_GET_DRIVER_VERSION SIOCIWFIRSTPRIV + 1 */
> -/*   SIOCIWFIRSTPRIV + 2 */
> -#define KS_WLAN_GET_FIRM_VERSION SIOCIWFIRSTPRIV + 3
> +/*   (SIOCIWFIRSTPRIV + 0) */
> +/* former KS_WLAN_GET_DRIVER_VERSION (SIOCIWFIRSTPRIV + 1) */
> +/*   (SIOCIWFIRSTPRIV + 2) */
> +#define KS_WLAN_GET_FIRM_VERSION (SIOCIWFIRSTPRIV + 3)
>  #ifdef WPS
> -#define KS_WLAN_SET_WPS_ENABLE   SIOCIWFIRSTPRIV + 4
> -#define KS_WLAN_GET_WPS_ENABLE   SIOCIWFIRSTPRIV + 5
> -#define KS_WLAN_SET_WPS_PROBE_REQSIOCIWFIRSTPRIV + 6
> +#define KS_WLAN_SET_WPS_ENABLE   (SIOCIWFIRSTPRIV + 4)
> +#define KS_WLAN_GET_WPS_ENABLE   (SIOCIWFIRSTPRIV + 5)
> +#define KS_WLAN_SET_WPS_PROBE_REQ(SIOCIWFIRSTPRIV + 6)
>  #endif
> -#define KS_WLAN_GET_EEPROM_CKSUM SIOCIWFIRSTPRIV + 7
> -#define KS_WLAN_SET_PREAMBLE SIOCIWFIRSTPRIV + 8
> -#define KS_WLAN_GET_PREAMBLE SIOCIWFIRSTPRIV + 9
> -#define KS_WLAN_SET_POWER_SAVE   SIOCIWFIRSTPRIV + 10
> -#define KS_WLAN_GET_POWER_SAVE   SIOCIWFIRSTPRIV + 11
> -#define KS_WLAN_SET_SCAN_TYPESIOCIWFIRSTPRIV + 12
> -#define KS_WLAN_GET_SCAN_TYPESIOCIWFIRSTPRIV + 13
> -#define KS_WLAN_SET_RX_GAIN  SIOCIWFIRSTPRIV + 14
> -#define KS_WLAN_GET_RX_GAIN  SIOCIWFIRSTPRIV + 15
> -#define KS_WLAN_HOSTTSIOCIWFIRSTPRIV + 16/* 
> unused */
> -//#define KS_WLAN_SET_REGIONSIOCIWFIRSTPRIV + 17
> -#define KS_WLAN_SET_BEACON_LOST  SIOCIWFIRSTPRIV + 18
> -#define KS_WLAN_GET_BEACON_LOST  SIOCIWFIRSTPRIV + 19
> +#define KS_WLAN_GET_EEPROM_CKSUM (SIOCIWFIRSTPRIV + 7)
> +#define KS_WLAN_SET_PREAMBLE (SIOCIWFIRSTPRIV + 8)
> +#define KS_WLAN_GET_PREAMBLE (SIOCIWFIRSTPRIV + 9)
> +#define KS_WLAN_SET_POWER_SAVE   (SIOCIWFIRSTPRIV + 10)
> +#define KS_WLAN_GET_POWER_SAVE   (SIOCIWFIRSTPRIV + 11)
> +#define KS_WLAN_SET_SCAN_TYPE(SIOCIWFIRSTPRIV + 12)
> +#define KS_WLAN_GET_SCAN_TYPE(SIOCIWFIRSTPRIV + 13)
> +#define KS_WLAN_SET_RX_GAIN  (SIOCIWFIRSTPRIV + 14)
> +#define KS_WLAN_GET_RX_GAIN  (SIOCIWFIRSTPRIV + 15)
> +#define KS_WLAN_HOSTT(SIOCIWFIRSTPRIV + 16) /* 
> unused */
> +//#define KS_WLAN_SET_REGION(SIOCIWFIRSTPRIV + 17)
> +#define KS_WLAN_SET_BEACON_LOST  (SIOCIWFIRSTPRIV + 18)
> +#define KS_WLAN_GET_BEACON_LOST  (SIOCIWFIRSTPRIV + 19)
>  
> -#define KS_WLAN_SET_TX_GAIN  SIOCIWFIRSTPRIV + 20
> -#define KS_WLAN_GET_TX_GAIN  SIOCIWFIRSTPRIV + 21
> +#define KS_WLAN_SET_TX_GAIN  (SIOCIWFIRSTPRIV + 20)
> +#define KS_WLAN_GET_TX_GAIN  (SIOCIWFIRSTPRIV + 21)
>  
>  /* for KS7010 */
> -#define KS_WLAN_SET_PHY_TYPE SIOCIWFIRSTPRIV + 22
> -#define KS_WLAN_GET_PHY_TYPE SIOCIWFIRSTPRIV + 23
> -#define KS_WLAN_SET_CTS_MODE SIOCIWFIRSTPRIV + 24
> -#define KS_WLAN_GET_CTS_MODE SIOCIWFIRSTPRIV + 25
> -/*   SIOCIWFIRSTPRIV + 26 */
> -/*   SIOCIWFIRSTPRIV + 27 */
> -#define KS_WLAN_SET_SLEEP_MODE   SIOCIWFIRSTPRIV + 28/* 
> sleep mode */
> -#define KS_WLAN_GET_SLEEP_MODE   SIOCIWFIRSTPRIV + 29/* 
> sleep mode */
> -/*   SIOCIWFIRSTPRIV + 30 */
> -/*   SIOCIWFIRSTPRIV + 31 */
> +#define KS_WLAN_SET_PHY_TYPE (SIOCIWFIRSTPRIV + 22)
> +#define KS_WLAN_GET_PHY_TYPE (SIOCIWFIRSTPRIV + 23)
> +#define KS_WLAN_SET_CTS_MODE (SIOCIWFIRSTPRIV + 24)
> +#define KS_WLAN_GET_CTS_MODE (SIOCIWFIRSTPRIV + 25)
> +/*   (SIOCIWFIRSTPRIV + 26) */
> +/*   (SIOCIWFIRSTPRIV + 27) */
> +#define KS_WLAN_SET_SLEEP_MODE   (SIOCIWFIRSTPRIV + 28) /* sleep 
> mode */
> +#define KS_WLAN_GET_SLEEP_MODE   (SIOCIWFIRSTPRIV + 29) /* sleep 
> mode */
> +/*   (SIOCIWFIRSTPRIV + 30) */
> +/*   (SIOCIWFIR

[PATCH] staging: comedi: db_pcidas64: invert conditional

[Tobin C. Harding]

Checkpatch emits WARNING: Avoid multiple line dereference. It is
possible to reduce the level of indentation by inverting a conditional
and continuing loop if new conditional evaluates to true.

Invert conditional. Continue loop if new conditional evaluates to
true. Reduce subsequent code indentation level by 1. Move multi-line
dereference onto single line.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/cb_pcidas64.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index efbf277..b0ca5c8 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1497,16 +1497,14 @@ static int alloc_and_init_dma_members(struct 
comedi_device *dev)
return -ENOMEM;
}
for (i = 0; i < AO_DMA_RING_COUNT; i++) {
-   if (ao_cmd_is_supported(board)) {
-   devpriv->ao_buffer[i] =
-   dma_alloc_coherent(&pcidev->dev,
-  DMA_BUFFER_SIZE,
-  &devpriv->
-  ao_buffer_bus_addr[i],
-  GFP_KERNEL);
-   if (!devpriv->ao_buffer[i])
-   return -ENOMEM;
-   }
+   if (!ao_cmd_is_supported(board))
+   continue;
+   devpriv->ao_buffer[i] =
+   dma_alloc_coherent(&pcidev->dev, DMA_BUFFER_SIZE,
+   &devpriv->ao_buffer_bus_addr[i],
+   GFP_KERNEL);
+   if (!devpriv->ao_buffer[i])
+   return -ENOMEM;
}
/* allocate dma descriptors */
devpriv->ai_dma_desc =
-- 
2.7.4

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

Re: [PATCH] staging: comedi: db_pcidas64: invert conditional

[Tobin C. Harding]

On Mon, Feb 27, 2017 at 10:46:34AM +, Ian Abbott wrote:
> On 27/02/17 06:36, Tobin C. Harding wrote:
> >Checkpatch emits WARNING: Avoid multiple line dereference. It is
> >possible to reduce the level of indentation by inverting a conditional
> >and continuing loop if new conditional evaluates to true.
> >
> >Invert conditional. Continue loop if new conditional evaluates to
> >true. Reduce subsequent code indentation level by 1. Move multi-line
> >dereference onto single line.
> >
> >Signed-off-by: Tobin C. Harding 
> >---
> > drivers/staging/comedi/drivers/cb_pcidas64.c | 18 --
> > 1 file changed, 8 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
> >b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >index efbf277..b0ca5c8 100644
> >--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> >+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >@@ -1497,16 +1497,14 @@ static int alloc_and_init_dma_members(struct 
> >comedi_device *dev)
> > return -ENOMEM;
> > }
> > for (i = 0; i < AO_DMA_RING_COUNT; i++) {
> >-if (ao_cmd_is_supported(board)) {
> >-devpriv->ao_buffer[i] =
> >-dma_alloc_coherent(&pcidev->dev,
> >-   DMA_BUFFER_SIZE,
> >-   &devpriv->
> >-   ao_buffer_bus_addr[i],
> >-   GFP_KERNEL);
> >-if (!devpriv->ao_buffer[i])
> >-return -ENOMEM;
> >-}
> >+if (!ao_cmd_is_supported(board))
> >+continue;
> >+devpriv->ao_buffer[i] =
> >+dma_alloc_coherent(&pcidev->dev, DMA_BUFFER_SIZE,
> >+&devpriv->ao_buffer_bus_addr[i],
> >+GFP_KERNEL);
> >+if (!devpriv->ao_buffer[i])
> >+return -ENOMEM;
> > }
> > /* allocate dma descriptors */
> > devpriv->ai_dma_desc =
> >
> 
> There is a typo on the patch subject line.  Apart from that, the patch seems
> okay.  Ideally, the function should be rearranged and/or refactored into
> smaller functions, since the 'if (ao_cmd_is_supported(board))' test is
> loop-invariant, but that can be done with other patches.

Oh nice, thanks for the tip. V2 to follow, with the correct driver
name.

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

Re: [Outreachy kernel] Re: [PATCH v2] staging: ks7010: remove code in comments.

[Tobin C. Harding]

On Mon, Feb 27, 2017 at 04:07:27PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 27 Feb 2017, Arushi Singhal wrote:
> 
> >
> >
> > On Mon, Feb 27, 2017 at 8:18 PM, Greg KH  wrote:
> >   On Sun, Feb 26, 2017 at 09:38:20PM +0530, Arushi Singhal wrote:
> >   > Commenting Code Is a Bad Idea.
> >   > Comments are their to explain the code and how the code
> >   achieves its
> >   > goal and as codes in the comments  does not explain what the
> >   code is
> >   > doing so there is no use of commenting them.
> >   > So in this patch codes in the comments are removed.
> >   >
> >   > Signed-off-by: Arushi Singhal
> >   
> >   > ---
> >   >  changes in v2
> >   >  - subject lines is made short.
> >   >
> >   >  drivers/staging/ks7010/ks7010_sdio.c | 4 
> >   >  1 file changed, 4 deletions(-)
> >
> >   This patch does not apply against my staging-testing branch :(
> >
> >
> > why?
> 
> You should figure this out yourself.  Pull Greg's tree again, and try to
> apply your patch.  It's deterministic, so you should have the same
> information that he has.

I have only just worked this out Arushi so I thought I'd share. You
may want to read up on remote tracking branches in git and set up a
branch tracking Greg's staging-testing branch. Then create (checkout
-b) your development branches from that branch.

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

[PATCH 0/2] staging: comedi: cb_pcidas64: refactor function

[Tobin C. Harding]

Function contains code at differing levels of abstraction. This is
shown by the fact that conditional is loop-invariant. Code may be
refactored out into a separate function.

Once refactored into separate function a number of minor code style
fixes may be implemented. Remove multi-line dereferece, fix function
parameter alignment.

This is a re-work of patch (and based of review of);

[PATCH] staging: comedi: db_pcidas64: invert conditional

However the subject was incorrect, hence a *new* patch series.

Tobin C. Harding (2):
  staging: comedi: cb_pcidas64: refactor out function
  staging: comedi: cb_pcidas64: refactor relocated code

 drivers/staging/comedi/drivers/cb_pcidas64.c | 27 +++
 1 file changed, 19 insertions(+), 8 deletions(-)

-- 
2.7.4

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

[PATCH 2/2] staging: comedi: cb_pcidas64: refactor relocated code

[Tobin C. Harding]

Code was moved to a separate function in a previous patch. Code
structure wast maintained to ease review. Code may now be refactored
to ease reading.

Move multi-line dereference onto single line. Give function parameter
more meaningful name. Fix minor alignment issue.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/cb_pcidas64.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 385f007..16d2491 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1480,18 +1480,17 @@ static void init_stc_registers(struct comedi_device 
*dev)
disable_ai_pacing(dev);
 };
 
-static int alloc_dma_member(int i, struct comedi_device *dev)
+static int alloc_dma_member(int member, struct comedi_device *dev)
 {
struct pci_dev *pcidev = comedi_to_pci_dev(dev);
struct pcidas64_private *devpriv = dev->private;
 
-   devpriv->ao_buffer[i] =
+   devpriv->ao_buffer[member] =
dma_alloc_coherent(&pcidev->dev,
-   DMA_BUFFER_SIZE,
-   &devpriv->
-   ao_buffer_bus_addr[i],
-   GFP_KERNEL);
-   if (!devpriv->ao_buffer[i])
+  DMA_BUFFER_SIZE,
+  &devpriv->ao_buffer_bus_addr[member],
+  GFP_KERNEL);
+   if (!devpriv->ao_buffer[member])
return -ENOMEM;
return 0;
 }
-- 
2.7.4

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

[PATCH 1/2] staging: comedi: cb_pcidas64: refactor out function

[Tobin C. Harding]

Function contains code at differing levels of abstraction. This is
shown by the fact that conditional is loop-invariant. Code may be
refactored out into a separate function.

Refactor out loop body to separate function. Do not alter original
code structure of loop body (to ease review). Add local variables to
new function in identical manner to calling function.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/comedi/drivers/cb_pcidas64.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 3b98193..385f007 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1480,12 +1480,29 @@ static void init_stc_registers(struct comedi_device 
*dev)
disable_ai_pacing(dev);
 };
 
+static int alloc_dma_member(int i, struct comedi_device *dev)
+{
+   struct pci_dev *pcidev = comedi_to_pci_dev(dev);
+   struct pcidas64_private *devpriv = dev->private;
+
+   devpriv->ao_buffer[i] =
+   dma_alloc_coherent(&pcidev->dev,
+   DMA_BUFFER_SIZE,
+   &devpriv->
+   ao_buffer_bus_addr[i],
+   GFP_KERNEL);
+   if (!devpriv->ao_buffer[i])
+   return -ENOMEM;
+   return 0;
+}
+
 static int alloc_and_init_dma_members(struct comedi_device *dev)
 {
const struct pcidas64_board *board = dev->board_ptr;
struct pci_dev *pcidev = comedi_to_pci_dev(dev);
struct pcidas64_private *devpriv = dev->private;
int i;
+   int ret;
 
/* allocate pci dma buffers */
for (i = 0; i < ai_dma_ring_count(board); i++) {
@@ -1498,14 +1515,9 @@ static int alloc_and_init_dma_members(struct 
comedi_device *dev)
}
for (i = 0; i < AO_DMA_RING_COUNT; i++) {
if (ao_cmd_is_supported(board)) {
-   devpriv->ao_buffer[i] =
-   dma_alloc_coherent(&pcidev->dev,
-  DMA_BUFFER_SIZE,
-  &devpriv->
-  ao_buffer_bus_addr[i],
-  GFP_KERNEL);
-   if (!devpriv->ao_buffer[i])
-   return -ENOMEM;
+   ret = alloc_dma_member(i, dev);
+   if (ret)
+   return ret; /* -ENOMEM */
}
}
/* allocate dma descriptors */
-- 
2.7.4

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

Re: [PATCH 2/2] staging: comedi: cb_pcidas64: refactor relocated code

[Tobin C. Harding]

On Tue, Feb 28, 2017 at 02:36:25PM +, Ian Abbott wrote:
> On 27/02/17 21:08, Tobin C. Harding wrote:
> >Code was moved to a separate function in a previous patch. Code
> >structure wast maintained to ease review. Code may now be refactored
> >to ease reading.
> >
> >Move multi-line dereference onto single line. Give function parameter
> >more meaningful name. Fix minor alignment issue.
> >
> >Signed-off-by: Tobin C. Harding 
> >---
> > drivers/staging/comedi/drivers/cb_pcidas64.c | 13 ++---
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
> >b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >index 385f007..16d2491 100644
> >--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> >+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >@@ -1480,18 +1480,17 @@ static void init_stc_registers(struct comedi_device 
> >*dev)
> > disable_ai_pacing(dev);
> > };
> >
> >-static int alloc_dma_member(int i, struct comedi_device *dev)
> >+static int alloc_dma_member(int member, struct comedi_device *dev)
> > {
> > struct pci_dev *pcidev = comedi_to_pci_dev(dev);
> > struct pcidas64_private *devpriv = dev->private;
> >
> >-devpriv->ao_buffer[i] =
> >+devpriv->ao_buffer[member] =
> > dma_alloc_coherent(&pcidev->dev,
> >-DMA_BUFFER_SIZE,
> >-&devpriv->
> >-ao_buffer_bus_addr[i],
> >-GFP_KERNEL);
> >-if (!devpriv->ao_buffer[i])
> >+   DMA_BUFFER_SIZE,
> >+   &devpriv->ao_buffer_bus_addr[member],
> >+   GFP_KERNEL);
> >+if (!devpriv->ao_buffer[member])
> > return -ENOMEM;
> > return 0;
> > }
> >
> 
> This could be merged with the other patch.  Also, the parameter 'member' is
> an index into the list of buffers, so you could call it 'index' or 'bufidx'
> or something.

Ok, thanks for the pointers. I'm going to wait until the merge window
closes then implement as suggested.

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

[PATCH 2/3] staging: ks7010: make whitespace changes

[Tobin C. Harding]

Function ks_sdio_interrupt may be refactored after previous patch
reduced the level of indentation in the function.

Refactor function inline with kernel coding style. Make only white
space changes. Do not alter the program logic.

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 45 +++-
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index cb7dc2e..18309ba 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -547,9 +547,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
if (priv->dev_state < DEVICE_STATE_BOOT)
goto intr_out;
 
-   retval =
-   ks7010_sdio_read(priv, INT_PENDING, &status,
-   sizeof(status));
+   retval = ks7010_sdio_read(priv, INT_PENDING, &status, sizeof(status));
if (retval) {
DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
goto intr_out;
@@ -561,21 +559,18 @@ static void ks_sdio_interrupt(struct sdio_func *func)
/* read (General Communication B register) */
/* bit5 -> Write Status Idle */
/* bit2 -> Read Status Busy  */
-   if (status & INT_GCR_B
-   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
-   retval =
-   ks7010_sdio_read(priv, GCR_B, &rw_data,
-   sizeof(rw_data));
+   if (status & INT_GCR_B ||
+   atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
+   retval = ks7010_sdio_read(priv, GCR_B, &rw_data,
+ sizeof(rw_data));
if (retval) {
DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
goto intr_out;
}
/* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
if (rw_data == GCR_B_ACTIVE) {
-   if (atomic_read(&priv->psstatus.status) ==
-   PS_SNOOZE) {
-   atomic_set(&priv->psstatus.status,
-   PS_WAKEUP);
+   if (atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
+   atomic_set(&priv->psstatus.status, PS_WAKEUP);
priv->wakeup_count = 0;
}
complete(&priv->psstatus.wakeup_wait);
@@ -584,30 +579,26 @@ static void ks_sdio_interrupt(struct sdio_func *func)
 
do {
/* read (WriteStatus/ReadDataSize FN1:00_0014) */
-   retval =
-   ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
-   sizeof(rw_data));
+   retval = ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
+ sizeof(rw_data));
if (retval) {
-   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n",
-   rw_data);
+   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n", rw_data);
goto intr_out;
}
DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
rsize = rw_data & RSIZE_MASK;
-   if (rsize) {/* Read schedule */
-   ks_wlan_hw_rx((void *)priv,
-   (uint16_t)(rsize << 4));
-   }
+   if (rsize)  /* Read schedule */
+   ks_wlan_hw_rx((void *)priv, (uint16_t)(rsize << 4));
+
if (rw_data & WSTATUS_MASK) {
 #if 0
-   if (status & INT_WRITE_STATUS
-   && !cnt_txqbody(priv)) {
+   if (status & INT_WRITE_STATUS &&
+   !cnt_txqbody(priv)) {
/* dummy write for interrupt clear */
rw_data = 0;
-   retval =
-   ks7010_sdio_write(priv, DATA_WINDOW,
-   &rw_data,
-   sizeof(rw_data));
+   retval = ks7010_sdio_write(priv, DATA_WINDOW,
+  &rw_data,
+  sizeof(rw_data));
if (retval) {
DPRINTK(1,
"write DATA_WINDOW 
Failed!!(%d)\n",
-- 
2.7.4

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

[PATCH 3/3] staging: ks7010: add additional goto target

[Tobin C. Harding]

Function contains multiple return sites. Function already contains a
goto target that makes a function call before returning, the same
function is called (with different arguments) before returning at an
internal location. It would be clearer if both functions were called
at the same place and labelled for use with goto.

Add additional goto target and remove multiple return sites. As a
bonus clear three checkpatch warnings (line over 80 and multi-line
dereference).

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 18309ba..ff2af9f 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -545,12 +545,12 @@ static void ks_sdio_interrupt(struct sdio_func *func)
DPRINTK(4, "\n");
 
if (priv->dev_state < DEVICE_STATE_BOOT)
-   goto intr_out;
+   goto intr_out_no_delay;
 
retval = ks7010_sdio_read(priv, INT_PENDING, &status, sizeof(status));
if (retval) {
DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
-   goto intr_out;
+   goto intr_out_no_delay;
}
DPRINTK(4, "INT_PENDING=%02X\n", rw_data);
 
@@ -565,7 +565,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
  sizeof(rw_data));
if (retval) {
DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
-   goto intr_out;
+   goto intr_out_no_delay;
}
/* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
if (rw_data == GCR_B_ACTIVE) {
@@ -583,7 +583,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
  sizeof(rw_data));
if (retval) {
DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n", rw_data);
-   goto intr_out;
+   goto intr_out_no_delay;
}
DPRINTK(4, "WSTATUS_RSIZE=%02X\n", rw_data);
rsize = rw_data & RSIZE_MASK;
@@ -610,12 +610,7 @@ static void ks_sdio_interrupt(struct sdio_func *func)
if (atomic_read(&priv->psstatus.status) == 
PS_SNOOZE) {
if (cnt_txqbody(priv)) {
ks_wlan_hw_wakeup_request(priv);
-   queue_delayed_work
-   (priv->ks_wlan_hw.
-   ks7010sdio_wq,
-   
&priv->ks_wlan_hw.
-   rw_wq, 1);
-   return;
+   goto intr_out_with_delay;
}
} else {
tx_device_task((void *)priv);
@@ -626,9 +621,13 @@ static void ks_sdio_interrupt(struct sdio_func *func)
}
} while (rsize);
 
- intr_out:
+ intr_out_no_delay:
queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
   &priv->ks_wlan_hw.rw_wq, 0);
+   return;
+ intr_out_with_delay:
+   queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
+  &priv->ks_wlan_hw.rw_wq, 1);
 }
 
 static int trx_device_init(struct ks_wlan_private *priv)
-- 
2.7.4

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

[PATCH 0/3] clean up ks_sdio_interrupt()

[Tobin C. Harding]

Checkpatch emits various warnings relating to function
ks_sdio_interrupt(). Function is deeply nested.

First reduce the indentation. Next make some whitespace changes,
refactoring in line with kernel coding style. Lastly add an additional
goto target, removing internal return's and clearing three checkpatch
warnings. 

Tobin C. Harding (3):
  staging: ks7010: remove one level of indentation
  staging: ks7010: make whitespace changes
  staging: ks7010: add additional goto target

 drivers/staging/ks7010/ks7010_sdio.c | 153 +--
 1 file changed, 72 insertions(+), 81 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] staging: ks7010: remove one level of indentation

[Tobin C. Harding]

Function ks_sdio_interrupt() is deeply nested. One level of indentation
may be removed by inverting an 'if' statement conditional. Body of
statement makes up the bulk of the function. Goto can be used to
return early from function. It is not necessary to alter the function
logic. Once completed, this will open the way for further refactoring.

Invert conditional. Add goto statement to return from function if new
conditional evaluates true. Reduce level of indentation by one. Do not
alter program logic. 

Signed-off-by: Tobin C. Harding 
---
 drivers/staging/ks7010/ks7010_sdio.c | 157 ++-
 1 file changed, 79 insertions(+), 78 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index 14580cb..cb7dc2e 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -1,4 +1,4 @@
-/*
+r/*
  *   Driver for KeyStream, KS7010 based SDIO cards.
  *
  *   Copyright (C) 2006-2008 KeyStream Corp.
@@ -544,95 +544,96 @@ static void ks_sdio_interrupt(struct sdio_func *func)
priv = card->priv;
DPRINTK(4, "\n");
 
-   if (priv->dev_state >= DEVICE_STATE_BOOT) {
+   if (priv->dev_state < DEVICE_STATE_BOOT)
+   goto intr_out;
+
+   retval =
+   ks7010_sdio_read(priv, INT_PENDING, &status,
+   sizeof(status));
+   if (retval) {
+   DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
+   goto intr_out;
+   }
+   DPRINTK(4, "INT_PENDING=%02X\n", rw_data);
+
+   /* schedule task for interrupt status */
+   /* bit7 -> Write General Communication B register */
+   /* read (General Communication B register) */
+   /* bit5 -> Write Status Idle */
+   /* bit2 -> Read Status Busy  */
+   if (status & INT_GCR_B
+   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
retval =
-   ks7010_sdio_read(priv, INT_PENDING, &status,
-sizeof(status));
+   ks7010_sdio_read(priv, GCR_B, &rw_data,
+   sizeof(rw_data));
if (retval) {
-   DPRINTK(1, "read INT_PENDING Failed!!(%d)\n", retval);
+   DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
goto intr_out;
}
-   DPRINTK(4, "INT_PENDING=%02X\n", rw_data);
-
-   /* schedule task for interrupt status */
-   /* bit7 -> Write General Communication B register */
-   /* read (General Communication B register) */
-   /* bit5 -> Write Status Idle */
-   /* bit2 -> Read Status Busy  */
-   if (status & INT_GCR_B
-   || atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
-   retval =
-   ks7010_sdio_read(priv, GCR_B, &rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1, " error : GCR_B=%02X\n", rw_data);
-   goto intr_out;
-   }
-   /* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
-   if (rw_data == GCR_B_ACTIVE) {
-   if (atomic_read(&priv->psstatus.status) ==
-   PS_SNOOZE) {
-   atomic_set(&priv->psstatus.status,
-  PS_WAKEUP);
-   priv->wakeup_count = 0;
-   }
-   complete(&priv->psstatus.wakeup_wait);
+   /* DPRINTK(1, "GCR_B=%02X\n", rw_data); */
+   if (rw_data == GCR_B_ACTIVE) {
+   if (atomic_read(&priv->psstatus.status) ==
+   PS_SNOOZE) {
+   atomic_set(&priv->psstatus.status,
+   PS_WAKEUP);
+   priv->wakeup_count = 0;
}
+   complete(&priv->psstatus.wakeup_wait);
}
+   }
 
-   do {
-   /* read (WriteStatus/ReadDataSize FN1:00_0014) */
-   retval =
-   ks7010_sdio_read(priv, WSTATUS_RSIZE, &rw_data,
-sizeof(rw_data));
-   if (retval) {
-   DPRINTK(1, " error : WSTATUS_RSIZE=%02X\n",
-   rw_data);
- 

[PATCH] staging: comedi: cb_pcidas64: move loop invariant

[Tobin C. Harding]

Loop invariant is inside the loop so code checks invariant on each
iteration of the loop. Invariant can be moved outside of the loop so
it is only checked once.

Move loop invariant outside of for loop.

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

diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
b/drivers/staging/comedi/drivers/cb_pcidas64.c
index 3b98193..dff0648 100644
--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
@@ -1496,8 +1496,8 @@ static int alloc_and_init_dma_members(struct 
comedi_device *dev)
if (!devpriv->ai_buffer[i])
return -ENOMEM;
}
-   for (i = 0; i < AO_DMA_RING_COUNT; i++) {
-   if (ao_cmd_is_supported(board)) {
+   if (ao_cmd_is_supported(board)) {
+   for (i = 0; i < AO_DMA_RING_COUNT; i++) {
devpriv->ao_buffer[i] =
dma_alloc_coherent(&pcidev->dev,
   DMA_BUFFER_SIZE,
-- 
2.7.4

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

Re: [PATCH 2/2] staging: comedi: cb_pcidas64: refactor relocated code

[Tobin C. Harding]

On Tue, Feb 28, 2017 at 02:36:25PM +, Ian Abbott wrote:
> On 27/02/17 21:08, Tobin C. Harding wrote:
> >Code was moved to a separate function in a previous patch. Code
> >structure wast maintained to ease review. Code may now be refactored
> >to ease reading.
> >
> >Move multi-line dereference onto single line. Give function parameter
> >more meaningful name. Fix minor alignment issue.
> >
> >Signed-off-by: Tobin C. Harding 
> >---
> > drivers/staging/comedi/drivers/cb_pcidas64.c | 13 ++---
> > 1 file changed, 6 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/staging/comedi/drivers/cb_pcidas64.c 
> >b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >index 385f007..16d2491 100644
> >--- a/drivers/staging/comedi/drivers/cb_pcidas64.c
> >+++ b/drivers/staging/comedi/drivers/cb_pcidas64.c
> >@@ -1480,18 +1480,17 @@ static void init_stc_registers(struct comedi_device 
> >*dev)
> > disable_ai_pacing(dev);
> > };
> >
> >-static int alloc_dma_member(int i, struct comedi_device *dev)
> >+static int alloc_dma_member(int member, struct comedi_device *dev)
> > {
> > struct pci_dev *pcidev = comedi_to_pci_dev(dev);
> > struct pcidas64_private *devpriv = dev->private;
> >
> >-devpriv->ao_buffer[i] =
> >+devpriv->ao_buffer[member] =
> > dma_alloc_coherent(&pcidev->dev,
> >-DMA_BUFFER_SIZE,
> >-&devpriv->
> >-ao_buffer_bus_addr[i],
> >-GFP_KERNEL);
> >-if (!devpriv->ao_buffer[i])
> >+   DMA_BUFFER_SIZE,
> >+   &devpriv->ao_buffer_bus_addr[member],
> >+   GFP_KERNEL);
> >+if (!devpriv->ao_buffer[member])
> > return -ENOMEM;
> > return 0;
> > }
> >
> 
> This could be merged with the other patch.  Also, the parameter 'member' is
> an index into the list of buffers, so you could call it 'index' or 'bufidx'
> or something.

When I went to redo this patch I noticed a better, more simple fix. It
does not remove the initial checkpatch warning but it's a fix none the
less. I have sent the patch with subject;

  [PATCH] staging: comedi: cb_pcidas64: move loop invariant

This email is just to link the work and give you some reference.

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