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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > 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 = <c2983_pm_ops,
> > > > + },
> > > > + .probe = ltc2983_probe,
> > > > + .id_table = ltc2983_id_table,
> > > > +};
> > > > +
> > > > +module_spi_driver(ltc2983_driver);
> > > > +
> > > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@xxxxxxxxxx>");
> > > > +MODULE_DESCRIPTION("Analog Devices LTC2983 SPI Temperature
> > > > sensors");
> > > > +MODULE_LICENSE("GPL");
