On Sun, 5 Mar 2023 13:36:04 -0800 Andrew Hepp <andrew.hepp@xxxxxxxxx> 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@xxxxxxxxx> 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; > +} > +
