linux-kernel - Re: [PATCH V4 1/1] iio: as6200: add AS6200 temperature sensor driver from ams AG

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]

Date:	Mon, 15 Aug 2016 12:43:27 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Jonathan Cameron <jic23@...nel.org>
Cc:	Florian Lobmaier <Florian.Lobmaier@....com>,
	Peter Meerwald-Stadler <pmeerw@...erw.net>,
	Elitsa Polizoeva <Elitsa.Polizoeva@....com>,
	"knaack.h@....de" <knaack.h@....de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Jean Delvare <jdelvare@...e.de>, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH V4 1/1] iio: as6200: add AS6200 temperature sensor driver
 from ams AG

On Mon, Aug 15, 2016 at 06:25:44PM +0100, Jonathan Cameron wrote:
> On 04/08/16 09:35, Florian Lobmaier wrote:
> > Hello Peter,
> > 
> > Thanks again for your valuable feedback. We use now IIO_EV_THRESH to
> > set high and low limits for temperature. Also removed all the custom
> > ABI as this are mainly settings which will be set one-time only. For
> > the removed custom ABI init defines where introduced which will be
> > written to the registers in the probe function. The remaining custom
> > ABI is now documented as well as the device tree bindings.> 
> > Br,
> > Florian
> > 
> > Signed-off-by: Florian Lobmaier <florian.lobmaier@....com>
> > Signed-off-by: Elitsa Polizoeva <elitsa.polizoeva@....com>
> Please post as a fresh email thread with a clean title. Otherwise
> people will assume it is simply a reply to a comment on an earlier
> version.  Also don't include earlier versions as you have here!
> 
> i.e. drop the RE from the title as it's confusing!
> 
> Anyhow, right back at v1 Peter mentioned that this might be more
> suitable as a hwmon driver than an IIO one.  If you have a good reason
> for supporting this part via IIO you should put it in the patch
> description. I'm afraid I've been more or less offline for the last
> couple fo weeks or I'd have highlighted that this question was
> important. A superficial look suggest to me that this is definitely
> a part targeting hardware monitoring applications.
> 
Conversion time, conversion rate, the presence of limit registers,
and the intended use cases suggest that this should be a hardware
monitoring driver.

Regarding the attributes, most of those would be standard attributes
in a hwmon driver. "alarm polarity" should be a  devicetree / platform
data configuration parameter, and interrupt vs. comparator mode could
be determined based on the presence of an interrupt line.

> I'll only take a border line part with agreement form Guenter / Jean
> who are the hwmon maintainers.
> 
> I'll do a quick review ignoring this question.
> Mostly pretty good though I really don't like the remaining
> custom ABI. Can't see a reason for its existence.

Me not either.

Guenter