On Thu, 23 Mar 2023 11:20:55 -0700 Andrew Hepp <andrew.hepp@xxxxxxxxx> wrote: > 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." > Might be worth calling out what EMF stands for as well. Otherwise that is fine. One follow up below. I took another look at the driver and other than the points Lars has raised, this looks good to me now. Thanks, Jonathan > >> > >> Datasheet: > >> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf > >> Signed-off-by: Andrew Hepp <andrew.hepp@xxxxxxxxx> > >> --- > >> [...] > >> 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? A failure to read the DEVICE_ID register at all is worth a print as this is the first time the driver will try to use the bus so if the device isn't there (or isn't responding) it would be good to shout about it. Jonathan
