Re: [PATCH 1/2] iio: temperature: Add support for LTC2983

From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: "jic23@kernel.org" <jic23@kernel.org>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"knaack.h@gmx.de" <knaack.h@gmx.de>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"pmeerw@pmeerw.net" <pmeerw@pmeerw.net>
Subject: Re: [PATCH 1/2] iio: temperature: Add support for LTC2983
Date: Mon, 23 Sep 2019 07:17:28 +0000	[thread overview]
Message-ID: <b149a6d42d053106cb7ed9890f127ef950a0e202.camel@analog.com> (raw)
In-Reply-To: <20190921180229.3483d88e@archlinux>

On Sat, 2019-09-21 at 18:02 +0100, Jonathan Cameron wrote:
> 
> On Mon, 16 Sep 2019 09:37:18 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > Hi Jonathan,
> > 
> > Thanks for the review.
> > Comments inline.
> > 
> > Nuno Sá
> > 
> > On Sun, 2019-09-15 at 12:27 +0100, Jonathan Cameron wrote:
> > > On Mon, 9 Sep 2019 16:45:49 +0200
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >   
> > > > The LTC2983 is a Multi-Sensor High Accuracy Digital Temperature
> > > > Measurement System. It measures a wide variety of temperature
> > > > sensors and
> > > > digitally outputs the result, in °C or °F, with 0.1°C accuracy
> > > > and
> > > > 0.001°C resolution. It can measure the temperature of all
> > > > standard
> > > > thermocouples (type B,E,J,K,N,S,R,T), standard 2-,3-,4-wire
> > > > RTDs,
> > > > thermistors and diodes.
> > > > 
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > > Some comments inline.  Main concern is around the interface, rest
> > > is
> > > minor
> > > stuff.
> > > 
> > > Jonathan
> > >   
> > > > ---
> > > >  .../testing/sysfs-bus-iio-temperature-ltc2983 |   43 +
> > > >  MAINTAINERS                                   |    7 +
> > > >  drivers/iio/temperature/Kconfig               |   10 +
> > > >  drivers/iio/temperature/Makefile              |    1 +
> > > >  drivers/iio/temperature/ltc2983.c             | 1327
> > > > +++++++++++++++++
> > > >  5 files changed, 1388 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-
> > > > temperature-ltc2983
> > > >  create mode 100644 drivers/iio/temperature/ltc2983.c
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-
> > > > temperature-
> > > > ltc2983 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-
> > > > ltc2983
> > > > new file mode 100644
> > > > index 000000000000..3ad3440c0986
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-
> > > > ltc2983
> > > > @@ -0,0 +1,43 @@
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/in_tempY_therm
> > > > istor_raw  
> > > For each of these, I presume we know which type of device is
> > > attached
> > > at any time?
> > > Using the channel naming to convey this (and I assume the fact
> > > that
> > > different
> > > conversions need to be done in userspace?) is a bit messy.  If we
> > > need
> > > to convey the channel type, then a separate in_tempY_mode
> > > attribute
> > > may make more
> > > sense.  That would keep this ABI 'closer' to standard. Software
> > > that
> > > just logs
> > > an unprocessed value could just work for example.
> > > 
> > > I'm not sure I've totally understood what is going on here
> > > though.
> > >   
> > So, the `extend_name` does not really bring any functional
> > advantage.
> > It was just an easy way for someone to know which kind of sensor
> > the
> > channel was referring to. In terms of conversions, all the work is
> > done
> > by the part for all the different sensor's and the scale is the
> > same
> > for all of them. So, I can just drop the extended name and use
> > standard
> > ABI if you prefer?
> 
> Please do.  It may make sense to add an additional attribute to
> provide
> the info on the type of sensor, but we don't want to do anything that
> will create new ABI in the basic read path.

There is already a v2 that I've sent last week which drops the
`extend_name`. I guess we would need the type of sensor plus the
channel number. Either way, I guess that can be added later if someone
actually needs it.

> ...
> > > > +
> > > > +static int __maybe_unused ltc2983_suspend(struct device *dev)
> > > > +{
> > > > +	struct ltc2983_data *st =
> > > > spi_get_drvdata(to_spi_device(dev));
> > > > +	int ret;
> > > > +
> > > > +	mutex_lock(&st->lock);
> > > > +	ret = regmap_write(st->regmap, LTC2983_STATUS_REG,
> > > > LTC2983_SLEEP);
> > > > +	st->reset = true;  
> > > 
> > > Naming seems a bit odd. The register field is called sleep, but
> > > we
> > > call
> > > it reset internally?  
> > I agree. Something like `suspend` or `sleep` for the boolean would
> > be
> > ok?
> 
> yes.

Renamed to `sleep`.

> > > > +	mutex_unlock(&st->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static SIMPLE_DEV_PM_OPS(ltc2983_pm_ops, ltc2983_suspend,
> > > > ltc2983_resume);
> > > > +
> > > > +static const struct spi_device_id ltc2983_id_table[] = {
> > > > +	{ "ltc2983" },
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(spi, ltc2983_id_table);
> > > > +
> > > > +static const struct of_device_id ltc2983_of_match[] = {
> > > > +	{ .compatible = "adi,ltc2983" },
> > > > +	{},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, ltc2983_id_table);
> > > > +
> > > > +static struct spi_driver ltc2983_driver = {
> > > > +	.driver = {
> > > > +		.name = "ltc2983",
> > > > +		.of_match_table = ltc2983_of_match,
> > > > +		.pm = &ltc2983_pm_ops,
> > > > +	},
> > > > +	.probe = ltc2983_probe,
> > > > +	.id_table = ltc2983_id_table,
> > > > +};
> > > > +
> > > > +module_spi_driver(ltc2983_driver);
> > > > +
> > > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> > > > +MODULE_DESCRIPTION("Analog Devices LTC2983 SPI Temperature
> > > > sensors");
> > > > +MODULE_LICENSE("GPL");  