linux-kernel - Re: [PATCH] staging: iio: light: isl29018: use regmap for register access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEYUcX3TbXFmKYCvfz00tpJ3K0bSExnQxV1R==dP=+vsK54KPQ@mail.gmail.com>
Date:	Tue, 17 Apr 2012 10:56:37 -0700
From:	Bryan Freed <bfreed@...omium.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	Jonathan Cameron <jic23@....ac.uk>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"max@...o.at" <max@...o.at>,
	"jbrenner@...sinc.com" <jbrenner@...sinc.com>,
	"lars@...afoo.de" <lars@...afoo.de>,
	"grundler@...omium.org" <grundler@...omium.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: iio: light: isl29018: use regmap for register access

Hey Laxman, Jonathan.

The regmap is pretty new to me.  In looking at it, I see
_regmap_write() will regcache_write() before attempting to write to
the device.
This effectively updates the cached value even when the device write
fails, which is the opposite of the current isl29018 cache policy.

It looks like we tried to change the policy back in August and got
this response:
  > I did change one basic behavior that I think was also broken: cache
  > the value regardless of if the transaction completed successfully or
  > not.
  Don't do that.  That means userspace will get an invalid value if it reads
  in the meantime. If you have an error on a hardware bus - tell userspace about
  it and don't 'guess' what is in the register.
See http://driverdev.linuxdriverproject.org/pipermail/devel/2011-August/020256.html
for details.

The result is likely to be that after a failed write to one field of a
register, the next successful write to another field of that register
will update them both.

bryan.

On Tue, Apr 17, 2012 at 4:59 AM, Laxman Dewangan <ldewangan@...dia.com> wrote:
>
> HI Jonathan,
>
> Thanks for fast review.
>
>
> On Tuesday 17 April 2012 05:03 PM, Jonathan Cameron wrote:
>>
>> On 4/17/2012 9:50 AM, Laxman Dewangan wrote:
>>>
>>> Using regmap for accessing register through i2c bus. This will
>>> remove the code for caching registers, read-modify-write logics.
>>> Also it will provide the debugfs feature to dump register
>>> through regmap debugfs.
>>
>> I'd prefer the intial tab fixup for the kconfig file as a separate
>> patch, but other than that
>> all looks good.
>>
>
> Yes, this make sense. I will create fist patch for fixing this and then next
> patch for actual my change.
>
>
>> This will probably cause issues alongside the series I sent to Greg the
>> other day though
>> so you may want to sit on it for a day or two and rebase.
>
>
> I will wait for your change to be in linux-next and then I will create next
> patches.
>
>
>>> Signed-off-by: Laxman Dewangan<ldewangan@...dia.com>
>>
>> Acked-by: Jonathan Cameron<jic23@...nel.org>
>>>
>>> ---
>>>   drivers/staging/iio/light/Kconfig    |   19 ++--
>>>   drivers/staging/iio/light/isl29018.c |  176
>>> +++++++++++++++++-----------------
>>>   2 files changed, 99 insertions(+), 96 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/light/Kconfig
>>> b/drivers/staging/iio/light/Kconfig
>>> index 53b49f7..fd39f72 100644
>>> --- a/drivers/staging/iio/light/Kconfig
>>> +++ b/drivers/staging/iio/light/Kconfig
>>> @@ -4,15 +4,16 @@
>>>   menu "Light sensors"
>>>
>>>   config SENSORS_ISL29018
>>> -        tristate "ISL 29018 light and proximity sensor"
>>> -        depends on I2C
>>> -        default n
>>> -        help
>>> -         If you say yes here you get support for ambient light sensing
>>> and
>>> -         proximity infrared sensing from Intersil ISL29018.
>>> -         This driver will provide the measurements of ambient light
>>> intensity
>>> -         in lux, proximity infrared sensing and normal infrared sensing.
>>> -         Data from sensor is accessible via sysfs.
>>> +     tristate "ISL 29018 light and proximity sensor"
>>> +     depends on I2C
>>> +     select REGMAP_I2C
>>> +     default n
>>> +     help
>>> +      If you say yes here you get support for ambient light sensing and
>>> +      proximity infrared sensing from Intersil ISL29018.
>>> +      This driver will provide the measurements of ambient light
>>> intensity
>>> +      in lux, proximity infrared sensing and normal infrared sensing.
>>> +      Data from sensor is accessible via sysfs.
>>
>> Down to here is a valid but unconnected change.  Can you break this out
>> to a separate patch?
>
>
> Sure, I will do.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/