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

[Jonathan Cameron <jic23@kernel.org>]

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.


...
> > > +
> > > +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.

> > > +	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");  
>