On Wed, 31 Aug 2022 17:35:38 +0300 George Mois <george.mois@xxxxxxxxxx> 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 Whilst there is some discussion of this going on in a different thread my opinion is that Datasheet is a formal tag so there should be no blank line here. A few other minor comments inline. > > Signed-off-by: George Mois <george.mois@xxxxxxxxxx> > --- > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c > index afeef779e1d0..9c93e71c94f1 100644 > --- a/drivers/iio/accel/adxl313_core.c > +++ b/drivers/iio/accel/adxl313_core.c ... > -static int adxl313_setup(struct device *dev, struct adxl313_data *data, > - int (*setup)(struct device *, struct regmap *)) > +static int adxl312_adxl314_check_id(struct device *dev, Naming a function after multiple supported parts has a history of going wrong over the longer term as that list of parts keeps getting longer. Just name it after the first one supported. > + struct adxl313_data *data) > { > unsigned int regval; > int ret; > > + > +static int adxl313_setup(struct device *dev, struct adxl313_data *data, > + int (*setup)(struct device *, struct regmap *)) > +{ > + int ret; > > - if (regval != ADXL313_PARTID) { > - dev_err(dev, "Invalid device ID: 0x%02x\n", regval); > - return -ENODEV; > + /* > + * If sw reset available, ensures the device is in a consistent > + * state after start up > + */ > + if (data->chip_info->soft_reset) { > + ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET, > + ADXL313_SOFT_RESET); > + if (ret) > + return ret; > } > > - /* 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; > + if (setup) { > + ret = setup(dev, data->regmap); > + if (ret) > + return ret; > + } > + > + if (data->chip_info->type == ADXL313) > + /* ADXL313 */ > + ret = adxl313_check_id(dev, data); I'd prefer these handled via a callback rather than checking on type. That leads to the driver being easier to extend for future supported parts as changes are focused in one place (the chip_info structures). Otherwise this new approach seems much cleaner. > + else > + /* ADXL312 or ADXL314 */ > + ret = adxl312_adxl314_check_id(dev, data); > > diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c > index c329765dbf60..0665f2945a27 100644 > --- a/drivers/iio/accel/adxl313_i2c.c > +++ b/drivers/iio/accel/adxl313_i2c.c > @@ -14,42 +14,80 @@ > > static const struct i2c_device_id adxl313_i2c_id[] = { > - { "adxl313" }, > + { "adxl312", ADXL312 }, As for SPI case, it would be cleaner to put a pointer in here as then the two paths will be more similar. > + { "adxl313", ADXL313 }, > + { "adxl314", ADXL314 }, > { } > }; > > MODULE_DEVICE_TABLE(i2c, adxl313_i2c_id); > > static const struct of_device_id adxl313_of_match[] = { > - { .compatible = "adi,adxl313" }, > + { > + .compatible = "adi,adxl312", > + .data = &adxl31x_chip_info[ADXL312], > + }, > + { > + .compatible = "adi,adxl313", > + .data = &adxl31x_chip_info[ADXL313], > + }, > + { > + .compatible = "adi,adxl314", > + .data = &adxl31x_chip_info[ADXL314], > + }, > { } > }; > > MODULE_DEVICE_TABLE(of, adxl313_of_match); > > +static int adxl313_i2c_probe(struct i2c_client *client) > +{ > + const struct adxl313_chip_info *chip_data; > + enum adxl313_device_type chip_type; > + struct regmap *regmap; > + > + chip_data = device_get_match_data(&client->dev); > + if (chip_data) > + chip_type = chip_data->type; > + else > + chip_type = i2c_match_id(adxl313_i2c_id, client)->driver_data; > + > + regmap = devm_regmap_init_i2c(client, > + &adxl31x_i2c_regmap_config[chip_type]); As below: I think it is cleaner to get chip_data for either registration path and extract chip_type from that. Aim being to avoid going backwards and forwards between the structure and the enum. > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "Error initializing i2c regmap: %ld\n", > + PTR_ERR(regmap)); > + return PTR_ERR(regmap); > + } > + > + return adxl313_core_probe(&client->dev, regmap, &adxl31x_chip_info[chip_type], NULL); > +} > + > static struct i2c_driver adxl313_i2c_driver = { > .driver = { > .name = "adxl313_i2c", > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c > index a3c6d553462d..2c3b094ef465 100644 > --- a/drivers/iio/accel/adxl313_spi.c > +++ b/drivers/iio/accel/adxl313_spi.c > @@ -11,17 +11,38 @@ > > static int adxl313_spi_setup(struct device *dev, struct regmap *regmap) > @@ -42,7 +63,8 @@ static int adxl313_spi_setup(struct device *dev, struct regmap *regmap) > > static int adxl313_spi_probe(struct spi_device *spi) > { > - const struct spi_device_id *id = spi_get_device_id(spi); > + const struct adxl313_chip_info *chip_data; > + enum adxl313_device_type chip_type; > struct regmap *regmap; > int ret; > > @@ -51,26 +73,47 @@ static int adxl313_spi_probe(struct spi_device *spi) > if (ret) > return ret; > > - regmap = devm_regmap_init_spi(spi, &adxl313_spi_regmap_config); > + chip_data = device_get_match_data(&spi->dev); > + if (chip_data) > + chip_type = chip_data->type; > + else > + chip_type = spi_get_device_id(spi)->driver_data; I'd rather see you set chip_data here for either path then use that directly in the core_probe() call below. Note comment on storing a pointer in spi_device_id->driver_data (it is carefully sized to allow that to be done) You will need to pull chip_type out for the regmap config, but at least we are only going one way rather than chip_data->chip_type->chip_data. > + > + regmap = devm_regmap_init_spi(spi, > + &adxl31x_spi_regmap_config[chip_type]); > + > 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, > - &adxl313_spi_setup); > + return adxl313_core_probe(&spi->dev, regmap, > + &adxl31x_chip_info[chip_type], &adxl313_spi_setup); > } > > static const struct spi_device_id adxl313_spi_id[] = { > - { "adxl313" }, > + { "adxl312", ADXL312 }, It's fine to store a pointer in the data field of this (will need casting appropriately) and that will simplify handling in probe() as both sources will be of the same type. > + { "adxl313", ADXL313 }, > + { "adxl314", ADXL314 }, > { } > }; > > MODULE_DEVICE_TABLE(spi, adxl313_spi_id); > > static const struct of_device_id adxl313_of_match[] = { > - { .compatible = "adi,adxl313" }, > + { > + .compatible = "adi,adxl312", > + .data = &adxl31x_chip_info[ADXL312], Not particularly important but it's fairly common practice to have these on one line. { .compatible = "adi,adxl312", .data = &adxl31x_chip_info[ADXL312] }, > + }, > + { > + .compatible = "adi,adxl313", > + .data = &adxl31x_chip_info[ADXL313], > + }, > + { > + .compatible = "adi,adxl314", > + .data = &adxl31x_chip_info[ADXL314], > + }, > { } > }; >
