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

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Aug 25, 2016 at 12:37:13PM +0000, Florian Lobmaier wrote:
> Hello Guenter, hello Jonathan,
> 
> we went for developing an IIO driver as the intended use-cases for the AS6200 temperature sensor are biosensors as well as environmental sensors. It is currently designed-in in an Android wearable device to measure the skin temperature. So we did not think hwmon is the right place. Of course it may also be used for such purposes, but due to its tiny size its more intended for wearable devices and smartphones to measure skin- or outside air temperature.
> 
Thermal monitoring looks very much like a hwmon use case to me.

Maybe it would make more sense for you to explain why you think that
the hwmon ABI doesn't fit or meet your needs ?

Thanks,
Guenter

> Regarding the remaining custom ABI you are right. We will move this setting to the device tree. Makes much more sense there ;)
> 
> Maybe you can give an answer regarding the hwmon/iio topic before we submit an update?
> 
> Thank you for your help,
> Florian Lobmaier
> 
> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net] 
> Sent: Montag, 15. August 2016 21:43
> To: Jonathan Cameron <jic23@kernel.org>
> Cc: Florian Lobmaier <Florian.Lobmaier@ams.com>; Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Elitsa Polizoeva <Elitsa.Polizoeva@ams.com>; knaack.h@gmx.de; linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Jean Delvare <jdelvare@suse.de>; linux-hwmon@vger.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@ams.com>
> > > Signed-off-by: Elitsa Polizoeva <elitsa.polizoeva@ams.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