Tue, May 16, 2023 at 01:32:45PM +0200, Andreas Klinger kirjoitti: > Honeywell mprls0025pa is a series of pressure sensors. > > Add initial I2C support. > > Note: > - IIO buffered mode is supported > - SPI mode is not supported ... > + * Data sheet: Datasheet > + * https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/ > + * products/sensors/pressure-sensors/board-mount-pressure-sensors/ > + * micropressure-mpr-series/documents/ > + * sps-siot-mpr-series-datasheet-32332628-ciid-172626.pdf Is it URL? Can we have it clickable, i.e. unwrapped? ... Missing bits.h > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/math64.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/units.h> ... > +/* > + * support _RAW sysfs interface: Support > + * > + * Calculation formula from the datasheet: > + * pressure = (press_cnt - outputmin) * scale + pmin > + * with: > + * * pressure - measured pressure in Pascal > + * * press_cnt - raw value read from sensor > + * * pmin - minimum pressure range value of sensor (data->pmin) > + * * pmax - maximum pressure range value of sensor (data->pmax) > + * * outputmin - minimum numerical range raw value delivered by sensor > + * (mpr_func_spec.output_min) > + * * outputmax - maximum numerical range raw value delivered by sensor > + * (mpr_func_spec.output_max) > + * * scale - (pmax - pmin) / (outputmax - outputmin) > + * > + * formula of the userspace: > + * pressure = (raw + offset) * scale > + * > + * Values given to the userspace in sysfs interface: > + * * raw - press_cnt > + * * offset - (-1 * outputmin) - pmin / scale > + * note: With all sensors from the datasheet pmin = 0 > + * which reduces the offset to (-1 * outputmin) > + */ ... > +/* > + * transfer function A: 10% to 90% of 2^24 > + * transfer function B: 2.5% to 22.5% of 2^24 > + * transfer function C: 20% to 80% of 2^24 > + */ Kernel doc? > +enum mpr_func_id { > + MPR_FUNCTION_A, > + MPR_FUNCTION_B, > + MPR_FUNCTION_C, > +}; ... > +struct mpr_func_spec { > + u32 output_min; > + u32 output_max; > +}; Can the linear_range.h be used for this? ... > +struct mpr_data { > + struct i2c_client *client; > + struct mutex lock; /* > + * access to device during read > + */ > + u32 pmin; /* minimal pressure in pascal */ > + u32 pmax; /* maximal pressure in pascal */ > + enum mpr_func_id function; /* transfer function */ > + u32 outmin; /* > + * minimal numerical range raw > + * value from sensor > + */ > + u32 outmax; /* > + * maximal numerical range raw > + * value from sensor > + */ > + int scale; /* int part of scale */ > + int scale2; /* nano part of scale */ > + int offset; /* int part of offset */ > + int offset2; /* nano part of offset */ > + struct gpio_desc *gpiod_reset; /* reset */ > + int irq; /* > + * end of conversion irq; > + * used to distinguish between > + * irq mode and reading in a > + * loop until data is ready > + */ > + struct completion completion; /* handshake from irq to read */ > + struct mpr_chan chan; /* > + * channel values for buffered > + * mode > + */ Why not kernel doc? > +}; ... > +static void mpr_reset(struct mpr_data *data) > +{ > + if (data->gpiod_reset) { Actually this dups gpiod_*() checks, so only needed by udelay(). > + gpiod_set_value(data->gpiod_reset, 0); > + udelay(10); > + gpiod_set_value(data->gpiod_reset, 1); Why not sleeping for all of them? > + } > +} ... > + if (ret != sizeof(wdata)) { > + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, > + (u32)sizeof(wdata)); Casting? Use proper specifier, i.e. %zu. > + return -EIO; > + } ... > + /* wait until status indicates data is ready */ > + for (i = 0; i < nloops; i++) { > + /* > + * datasheet only says to wait at least 5 ms for the Datasheet > + * data but leave the maximum response time open > + * --> let's try it nloops (10) times which seems to be > + * quite long > + */ > + usleep_range(5000, 10000); > + status = i2c_smbus_read_byte(data->client); > + if (status < 0) { > + dev_err(dev, > + "error while reading, status: %d\n", > + status); > + return status; > + } > + if (!(status & MPR_I2C_BUSY)) > + break; > + } > + if (i == nloops) { > + dev_err(dev, "timeout while reading\n"); > + return -ETIMEDOUT; > + } iopoll.h provides a macro for this. Reduces codebase a lot in this case. > + } ... > + dev_err(dev, "received size doesn't fit - ret: %d / %u\n", ret, > + (u32)sizeof(buf)); Use proper specifier. ... > + if (buf[0] & MPR_I2C_BUSY) { > + /* > + * it should never be the case that status still indicates > + * business > + */ > + dev_err(dev, "data still not ready: %08x\n", buf[0]); Why 8? Is the buffer is of 32-bit values? > + return -ETIMEDOUT; > + } ... > +err: err_unlock: > + mutex_unlock(&data->lock); > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; ... > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > + return dev_err_probe(dev, -ENOMEM, "couldn't get iio_dev\n"); -ENOMEM shouldn't be without error message, we will get one in that case. ... > + if (dev_fwnode(dev)) { Why not simply use defaults? > + ret = device_property_read_u32(dev, "honeywell,pmin-pascal", > + &data->pmin); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,pmin-pascal could not be read\n"); > + ret = device_property_read_u32(dev, "honeywell,pmax-pascal", > + &data->pmax); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,pmax-pascal could not be read\n"); > + ret = device_property_read_u32(dev, > + "honeywell,transfer-function", &data->function); > + if (ret) > + return dev_err_probe(dev, ret, > + "honeywell,transfer-function could not be read\n"); > + if (data->function > MPR_FUNCTION_C) > + return dev_err_probe(dev, -EINVAL, -ERANGE ? > + "honeywell,transfer-function %d invalid\n", > + data->function); > + } else { > + /* when loaded as i2c device we need to use default values */ > + dev_notice(dev, "firmware node not found; using defaults\n"); > + data->pmin = 0; > + data->pmax = 172369; /* 25 psi */ > + data->function = MPR_FUNCTION_A; > + } ... > + /* > + * multiply with NANO before dividing by scale and later divide by NANO Multiply > + * again. > + */ ... > + if (ret) > + return dev_err_probe(dev, ret, > + "iio triggered buffer setup failed\n"); One line? ... > + if (ret) > + return dev_err_probe(dev, ret, > + "unable to register iio device\n"); Ditto. -- With Best Regards, Andy Shevchenko
