| Message ID | 20220816102828.182345-3-george.mois@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ADXL312 and ADXL314 accelerometers | expand |
On 16/08/2022 13:28, George Mois wrote: > ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers > with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g > respectively. > Thank you for your patch. There is something to discuss/improve. > > static const struct spi_device_id adxl313_spi_id[] = { > - { "adxl313" }, > + { "adxl312", ADXL312 }, > + { "adxl313", ADXL313 }, > + { "adxl314", ADXL314 }, > { } > }; > > MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > > static const struct of_device_id adxl313_of_match[] = { > + { .compatible = "adi,adxl312" }, > { .compatible = "adi,adxl313" }, > + { .compatible = "adi,adxl314" }, You miss here driver data. I don't remember which driver matching takes precedence (especially in various cases like DT platforms with device instantiated by MFD), but for consistency I think both device id tables should have same driver data. Best regards, Krzysztof
On Tue, 16 Aug 2022 14:16:13 +0300 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 16/08/2022 13:28, George Mois wrote: > > ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers > > with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g > > respectively. > > > > Thank you for your patch. There is something to discuss/improve. > > > > > static const struct spi_device_id adxl313_spi_id[] = { > > - { "adxl313" }, > > + { "adxl312", ADXL312 }, > > + { "adxl313", ADXL313 }, > > + { "adxl314", ADXL314 }, > > { } > > }; > > > > MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > > > > static const struct of_device_id adxl313_of_match[] = { > > + { .compatible = "adi,adxl312" }, > > { .compatible = "adi,adxl313" }, > > + { .compatible = "adi,adxl314" }, > > You miss here driver data. I don't remember which driver matching takes > precedence (especially in various cases like DT platforms with device > instantiated by MFD), but for consistency I think both device id tables > should have same driver data. You can set it up to try device_get_match_data() first then fallback to the adxl313_spi_id[] table but there isn't a nice 'standard' way to do it. If that isn't done, then IIRC the match is against the compatible with the vendor ID dropped and the table used is the spi_device_id one. Which is just annoyingly complex and relies on the strings matching. In the ideal world the spi_device_id table would go away but there are still a few users (greybus - I think + remaining board files). So for now something like a = device_get_match_data(dev); if (!a) a = &adxl31x_spi_regmap_config[id->data]; Provides a good way of ensuring the id tables don't need to remain in sync. Jonathan > > > Best regards, > Krzysztof
On 16/08/2022 15:44, Jonathan Cameron wrote: >>> >>> MODULE_DEVICE_TABLE(spi, adxl313_spi_id); >>> >>> static const struct of_device_id adxl313_of_match[] = { >>> + { .compatible = "adi,adxl312" }, >>> { .compatible = "adi,adxl313" }, >>> + { .compatible = "adi,adxl314" }, >> >> You miss here driver data. I don't remember which driver matching takes >> precedence (especially in various cases like DT platforms with device >> instantiated by MFD), but for consistency I think both device id tables >> should have same driver data. > > You can set it up to try device_get_match_data() first then fallback > to the adxl313_spi_id[] table but there isn't a nice 'standard' way to > do it. > > If that isn't done, then IIRC the match is against the compatible with > the vendor ID dropped and the table used is the spi_device_id one. > Which is just annoyingly complex and relies on the strings matching. > > In the ideal world the spi_device_id table would go away but there are > still a few users (greybus - I think + remaining board files). > So for now something like > > a = device_get_match_data(dev); > if (!a) > a = &adxl31x_spi_regmap_config[id->data]; > > Provides a good way of ensuring the id tables don't need to remain > in sync. > I guess the only minor issue is that first driver data - ADXL312 - is equal to 0, so above code would make consider ADXL312 as missing data. Best regards, Krzysztof
On Tue, 16 Aug 2022 16:34:59 +0300 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 16/08/2022 15:44, Jonathan Cameron wrote: > >>> > >>> MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > >>> > >>> static const struct of_device_id adxl313_of_match[] = { > >>> + { .compatible = "adi,adxl312" }, > >>> { .compatible = "adi,adxl313" }, > >>> + { .compatible = "adi,adxl314" }, > >> > >> You miss here driver data. I don't remember which driver matching takes > >> precedence (especially in various cases like DT platforms with device > >> instantiated by MFD), but for consistency I think both device id tables > >> should have same driver data. > > > > You can set it up to try device_get_match_data() first then fallback > > to the adxl313_spi_id[] table but there isn't a nice 'standard' way to > > do it. > > > > If that isn't done, then IIRC the match is against the compatible with > > the vendor ID dropped and the table used is the spi_device_id one. > > Which is just annoyingly complex and relies on the strings matching. > > > > In the ideal world the spi_device_id table would go away but there are > > still a few users (greybus - I think + remaining board files). > > So for now something like > > > > a = device_get_match_data(dev); > > if (!a) > > a = &adxl31x_spi_regmap_config[id->data]; > > > > Provides a good way of ensuring the id tables don't need to remain > > in sync. > > > > I guess the only minor issue is that first driver data - ADXL312 - is > equal to 0, so above code would make consider ADXL312 as missing data. Should have given a type to a. struct adxl31x_chip_info *a; It would be a pointer not an enum. Though we might run into some problems with that clang issue of whether array lookups are const (I've not really gotten my head around that yet). Jonathan > > Best regards, > Krzysztof
On Tue, 16 Aug 2022 13:28:28 +0300 George Mois <george.mois@analog.com> wrote: > ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers > with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g > respectively. > > Implement support for ADXL312 and ADXL314 by extending the ADXL313 > driver. > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL312.pdf > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL314.pdf > > Signed-off-by: George Mois <george.mois@analog.com> Hi George, Various comments inline. Thanks, Jonathan > --- > drivers/iio/accel/adxl313.h | 15 ++- > drivers/iio/accel/adxl313_core.c | 164 +++++++++++++++++++++++-------- > drivers/iio/accel/adxl313_spi.c | 40 +++++++- > 3 files changed, 173 insertions(+), 46 deletions(-) > > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h > index 4415f2fc07e1..7428b1f7768f 100644 > --- a/drivers/iio/accel/adxl313.h > +++ b/drivers/iio/accel/adxl313.h > @@ -26,6 +26,7 @@ > #define ADXL313_REG_FIFO_STATUS 0x39 > > #define ADXL313_DEVID0 0xAD > +#define ADXL313_DEVID0_ADXL312_314 0xE5 > #define ADXL313_DEVID1 0x1D > #define ADXL313_PARTID 0xCB > #define ADXL313_SOFT_RESET 0x52 > @@ -37,18 +38,28 @@ > #define ADXL313_MEASUREMENT_MODE BIT(3) > > #define ADXL313_RANGE_MSK GENMASK(1, 0) > -#define ADXL313_RANGE_4G 3 > +#define ADXL313_RANGE_MAX 3 > > #define ADXL313_FULL_RES BIT(3) > #define ADXL313_SPI_3WIRE BIT(6) > #define ADXL313_I2C_DISABLE BIT(6) > > +extern const struct regmap_access_table adxl312_readable_regs_table; > extern const struct regmap_access_table adxl313_readable_regs_table; > +extern const struct regmap_access_table adxl314_readable_regs_table; > > +extern const struct regmap_access_table adxl312_writable_regs_table; > extern const struct regmap_access_table adxl313_writable_regs_table; > +extern const struct regmap_access_table adxl314_writable_regs_table; > + > +enum adxl313_device_type { > + ADXL312, > + ADXL313, > + ADXL314, > +}; > > int adxl313_core_probe(struct device *dev, > struct regmap *regmap, > - const char *name, > + const struct spi_device_id *id, > int (*setup)(struct device *, struct regmap *)); > #endif /* _ADXL313_H_ */ > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c > index afeef779e1d0..eea7d235950e 100644 > --- a/drivers/iio/accel/adxl313_core.c > +++ b/drivers/iio/accel/adxl313_core.c > @@ -11,9 +11,17 @@ > #include <linux/iio/iio.h> > #include <linux/module.h> > #include <linux/regmap.h> > +#include <linux/spi/spi.h> Not in the core driver. This needs to be kept unware of the buses that migh be used. > > #include "adxl313.h" > > +static const struct regmap_range adxl312_readable_reg_range[] = { > + regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0), > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > +}; > + > static const struct regmap_range adxl313_readable_reg_range[] = { > regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_XID), > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > @@ -22,12 +30,32 @@ static const struct regmap_range adxl313_readable_reg_range[] = { > regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > }; > > +const struct regmap_access_table adxl312_readable_regs_table = { > + .yes_ranges = adxl312_readable_reg_range, > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > +}; > +EXPORT_SYMBOL_NS_GPL(adxl312_readable_regs_table, IIO_ADXL312); > + > const struct regmap_access_table adxl313_readable_regs_table = { > .yes_ranges = adxl313_readable_reg_range, > .n_yes_ranges = ARRAY_SIZE(adxl313_readable_reg_range), > }; > EXPORT_SYMBOL_NS_GPL(adxl313_readable_regs_table, IIO_ADXL313); > > +const struct regmap_access_table adxl314_readable_regs_table = { > + .yes_ranges = adxl312_readable_reg_range, > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > +}; > +EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL314); > + > +static const struct regmap_range adxl312_writable_reg_range[] = { > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_INT_MAP), > + regmap_reg_range(ADXL313_REG_DATA_FORMAT, ADXL313_REG_DATA_FORMAT), > + regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > +}; > + > static const struct regmap_range adxl313_writable_reg_range[] = { > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > @@ -37,16 +65,30 @@ static const struct regmap_range adxl313_writable_reg_range[] = { > regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > }; > > +const struct regmap_access_table adxl312_writable_regs_table = { > + .yes_ranges = adxl312_writable_reg_range, > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > +}; > +EXPORT_SYMBOL_NS_GPL(adxl312_writable_regs_table, IIO_ADXL312); > + > const struct regmap_access_table adxl313_writable_regs_table = { > .yes_ranges = adxl313_writable_reg_range, > .n_yes_ranges = ARRAY_SIZE(adxl313_writable_reg_range), > }; > EXPORT_SYMBOL_NS_GPL(adxl313_writable_regs_table, IIO_ADXL313); > > +const struct regmap_access_table adxl314_writable_regs_table = { > + .yes_ranges = adxl312_writable_reg_range, > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > +}; > +EXPORT_SYMBOL_NS_GPL(adxl314_writable_regs_table, IIO_ADXL314); > + > struct adxl313_data { > struct regmap *regmap; > + const struct spi_device_id *id; > + int scale_factor; > struct mutex lock; /* lock to protect transf_buf */ > - __le16 transf_buf __aligned(IIO_DMA_MINALIGN); > + __le16 transf_buf ____cacheline_aligned; Check your patch for accidental reverts of other changes like this... > }; > > static const int adxl313_odr_freqs[][2] = { > @@ -156,12 +198,10 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > *val = sign_extend32(ret, chan->scan_type.realbits - 1); > return IIO_VAL_INT; > case IIO_CHAN_INFO_SCALE: > - /* > - * Scale for any g range is given in datasheet as > - * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 > - */ > *val = 0; > - *val2 = 9576806; > + > + *val2 = data->scale_factor; I'd move this into a structure containing all the chip type specific data. Hence this would be data->chip.scale_factor. > + > return IIO_VAL_INT_PLUS_NANO; > case IIO_CHAN_INFO_CALIBBIAS: > ret = regmap_read(data->regmap, > @@ -170,7 +210,7 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > return ret; > > /* > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > + * 8-bit resolution at minimum range, that is 4x accel data scale > * factor at full resolution > */ > *val = sign_extend32(regval, 7) * 4; > @@ -198,7 +238,7 @@ static int adxl313_write_raw(struct iio_dev *indio_dev, > switch (mask) { > case IIO_CHAN_INFO_CALIBBIAS: > /* > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > + * 8-bit resolution at minimum range, that is 4x accel data scale > * factor at full resolution > */ > if (clamp_val(val, -128 * 4, 127 * 4) != val) > @@ -223,14 +263,17 @@ static const struct iio_info adxl313_info = { > static int adxl313_setup(struct device *dev, struct adxl313_data *data, > int (*setup)(struct device *, struct regmap *)) > { > + enum adxl313_device_type dev_type = data->id->driver_data; > unsigned int regval; > int ret; > > - /* Ensures the device is in a consistent state after start up */ > - ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > - ADXL313_SOFT_RESET); > - if (ret) > - return ret; > + /* If ADXL313, ensures the device is in a consistent state after start up */ > + if (dev_type == ADXL313) { Add a flag to the chip_info structure suggested below for 'soft_reset' and base this decision on that. The aim is to encode all the variations in chips in one place so adding future chips is isolated there. > + ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > + ADXL313_SOFT_RESET); > + if (ret) > + return ret; > + } > > if (setup) { > ret = setup(dev, data->regmap); > @@ -242,41 +285,54 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, > if (ret) > return ret; > > - if (regval != ADXL313_DEVID0) { > + if (dev_type == ADXL313 && regval != ADXL313_DEVID0) { Obviously this predates your changes... Don't fail in this case. Warn only. We may be dealing with a newer compatible device that falls back to an the ADXL313 compatible in dt. It's fine to express we don't know what it is - particularly if ID matches something else, but not to fail as a result. > dev_err(dev, "Invalid manufacturer ID: 0x%02x\n", regval); > return -ENODEV; > } > > - ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); > - if (ret) > - return ret; > + /* If ADXL313, check DEVID1 and PARTID */ > + if (regval == ADXL313_DEVID0) { > + ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); > + if (ret) > + return ret; > > - if (regval != ADXL313_DEVID1) { As above, should not fail - but just give a warning message (or dev_info perhaps). > - dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); > - return -ENODEV; > - } > + if (regval != ADXL313_DEVID1) { > + dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); > + return -ENODEV; > + } > > - ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); > - if (ret) > - return ret; > + ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); > + if (ret) > + return ret; > > - if (regval != ADXL313_PARTID) { > - dev_err(dev, "Invalid device ID: 0x%02x\n", regval); > + if (regval != ADXL313_PARTID) { > + dev_err(dev, "Invalid device ID: 0x%02x\n", regval); > + return -ENODEV; > + } > + } > + > + /* If ADXL312 or ADXL314 device, check DEVID0 */ > + if (dev_type != ADXL313 && regval != ADXL313_DEVID0_ADXL312_314) { > + dev_err(dev, "Invalid manufacturer ID: %#02x\n", regval); > return -ENODEV; > } > > - /* Sets the range to +/- 4g */ > - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > - ADXL313_RANGE_MSK, > - FIELD_PREP(ADXL313_RANGE_MSK, ADXL313_RANGE_4G)); > - if (ret) > - return ret; > + dev_info(dev, "%s detected.\n", data->id->name); Noise in the log. Drop this. It's easy to find out the name from sysfs. > > - /* Enables full resolution */ > - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > - ADXL313_FULL_RES, ADXL313_FULL_RES); > - if (ret) > - return ret; > + /* Sets the range to maximum, full resolution, for ADXL312 and ADXL313 */ > + if (dev_type == ADXL312 || dev_type == ADXL313) { I'm reading this backwards btw. As below, this sort of thing should depend on contents of a chip_info structure. In this case perhaps a flag called 'set max range'). > + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > + ADXL313_RANGE_MSK, > + FIELD_PREP(ADXL313_RANGE_MSK, 0x02)); 0x02 looks like a magic numbers, you should ideally provide a define. > + if (ret) > + return ret; > + > + /* Enables full resolution */ > + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > + ADXL313_FULL_RES, 0); > + if (ret) > + return ret; > + } > > /* Enables measurement mode */ > return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL, > @@ -296,7 +352,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, > */ > int adxl313_core_probe(struct device *dev, > struct regmap *regmap, > - const char *name, > + const struct spi_device_id *id, A core probe function shouldn't be taking an SPI specific structure. Fine to pass the id enum entry. That can then index a chip_info structure that includes the part name - we don't want to pass that through this interface as well. > int (*setup)(struct device *, struct regmap *)) > { > struct adxl313_data *data; > @@ -309,9 +365,35 @@ int adxl313_core_probe(struct device *dev, > > data = iio_priv(indio_dev); > data->regmap = regmap; > + data->id = id; > + > + if (id->driver_data == ADXL312) Whenever you hit a case like this, it almost always means you should be selecting from an array of chip_info structures. One per type of chip. If it is possible to convert code to data that is almost always a good idea. > + /* > + * ADXL312 > + * Scale for any g range (full range) is given in datasheet as > + * 345 LSB/g = 0.0028985507 * 9.80665 = 0.028425072222155 m/s^2 > + */ > + data->scale_factor = 28425072; > + > + if (id->driver_data == ADXL313) > + /* > + * * ADXL313 > + * Scale for any g range is given in datasheet as > + * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 > + */ > + data->scale_factor = 9576806; > + > + if (id->driver_data == ADXL314) > + /* > + * ADXL314 > + * At +/-200g with 13-bit resolution, scale factor is given in datasheet as > + * 48.83 mg/LSB = 0.0488300 * 9.80665 = 0.4788587195 m/s^2. > + */ > + data->scale_factor = 478858719; > + > mutex_init(&data->lock); > > - indio_dev->name = name; > + indio_dev->name = id->name; > indio_dev->info = &adxl313_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = adxl313_channels; > @@ -319,13 +401,13 @@ int adxl313_core_probe(struct device *dev, > > ret = adxl313_setup(dev, data, setup); > if (ret) { > - dev_err(dev, "ADXL313 setup failed\n"); > + dev_err(dev, "Device %s setup failed\n", id->name); I doubt we'll ever care which part it was that failed. I'd just leave that as it waas. > return ret; > } > > return devm_iio_device_register(dev, indio_dev); > } > -EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313); > +EXPORT_SYMBOL_GPL(adxl313_core_probe); Please don't remove the namespacing. Add any new exports to the existing IIO_ADXL313 NS. > > MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); > MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer core driver"); > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c > index a3c6d553462d..19cd096373b0 100644 > --- a/drivers/iio/accel/adxl313_spi.c > +++ b/drivers/iio/accel/adxl313_spi.c > @@ -14,6 +14,16 @@ > > #include "adxl313.h" > > +static const struct regmap_config adxl312_spi_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .rd_table = &adxl312_readable_regs_table, > + .wr_table = &adxl312_writable_regs_table, > + .max_register = 0x39, > + /* Setting bits 7 and 6 enables multiple-byte read */ > + .read_flag_mask = BIT(7) | BIT(6), > +}; > + > static const struct regmap_config adxl313_spi_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > @@ -24,6 +34,22 @@ static const struct regmap_config adxl313_spi_regmap_config = { > .read_flag_mask = BIT(7) | BIT(6), > }; > > +static const struct regmap_config adxl314_spi_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .rd_table = &adxl314_readable_regs_table, > + .wr_table = &adxl314_writable_regs_table, > + .max_register = 0x39, > + /* Setting bits 7 and 6 enables multiple-byte read */ > + .read_flag_mask = BIT(7) | BIT(6), > +}; > + > +static const struct regmap_config adxl31x_spi_regmap_config[] = { > + adxl312_spi_regmap_config, > + adxl313_spi_regmap_config, > + adxl314_spi_regmap_config > +}; > + > static int adxl313_spi_setup(struct device *dev, struct regmap *regmap) > { > struct spi_device *spi = container_of(dev, struct spi_device, dev); > @@ -51,26 +77,32 @@ static int adxl313_spi_probe(struct spi_device *spi) > if (ret) > return ret; > > - regmap = devm_regmap_init_spi(spi, &adxl313_spi_regmap_config); > + regmap = devm_regmap_init_spi(spi, > + &adxl31x_spi_regmap_config[id->driver_data]); > + > if (IS_ERR(regmap)) { > dev_err(&spi->dev, "Error initializing spi regmap: %ld\n", > PTR_ERR(regmap)); > return PTR_ERR(regmap); > } > > - return adxl313_core_probe(&spi->dev, regmap, id->name, > + return adxl313_core_probe(&spi->dev, regmap, id, > &adxl313_spi_setup); > } > > static const struct spi_device_id adxl313_spi_id[] = { > - { "adxl313" }, > + { "adxl312", ADXL312 }, > + { "adxl313", ADXL313 }, > + { "adxl314", ADXL314 }, > { } > }; > > MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > > static const struct of_device_id adxl313_of_match[] = { > + { .compatible = "adi,adxl312" }, > { .compatible = "adi,adxl313" }, > + { .compatible = "adi,adxl314" }, > { } As mentioned in other thread, there is a nicer way of handling the different types of firmware ID. > }; > > @@ -90,4 +122,6 @@ module_spi_driver(adxl313_spi_driver); > MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); > MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer SPI driver"); > MODULE_LICENSE("GPL v2"); > +MODULE_IMPORT_NS(IIO_ADXL312); Take a look at what this is doing. There is only one namespace for the driver, not one per chip type. > MODULE_IMPORT_NS(IIO_ADXL313); > +MODULE_IMPORT_NS(IIO_ADXL314);
Hi Jonathan, Thank you for your review! Regards, George > > On Tue, 16 Aug 2022 13:28:28 +0300 > George Mois <george.mois@analog.com> wrote: > > > ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers > > with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g > > respectively. > > > > Implement support for ADXL312 and ADXL314 by extending the ADXL313 > > driver. > > > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL312.pdf > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL314.pdf > > > > Signed-off-by: George Mois <george.mois@analog.com> > > Hi George, > > Various comments inline. > > Thanks, > > Jonathan > > --- > > drivers/iio/accel/adxl313.h | 15 ++- > > drivers/iio/accel/adxl313_core.c | 164 +++++++++++++++++++++++-------- > > drivers/iio/accel/adxl313_spi.c | 40 +++++++- > > 3 files changed, 173 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h > > index 4415f2fc07e1..7428b1f7768f 100644 > > --- a/drivers/iio/accel/adxl313.h > > +++ b/drivers/iio/accel/adxl313.h > > @@ -26,6 +26,7 @@ > > #define ADXL313_REG_FIFO_STATUS 0x39 > > > > #define ADXL313_DEVID0 0xAD > > +#define ADXL313_DEVID0_ADXL312_314 0xE5 > > #define ADXL313_DEVID1 0x1D > > #define ADXL313_PARTID 0xCB > > #define ADXL313_SOFT_RESET 0x52 > > @@ -37,18 +38,28 @@ > > #define ADXL313_MEASUREMENT_MODE BIT(3) > > > > #define ADXL313_RANGE_MSK GENMASK(1, 0) > > -#define ADXL313_RANGE_4G 3 > > +#define ADXL313_RANGE_MAX 3 > > > > #define ADXL313_FULL_RES BIT(3) > > #define ADXL313_SPI_3WIRE BIT(6) > > #define ADXL313_I2C_DISABLE BIT(6) > > > > +extern const struct regmap_access_table adxl312_readable_regs_table; > > extern const struct regmap_access_table adxl313_readable_regs_table; > > +extern const struct regmap_access_table adxl314_readable_regs_table; > > > > +extern const struct regmap_access_table adxl312_writable_regs_table; > > extern const struct regmap_access_table adxl313_writable_regs_table; > > +extern const struct regmap_access_table adxl314_writable_regs_table; > > + > > +enum adxl313_device_type { > > + ADXL312, > > + ADXL313, > > + ADXL314, > > +}; > > > > int adxl313_core_probe(struct device *dev, > > struct regmap *regmap, > > - const char *name, > > + const struct spi_device_id *id, > > int (*setup)(struct device *, struct regmap *)); > > #endif /* _ADXL313_H_ */ > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c > > index afeef779e1d0..eea7d235950e 100644 > > --- a/drivers/iio/accel/adxl313_core.c > > +++ b/drivers/iio/accel/adxl313_core.c > > @@ -11,9 +11,17 @@ > > #include <linux/iio/iio.h> > > #include <linux/module.h> > > #include <linux/regmap.h> > > +#include <linux/spi/spi.h> > > Not in the core driver. This needs to be kept unware of the > buses that migh be used. > > > > > #include "adxl313.h" > > > > +static const struct regmap_range adxl312_readable_reg_range[] = { > > + regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0), > > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > > +}; > > + > > static const struct regmap_range adxl313_readable_reg_range[] = { > > regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_XID), > > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > > @@ -22,12 +30,32 @@ static const struct regmap_range adxl313_readable_reg_range[] = { > > regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > > }; > > > > +const struct regmap_access_table adxl312_readable_regs_table = { > > + .yes_ranges = adxl312_readable_reg_range, > > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > > +}; > > +EXPORT_SYMBOL_NS_GPL(adxl312_readable_regs_table, IIO_ADXL312); > > + > > const struct regmap_access_table adxl313_readable_regs_table = { > > .yes_ranges = adxl313_readable_reg_range, > > .n_yes_ranges = ARRAY_SIZE(adxl313_readable_reg_range), > > }; > > EXPORT_SYMBOL_NS_GPL(adxl313_readable_regs_table, IIO_ADXL313); > > > > +const struct regmap_access_table adxl314_readable_regs_table = { > > + .yes_ranges = adxl312_readable_reg_range, > > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > > +}; > > +EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL314); > > + > > +static const struct regmap_range adxl312_writable_reg_range[] = { > > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_INT_MAP), > > + regmap_reg_range(ADXL313_REG_DATA_FORMAT, ADXL313_REG_DATA_FORMAT), > > + regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > > +}; > > + > > static const struct regmap_range adxl313_writable_reg_range[] = { > > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > > regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > @@ -37,16 +65,30 @@ static const struct regmap_range adxl313_writable_reg_range[] = { > > regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > > }; > > > > +const struct regmap_access_table adxl312_writable_regs_table = { > > + .yes_ranges = adxl312_writable_reg_range, > > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > > +}; > > +EXPORT_SYMBOL_NS_GPL(adxl312_writable_regs_table, IIO_ADXL312); > > + > > const struct regmap_access_table adxl313_writable_regs_table = { > > .yes_ranges = adxl313_writable_reg_range, > > .n_yes_ranges = ARRAY_SIZE(adxl313_writable_reg_range), > > }; > > EXPORT_SYMBOL_NS_GPL(adxl313_writable_regs_table, IIO_ADXL313); > > > > +const struct regmap_access_table adxl314_writable_regs_table = { > > + .yes_ranges = adxl312_writable_reg_range, > > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > > +}; > > +EXPORT_SYMBOL_NS_GPL(adxl314_writable_regs_table, IIO_ADXL314); > > + > > struct adxl313_data { > > struct regmap *regmap; > > + const struct spi_device_id *id; > > + int scale_factor; > > struct mutex lock; /* lock to protect transf_buf */ > > - __le16 transf_buf __aligned(IIO_DMA_MINALIGN); > > + __le16 transf_buf ____cacheline_aligned; > Check your patch for accidental reverts of other changes like this... > > > }; > > > > static const int adxl313_odr_freqs[][2] = { > > @@ -156,12 +198,10 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > > *val = sign_extend32(ret, chan->scan_type.realbits - 1); > > return IIO_VAL_INT; > > case IIO_CHAN_INFO_SCALE: > > - /* > > - * Scale for any g range is given in datasheet as > > - * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 > > - */ > > *val = 0; > > - *val2 = 9576806; > > + > > + *val2 = data->scale_factor; > I'd move this into a structure containing all the chip type specific data. > Hence this would be > data->chip.scale_factor. > > > + > > return IIO_VAL_INT_PLUS_NANO; > > case IIO_CHAN_INFO_CALIBBIAS: > > ret = regmap_read(data->regmap, > > @@ -170,7 +210,7 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > > return ret; > > > > /* > > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > > + * 8-bit resolution at minimum range, that is 4x accel data scale > > * factor at full resolution > > */ > > *val = sign_extend32(regval, 7) * 4; > > @@ -198,7 +238,7 @@ static int adxl313_write_raw(struct iio_dev *indio_dev, > > switch (mask) { > > case IIO_CHAN_INFO_CALIBBIAS: > > /* > > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > > + * 8-bit resolution at minimum range, that is 4x accel data scale > > * factor at full resolution > > */ > > if (clamp_val(val, -128 * 4, 127 * 4) != val) > > @@ -223,14 +263,17 @@ static const struct iio_info adxl313_info = { > > static int adxl313_setup(struct device *dev, struct adxl313_data *data, > > int (*setup)(struct device *, struct regmap *)) > > { > > + enum adxl313_device_type dev_type = data->id->driver_data; > > unsigned int regval; > > int ret; > > > > - /* Ensures the device is in a consistent state after start up */ > > - ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > > - ADXL313_SOFT_RESET); > > - if (ret) > > - return ret; > > + /* If ADXL313, ensures the device is in a consistent state after start up */ > > + if (dev_type == ADXL313) { > > Add a flag to the chip_info structure suggested below for 'soft_reset' and > base this decision on that. The aim is to encode all the variations in chips > in one place so adding future chips is isolated there. > > > + ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > > + ADXL313_SOFT_RESET); > > + if (ret) > > + return ret; > > + } > > > > if (setup) { > > ret = setup(dev, data->regmap); > > @@ -242,41 +285,54 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, > > if (ret) > > return ret; > > > > - if (regval != ADXL313_DEVID0) { > > + if (dev_type == ADXL313 && regval != ADXL313_DEVID0) { > > Obviously this predates your changes... > Don't fail in this case. Warn only. We may be dealing with a newer compatible > device that falls back to an the ADXL313 compatible in dt. It's fine to express > we don't know what it is - particularly if ID matches something else, but > not to fail as a result. > Is this change mandatory? I am asking this because we have setups that require the driver to fail if not the correct device is plugged in or if there is no device attached to the system. Returning only warnings will cause the device to probe, but abnormal values to be produced. Moreover, most of the iio drivers perform a chip ID check and return an error code if the physical device is not found. Wouldn't it be better to treat the situation of new compatible devices when they appear? > > dev_err(dev, "Invalid manufacturer ID: 0x%02x\n", regval); > > return -ENODEV; > > } > > > > - ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); > > - if (ret) > > - return ret; > > + /* If ADXL313, check DEVID1 and PARTID */ > > + if (regval == ADXL313_DEVID0) { > > + ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); > > + if (ret) > > + return ret; > > > > - if (regval != ADXL313_DEVID1) { > As above, should not fail - but just give a warning message (or dev_info perhaps). > > > - dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); > > - return -ENODEV; > > - } > > + if (regval != ADXL313_DEVID1) { > > + dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); > > + return -ENODEV; > > + } > > > > - ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); > > - if (ret) > > - return ret; > > + ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); > > + if (ret) > > + return ret; > > > > - if (regval != ADXL313_PARTID) { > > - dev_err(dev, "Invalid device ID: 0x%02x\n", regval); > > + if (regval != ADXL313_PARTID) { > > + dev_err(dev, "Invalid device ID: 0x%02x\n", regval); > > + return -ENODEV; > > + } > > + } > > + > > + /* If ADXL312 or ADXL314 device, check DEVID0 */ > > + if (dev_type != ADXL313 && regval != ADXL313_DEVID0_ADXL312_314) { > > + dev_err(dev, "Invalid manufacturer ID: %#02x\n", regval); > > return -ENODEV; > > } > > > > - /* Sets the range to +/- 4g */ > > - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > > - ADXL313_RANGE_MSK, > > - FIELD_PREP(ADXL313_RANGE_MSK, ADXL313_RANGE_4G)); > > - if (ret) > > - return ret; > > + dev_info(dev, "%s detected.\n", data->id->name); > > Noise in the log. Drop this. It's easy to find out the name from sysfs. > > > > > - /* Enables full resolution */ > > - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > > - ADXL313_FULL_RES, ADXL313_FULL_RES); > > - if (ret) > > - return ret; > > + /* Sets the range to maximum, full resolution, for ADXL312 and ADXL313 */ > > + if (dev_type == ADXL312 || dev_type == ADXL313) { > I'm reading this backwards btw. As below, this sort of thing should depend on > contents of a chip_info structure. > In this case perhaps a flag called 'set max range'). > > > + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > > + ADXL313_RANGE_MSK, > > + FIELD_PREP(ADXL313_RANGE_MSK, 0x02)); > > 0x02 looks like a magic numbers, you should ideally provide a define. > > > + if (ret) > > + return ret; > > + > > + /* Enables full resolution */ > > + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, > > + ADXL313_FULL_RES, 0); > > + if (ret) > > + return ret; > > + } > > > > /* Enables measurement mode */ > > return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL, > > @@ -296,7 +352,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, > > */ > > int adxl313_core_probe(struct device *dev, > > struct regmap *regmap, > > - const char *name, > > + const struct spi_device_id *id, > > A core probe function shouldn't be taking an SPI specific structure. > Fine to pass the id enum entry. That can then index a chip_info structure > that includes the part name - we don't want to pass that through this interface > as well. > > > int (*setup)(struct device *, struct regmap *)) > > { > > struct adxl313_data *data; > > @@ -309,9 +365,35 @@ int adxl313_core_probe(struct device *dev, > > > > data = iio_priv(indio_dev); > > data->regmap = regmap; > > + data->id = id; > > + > > + if (id->driver_data == ADXL312) > > Whenever you hit a case like this, it almost always means you should be selecting > from an array of chip_info structures. One per type of chip. > If it is possible to convert code to data that is almost always a good idea. > > > + /* > > + * ADXL312 > > + * Scale for any g range (full range) is given in datasheet as > > + * 345 LSB/g = 0.0028985507 * 9.80665 = 0.028425072222155 m/s^2 > > + */ > > + data->scale_factor = 28425072; > > + > > + if (id->driver_data == ADXL313) > > + /* > > + * * ADXL313 > > + * Scale for any g range is given in datasheet as > > + * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 > > + */ > > + data->scale_factor = 9576806; > > + > > + if (id->driver_data == ADXL314) > > + /* > > + * ADXL314 > > + * At +/-200g with 13-bit resolution, scale factor is given in datasheet as > > + * 48.83 mg/LSB = 0.0488300 * 9.80665 = 0.4788587195 m/s^2. > > + */ > > + data->scale_factor = 478858719; > > + > > mutex_init(&data->lock); > > > > - indio_dev->name = name; > > + indio_dev->name = id->name; > > indio_dev->info = &adxl313_info; > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->channels = adxl313_channels; > > @@ -319,13 +401,13 @@ int adxl313_core_probe(struct device *dev, > > > > ret = adxl313_setup(dev, data, setup); > > if (ret) { > > - dev_err(dev, "ADXL313 setup failed\n"); > > + dev_err(dev, "Device %s setup failed\n", id->name); > > I doubt we'll ever care which part it was that failed. I'd > just leave that as it waas. > > > return ret; > > } > > > > return devm_iio_device_register(dev, indio_dev); > > } > > -EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313); > > +EXPORT_SYMBOL_GPL(adxl313_core_probe); > > Please don't remove the namespacing. Add any new exports to the > existing IIO_ADXL313 NS. > > > > > MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); > > MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer core driver"); > > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c > > index a3c6d553462d..19cd096373b0 100644 > > --- a/drivers/iio/accel/adxl313_spi.c > > +++ b/drivers/iio/accel/adxl313_spi.c > > @@ -14,6 +14,16 @@ > > > > #include "adxl313.h" > > > > +static const struct regmap_config adxl312_spi_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .rd_table = &adxl312_readable_regs_table, > > + .wr_table = &adxl312_writable_regs_table, > > + .max_register = 0x39, > > + /* Setting bits 7 and 6 enables multiple-byte read */ > > + .read_flag_mask = BIT(7) | BIT(6), > > +}; > > + > > static const struct regmap_config adxl313_spi_regmap_config = { > > .reg_bits = 8, > > .val_bits = 8, > > @@ -24,6 +34,22 @@ static const struct regmap_config adxl313_spi_regmap_config = { > > .read_flag_mask = BIT(7) | BIT(6), > > }; > > > > +static const struct regmap_config adxl314_spi_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > + .rd_table = &adxl314_readable_regs_table, > > + .wr_table = &adxl314_writable_regs_table, > > + .max_register = 0x39, > > + /* Setting bits 7 and 6 enables multiple-byte read */ > > + .read_flag_mask = BIT(7) | BIT(6), > > +}; > > + > > +static const struct regmap_config adxl31x_spi_regmap_config[] = { > > + adxl312_spi_regmap_config, > > + adxl313_spi_regmap_config, > > + adxl314_spi_regmap_config > > +}; > > + > > static int adxl313_spi_setup(struct device *dev, struct regmap *regmap) > > { > > struct spi_device *spi = container_of(dev, struct spi_device, dev); > > @@ -51,26 +77,32 @@ static int adxl313_spi_probe(struct spi_device *spi) > > if (ret) > > return ret; > > > > - regmap = devm_regmap_init_spi(spi, &adxl313_spi_regmap_config); > > + regmap = devm_regmap_init_spi(spi, > > + &adxl31x_spi_regmap_config[id->driver_data]); > > + > > if (IS_ERR(regmap)) { > > dev_err(&spi->dev, "Error initializing spi regmap: %ld\n", > > PTR_ERR(regmap)); > > return PTR_ERR(regmap); > > } > > > > - return adxl313_core_probe(&spi->dev, regmap, id->name, > > + return adxl313_core_probe(&spi->dev, regmap, id, > > &adxl313_spi_setup); > > } > > > > static const struct spi_device_id adxl313_spi_id[] = { > > - { "adxl313" }, > > + { "adxl312", ADXL312 }, > > + { "adxl313", ADXL313 }, > > + { "adxl314", ADXL314 }, > > { } > > }; > > > > MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > > > > static const struct of_device_id adxl313_of_match[] = { > > + { .compatible = "adi,adxl312" }, > > { .compatible = "adi,adxl313" }, > > + { .compatible = "adi,adxl314" }, > > { } > As mentioned in other thread, there is a nicer way of handling > the different types of firmware ID. > > }; > > > > @@ -90,4 +122,6 @@ module_spi_driver(adxl313_spi_driver); > > MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); > > MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer SPI driver"); > > MODULE_LICENSE("GPL v2"); > > +MODULE_IMPORT_NS(IIO_ADXL312); > Take a look at what this is doing. There is only one namespace for the > driver, not one per chip type. > > > MODULE_IMPORT_NS(IIO_ADXL313); > > +MODULE_IMPORT_NS(IIO_ADXL314);
On Thu, 18 Aug 2022 10:11:16 +0000 "Mois, George" <George.Mois@analog.com> wrote: > Hi Jonathan, > > Thank you for your review! > > Regards, > George > > > > On Tue, 16 Aug 2022 13:28:28 +0300 > > George Mois <george.mois@analog.com> wrote: > > > > > ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers > > > with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g > > > respectively. > > > > > > Implement support for ADXL312 and ADXL314 by extending the ADXL313 > > > driver. > > > > > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL312.pdf > > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL314.pdf > > > > > > Signed-off-by: George Mois <george.mois@analog.com> > > > > Hi George, > > > > Various comments inline. > > > > Thanks, > > > > Jonathan > > > --- > > > drivers/iio/accel/adxl313.h | 15 ++- > > > drivers/iio/accel/adxl313_core.c | 164 +++++++++++++++++++++++-------- > > > drivers/iio/accel/adxl313_spi.c | 40 +++++++- > > > 3 files changed, 173 insertions(+), 46 deletions(-) > > > > > > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h > > > index 4415f2fc07e1..7428b1f7768f 100644 > > > --- a/drivers/iio/accel/adxl313.h > > > +++ b/drivers/iio/accel/adxl313.h > > > @@ -26,6 +26,7 @@ > > > #define ADXL313_REG_FIFO_STATUS 0x39 > > > > > > #define ADXL313_DEVID0 0xAD > > > +#define ADXL313_DEVID0_ADXL312_314 0xE5 > > > #define ADXL313_DEVID1 0x1D > > > #define ADXL313_PARTID 0xCB > > > #define ADXL313_SOFT_RESET 0x52 > > > @@ -37,18 +38,28 @@ > > > #define ADXL313_MEASUREMENT_MODE BIT(3) > > > > > > #define ADXL313_RANGE_MSK GENMASK(1, 0) > > > -#define ADXL313_RANGE_4G 3 > > > +#define ADXL313_RANGE_MAX 3 > > > > > > #define ADXL313_FULL_RES BIT(3) > > > #define ADXL313_SPI_3WIRE BIT(6) > > > #define ADXL313_I2C_DISABLE BIT(6) > > > > > > +extern const struct regmap_access_table adxl312_readable_regs_table; > > > extern const struct regmap_access_table adxl313_readable_regs_table; > > > +extern const struct regmap_access_table adxl314_readable_regs_table; > > > > > > +extern const struct regmap_access_table adxl312_writable_regs_table; > > > extern const struct regmap_access_table adxl313_writable_regs_table; > > > +extern const struct regmap_access_table adxl314_writable_regs_table; > > > + > > > +enum adxl313_device_type { > > > + ADXL312, > > > + ADXL313, > > > + ADXL314, > > > +}; > > > > > > int adxl313_core_probe(struct device *dev, > > > struct regmap *regmap, > > > - const char *name, > > > + const struct spi_device_id *id, > > > int (*setup)(struct device *, struct regmap *)); > > > #endif /* _ADXL313_H_ */ > > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c > > > index afeef779e1d0..eea7d235950e 100644 > > > --- a/drivers/iio/accel/adxl313_core.c > > > +++ b/drivers/iio/accel/adxl313_core.c > > > @@ -11,9 +11,17 @@ > > > #include <linux/iio/iio.h> > > > #include <linux/module.h> > > > #include <linux/regmap.h> > > > +#include <linux/spi/spi.h> > > > > Not in the core driver. This needs to be kept unware of the > > buses that migh be used. > > > > > > > > #include "adxl313.h" > > > > > > +static const struct regmap_range adxl312_readable_reg_range[] = { > > > + regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0), > > > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > > > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > > > +}; > > > + > > > static const struct regmap_range adxl313_readable_reg_range[] = { > > > regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_XID), > > > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > > > @@ -22,12 +30,32 @@ static const struct regmap_range adxl313_readable_reg_range[] = { > > > regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), > > > }; > > > > > > +const struct regmap_access_table adxl312_readable_regs_table = { > > > + .yes_ranges = adxl312_readable_reg_range, > > > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > > > +}; > > > +EXPORT_SYMBOL_NS_GPL(adxl312_readable_regs_table, IIO_ADXL312); > > > + > > > const struct regmap_access_table adxl313_readable_regs_table = { > > > .yes_ranges = adxl313_readable_reg_range, > > > .n_yes_ranges = ARRAY_SIZE(adxl313_readable_reg_range), > > > }; > > > EXPORT_SYMBOL_NS_GPL(adxl313_readable_regs_table, IIO_ADXL313); > > > > > > +const struct regmap_access_table adxl314_readable_regs_table = { > > > + .yes_ranges = adxl312_readable_reg_range, > > > + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), > > > +}; > > > +EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL314); > > > + > > > +static const struct regmap_range adxl312_writable_reg_range[] = { > > > + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > > + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), > > > + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_INT_MAP), > > > + regmap_reg_range(ADXL313_REG_DATA_FORMAT, ADXL313_REG_DATA_FORMAT), > > > + regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > > > +}; > > > + > > > static const struct regmap_range adxl313_writable_reg_range[] = { > > > regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), > > > regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), > > > @@ -37,16 +65,30 @@ static const struct regmap_range adxl313_writable_reg_range[] = { > > > regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), > > > }; > > > > > > +const struct regmap_access_table adxl312_writable_regs_table = { > > > + .yes_ranges = adxl312_writable_reg_range, > > > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > > > +}; > > > +EXPORT_SYMBOL_NS_GPL(adxl312_writable_regs_table, IIO_ADXL312); > > > + > > > const struct regmap_access_table adxl313_writable_regs_table = { > > > .yes_ranges = adxl313_writable_reg_range, > > > .n_yes_ranges = ARRAY_SIZE(adxl313_writable_reg_range), > > > }; > > > EXPORT_SYMBOL_NS_GPL(adxl313_writable_regs_table, IIO_ADXL313); > > > > > > +const struct regmap_access_table adxl314_writable_regs_table = { > > > + .yes_ranges = adxl312_writable_reg_range, > > > + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), > > > +}; > > > +EXPORT_SYMBOL_NS_GPL(adxl314_writable_regs_table, IIO_ADXL314); > > > + > > > struct adxl313_data { > > > struct regmap *regmap; > > > + const struct spi_device_id *id; > > > + int scale_factor; > > > struct mutex lock; /* lock to protect transf_buf */ > > > - __le16 transf_buf __aligned(IIO_DMA_MINALIGN); > > > + __le16 transf_buf ____cacheline_aligned; > > Check your patch for accidental reverts of other changes like this... > > > > > }; > > > > > > static const int adxl313_odr_freqs[][2] = { > > > @@ -156,12 +198,10 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > > > *val = sign_extend32(ret, chan->scan_type.realbits - 1); > > > return IIO_VAL_INT; > > > case IIO_CHAN_INFO_SCALE: > > > - /* > > > - * Scale for any g range is given in datasheet as > > > - * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 > > > - */ > > > *val = 0; > > > - *val2 = 9576806; > > > + > > > + *val2 = data->scale_factor; > > I'd move this into a structure containing all the chip type specific data. > > Hence this would be > > data->chip.scale_factor. > > > > > + > > > return IIO_VAL_INT_PLUS_NANO; > > > case IIO_CHAN_INFO_CALIBBIAS: > > > ret = regmap_read(data->regmap, > > > @@ -170,7 +210,7 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, > > > return ret; > > > > > > /* > > > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > > > + * 8-bit resolution at minimum range, that is 4x accel data scale > > > * factor at full resolution > > > */ > > > *val = sign_extend32(regval, 7) * 4; > > > @@ -198,7 +238,7 @@ static int adxl313_write_raw(struct iio_dev *indio_dev, > > > switch (mask) { > > > case IIO_CHAN_INFO_CALIBBIAS: > > > /* > > > - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale > > > + * 8-bit resolution at minimum range, that is 4x accel data scale > > > * factor at full resolution > > > */ > > > if (clamp_val(val, -128 * 4, 127 * 4) != val) > > > @@ -223,14 +263,17 @@ static const struct iio_info adxl313_info = { > > > static int adxl313_setup(struct device *dev, struct adxl313_data *data, > > > int (*setup)(struct device *, struct regmap *)) > > > { > > > + enum adxl313_device_type dev_type = data->id->driver_data; > > > unsigned int regval; > > > int ret; > > > > > > - /* Ensures the device is in a consistent state after start up */ > > > - ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > > > - ADXL313_SOFT_RESET); > > > - if (ret) > > > - return ret; > > > + /* If ADXL313, ensures the device is in a consistent state after start up */ > > > + if (dev_type == ADXL313) { > > > > Add a flag to the chip_info structure suggested below for 'soft_reset' and > > base this decision on that. The aim is to encode all the variations in chips > > in one place so adding future chips is isolated there. > > > > > + ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > > > + ADXL313_SOFT_RESET); > > > + if (ret) > > > + return ret; > > > + } > > > > > > if (setup) { > > > ret = setup(dev, data->regmap); > > > @@ -242,41 +285,54 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, > > > if (ret) > > > return ret; > > > > > > - if (regval != ADXL313_DEVID0) { > > > + if (dev_type == ADXL313 && regval != ADXL313_DEVID0) { > > > > Obviously this predates your changes... > > Don't fail in this case. Warn only. We may be dealing with a newer compatible > > device that falls back to an the ADXL313 compatible in dt. It's fine to express > > we don't know what it is - particularly if ID matches something else, but > > not to fail as a result. > > > > Is this change mandatory? I am asking this because we have setups that require > the driver to fail if not the correct device is plugged in or if there is no device > attached to the system. Yes unfortunately. There have been a couple of discussions with the device tree maintainers on this recently. The problem case is that a new compatible chip with a different device ID is introduced. The expectation is that compatible = "newchip", "olderchip-with-otherwise identical software interfaces" will work with an old kernel that knows nothing about new chip. Note this extends to newer devices that add features but maintain backwards compatibility with older devices as well. The idea is we get partial support in older kernels and the new compatible will be used only when new features are added. As a general rule new IIO drivers are tending to warn/info on unexpected IDs because there is a non trivial risk of the part being wrong. If they know the ID but it's for a different device they support, they should assume the device is the one the ID matches for (and warn that the DT is probably wrong for the board). There are corner cases that are fine for detecting 'no device' such as when we know the manufacturer doesn't use 0x00 or 0xff (likely false values on SPI) for device IDs. In those cases it is fine to fail on basis device isn't there. > > Returning only warnings will cause the device to probe, but abnormal values > to be produced. Moreover, most of the iio drivers perform a chip ID check and > return an error code if the physical device is not found. We had this wrong the past unfortunately and haven't 'fixed' all drivers yet. However, note that I expect we will do this over time. If the DT is wrong then you are on your own. Correct solution is perform some form of detection in an early boot firmware (uboot or similar) and patch the DT appropriately. By the time Linux sees DT it should be correct for the board. > > Wouldn't it be better to treat the situation of new compatible devices when > they appear? Unfortunately people insist on not upgrading their kernels as quick as new devices turn up in device trees + the intent is that there is no need to if the device is compatible with an older one (up to the ID). Jonathan > > > > dev_err(dev, "Invalid manufacturer ID: 0x%02x\n", regval); > > > return -ENODEV; > > > }
diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h index 4415f2fc07e1..7428b1f7768f 100644 --- a/drivers/iio/accel/adxl313.h +++ b/drivers/iio/accel/adxl313.h @@ -26,6 +26,7 @@ #define ADXL313_REG_FIFO_STATUS 0x39 #define ADXL313_DEVID0 0xAD +#define ADXL313_DEVID0_ADXL312_314 0xE5 #define ADXL313_DEVID1 0x1D #define ADXL313_PARTID 0xCB #define ADXL313_SOFT_RESET 0x52 @@ -37,18 +38,28 @@ #define ADXL313_MEASUREMENT_MODE BIT(3) #define ADXL313_RANGE_MSK GENMASK(1, 0) -#define ADXL313_RANGE_4G 3 +#define ADXL313_RANGE_MAX 3 #define ADXL313_FULL_RES BIT(3) #define ADXL313_SPI_3WIRE BIT(6) #define ADXL313_I2C_DISABLE BIT(6) +extern const struct regmap_access_table adxl312_readable_regs_table; extern const struct regmap_access_table adxl313_readable_regs_table; +extern const struct regmap_access_table adxl314_readable_regs_table; +extern const struct regmap_access_table adxl312_writable_regs_table; extern const struct regmap_access_table adxl313_writable_regs_table; +extern const struct regmap_access_table adxl314_writable_regs_table; + +enum adxl313_device_type { + ADXL312, + ADXL313, + ADXL314, +}; int adxl313_core_probe(struct device *dev, struct regmap *regmap, - const char *name, + const struct spi_device_id *id, int (*setup)(struct device *, struct regmap *)); #endif /* _ADXL313_H_ */ diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c index afeef779e1d0..eea7d235950e 100644 --- a/drivers/iio/accel/adxl313_core.c +++ b/drivers/iio/accel/adxl313_core.c @@ -11,9 +11,17 @@ #include <linux/iio/iio.h> #include <linux/module.h> #include <linux/regmap.h> +#include <linux/spi/spi.h> #include "adxl313.h" +static const struct regmap_range adxl312_readable_reg_range[] = { + regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0), + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), +}; + static const struct regmap_range adxl313_readable_reg_range[] = { regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_XID), regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), @@ -22,12 +30,32 @@ static const struct regmap_range adxl313_readable_reg_range[] = { regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_FIFO_STATUS), }; +const struct regmap_access_table adxl312_readable_regs_table = { + .yes_ranges = adxl312_readable_reg_range, + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), +}; +EXPORT_SYMBOL_NS_GPL(adxl312_readable_regs_table, IIO_ADXL312); + const struct regmap_access_table adxl313_readable_regs_table = { .yes_ranges = adxl313_readable_reg_range, .n_yes_ranges = ARRAY_SIZE(adxl313_readable_reg_range), }; EXPORT_SYMBOL_NS_GPL(adxl313_readable_regs_table, IIO_ADXL313); +const struct regmap_access_table adxl314_readable_regs_table = { + .yes_ranges = adxl312_readable_reg_range, + .n_yes_ranges = ARRAY_SIZE(adxl312_readable_reg_range), +}; +EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL314); + +static const struct regmap_range adxl312_writable_reg_range[] = { + regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), + regmap_reg_range(ADXL313_REG_THRESH_ACT, ADXL313_REG_ACT_INACT_CTL), + regmap_reg_range(ADXL313_REG_BW_RATE, ADXL313_REG_INT_MAP), + regmap_reg_range(ADXL313_REG_DATA_FORMAT, ADXL313_REG_DATA_FORMAT), + regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), +}; + static const struct regmap_range adxl313_writable_reg_range[] = { regmap_reg_range(ADXL313_REG_SOFT_RESET, ADXL313_REG_SOFT_RESET), regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)), @@ -37,16 +65,30 @@ static const struct regmap_range adxl313_writable_reg_range[] = { regmap_reg_range(ADXL313_REG_FIFO_CTL, ADXL313_REG_FIFO_CTL), }; +const struct regmap_access_table adxl312_writable_regs_table = { + .yes_ranges = adxl312_writable_reg_range, + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), +}; +EXPORT_SYMBOL_NS_GPL(adxl312_writable_regs_table, IIO_ADXL312); + const struct regmap_access_table adxl313_writable_regs_table = { .yes_ranges = adxl313_writable_reg_range, .n_yes_ranges = ARRAY_SIZE(adxl313_writable_reg_range), }; EXPORT_SYMBOL_NS_GPL(adxl313_writable_regs_table, IIO_ADXL313); +const struct regmap_access_table adxl314_writable_regs_table = { + .yes_ranges = adxl312_writable_reg_range, + .n_yes_ranges = ARRAY_SIZE(adxl312_writable_reg_range), +}; +EXPORT_SYMBOL_NS_GPL(adxl314_writable_regs_table, IIO_ADXL314); + struct adxl313_data { struct regmap *regmap; + const struct spi_device_id *id; + int scale_factor; struct mutex lock; /* lock to protect transf_buf */ - __le16 transf_buf __aligned(IIO_DMA_MINALIGN); + __le16 transf_buf ____cacheline_aligned; }; static const int adxl313_odr_freqs[][2] = { @@ -156,12 +198,10 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, *val = sign_extend32(ret, chan->scan_type.realbits - 1); return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - /* - * Scale for any g range is given in datasheet as - * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 - */ *val = 0; - *val2 = 9576806; + + *val2 = data->scale_factor; + return IIO_VAL_INT_PLUS_NANO; case IIO_CHAN_INFO_CALIBBIAS: ret = regmap_read(data->regmap, @@ -170,7 +210,7 @@ static int adxl313_read_raw(struct iio_dev *indio_dev, return ret; /* - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale + * 8-bit resolution at minimum range, that is 4x accel data scale * factor at full resolution */ *val = sign_extend32(regval, 7) * 4; @@ -198,7 +238,7 @@ static int adxl313_write_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_CALIBBIAS: /* - * 8-bit resolution at +/- 0.5g, that is 4x accel data scale + * 8-bit resolution at minimum range, that is 4x accel data scale * factor at full resolution */ if (clamp_val(val, -128 * 4, 127 * 4) != val) @@ -223,14 +263,17 @@ static const struct iio_info adxl313_info = { static int adxl313_setup(struct device *dev, struct adxl313_data *data, int (*setup)(struct device *, struct regmap *)) { + enum adxl313_device_type dev_type = data->id->driver_data; unsigned int regval; int ret; - /* Ensures the device is in a consistent state after start up */ - ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, - ADXL313_SOFT_RESET); - if (ret) - return ret; + /* If ADXL313, ensures the device is in a consistent state after start up */ + if (dev_type == ADXL313) { + ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, + ADXL313_SOFT_RESET); + if (ret) + return ret; + } if (setup) { ret = setup(dev, data->regmap); @@ -242,41 +285,54 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, if (ret) return ret; - if (regval != ADXL313_DEVID0) { + if (dev_type == ADXL313 && regval != ADXL313_DEVID0) { dev_err(dev, "Invalid manufacturer ID: 0x%02x\n", regval); return -ENODEV; } - ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); - if (ret) - return ret; + /* If ADXL313, check DEVID1 and PARTID */ + if (regval == ADXL313_DEVID0) { + ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, ®val); + if (ret) + return ret; - if (regval != ADXL313_DEVID1) { - dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); - return -ENODEV; - } + if (regval != ADXL313_DEVID1) { + dev_err(dev, "Invalid mems ID: 0x%02x\n", regval); + return -ENODEV; + } - ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); - if (ret) - return ret; + ret = regmap_read(data->regmap, ADXL313_REG_PARTID, ®val); + if (ret) + return ret; - if (regval != ADXL313_PARTID) { - dev_err(dev, "Invalid device ID: 0x%02x\n", regval); + if (regval != ADXL313_PARTID) { + dev_err(dev, "Invalid device ID: 0x%02x\n", regval); + return -ENODEV; + } + } + + /* If ADXL312 or ADXL314 device, check DEVID0 */ + if (dev_type != ADXL313 && regval != ADXL313_DEVID0_ADXL312_314) { + dev_err(dev, "Invalid manufacturer ID: %#02x\n", regval); return -ENODEV; } - /* Sets the range to +/- 4g */ - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, - ADXL313_RANGE_MSK, - FIELD_PREP(ADXL313_RANGE_MSK, ADXL313_RANGE_4G)); - if (ret) - return ret; + dev_info(dev, "%s detected.\n", data->id->name); - /* Enables full resolution */ - ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, - ADXL313_FULL_RES, ADXL313_FULL_RES); - if (ret) - return ret; + /* Sets the range to maximum, full resolution, for ADXL312 and ADXL313 */ + if (dev_type == ADXL312 || dev_type == ADXL313) { + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, + ADXL313_RANGE_MSK, + FIELD_PREP(ADXL313_RANGE_MSK, 0x02)); + if (ret) + return ret; + + /* Enables full resolution */ + ret = regmap_update_bits(data->regmap, ADXL313_REG_DATA_FORMAT, + ADXL313_FULL_RES, 0); + if (ret) + return ret; + } /* Enables measurement mode */ return regmap_update_bits(data->regmap, ADXL313_REG_POWER_CTL, @@ -296,7 +352,7 @@ static int adxl313_setup(struct device *dev, struct adxl313_data *data, */ int adxl313_core_probe(struct device *dev, struct regmap *regmap, - const char *name, + const struct spi_device_id *id, int (*setup)(struct device *, struct regmap *)) { struct adxl313_data *data; @@ -309,9 +365,35 @@ int adxl313_core_probe(struct device *dev, data = iio_priv(indio_dev); data->regmap = regmap; + data->id = id; + + if (id->driver_data == ADXL312) + /* + * ADXL312 + * Scale for any g range (full range) is given in datasheet as + * 345 LSB/g = 0.0028985507 * 9.80665 = 0.028425072222155 m/s^2 + */ + data->scale_factor = 28425072; + + if (id->driver_data == ADXL313) + /* + * * ADXL313 + * Scale for any g range is given in datasheet as + * 1024 LSB/g = 0.0009765625 * 9.80665 = 0.009576806640625 m/s^2 + */ + data->scale_factor = 9576806; + + if (id->driver_data == ADXL314) + /* + * ADXL314 + * At +/-200g with 13-bit resolution, scale factor is given in datasheet as + * 48.83 mg/LSB = 0.0488300 * 9.80665 = 0.4788587195 m/s^2. + */ + data->scale_factor = 478858719; + mutex_init(&data->lock); - indio_dev->name = name; + indio_dev->name = id->name; indio_dev->info = &adxl313_info; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = adxl313_channels; @@ -319,13 +401,13 @@ int adxl313_core_probe(struct device *dev, ret = adxl313_setup(dev, data, setup); if (ret) { - dev_err(dev, "ADXL313 setup failed\n"); + dev_err(dev, "Device %s setup failed\n", id->name); return ret; } return devm_iio_device_register(dev, indio_dev); } -EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313); +EXPORT_SYMBOL_GPL(adxl313_core_probe); MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer core driver"); diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c index a3c6d553462d..19cd096373b0 100644 --- a/drivers/iio/accel/adxl313_spi.c +++ b/drivers/iio/accel/adxl313_spi.c @@ -14,6 +14,16 @@ #include "adxl313.h" +static const struct regmap_config adxl312_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .rd_table = &adxl312_readable_regs_table, + .wr_table = &adxl312_writable_regs_table, + .max_register = 0x39, + /* Setting bits 7 and 6 enables multiple-byte read */ + .read_flag_mask = BIT(7) | BIT(6), +}; + static const struct regmap_config adxl313_spi_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -24,6 +34,22 @@ static const struct regmap_config adxl313_spi_regmap_config = { .read_flag_mask = BIT(7) | BIT(6), }; +static const struct regmap_config adxl314_spi_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .rd_table = &adxl314_readable_regs_table, + .wr_table = &adxl314_writable_regs_table, + .max_register = 0x39, + /* Setting bits 7 and 6 enables multiple-byte read */ + .read_flag_mask = BIT(7) | BIT(6), +}; + +static const struct regmap_config adxl31x_spi_regmap_config[] = { + adxl312_spi_regmap_config, + adxl313_spi_regmap_config, + adxl314_spi_regmap_config +}; + static int adxl313_spi_setup(struct device *dev, struct regmap *regmap) { struct spi_device *spi = container_of(dev, struct spi_device, dev); @@ -51,26 +77,32 @@ static int adxl313_spi_probe(struct spi_device *spi) if (ret) return ret; - regmap = devm_regmap_init_spi(spi, &adxl313_spi_regmap_config); + regmap = devm_regmap_init_spi(spi, + &adxl31x_spi_regmap_config[id->driver_data]); + if (IS_ERR(regmap)) { dev_err(&spi->dev, "Error initializing spi regmap: %ld\n", PTR_ERR(regmap)); return PTR_ERR(regmap); } - return adxl313_core_probe(&spi->dev, regmap, id->name, + return adxl313_core_probe(&spi->dev, regmap, id, &adxl313_spi_setup); } static const struct spi_device_id adxl313_spi_id[] = { - { "adxl313" }, + { "adxl312", ADXL312 }, + { "adxl313", ADXL313 }, + { "adxl314", ADXL314 }, { } }; MODULE_DEVICE_TABLE(spi, adxl313_spi_id); static const struct of_device_id adxl313_of_match[] = { + { .compatible = "adi,adxl312" }, { .compatible = "adi,adxl313" }, + { .compatible = "adi,adxl314" }, { } }; @@ -90,4 +122,6 @@ module_spi_driver(adxl313_spi_driver); MODULE_AUTHOR("Lucas Stankus <lucas.p.stankus@gmail.com>"); MODULE_DESCRIPTION("ADXL313 3-Axis Digital Accelerometer SPI driver"); MODULE_LICENSE("GPL v2"); +MODULE_IMPORT_NS(IIO_ADXL312); MODULE_IMPORT_NS(IIO_ADXL313); +MODULE_IMPORT_NS(IIO_ADXL314);
ADXL312 and ADXL314 are small, thin, low power, 3-axis accelerometers with high resolution (13-bit) measurement up to +/-12 g and +/- 200 g respectively. Implement support for ADXL312 and ADXL314 by extending the ADXL313 driver. Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL312.pdf Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL314.pdf Signed-off-by: George Mois <george.mois@analog.com> --- drivers/iio/accel/adxl313.h | 15 ++- drivers/iio/accel/adxl313_core.c | 164 +++++++++++++++++++++++-------- drivers/iio/accel/adxl313_spi.c | 40 +++++++- 3 files changed, 173 insertions(+), 46 deletions(-)