Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter

[Andrew Hepp <andrew.hepp@ahepp.dev>]

On 3/19/23 11:59 AM, Lars-Peter Clausen wrote:
> This looks really good. I have some small comments, and I apologize for 
> only having them so late in the review cycle.

No worries at all! I really appreciate the time and effort you, 
Jonathan, and Krzysztof have put into reviewing this.

> 
> On 3/19/23 11:47, Andrew Hepp wrote:
>> Add support for the MCP9600 thermocouple EMF converter.
> 
> Would be nice to have a very short description of the capabilities of 
> the sensor in the commit message.
> 

That seems like a good idea! Should the message be about the 
capabilities of the sensor, or the capabilities of the driver? The 
sensor supports a lot of advanced features that the driver currently 
doesn't support.

Currently I'm leaning towards

"Add support for the MCP9600 thermocouple EMF converter. The sensor has 
integrated cold junction compensation and a typical accuracy of 0.5 
degrees Celsius. The driver supports a resolution of 0.0625 degrees 
Celsius."

>>
>> Datasheet: 
>> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
>> Signed-off-by: Andrew Hepp <andrew.hepp@ahepp.dev>
>> ---
>> [...]
>> diff --git a/drivers/iio/temperature/mcp9600.c 
>> b/drivers/iio/temperature/mcp9600.c
>> new file mode 100644
>> index 000000000000..b6d8ffb90c36
>> --- /dev/null
>> +++ b/drivers/iio/temperature/mcp9600.c
>> @@ -0,0 +1,145 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> [...]
>> +static const struct iio_chan_spec mcp9600_channels[] = {
>> +    {
>> +        .type = IIO_TEMP,
>> +        .address = MCP9600_HOT_JUNCTION,
>> +        .info_mask_separate =
>> +            BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +    },
>> +    {
>> +        .type = IIO_TEMP,
>> +        .address = MCP9600_COLD_JUNCTION,
>> +        .channel2 = IIO_MOD_TEMP_AMBIENT,
>> +        .modified = 1,
>> +        .info_mask_separate =
>> +            BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> +    },
>> +    IIO_CHAN_SOFT_TIMESTAMP(2),
> If you do not have supported for buffered capture there is no need to 
> include a timestamp in the channel spec. There is no way to read it 
> without buffered support.

Ack

>> +};
>> +
>> +struct mcp9600_data {
>> +    struct i2c_client *client;
>> +    struct mutex read_lock; /* lock to prevent concurrent reads */
>> +};
>> +
>> +static int mcp9600_read(struct mcp9600_data *data,
>> +            struct iio_chan_spec const *chan, int *val)
>> +{
>> +    __be16 buf;
> buf does not seem to be used.

Oops, sorry about that, I'll make sure to build with warnings as errors 
next submission. I tested the module after changing from 
i2c_smbus_read_block_data but looks like I got a bit ahead of myself 
submitting.

>> +    int ret; >> +
>> +    mutex_lock(&data->read_lock);
> Do you actually need the custom lock? i2c_smbus_read_word_swapped itself 
> should provide locking and there is only a single operation under your 
> custom lock, which will already be atomic.

That seems like a convincing argument to me. It certainly doesn't seem 
like the lock is doing anything, since i2c_smbus_read_word_swapped 
provides locking.

>> +    ret = i2c_smbus_read_word_swapped(data->client, chan->address);
>> +    mutex_unlock(&data->read_lock);
>> +
>> +    if (ret < 0)
>> +        return ret;
>> +    *val = ret;
>> +
>> +    return 0;
>> +}
>> +
>> [...]
>> +static int mcp9600_probe(struct i2c_client *client)
>> +{
>> +    struct iio_dev *indio_dev;
>> +    struct mcp9600_data *data;
>> +    int ret;
>> +
>> +    ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
>> +    if (ret < 0)
>> +        return ret;
> 
> Might as well throw an error message in here for better diagnostics.
> 
>      return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
> 
> 

I think this is how I did it in my original submission, but it sounds 
like the preferred way of doing things is to warn without returning an 
error, in order to support fallback compatibilities?