On Thu, 4 Jul 2024 10:09:26 +0200 Petar Stoykov <pd.pstoykov@xxxxxxxxx> wrote: > On Tue, Jul 2, 2024 at 10:16 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > On Tue, 02 Jul 2024 16:59:09 +0200 > > Petar Stoykov via B4 Relay <devnull+pd.pstoykov.gmail.com@xxxxxxxxxx> wrote: > > > > > From: Petar Stoykov <pd.pstoykov@xxxxxxxxx> > > > > > > Sensirion SDP500 is a digital differential pressure sensor. The sensor is > > > accessed over I2C. > > > > > > Signed-off-by: Petar Stoykov <pd.pstoykov@xxxxxxxxx> > > Hi Petar > > > > Given you are going to be doing a v4, a few comments inline on things > > to tidy up in here. > > > > Note that I've already tagged what may be the last pull request from me > > for the 6.11 merge window, so this is probably 6.12 material now anyway. > > Hence plenty of time to tidy up. > > > > Jonathan > > > > Hi Jonathan, > I have no rush, thank you for the explanation! > > > > diff --git a/drivers/iio/pressure/sdp500.c b/drivers/iio/pressure/sdp500.c > > > new file mode 100644 > > > index 000000000000..661c70bc1b5b > > > --- /dev/null > > > +++ b/drivers/iio/pressure/sdp500.c > > > @@ -0,0 +1,153 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Driver for Sensirion sdp500 and sdp510 pressure sensors > > > + * > > > + * Datasheet: https://sensirion.com/resource/datasheet/sdp600 > > > + */ > > > + > > > +#include <linux/i2c.h> > > > +#include <linux/crc8.h> > > > +#include <linux/iio/iio.h> > > > > #include <linux/mod_devicetable.h> appropriate for the id tables. > > I don't understand. Why add an extra header to a working driver? > I will add it and test the driver in the meantime. It's included via another path today (at least on the architectures you've tested with), but we more or less prefer to operate on an include what you use principle. There are exceptions where it is considered very obvious that the headers will always be included but this isn't one of them. (there is no hard list of which ones are included though :( > > > > > > +#include <linux/regulator/consumer.h> > > > +#include <asm/unaligned.h> > > > + > > > +#define SDP500_CRC8_POLYNOMIAL 0x31 // x8 + x5 + x4 + 1 (normalized to 0x31) > > > > For IIO we tend to just use c style comments /* xxx */ > > and not c++ ones. > > I try to follow this rule but once in a while I slip to my old habits. > My teachers all used // for C for whatever reason. Will fix. > > > > > > +#define SDP500_READ_SIZE 3 > > > > > +static int sdp500_read_raw(struct iio_dev *indio_dev, > > > + struct iio_chan_spec const *chan, > > > + int *val, int *val2, long mask) > > > +{ > > > + int ret; > > > + u8 rxbuf[SDP500_READ_SIZE]; > > > + u8 received_crc, calculated_crc; > > > + struct sdp500_data *data = iio_priv(indio_dev); > > > + struct i2c_client *client = to_i2c_client(data->dev); > > > + > > > + switch (mask) { > > > + case IIO_CHAN_INFO_RAW: > > > + ret = i2c_master_recv(client, rxbuf, SDP500_READ_SIZE); > > > + if (ret < 0) { > > > + dev_err(data->dev, "Failed to receive data"); > > > + return ret; > > > + } > > > + if (ret != SDP500_READ_SIZE) { > > > + dev_err(data->dev, "Data is received wrongly"); > > > + return -EIO; > > > + } > > > + > > > + received_crc = rxbuf[2]; > > > + calculated_crc = crc8(sdp500_crc8_table, rxbuf, sizeof(rxbuf) - 1, 0x00); > > A line break in here to keep it under 80 chars would be good (see below). > > I find the 80 chars too constraining and also saw that the limit was updated > to 100 somewhere in kernel.org which made me happy. But.. you seem to > feel strongly enough about this so I'll trim lines to 80. 100 is fine if it helps readability. I'd argue it doesn't significantly help here. Some parts of the kernel are fussier than others, but it's rare you'll get told to increase your wrap length above 80 unless there is a readability advantage. Jonathan
