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

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  5 Mar 2023 13:36:04 -0800
Andrew Hepp <andrew.hepp@ahepp.dev> wrote:

> Add support for the MCP9600 thermocouple EMF converter.
> 
> Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
> Signed-off-by: Andrew Hepp <andrew.hepp@ahepp.dev>

Hi Andrew,

One minor improvement suggested inline.

If you can test with 
i2c_smbus_read_word_swapped() as suggested (I'm never sure when we need
the swapped form) that would be great.

If we had been later in the cycle I'd have taken this anyway and suggested
that change as a follow up patch, but we have lots of time, so no rush.

Thanks,

Jonathan

> +static int mcp9600_read(struct mcp9600_data *data,
> +			struct iio_chan_spec const *chan, int *val)
> +{
> +	__be16 buf;
> +	int ret;
> +
> +	mutex_lock(&data->read_lock);
> +	ret = i2c_smbus_read_i2c_block_data(data->client, chan->address, 2,
> +					    (u8 *)&buf);

Rare to see this call, so I went looking in the datasheet
https://www.kernel.org/doc/html/v5.5/i2c/smbus-protocol.html gives the structure
of this command as
S Addr Wr [A] Comm [A]
           S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P

which matches the datasheet. However for two bytes it's also the same as...

S Addr Wr [A] Comm [A] S Addr Rd [A] [DataLow] A [DataHigh] NA P
which is the more common

i2c_smbus_read_word_data() which has a defined endian type and which
I think is the wrong one here.

Given that's a common situation we also have
i2c_smbus_read_word_swapped() which is same thing but for data the opposite
way around and will avoid the need for an explicit endian swap.

Jonathan



> +	mutex_unlock(&data->read_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +	*val = be16_to_cpu(buf);
> +
> +	return 0;
> +}
> +