| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220904162834.564a021c@jic23-huawei>
Date: Sun, 4 Sep 2022 16:28:34 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: George Mois <george.mois@...log.com>
Cc: <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <lucas.p.stankus@...il.com>
Subject: Re: [PATCH v3 2/2] drivers: iio: accel: adxl312 and adxl314 support
On Wed, 31 Aug 2022 17:35:38 +0300
George Mois <george.mois@...log.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
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@...log.com>
> ---
> 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],
> + },
> { }
> };
>
Powered by blists - more mailing lists