| 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
| ||
|
Date: Fri, 21 Apr 2023 09:19:32 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Mehdi Djait <mehdi.djait.k@...il.com>, jic23@...nel.org
Cc: krzysztof.kozlowski+dt@...aro.org,
andriy.shevchenko@...ux.intel.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 3/5] iio: accel: kionix-kx022a: Refactor driver and add
chip_info structure
Hi Mehdi,
Thanks for working on this driver :) Much appreciated!
On 4/20/23 23:22, Mehdi Djait wrote:
> Refactor the kx022a driver implementation to make it more generic and
> extensible.
> Introduce an i2c_device_id table.
> Add the chip_info structure to the driver's private data to hold all
> the device specific infos.
>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@...il.com>
> ---
> v2:
> - mentioned the introduction of the i2c_device_id table in the commit
Maybe adding the i2c_device_id table could be done in a separate patch
with a Fixes tag? That would help backporting (and I think changes like
this are worth it).
> - get i2c_/spi_get_device_id only with device get match fails
> - removed the generic KX_define
> - removed the kx022a_device_type enum
> - added comments for the chip_info struct elements
> - fixed errors pointed out by the kernel test robot
>
> drivers/iio/accel/kionix-kx022a-i2c.c | 20 ++++-
> drivers/iio/accel/kionix-kx022a-spi.c | 21 ++++--
> drivers/iio/accel/kionix-kx022a.c | 102 ++++++++++++++++----------
> drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++++-
> 4 files changed, 146 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
> index e6fd02d931b6..ce299d0446f7 100644
> --- a/drivers/iio/accel/kionix-kx022a-i2c.c
> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c
> @@ -15,6 +15,7 @@
> static int kx022a_i2c_probe(struct i2c_client *i2c)
> {
> struct device *dev = &i2c->dev;
> + const struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
>
> if (!i2c->irq) {
> @@ -22,16 +23,28 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
> return -EINVAL;
> }
>
> - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
> + chip_info = device_get_match_data(&i2c->dev);
> + if (!chip_info) {
> + const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
> + chip_info = (const struct kx022a_chip_info *)id->driver_data;
> + }
> +
> + regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap),
> "Failed to initialize Regmap\n");
>
> - return kx022a_probe_internal(dev);
> + return kx022a_probe_internal(dev, chip_info);
> }
>
> +static const struct i2c_device_id kx022a_i2c_id[] = {
> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
> +
> static const struct of_device_id kx022a_of_match[] = {
> - { .compatible = "kionix,kx022a", },
> + { .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> @@ -42,6 +55,7 @@ static struct i2c_driver kx022a_i2c_driver = {
> .of_match_table = kx022a_of_match,
> },
> .probe_new = kx022a_i2c_probe,
> + .id_table = kx022a_i2c_id,
> };
> module_i2c_driver(kx022a_i2c_driver);
>
> diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
> index 9cd047f7b346..7bc81588e40e 100644
> --- a/drivers/iio/accel/kionix-kx022a-spi.c
> +++ b/drivers/iio/accel/kionix-kx022a-spi.c
> @@ -15,6 +15,7 @@
> static int kx022a_spi_probe(struct spi_device *spi)
> {
> struct device *dev = &spi->dev;
> + const struct kx022a_chip_info *chip_info;
> struct regmap *regmap;
>
> if (!spi->irq) {
> @@ -22,22 +23,28 @@ static int kx022a_spi_probe(struct spi_device *spi)
> return -EINVAL;
> }
>
> - regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
> + chip_info = device_get_match_data(&spi->dev);
> + if (!chip_info) {
> + const struct spi_device_id *id = spi_get_device_id(spi);
> + chip_info = (const struct kx022a_chip_info *)id->driver_data;
> + }
> +
> + regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
> if (IS_ERR(regmap))
> return dev_err_probe(dev, PTR_ERR(regmap),
> "Failed to initialize Regmap\n");
>
> - return kx022a_probe_internal(dev);
> + return kx022a_probe_internal(dev, chip_info);
> }
>
> -static const struct spi_device_id kx022a_id[] = {
> - { "kx022a" },
> +static const struct spi_device_id kx022a_spi_id[] = {
Did you have a reason to change the struct name? Please, keep the
original name if there is no reason to change, it helps decreasing the
size of the patch...
> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
> { }
> };
> -MODULE_DEVICE_TABLE(spi, kx022a_id);
> +MODULE_DEVICE_TABLE(spi, kx022a_spi_id);
...this would not need to be changed if original name was kept...
>
> static const struct of_device_id kx022a_of_match[] = {
> - { .compatible = "kionix,kx022a", },
> + { .compatible = "kionix,kx022a", .data = &kx022a_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(of, kx022a_of_match);
> @@ -48,7 +55,7 @@ static struct spi_driver kx022a_spi_driver = {
> .of_match_table = kx022a_of_match,
> },
> .probe = kx022a_spi_probe,
> - .id_table = kx022a_id,
> + .id_table = kx022a_spi_id,
...and this neither.
> };
> module_spi_driver(kx022a_spi_driver);
>
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 70530005cad3..7f9a2c29790b 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -48,7 +48,7 @@ enum {
> KX022A_STATE_FIFO,
> };
>
> -/* Regmap configs */
> +/* kx022a Regmap configs */
> static const struct regmap_range kx022a_volatile_ranges[] = {
> {
> .range_min = KX022A_REG_XHP_L,
> @@ -138,7 +138,7 @@ static const struct regmap_access_table kx022a_nir_regs = {
> .n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
> };
>
> -const struct regmap_config kx022a_regmap = {
> +static const struct regmap_config kx022a_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> .volatile_table = &kx022a_volatile_regs,
> @@ -149,7 +149,6 @@ const struct regmap_config kx022a_regmap = {
> .max_register = KX022A_MAX_REGISTER,
> .cache_type = REGCACHE_RBTREE,
> };
> -EXPORT_SYMBOL_NS_GPL(kx022a_regmap, IIO_KX022A);
>
> struct kx022a_data {
> struct regmap *regmap;
> @@ -208,7 +207,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> { }
> };
Does this compile? Shouldn't the chip_info be added in the struct
kx022a_data?
>
> -#define KX022A_ACCEL_CHAN(axis, index) \
> +#define KX022A_ACCEL_CHAN(axis, reg, index) \
> { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -220,7 +219,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> .ext_info = kx022a_ext_info, \
> - .address = KX022A_REG_##axis##OUT_L, \
> + .address = reg, \
> .scan_index = index, \
> .scan_type = { \
> .sign = 's', \
> @@ -231,9 +230,9 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
> }
>
> static const struct iio_chan_spec kx022a_channels[] = {
> - KX022A_ACCEL_CHAN(X, 0),
> - KX022A_ACCEL_CHAN(Y, 1),
> - KX022A_ACCEL_CHAN(Z, 2),
> + KX022A_ACCEL_CHAN(X, KX022A_REG_XOUT_L, 0),
> + KX022A_ACCEL_CHAN(Y, KX022A_REG_YOUT_L, 1),
> + KX022A_ACCEL_CHAN(Z, KX022A_REG_ZOUT_L, 2),
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> @@ -332,10 +331,10 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> int ret;
>
> if (on)
> - ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
> + ret = regmap_set_bits(data->regmap, data->chip_info->cntl,
> KX022A_MASK_PC1);
Hm, do you have the "chip_info" member added in kx022a_data? I didn't
spot it from this patch.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
Powered by blists - more mailing lists