* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 20:56 ` [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver Matt Porter
@ 2015-08-03 21:26 ` Peter Meerwald
2015-08-03 23:13 ` Matt Porter
2015-08-06 17:38 ` Matt Porter
2015-08-03 22:39 ` Matt Ranostay
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Peter Meerwald @ 2015-08-03 21:26 UTC (permalink / raw)
To: Matt Porter; +Cc: Linux IIO List, Jonathan Cameron, Linux Kernel Mailing List
On Mon, 3 Aug 2015, Matt Porter wrote:
> Add a driver for the MAX6675 thermocouple converter. This
> device interfaces with K-type thermocouples and provides
> cold-junction compensated temperature readings via a
> SPI interface.
comments below
> Signed-off-by: Matt Porter <mporter@konsulko.com>
> ---
> drivers/iio/temperature/Kconfig | 11 +++
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/max6675.c | 155 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 drivers/iio/temperature/max6675.c
>
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 21feaa4..b73fbf4 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,17 @@
> #
> menu "Temperature sensors"
>
> +config MAX6675
> + tristate "MAX6675 thermocouple converter"
> + depends on SPI
> + help
> + If you say yes here you get support for the Maxim
> + MAX6675 thermocouple converter connected with SPI.
> +
> + This driver can also be built as a module. If so, the module will
> + be called max6675.
> +
extra newline
> +
> config MLX90614
> tristate "MLX90614 contact-less infrared sensor"
> depends on I2C
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 40710a8..d67ef40 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,5 +2,6 @@
> # Makefile for industrial I/O temperature drivers
> #
>
> +obj-$(CONFIG_MAX6675) += max6675.o
> obj-$(CONFIG_MLX90614) += mlx90614.o
> obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/max6675.c b/drivers/iio/temperature/max6675.c
> new file mode 100644
> index 0000000..2d1fda2
> --- /dev/null
> +++ b/drivers/iio/temperature/max6675.c
> @@ -0,0 +1,155 @@
> +/*
> + * max6675.c - MAX6675 thermocouple converter driver
> + *
> + * Copyright (C) 2015 Konsulko Group, Matt Porter <mporter@konsulko.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/iio/iio.h>
> +#include <linux/spi/spi.h>
> +
> +struct max6675_state {
> + struct spi_device *spi;
> +};
> +
> +static const struct iio_chan_spec max6675_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + },
> +};
> +
> +static int max6675_read(struct max6675_state *st, int *val)
> +{
> + int ret;
> +
> + ret = spi_read(st->spi, val, 2);
> + if (ret < 0)
> + return ret;
> +
> + /* Temperature is bits 14..3 */
> + *val = (*val >> 3) & 0xfff;
what about endianness conversion?
use be16_to_cpu()
> +
> + return ret;
> +}
> +
> +static int max6675_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long m)
> +{
> + struct max6675_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (m == IIO_CHAN_INFO_RAW) {
> + *val2 = 0;
no need to set *val2 to 0 when returning VAL_INT
> + ret = max6675_read(st, val);
> + if (ret)
> + return ret;
> + } else if (m == IIO_CHAN_INFO_SCALE) {
> + *val = 250;
> + *val2 = 0;
> + } else
> + return -EINVAL;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info max6675_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &max6675_read_raw,
> +};
> +
> +static int max6675_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max6675_state *st;
> + int ret = 0;
no need to initialize ret
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + spi->mode = SPI_MODE_1;
> + spi->bits_per_word = 16;
> +
> + spi_set_drvdata(spi, indio_dev);
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->channels = max6675_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max6675_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &max6675_info;
> +
> + ret = iio_device_register(indio_dev);
devm_ and drop _remove()
> + if (ret < 0)
> + dev_err(&spi->dev, "unable to register device\n");
> +
> + return ret;
> +}
> +
> +static int max6675_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> + iio_device_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id max6675_acpi_ids[] = {
> + { "MXIM6675", 0 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, max6675_acpi_ids);
> +
> +static const struct of_device_id max6675_dt_ids[] = {
> + { .compatible = "maxim,max6675" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, max6675_dt_ids);
> +
> +static const struct spi_device_id max6675_spi_ids[] = {
> + {"max6675", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
> +
> +static struct spi_driver max6675_driver = {
> + .driver = {
> + .name = "max6675",
> + .owner = THIS_MODULE,
> + .acpi_match_table = ACPI_PTR(max6675_acpi_ids),
> + .of_match_table = of_match_ptr(max6675_dt_ids),
> + },
> + .probe = max6675_probe,
> + .remove = max6675_remove,
> + .id_table = max6675_spi_ids,
> +};
> +module_spi_driver(max6675_driver);
> +
> +MODULE_AUTHOR("Matt Porter <mporter@konsulko.com>");
> +MODULE_DESCRIPTION("MAX6675 thermocouple converter driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:max6675");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 21:26 ` Peter Meerwald
@ 2015-08-03 23:13 ` Matt Porter
2015-08-06 17:38 ` Matt Porter
1 sibling, 0 replies; 21+ messages in thread
From: Matt Porter @ 2015-08-03 23:13 UTC (permalink / raw)
To: Peter Meerwald
Cc: Linux IIO List, Jonathan Cameron, Linux Kernel Mailing List
On Mon, Aug 03, 2015 at 11:26:12PM +0200, Peter Meerwald wrote:
> On Mon, 3 Aug 2015, Matt Porter wrote:
>
> > Add a driver for the MAX6675 thermocouple converter. This
> > device interfaces with K-type thermocouples and provides
> > cold-junction compensated temperature readings via a
> > SPI interface.
>
> comments below
Thanks for the fast turnaround on the review! Will incorporate
the comments into v2 as noted below.
> > Signed-off-by: Matt Porter <mporter@konsulko.com>
> > ---
> > drivers/iio/temperature/Kconfig | 11 +++
> > drivers/iio/temperature/Makefile | 1 +
> > drivers/iio/temperature/max6675.c | 155 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 167 insertions(+)
> > create mode 100644 drivers/iio/temperature/max6675.c
> >
> > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> > index 21feaa4..b73fbf4 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -3,6 +3,17 @@
> > #
> > menu "Temperature sensors"
> >
> > +config MAX6675
> > + tristate "MAX6675 thermocouple converter"
> > + depends on SPI
> > + help
> > + If you say yes here you get support for the Maxim
> > + MAX6675 thermocouple converter connected with SPI.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called max6675.
> > +
>
> extra newline
Ok
>
> > +
> > config MLX90614
> > tristate "MLX90614 contact-less infrared sensor"
> > depends on I2C
> > diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> > index 40710a8..d67ef40 100644
> > --- a/drivers/iio/temperature/Makefile
> > +++ b/drivers/iio/temperature/Makefile
> > @@ -2,5 +2,6 @@
> > # Makefile for industrial I/O temperature drivers
> > #
> >
> > +obj-$(CONFIG_MAX6675) += max6675.o
> > obj-$(CONFIG_MLX90614) += mlx90614.o
> > obj-$(CONFIG_TMP006) += tmp006.o
> > diff --git a/drivers/iio/temperature/max6675.c b/drivers/iio/temperature/max6675.c
> > new file mode 100644
> > index 0000000..2d1fda2
> > --- /dev/null
> > +++ b/drivers/iio/temperature/max6675.c
> > @@ -0,0 +1,155 @@
> > +/*
> > + * max6675.c - MAX6675 thermocouple converter driver
> > + *
> > + * Copyright (C) 2015 Konsulko Group, Matt Porter <mporter@konsulko.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/spi/spi.h>
> > +
> > +struct max6675_state {
> > + struct spi_device *spi;
> > +};
> > +
> > +static const struct iio_chan_spec max6675_channels[] = {
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + },
> > +};
> > +
> > +static int max6675_read(struct max6675_state *st, int *val)
> > +{
> > + int ret;
> > +
> > + ret = spi_read(st->spi, val, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Temperature is bits 14..3 */
> > + *val = (*val >> 3) & 0xfff;
>
> what about endianness conversion?
> use be16_to_cpu()
Ok, will fix.
> > +
> > + return ret;
> > +}
> > +
> > +static int max6675_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val,
> > + int *val2,
> > + long m)
> > +{
> > + struct max6675_state *st = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (m == IIO_CHAN_INFO_RAW) {
> > + *val2 = 0;
>
> no need to set *val2 to 0 when returning VAL_INT
Ok
>
> > + ret = max6675_read(st, val);
> > + if (ret)
> > + return ret;
> > + } else if (m == IIO_CHAN_INFO_SCALE) {
> > + *val = 250;
> > + *val2 = 0;
> > + } else
> > + return -EINVAL;
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static const struct iio_info max6675_info = {
> > + .driver_module = THIS_MODULE,
> > + .read_raw = &max6675_read_raw,
> > +};
> > +
> > +static int max6675_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct max6675_state *st;
> > + int ret = 0;
>
> no need to initialize ret
Ok
>
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + spi->mode = SPI_MODE_1;
> > + spi->bits_per_word = 16;
> > +
> > + spi_set_drvdata(spi, indio_dev);
> > +
> > + indio_dev->dev.parent = &spi->dev;
> > + indio_dev->name = spi_get_device_id(spi)->name;
> > + indio_dev->channels = max6675_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(max6675_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &max6675_info;
> > +
> > + ret = iio_device_register(indio_dev);
>
> devm_ and drop _remove()
Ok
>
> > + if (ret < 0)
> > + dev_err(&spi->dev, "unable to register device\n");
> > +
> > + return ret;
> > +}
> > +
> > +static int max6675_remove(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct acpi_device_id max6675_acpi_ids[] = {
> > + { "MXIM6675", 0 },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, max6675_acpi_ids);
> > +
> > +static const struct of_device_id max6675_dt_ids[] = {
> > + { .compatible = "maxim,max6675" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, max6675_dt_ids);
> > +
> > +static const struct spi_device_id max6675_spi_ids[] = {
> > + {"max6675", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
> > +
> > +static struct spi_driver max6675_driver = {
> > + .driver = {
> > + .name = "max6675",
> > + .owner = THIS_MODULE,
> > + .acpi_match_table = ACPI_PTR(max6675_acpi_ids),
> > + .of_match_table = of_match_ptr(max6675_dt_ids),
> > + },
> > + .probe = max6675_probe,
> > + .remove = max6675_remove,
> > + .id_table = max6675_spi_ids,
> > +};
> > +module_spi_driver(max6675_driver);
> > +
> > +MODULE_AUTHOR("Matt Porter <mporter@konsulko.com>");
> > +MODULE_DESCRIPTION("MAX6675 thermocouple converter driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("spi:max6675");
> >
>
> --
>
> Peter Meerwald
> +43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 21:26 ` Peter Meerwald
2015-08-03 23:13 ` Matt Porter
@ 2015-08-06 17:38 ` Matt Porter
2015-08-08 11:39 ` Jonathan Cameron
1 sibling, 1 reply; 21+ messages in thread
From: Matt Porter @ 2015-08-06 17:38 UTC (permalink / raw)
To: Peter Meerwald
Cc: Linux IIO List, Jonathan Cameron, Linux Kernel Mailing List
On Mon, Aug 03, 2015 at 11:26:12PM +0200, Peter Meerwald wrote:
> On Mon, 3 Aug 2015, Matt Porter wrote:
...
> > +static int max6675_read(struct max6675_state *st, int *val)
> > +{
> > + int ret;
> > +
> > + ret = spi_read(st->spi, val, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Temperature is bits 14..3 */
> > + *val = (*val >> 3) & 0xfff;
>
> what about endianness conversion?
> use be16_to_cpu()
Apologies, I spoke before engaging the brain on my first reply to this
As specified by the SPI subsystem docs, SPI buffers are always stored
in native endian order. There is no need for endianness conversion here.
-Matt
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-06 17:38 ` Matt Porter
@ 2015-08-08 11:39 ` Jonathan Cameron
2015-08-20 0:23 ` Matt Porter
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2015-08-08 11:39 UTC (permalink / raw)
To: Matt Porter, Peter Meerwald; +Cc: Linux IIO List, Linux Kernel Mailing List
On 06/08/15 18:38, Matt Porter wrote:
> On Mon, Aug 03, 2015 at 11:26:12PM +0200, Peter Meerwald wrote:
>> On Mon, 3 Aug 2015, Matt Porter wrote:
>
> ...
>
>>> +static int max6675_read(struct max6675_state *st, int *val)
>>> +{
>>> + int ret;
>>> +
>>> + ret = spi_read(st->spi, val, 2);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* Temperature is bits 14..3 */
>>> + *val = (*val >> 3) & 0xfff;
>>
>> what about endianness conversion?
>> use be16_to_cpu()
>
> Apologies, I spoke before engaging the brain on my first reply to this
> As specified by the SPI subsystem docs, SPI buffers are always stored
> in native endian order. There is no need for endianness conversion here.
First of all, which doc say this?
Secondly how does SPI know the endianness of the sensor which is what
actually matters here? I2C can in theory make these guarantees as there
is an expected byte order on the wire (even if quite a few drivers don't
conform to the spec anyway). No such guarantee can exist for SPI.
Jonathan
>
> -Matt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-08 11:39 ` Jonathan Cameron
@ 2015-08-20 0:23 ` Matt Porter
2015-08-23 15:44 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Matt Porter @ 2015-08-20 0:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Peter Meerwald, Linux IIO List, Linux Kernel Mailing List
On Sat, Aug 08, 2015 at 12:39:40PM +0100, Jonathan Cameron wrote:
> On 06/08/15 18:38, Matt Porter wrote:
> > On Mon, Aug 03, 2015 at 11:26:12PM +0200, Peter Meerwald wrote:
> >> On Mon, 3 Aug 2015, Matt Porter wrote:
> >
> > ...
> >
> >>> +static int max6675_read(struct max6675_state *st, int *val)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + ret = spi_read(st->spi, val, 2);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + /* Temperature is bits 14..3 */
> >>> + *val = (*val >> 3) & 0xfff;
> >>
> >> what about endianness conversion?
> >> use be16_to_cpu()
> >
> > Apologies, I spoke before engaging the brain on my first reply to this
> > As specified by the SPI subsystem docs, SPI buffers are always stored
> > in native endian order. There is no need for endianness conversion here.
> First of all, which doc say this?
> Secondly how does SPI know the endianness of the sensor which is what
> actually matters here? I2C can in theory make these guarantees as there
> is an expected byte order on the wire (even if quite a few drivers don't
> conform to the spec anyway). No such guarantee can exist for SPI.
include/linux/spi/spi.h:
* In-memory data values are always in native CPU byte order, translated
* from the wire byte order (big-endian except with SPI_LSB_FIRST). So
* for example when bits_per_word is sixteen, buffers are 2N bytes long
* (@len = 2N) and hold N sixteen bit words in CPU byte order.
So, as you mention, there's no standardized byte order but it's
controlled with the per transfer flag and big endian by default.
-Matt
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-20 0:23 ` Matt Porter
@ 2015-08-23 15:44 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2015-08-23 15:44 UTC (permalink / raw)
To: Matt Porter; +Cc: Peter Meerwald, Linux IIO List, Linux Kernel Mailing List
On 20/08/15 01:23, Matt Porter wrote:
> On Sat, Aug 08, 2015 at 12:39:40PM +0100, Jonathan Cameron wrote:
>> On 06/08/15 18:38, Matt Porter wrote:
>>> On Mon, Aug 03, 2015 at 11:26:12PM +0200, Peter Meerwald wrote:
>>>> On Mon, 3 Aug 2015, Matt Porter wrote:
>>>
>>> ...
>>>
>>>>> +static int max6675_read(struct max6675_state *st, int *val)
>>>>> +{
>>>>> + int ret;
>>>>> +
>>>>> + ret = spi_read(st->spi, val, 2);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + /* Temperature is bits 14..3 */
>>>>> + *val = (*val >> 3) & 0xfff;
>>>>
>>>> what about endianness conversion?
>>>> use be16_to_cpu()
>>>
>>> Apologies, I spoke before engaging the brain on my first reply to this
>>> As specified by the SPI subsystem docs, SPI buffers are always stored
>>> in native endian order. There is no need for endianness conversion here.
>> First of all, which doc say this?
>> Secondly how does SPI know the endianness of the sensor which is what
>> actually matters here? I2C can in theory make these guarantees as there
>> is an expected byte order on the wire (even if quite a few drivers don't
>> conform to the spec anyway). No such guarantee can exist for SPI.
>
> include/linux/spi/spi.h:
>
> * In-memory data values are always in native CPU byte order, translated
> * from the wire byte order (big-endian except with SPI_LSB_FIRST). So
> * for example when bits_per_word is sixteen, buffers are 2N bytes long
> * (@len = 2N) and hold N sixteen bit words in CPU byte order.
>
> So, as you mention, there's no standardized byte order but it's
> controlled with the per transfer flag and big endian by default.
>
Thanks, I'd never picked up on that before!
Jonathan
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 20:56 ` [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver Matt Porter
2015-08-03 21:26 ` Peter Meerwald
@ 2015-08-03 22:39 ` Matt Ranostay
2015-08-03 23:10 ` Matt Porter
2015-08-04 7:50 ` Daniel Baluta
2015-08-04 9:30 ` Paul Bolle
3 siblings, 1 reply; 21+ messages in thread
From: Matt Ranostay @ 2015-08-03 22:39 UTC (permalink / raw)
To: Matt Porter
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On Mon, Aug 3, 2015 at 1:56 PM, Matt Porter <mporter@konsulko.com> wrote:
> Add a driver for the MAX6675 thermocouple converter. This
> device interfaces with K-type thermocouples and provides
> cold-junction compensated temperature readings via a
> SPI interface.
>
> Signed-off-by: Matt Porter <mporter@konsulko.com>
> ---
> drivers/iio/temperature/Kconfig | 11 +++
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/max6675.c | 155 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 drivers/iio/temperature/max6675.c
>
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 21feaa4..b73fbf4 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,17 @@
> #
> menu "Temperature sensors"
>
> +config MAX6675
> + tristate "MAX6675 thermocouple converter"
> + depends on SPI
> + help
> + If you say yes here you get support for the Maxim
> + MAX6675 thermocouple converter connected with SPI.
> +
> + This driver can also be built as a module. If so, the module will
> + be called max6675.
> +
> +
> config MLX90614
> tristate "MLX90614 contact-less infrared sensor"
> depends on I2C
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 40710a8..d67ef40 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,5 +2,6 @@
> # Makefile for industrial I/O temperature drivers
> #
>
> +obj-$(CONFIG_MAX6675) += max6675.o
> obj-$(CONFIG_MLX90614) += mlx90614.o
> obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/max6675.c b/drivers/iio/temperature/max6675.c
> new file mode 100644
> index 0000000..2d1fda2
> --- /dev/null
> +++ b/drivers/iio/temperature/max6675.c
> @@ -0,0 +1,155 @@
> +/*
> + * max6675.c - MAX6675 thermocouple converter driver
> + *
> + * Copyright (C) 2015 Konsulko Group, Matt Porter <mporter@konsulko.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/iio/iio.h>
> +#include <linux/spi/spi.h>
> +
> +struct max6675_state {
> + struct spi_device *spi;
> +};
> +
> +static const struct iio_chan_spec max6675_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + },
> +};
> +
> +static int max6675_read(struct max6675_state *st, int *val)
> +{
> + int ret;
> +
> + ret = spi_read(st->spi, val, 2);
> + if (ret < 0)
> + return ret;
> +
> + /* Temperature is bits 14..3 */
> + *val = (*val >> 3) & 0xfff;
> +
> + return ret;
> +}
> +
> +static int max6675_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long m)
> +{
> + struct max6675_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (m == IIO_CHAN_INFO_RAW) {
> + *val2 = 0;
> + ret = max6675_read(st, val);
> + if (ret)
> + return ret;
> + } else if (m == IIO_CHAN_INFO_SCALE) {
> + *val = 250;
> + *val2 = 0;
> + } else
> + return -EINVAL;
> +
> + return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info max6675_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &max6675_read_raw,
> +};
> +
> +static int max6675_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max6675_state *st;
> + int ret = 0;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + spi->mode = SPI_MODE_1;
> + spi->bits_per_word = 16;
Have this error or display a warning when it doesn't match the passed
DT binding settings. Otherwise it may get confusing why other SPI
modes and word sizes don't work.
> +
> + spi_set_drvdata(spi, indio_dev);
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->name = spi_get_device_id(spi)->name;
> + indio_dev->channels = max6675_channels;
> + indio_dev->num_channels = ARRAY_SIZE(max6675_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &max6675_info;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + dev_err(&spi->dev, "unable to register device\n");
> +
> + return ret;
> +}
> +
> +static int max6675_remove(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> + iio_device_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct acpi_device_id max6675_acpi_ids[] = {
> + { "MXIM6675", 0 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, max6675_acpi_ids);
> +
> +static const struct of_device_id max6675_dt_ids[] = {
> + { .compatible = "maxim,max6675" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, max6675_dt_ids);
> +
> +static const struct spi_device_id max6675_spi_ids[] = {
> + {"max6675", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
> +
> +static struct spi_driver max6675_driver = {
> + .driver = {
> + .name = "max6675",
> + .owner = THIS_MODULE,
> + .acpi_match_table = ACPI_PTR(max6675_acpi_ids),
> + .of_match_table = of_match_ptr(max6675_dt_ids),
> + },
> + .probe = max6675_probe,
> + .remove = max6675_remove,
> + .id_table = max6675_spi_ids,
> +};
> +module_spi_driver(max6675_driver);
> +
> +MODULE_AUTHOR("Matt Porter <mporter@konsulko.com>");
> +MODULE_DESCRIPTION("MAX6675 thermocouple converter driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:max6675");
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 22:39 ` Matt Ranostay
@ 2015-08-03 23:10 ` Matt Porter
0 siblings, 0 replies; 21+ messages in thread
From: Matt Porter @ 2015-08-03 23:10 UTC (permalink / raw)
To: Matt Ranostay
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On Mon, Aug 03, 2015 at 03:39:00PM -0700, Matt Ranostay Matt Ranostay wrote:
> On Mon, Aug 3, 2015 at 1:56 PM, Matt Porter <mporter@konsulko.com> wrote:
...
> > +static int max6675_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct max6675_state *st;
> > + int ret = 0;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + st = iio_priv(indio_dev);
> > + st->spi = spi;
> > +
> > + spi->mode = SPI_MODE_1;
> > + spi->bits_per_word = 16;
>
> Have this error or display a warning when it doesn't match the passed
> DT binding settings. Otherwise it may get confusing why other SPI
> modes and word sizes don't work.
Ok, good point. The only thing here is that I've specified that spi-cpha
is required in the binding, indicating that Mode 1 will be used. I need
this driver to be instantiated via three methods: ACPI, DT, and "board
file" so for the latter I'm hardcoding in the driver the mode. The
device only works in Mode 1 so this seems sane, it's not configurable.
I don't parse that mode from either DT or ACPI data since it's not
needed. About the only thing I could do is pedantically check for
spi-cpha and if not present complain..but I think it's fine to simply
not parse at all given that we've hardcoded this for the allowed mode.
There's no property for bits_per_word, it's configured on a per-transfer
basis. In this case, this configures the default to do a 16-bit transfer
as required by the device. There is no way to modify this by a client of
this driver.
-Matt
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 20:56 ` [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver Matt Porter
2015-08-03 21:26 ` Peter Meerwald
2015-08-03 22:39 ` Matt Ranostay
@ 2015-08-04 7:50 ` Daniel Baluta
2015-08-04 13:01 ` Matt Porter
2015-08-04 9:30 ` Paul Bolle
3 siblings, 1 reply; 21+ messages in thread
From: Daniel Baluta @ 2015-08-04 7:50 UTC (permalink / raw)
To: Matt Porter
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On Mon, Aug 3, 2015 at 11:56 PM, Matt Porter <mporter@konsulko.com> wrote:
> Add a driver for the MAX6675 thermocouple converter. This
> device interfaces with K-type thermocouples and provides
> cold-junction compensated temperature readings via a
> SPI interface.
./scripts/checkpatch.pl --strict says:
CHECK: braces {} should be used on all arms of this statement
#118: FILE: drivers/iio/temperature/max6675.c:61:
thanks,
Daniel.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-04 7:50 ` Daniel Baluta
@ 2015-08-04 13:01 ` Matt Porter
0 siblings, 0 replies; 21+ messages in thread
From: Matt Porter @ 2015-08-04 13:01 UTC (permalink / raw)
To: Daniel Baluta
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On Tue, Aug 04, 2015 at 10:50:32AM +0300, Daniel Baluta wrote:
> On Mon, Aug 3, 2015 at 11:56 PM, Matt Porter <mporter@konsulko.com> wrote:
> > Add a driver for the MAX6675 thermocouple converter. This
> > device interfaces with K-type thermocouples and provides
> > cold-junction compensated temperature readings via a
> > SPI interface.
>
> ./scripts/checkpatch.pl --strict says:
>
> CHECK: braces {} should be used on all arms of this statement
> #118: FILE: drivers/iio/temperature/max6675.c:61:
Will address in v2, thanks.
-Matt
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-03 20:56 ` [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver Matt Porter
` (2 preceding siblings ...)
2015-08-04 7:50 ` Daniel Baluta
@ 2015-08-04 9:30 ` Paul Bolle
2015-08-04 13:18 ` Matt Porter
3 siblings, 1 reply; 21+ messages in thread
From: Paul Bolle @ 2015-08-04 9:30 UTC (permalink / raw)
To: Matt Porter
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On ma, 2015-08-03 at 16:56 -0400, Matt Porter wrote:
> --- /dev/null
> +++ b/drivers/iio/temperature/max6675.c
> +static const struct spi_device_id max6675_spi_ids[] = {
> + {"max6675", 0},
> + {},
> +};
> +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
> +MODULE_ALIAS("spi:max6675");
For the "spi" alias this is "belt and suspenders":
modinfo ./max6675.ko | grep alias
alias: spi:max6675
alias: acpi*:MXIM6675:*
alias: of:N*T*Cmaxim,max6675*
alias: spi:max6675
I'd drop the MODULE_ALIAS().
(Mark Brown made it quite clear I shouldn't nag people about the origin
of the various strings used in these module aliases. So I won't. But if
you'd volunteer to explain me where "max6675" might come from for the
spi alias that would, at least, satisfy my curiosity.)
Thanks,
Paul Bolle
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/3] iio: temperature: add max6675 thermocouple converter driver
2015-08-04 9:30 ` Paul Bolle
@ 2015-08-04 13:18 ` Matt Porter
0 siblings, 0 replies; 21+ messages in thread
From: Matt Porter @ 2015-08-04 13:18 UTC (permalink / raw)
To: Paul Bolle
Cc: Linux IIO List, Devicetree List, Jonathan Cameron, Rob Herring,
Mark Rutland, Linux Kernel Mailing List
On Tue, Aug 04, 2015 at 11:30:36AM +0200, Paul Bolle wrote:
> On ma, 2015-08-03 at 16:56 -0400, Matt Porter wrote:
> > --- /dev/null
> > +++ b/drivers/iio/temperature/max6675.c
> > +static const struct spi_device_id max6675_spi_ids[] = {
> > + {"max6675", 0},
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(spi, max6675_spi_ids);
>
> > +MODULE_ALIAS("spi:max6675");
>
> For the "spi" alias this is "belt and suspenders":
> modinfo ./max6675.ko | grep alias
> alias: spi:max6675
> alias: acpi*:MXIM6675:*
> alias: of:N*T*Cmaxim,max6675*
> alias: spi:max6675
>
> I'd drop the MODULE_ALIAS().
Ok, given that it generates a redundant alias I'll drop it.
> (Mark Brown made it quite clear I shouldn't nag people about the origin
> of the various strings used in these module aliases. So I won't. But if
> you'd volunteer to explain me where "max6675" might come from for the
> spi alias that would, at least, satisfy my curiosity.)
Sure, one might hotplug in an entire SPI master and this SPI slave device
via any discoverable bus and need the alias to match the module. It's
also typically used in a board file on embedded x86 stuff where overriding
the DSDT is not desirable.
-Matt
^ permalink raw reply [flat|nested] 21+ messages in thread