From: Luis de Bethencourt <luisbg@osg.samsung.com>
To: Jonathan Cameron <jic23@kernel.org>, linux-kernel@vger.kernel.org
Cc: gregkh@linuxfoundation.org, knaack.h@gmx.de, lars@metafoo.de,
ciorneiioana@gmail.com, janani.rvchndrn@gmail.com, afd@ti.com,
linux-iio@vger.kernel.org, devel@driverdev.osuosl.org
Subject: Re: [PATCH] staging: iio: accel: remove impossible condition
Date: Tue, 31 May 2016 21:53:23 +0100 [thread overview]
Message-ID: <574DF9C3.5040405@osg.samsung.com> (raw)
In-Reply-To: <40EDFB63-D09B-4933-8935-9540119FC00C@kernel.org>
On 31/05/16 21:23, Jonathan Cameron wrote:
>
>
> On 31 May 2016 20:47:50 BST, Luis de Bethencourt <luisbg@osg.samsung.com> wrote:
>> val is set to the value of ret right after ret is checked. If ret is
>> not
>> zero it goes to error_ret. So only value ret can have is zero, which
>> makes
>> the switch (val & 0x03) only match the case 0x00. Removing the switch
>> and
>> since val is only used for this, removing val as well.
> There is clearly an issue here. However it looks like it is that if(ret) which is wrong.
>
> The code as it stands clearly doesn't work as intended. Fixing the bug would be
>
> more useful than removing code that 'should' be accessible.
>
> I happen to fire the relevant hardware up yesterday for the first time in a while so
>
> can easily verify the operation of a fix if you want to take another look.
>
> Jonathan
Jonathan and Andrew are right.
sca3000_read_ctrl_reg() returns a negative number on failure.
So line 597 should be: if (ret < 0)
If everything goes well in sca3000_read_ctrl_reg() it returns st->rx[0], which explains
the switch case.
I am going to send a new patch with this fix.
Thanks so much for the review and sorry for the initial confusion,
Luis
prev parent reply other threads:[~2016-05-31 20:53 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-31 19:47 [PATCH] staging: iio: accel: remove impossible condition Luis de Bethencourt
2016-05-31 20:13 ` Andrew F. Davis
2016-05-31 20:23 ` Jonathan Cameron
2016-05-31 20:53 ` Luis de Bethencourt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=574DF9C3.5040405@osg.samsung.com \
--to=luisbg@osg.samsung.com \
--cc=afd@ti.com \
--cc=ciorneiioana@gmail.com \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=janani.rvchndrn@gmail.com \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).