| Message ID | 3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: Add support for Kionix/ROHM KX132 accelerometer | expand |
Hi Mehdi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on next-20230316]
[cannot apply to linus/master v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230317/202303170813.jSOLGCL5-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/iio/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303170813.jSOLGCL5-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/iio/accel/kionix-kx022a.c: In function '__kx022a_fifo_flush':
>> drivers/iio/accel/kionix-kx022a.c:598:9: warning: ISO C90 forbids variable length array 'buffer' [-Wvla]
598 | __le16 buffer[data->chip_info->fifo_length * 3];
| ^~~~~~
--
drivers/iio/accel/kionix-kx022a-i2c.c: In function 'kx022a_i2c_probe':
>> drivers/iio/accel/kionix-kx022a-i2c.c:27:19: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
27 | chip_info = device_get_match_data(&i2c->dev);
| ^
drivers/iio/accel/kionix-kx022a-i2c.c:29:27: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
29 | chip_info = (const struct kx022a_chip_info *) id->driver_data;
| ^
--
drivers/iio/accel/kionix-kx022a-spi.c: In function 'kx022a_spi_probe':
>> drivers/iio/accel/kionix-kx022a-spi.c:27:19: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
27 | chip_info = device_get_match_data(&spi->dev);
| ^
drivers/iio/accel/kionix-kx022a-spi.c:29:27: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
29 | chip_info = (const struct kx022a_chip_info *) id->driver_data;
| ^
vim +/buffer +598 drivers/iio/accel/kionix-kx022a.c
593
594 static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
595 bool irq)
596 {
597 struct kx022a_data *data = iio_priv(idev);
> 598 __le16 buffer[data->chip_info->fifo_length * 3];
599 uint64_t sample_period;
600 int count, fifo_bytes;
601 bool renable = false;
602 int64_t tstamp;
603 int ret, i;
604
605 fifo_bytes = kx022a_get_fifo_bytes(data);
606 count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES;
607 if (!count)
608 return 0;
609
610 /*
611 * If we are being called from IRQ handler we know the stored timestamp
612 * is fairly accurate for the last stored sample. Otherwise, if we are
613 * called as a result of a read operation from userspace and hence
614 * before the watermark interrupt was triggered, take a timestamp
615 * now. We can fall anywhere in between two samples so the error in this
616 * case is at most one sample period.
617 */
618 if (!irq) {
619 /*
620 * We need to have the IRQ disabled or we risk of messing-up
621 * the timestamps. If we are ran from IRQ, then the
622 * IRQF_ONESHOT has us covered - but if we are ran by the
623 * user-space read we need to disable the IRQ to be on a safe
624 * side. We do this usng synchronous disable so that if the
625 * IRQ thread is being ran on other CPU we wait for it to be
626 * finished.
627 */
628 disable_irq(data->irq);
629 renable = true;
630
631 data->old_timestamp = data->timestamp;
632 data->timestamp = iio_get_time_ns(idev);
633 }
634
635 /*
636 * Approximate timestamps for each of the sample based on the sampling
637 * frequency, timestamp for last sample and number of samples.
638 *
639 * We'd better not use the current bandwidth settings to compute the
640 * sample period. The real sample rate varies with the device and
641 * small variation adds when we store a large number of samples.
642 *
643 * To avoid this issue we compute the actual sample period ourselves
644 * based on the timestamp delta between the last two flush operations.
645 */
646 if (data->old_timestamp) {
647 sample_period = data->timestamp - data->old_timestamp;
648 do_div(sample_period, count);
649 } else {
650 sample_period = data->odr_ns;
651 }
652 tstamp = data->timestamp - (count - 1) * sample_period;
653
654 if (samples && count > samples) {
655 /*
656 * Here we leave some old samples to the buffer. We need to
657 * adjust the timestamp to match the first sample in the buffer
658 * or we will miscalculate the sample_period at next round.
659 */
660 data->timestamp -= (count - samples) * sample_period;
661 count = samples;
662 }
663
664 fifo_bytes = count * KX_FIFO_SAMPLES_SIZE_BYTES;
665 ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read,
666 &buffer[0], fifo_bytes);
667 if (ret)
668 goto renable_out;
669
670 for (i = 0; i < count; i++) {
671 __le16 *sam = &buffer[i * 3];
672 __le16 *chs;
673 int bit;
674
675 chs = &data->scan.channels[0];
676 for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
677 chs[bit] = sam[bit];
678
679 iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
680
681 tstamp += sample_period;
682 }
683
684 ret = count;
685
686 renable_out:
687 if (renable)
688 enable_irq(data->irq);
689
690 return ret;
691 }
692
Hi Mehdi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on next-20230317]
[cannot apply to linus/master v6.3-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: i386-randconfig-a011-20230313 (https://download.01.org/0day-ci/archive/20230317/202303171208.uYJzdkrv-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303171208.uYJzdkrv-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
>> drivers/iio/accel/kionix-kx022a-i2c.c:27:12: error: assigning to 'struct kx022a_chip_info *' from 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = device_get_match_data(&i2c->dev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/kionix-kx022a-i2c.c:29:13: error: assigning to 'struct kx022a_chip_info *' from 'const struct kx022a_chip_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = (const struct kx022a_chip_info *) id->driver_data;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
--
>> drivers/iio/accel/kionix-kx022a.c:598:16: warning: variable length array used [-Wvla]
__le16 buffer[data->chip_info->fifo_length * 3];
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
--
>> drivers/iio/accel/kionix-kx022a-spi.c:27:12: error: assigning to 'struct kx022a_chip_info *' from 'const void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = device_get_match_data(&spi->dev);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/accel/kionix-kx022a-spi.c:29:13: error: assigning to 'struct kx022a_chip_info *' from 'const struct kx022a_chip_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
chip_info = (const struct kx022a_chip_info *) id->driver_data;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
vim +27 drivers/iio/accel/kionix-kx022a-i2c.c
14
15 static int kx022a_i2c_probe(struct i2c_client *i2c)
16 {
17 struct device *dev = &i2c->dev;
18 struct kx022a_chip_info *chip_info;
19 struct regmap *regmap;
20 const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
21
22 if (!i2c->irq) {
23 dev_err(dev, "No IRQ configured\n");
24 return -EINVAL;
25 }
26
> 27 chip_info = device_get_match_data(&i2c->dev);
28 if (!chip_info)
> 29 chip_info = (const struct kx022a_chip_info *) id->driver_data;
30
31 regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
32 if (IS_ERR(regmap))
33 return dev_err_probe(dev, PTR_ERR(regmap),
34 "Failed to initialize Regmap\n");
35
36 return kx022a_probe_internal(dev, chip_info);
37 }
38
On Fri, Mar 17, 2023 at 12:48:36AM +0100, Mehdi Djait wrote: > Refactor the kx022a driver implementation to make it more > generic and extensible. > Add the chip_info structure will to the driver's private > data to hold all the device specific infos. > Move the enum, struct and constants definitions to the header > file. Please, compile and test before sending. ... > .driver = { > - .name = "kx022a-spi", > + .name = "kx022a-spi", > .of_match_table = kx022a_of_match, > }, What was changed here? ... > - .id_table = kx022a_id, > + .id_table = kx022a_spi_id, Why do we need this change? ... > - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a", > + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel", > dev_name(data->dev)); Shouldn't you use the name from chip info? ... > +#define KX_MASK_BRES16 BIT(6) > + > + One blank line is enough. > #define KX022A_REG_WHO 0x0f > #define KX022A_ID 0xc8
Hi Andy, On Fri, Mar 17, 2023 at 02:06:39PM +0200, Andy Shevchenko wrote: > On Fri, Mar 17, 2023 at 12:48:36AM +0100, Mehdi Djait wrote: > > Refactor the kx022a driver implementation to make it more > > generic and extensible. > > Add the chip_info structure will to the driver's private > > data to hold all the device specific infos. > > Move the enum, struct and constants definitions to the header > > file. > > Please, compile and test before sending. My bad, I ignored the warnings... I will fix it. > > ... > > > .driver = { > > - .name = "kx022a-spi", > > + .name = "kx022a-spi", > > .of_match_table = kx022a_of_match, > > }, > > What was changed here? Nothing. I will fix it > > ... > > > - .id_table = kx022a_id, > > + .id_table = kx022a_spi_id, > > Why do we need this change? > For consistency: i2c: .id_table = kx022a_i2c_id, spi: .id_table = kx022a_spi_id, > ... > > > - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a", > > + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel", > > dev_name(data->dev)); > > Shouldn't you use the name from chip info? I can use the name from chip info. dev_name(data->dev) is the original implementation > > ... > > > +#define KX_MASK_BRES16 BIT(6) > > + > > + > > One blank line is enough. I will change it in v2 -- Kind Regards Mehdi Djait
On Fri, 17 Mar 2023 00:48:36 +0100 Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > Refactor the kx022a driver implementation to make it more > generic and extensible. > Add the chip_info structure will to the driver's private > data to hold all the device specific infos. > Move the enum, struct and constants definitions to the header > file. You also introduce an i2c_device_id table Without that I think module autoloading will be broken anyway so that's definitely a good thing to add. A few comments inline. Mostly around reducing the name changes. Wild cards (or simply shorted 'generic' prefixes like KX_) almost always bite back in the long run. Hence we generally just try to name things after the first device that they were relevant to. Thanks, Jonathan > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > --- > drivers/iio/accel/kionix-kx022a-i2c.c | 19 +- > drivers/iio/accel/kionix-kx022a-spi.c | 22 +- > drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++-------------- > drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++-- > 4 files changed, 274 insertions(+), 184 deletions(-) > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > index e6fd02d931b6..21c4c0ae1a68 100644 > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > @@ -15,23 +15,35 @@ > static int kx022a_i2c_probe(struct i2c_client *i2c) > { > struct device *dev = &i2c->dev; > + struct kx022a_chip_info *chip_info; > struct regmap *regmap; > + const struct i2c_device_id *id = i2c_client_get_device_id(i2c); > > if (!i2c->irq) { > dev_err(dev, "No IRQ configured\n"); > return -EINVAL; > } > > - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap); > + chip_info = device_get_match_data(&i2c->dev); > + if (!chip_info) > + chip_info = (const struct kx022a_chip_info *) id->driver_data; Get id only if the device_get_match_data() fails. if (!chip_info) { const struct i2c_device_id *id = i2c_client_get_device_id(i2c); chip_info = (const struct kx...) } > + > + 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)&kx_chip_info[KX022A] }, If there are a small set and we aren't ever going to index the chip_info structure we might be better off not bothering with the enum and instead using a separate instance of the structure for each chip. .driver_data = (kernel_ulong_t)&kx022a_chip_info, etc. > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id); > + > static const struct of_device_id kx022a_of_match[] = { > - { .compatible = "kionix,kx022a", }, > + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] }, > { } > }; > MODULE_DEVICE_TABLE(of, kx022a_of_match); > @@ -42,6 +54,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..ec076af0f261 100644 > --- a/drivers/iio/accel/kionix-kx022a-spi.c > +++ b/drivers/iio/accel/kionix-kx022a-spi.c > @@ -15,40 +15,46 @@ > static int kx022a_spi_probe(struct spi_device *spi) > { > struct device *dev = &spi->dev; > + struct kx022a_chip_info *chip_info; > struct regmap *regmap; > + const struct spi_device_id *id = spi_get_device_id(spi); > > if (!spi->irq) { > dev_err(dev, "No IRQ configured\n"); > return -EINVAL; > } > > - regmap = devm_regmap_init_spi(spi, &kx022a_regmap); > + chip_info = device_get_match_data(&spi->dev); > + if (!chip_info) As above. Get the id only if we are going to use it. > + 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[] = { > + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] }, > { } > }; > -MODULE_DEVICE_TABLE(spi, kx022a_id); > +MODULE_DEVICE_TABLE(spi, kx022a_spi_id); > > static const struct of_device_id kx022a_of_match[] = { > - { .compatible = "kionix,kx022a", }, > + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] }, > { } > }; > MODULE_DEVICE_TABLE(of, kx022a_of_match); > > static struct spi_driver kx022a_spi_driver = { > .driver = { > - .name = "kx022a-spi", > + .name = "kx022a-spi", > .of_match_table = kx022a_of_match, > }, > .probe = kx022a_spi_probe, > - .id_table = kx022a_id, > + .id_table = kx022a_spi_id, > }; > module_spi_driver(kx022a_spi_driver); > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index 8dac0337c249..27e8642aa8f5 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -26,29 +26,7 @@ .. > -#define KX022A_ACCEL_CHAN(axis, index) \ > +#define KX022A_ACCEL_CHAN(axis, index, device) \ > { \ > .type = IIO_ACCEL, \ > .modified = 1, \ > @@ -220,7 +158,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 = device##_REG_##axis##OUT_L, \ I'm not particularly keen on this because it is hard to search for. It wasn't great before, but it's getting worse. Perhaps just put the fully defined address in as a parameter to the macro. > .scan_index = index, \ > .scan_type = { \ > .sign = 's', \ > @@ -231,9 +169,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, 0, KX022A), > + KX022A_ACCEL_CHAN(Y, 1, KX022A), > + KX022A_ACCEL_CHAN(Z, 2, KX022A), > IIO_CHAN_SOFT_TIMESTAMP(3), > }; > > @@ -286,6 +224,34 @@ static const int kx022a_scale_table[][2] = { > { 4788, 403320 }, > }; > > +const struct kx022a_chip_info kx_chip_info[] = { > + [KX022A] = { > + .name = "kx022a", > + .type = KX022A, > + .regmap_config = &kx022a_regmap_config, > + .channels = kx022a_channels, > + .num_channels = ARRAY_SIZE(kx022a_channels), > + .fifo_length = KX022A_FIFO_LENGTH, > + .who = KX022A_REG_WHO, > + .id = KX022A_ID, > + .cntl = KX022A_REG_CNTL, > + .cntl2 = KX022A_REG_CNTL2, > + .odcntl = KX022A_REG_ODCNTL, > + .buf_cntl1 = KX022A_REG_BUF_CNTL1, > + .buf_cntl2 = KX022A_REG_BUF_CNTL2, > + .buf_clear = KX022A_REG_BUF_CLEAR, > + .buf_status1 = KX022A_REG_BUF_STATUS_1, > + .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL, > + .buf_read = KX022A_REG_BUF_READ, > + .inc1 = KX022A_REG_INC1, > + .inc4 = KX022A_REG_INC4, > + .inc5 = KX022A_REG_INC5, > + .inc6 = KX022A_REG_INC6, > + .xout_l = KX022A_REG_XOUT_L, > + }, > +}; > +EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A); ... > > +static int kx022a_get_fifo_bytes(struct kx022a_data *data) Factoring this out looks like an unrelated change. Fine to do it but should be a separate patch. If you need a device type specific version of this add a function pointer to your chip_info structure. Given you don't add one for the next patch I think this is just refactoring and so fine to do, but needs to be in a separate patch from this one with a description that says why you are doing it. > +{ > + struct device *dev = regmap_get_device(data->regmap); > + __le16 buf_status; > + int ret, fifo_bytes; > + > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); > + if (ret) { > + dev_err(dev, "Error reading buffer status\n"); > + return ret; > + } > + > + buf_status &= data->chip_info->buf_smp_lvl_mask; > + fifo_bytes = le16_to_cpu(buf_status); > + > + /* > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note > + * is that full 258 bytes of data is indicated using the max value 255. > + */ > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) > + fifo_bytes = KX022A_FIFO_MAX_BYTES; > + > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > + > + return fifo_bytes; > +} > + > static int kx022a_drop_fifo_contents(struct kx022a_data *data) > { > /* > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data) > */ > data->timestamp = 0; > > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0); > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0); > } > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > bool irq) > { > struct kx022a_data *data = iio_priv(idev); > - struct device *dev = regmap_get_device(data->regmap); > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > + __le16 buffer[data->chip_info->fifo_length * 3]; > uint64_t sample_period; > int count, fifo_bytes; > bool renable = false; > int64_t tstamp; > int ret, i; > > - ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); > - if (ret) { > - dev_err(dev, "Error reading buffer status\n"); > - return ret; > - } > - > - /* Let's not overflow if we for some reason get bogus value from i2c */ > - if (fifo_bytes == KX022A_FIFO_FULL_VALUE) > - fifo_bytes = KX022A_FIFO_MAX_BYTES; > - > - if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) > - dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > - > - count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES; > + fifo_bytes = kx022a_get_fifo_bytes(data); > + count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES; > if (!count) > return 0; ... > > @@ -969,25 +949,25 @@ static int kx022a_chip_init(struct kx022a_data *data) > */ > msleep(1); > > - ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val, > - !(val & KX022A_MASK_SRST), > - KX022A_SOFT_RESET_WAIT_TIME_US, > - KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US); > + ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val, > + !(val & KX_MASK_SRST), > + KX_SOFT_RESET_WAIT_TIME_US, > + KX_SOFT_RESET_TOTAL_WAIT_TIME_US); Where ever there are lots of accesses to data->chip_info I'd add a local chip_info variable to improve readabilty a bit. Might be worth doing the same with regmap (in a different patch) > if (ret) { > dev_err(data->dev, "Sensor reset %s\n", > - val & KX022A_MASK_SRST ? "timeout" : "fail#"); > + val & KX_MASK_SRST ? "timeout" : "fail#"); > return ret; > } > > - ret = regmap_reinit_cache(data->regmap, &kx022a_regmap); > + ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config); > if (ret) { > dev_err(data->dev, "Failed to reinit reg cache\n"); > return ret; > } > > /* set data res 16bit */ > - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, > - KX022A_MASK_BRES16); > + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, > + KX_MASK_BRES16); > if (ret) { > dev_err(data->dev, "Failed to set data resolution\n"); > return ret; > @@ -996,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data) > return kx022a_prepare_irq_pin(data); > } > > -int kx022a_probe_internal(struct device *dev) > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) > { > static const char * const regulator_names[] = {"io-vdd", "vdd"}; > struct iio_trigger *indio_trig; > @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev) > return -ENOMEM; > > data = iio_priv(idev); > + data->chip_info = chip_info; > > /* > * VDD is the analog and digital domain voltage supply and > @@ -1033,37 +1014,37 @@ int kx022a_probe_internal(struct device *dev) > if (ret && ret != -ENODEV) > return dev_err_probe(dev, ret, "failed to enable regulator\n"); > > - ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id); > + ret = regmap_read(regmap, data->chip_info->who, &chip_id); I think you have chip_info available as a local variable. Use that in this function to shorten lines with no loss of readability. > if (ret) > return dev_err_probe(dev, ret, "Failed to access sensor\n"); > > - if (chip_id != KX022A_ID) { > + if (chip_id != data->chip_info->id) { Recently we've tried to avoid introduce error returns on a failure to match and instead just warn and go ahead with assumption that we have a correct dt-binding telling us that some new device with a different ID is backwards compatible with the old one. Obviously not part of this patch though but something to think about later (if you don't do it later in this series). > dev_err(dev, "unsupported device 0x%x\n", chip_id); > return -EINVAL; > } ... > > data->regmap = regmap; > data->dev = dev; > data->irq = irq; > - data->odr_ns = KX022A_DEFAULT_PERIOD_NS; > + data->odr_ns = KX_DEFAULT_PERIOD_NS; > mutex_init(&data->mutex); > > - idev->channels = kx022a_channels; > - idev->num_channels = ARRAY_SIZE(kx022a_channels); > - idev->name = "kx022-accel"; Ah. Missed this naming in original driver review. We only normally postfix with accel in devices that have multiple sensors with separate drivers. Don't think that is true of the kx022a. It's ABI so we are stuck with it, but avoid repeating that issue for new devices. > + idev->channels = data->chip_info->channels; > + idev->num_channels = data->chip_info->num_channels; > + idev->name = data->chip_info->name; > idev->info = &kx022a_info; > idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; > idev->available_scan_masks = kx022a_scan_masks; > @@ -1112,7 +1093,7 @@ int kx022a_probe_internal(struct device *dev) > * No need to check for NULL. request_threaded_irq() defaults to > * dev_name() should the alloc fail. > */ > - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a", > + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel", > dev_name(data->dev)); Why name change here? It's not particularly important but if we can avoid it that would be a nice to have. > > ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler, > diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h > index 12424649d438..3bb40e9f5613 100644 > --- a/drivers/iio/accel/kionix-kx022a.h > +++ b/drivers/iio/accel/kionix-kx022a.h > @@ -11,24 +11,48 @@ > #include <linux/bits.h> > #include <linux/regmap.h> > > +#include <linux/iio/iio.h> > + > +/* Common for all supported devices */ > +#define KX_FIFO_SAMPLES_SIZE_BYTES 6 Even if they are used across multiple parts, don't rename them to be generic. It almost always causes churn / name clashes etc. It is absolutely fine to have driver specific naming that is based on the first supported part rather than trying to make it cover them all. > +#define KX_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC) > +#define KX_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC) > +#define KX_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC) > +#define KX_MASK_GSEL GENMASK(4, 3) > +#define KX_MASK_ODR GENMASK(3, 0) > +#define KX_MASK_WM_TH GENMASK(6, 0) > +#define KX_GSEL_SHIFT 3 > +#define KX_MASK_IEN BIT(5) > +#define KX_MASK_DRDY BIT(5) > +#define KX_MASK_PC1 BIT(7) > +#define KX_MASK_IPOL BIT(4) > +#define KX_IPOL_LOW 0 > +#define KX_MASK_ITYP BIT(3) > +#define KX_ITYP_LEVEL 0 > +#define KX_MASK_INS2_DRDY BIT(4) > +#define KX_MASK_WMI BIT(5) > +#define KX_MASK_BUF_EN BIT(7) > +#define KX_MASK_SRST BIT(7) > +#define KX_MASK_BRES16 BIT(6) > + > + > #define KX022A_REG_WHO 0x0f > #define KX022A_ID 0xc8 > > +#define KX022A_FIFO_LENGTH 43 > +#define KX022A_FIFO_FULL_VALUE 255 > +#define KX022A_FIFO_MAX_BYTES \ > + (KX022A_FIFO_LENGTH * KX_FIFO_SAMPLES_SIZE_BYTES) > + > #define KX022A_REG_CNTL2 0x19 > -#define KX022A_MASK_SRST BIT(7) > #define KX022A_REG_CNTL 0x18 > -#define KX022A_MASK_PC1 BIT(7) > #define KX022A_MASK_RES BIT(6) > -#define KX022A_MASK_DRDY BIT(5) > -#define KX022A_MASK_GSEL GENMASK(4, 3) > -#define KX022A_GSEL_SHIFT 3 > #define KX022A_GSEL_2 0x0 > #define KX022A_GSEL_4 BIT(3) > #define KX022A_GSEL_8 BIT(4) > #define KX022A_GSEL_16 GENMASK(4, 3) > > #define KX022A_REG_INS2 0x13 > -#define KX022A_MASK_INS2_DRDY BIT(4) > #define KX122_MASK_INS2_WMI BIT(5) > > #define KX022A_REG_XHP_L 0x0 > @@ -45,38 +69,104 @@ > #define KX022A_REG_MAN_WAKE 0x2c > > #define KX022A_REG_BUF_CNTL1 0x3a > -#define KX022A_MASK_WM_TH GENMASK(6, 0) > #define KX022A_REG_BUF_CNTL2 0x3b > -#define KX022A_MASK_BUF_EN BIT(7) > -#define KX022A_MASK_BRES16 BIT(6) > #define KX022A_REG_BUF_STATUS_1 0x3c > #define KX022A_REG_BUF_STATUS_2 0x3d > +#define KX022A_MASK_BUF_SMP_LVL GENMASK(6, 0) > #define KX022A_REG_BUF_CLEAR 0x3e > #define KX022A_REG_BUF_READ 0x3f > -#define KX022A_MASK_ODR GENMASK(3, 0) > #define KX022A_ODR_SHIFT 3 > #define KX022A_FIFO_MAX_WMI_TH 41 > > #define KX022A_REG_INC1 0x1c > #define KX022A_REG_INC5 0x20 > #define KX022A_REG_INC6 0x21 > -#define KX022A_MASK_IEN BIT(5) > -#define KX022A_MASK_IPOL BIT(4) > #define KX022A_IPOL_LOW 0 > -#define KX022A_IPOL_HIGH KX022A_MASK_IPOL1 > -#define KX022A_MASK_ITYP BIT(3) > -#define KX022A_ITYP_PULSE KX022A_MASK_ITYP > -#define KX022A_ITYP_LEVEL 0 > +#define KX022A_IPOL_HIGH KX_MASK_IPOL > +#define KX022A_ITYP_PULSE KX_MASK_ITYP > > #define KX022A_REG_INC4 0x1f > -#define KX022A_MASK_WMI BIT(5) > > #define KX022A_REG_SELF_TEST 0x60 > #define KX022A_MAX_REGISTER 0x60 > > +enum kx022a_device_type { > + KX022A, > +}; As below. I'd avoid using the enum unless needed. That can make sense where a driver supports lots of devices but I don't think it does here. > + > +enum { > + KX_STATE_SAMPLE, > + KX_STATE_FIFO, > +}; > + > +enum { > + AXIS_X, > + AXIS_Y, > + AXIS_Z, > + AXIS_MAX > +}; > + > struct device; > > -int kx022a_probe_internal(struct device *dev); > -extern const struct regmap_config kx022a_regmap; > +struct kx022a_chip_info { > + const char *name; > + enum kx022a_device_type type; > + const struct regmap_config *regmap_config; > + const struct iio_chan_spec *channels; > + unsigned int num_channels; > + unsigned int fifo_length; > + u8 who; Some of these are no immediately obvious so either rename the field so it is more obvious what it is, or add comments. > + u8 id; > + u8 cntl; > + u8 cntl2; > + u8 odcntl; > + u8 buf_cntl1; > + u8 buf_cntl2; > + u8 buf_clear; > + u8 buf_status1; > + u16 buf_smp_lvl_mask; > + u8 buf_read; > + u8 inc1; > + u8 inc4; > + u8 inc5; > + u8 inc6; > + u8 xout_l; > +}; > + > +struct kx022a_data { Why move this to the header? Unless there is a strong reason I'd prefer this to stay down in the .c file. > + const struct kx022a_chip_info *chip_info; > + struct regmap *regmap; > + struct iio_trigger *trig; > + struct device *dev; > + struct iio_mount_matrix orientation; > + int64_t timestamp, old_timestamp; > + > + int irq; > + int inc_reg; > + int ien_reg; > + > + unsigned int state; > + unsigned int odr_ns; > + > + bool trigger_enabled; > + /* > + * Prevent toggling the sensor stby/active state (PC1 bit) in the > + * middle of a configuration, or when the fifo is enabled. Also, > + * protect the data stored/retrieved from this structure from > + * concurrent accesses. > + */ > + struct mutex mutex; > + u8 watermark; > + > + /* 3 x 16bit accel data + timestamp */ > + __le16 buffer[8] __aligned(IIO_DMA_MINALIGN); > + struct { > + __le16 channels[3]; > + s64 ts __aligned(8); > + } scan; > +}; > + > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info); > +extern const struct kx022a_chip_info kx_chip_info[]; As mentioned in the bus specific driver, given there is a small set and we need the enum just to index this, I'd just have one per supported device. extern const struct kx022a_chip_info kx022a_chip_info; extern const struct kx022a_chip_info kx321_chip_info; etc > > #endif
Hi Mehdi and Jonathan, Just my take on couple of comments from Jonathan :) I still have my own review to do though... On 3/19/23 18:20, Jonathan Cameron wrote: > On Fri, 17 Mar 2023 00:48:36 +0100 > Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > >> Refactor the kx022a driver implementation to make it more >> generic and extensible. >> Add the chip_info structure will to the driver's private >> data to hold all the device specific infos. >> Move the enum, struct and constants definitions to the header >> file. > > You also introduce an i2c_device_id table > > Without that I think module autoloading will be broken anyway so that's > definitely a good thing to add. I am pretty sure the autoloading worked for OF-systems. But yes, adding the i2c_device_id is probably a good idea. Thanks. > A few comments inline. Mostly around reducing the name changes. > Wild cards (or simply shorted 'generic' prefixes like KX_) > almost always bite back in the long run. Hence we generally just try > to name things after the first device that they were relevant to. I must say I disagree on this with you Jonathan. I know wildcards tend to get confusing - but I still like the idea of showing which of the definitions are IC specific and which ones are generic or at least used by more than one variant - especially as long as we only have two supported ICs. I definitely like the macro naming added by Mehdi. This approach has been very helpful for me for example in the BD718x7 (BD71837/BD71847/BD71850) PMIC driver. My take on this is: 1) I like the generic KX_define. 2) I would not try adding wildcards like KX_X22 - to denote support for 122 and 022 - while not supporting 132 - in my experience - that won't scale. 3) I definitely like the idea of using exact model number prefix for 'stuff' which is intended to work only on one exact model. Regarding the 3) - I am not so strict on how the register/mask defines are handled - I _like_ the 1) 2) 3) approach above - but mask/register defines tend to get set (correctly) once and not required to be looked up after this. But. When the 'stuff' is functions - this gets very useful as one is very often required to see which functions are executed on which IC variant. Same goes to structs. So, if we manage to convince Jonathan about the naming, then I like what yoo had here! I would hovever do it in two steps. I would at first do renaming patch where the generic defines were renamed - without any functional changes - and only then add the kx132 stuff in a subsequent patch. That would simplify seeing which changes are just renaming and which are functional ones. But here, I must go with the wind - if subsystem maintainer says the code should not have naming like this - then I have no say over it... :/ >> >> +static const struct i2c_device_id kx022a_i2c_id[] = { >> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] }, > If there are a small set and we aren't ever going to index the chip_info structure > we might be better off not bothering with the enum and instead using a separate > instance of the structure for each chip. > I kind of like also the table added by Mehdi. I admit I was at first just thinking that we should have a pointer to the struct here without any tables - but... After I took a peek in the kionix-kx022a.c - I kind of liked the table and not exporting the struct names. So, I don't have a strong opinion on this. I think it's worth noting that this driver could (maybe easily enough) be extended to support also a few other kionix accelerometers. Maybe, if we don't scare Mehdi away, we will see a few other variants supported as well ;) >> data->regmap = regmap; >> data->dev = dev; >> data->irq = irq; >> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS; >> + data->odr_ns = KX_DEFAULT_PERIOD_NS; >> mutex_init(&data->mutex); >> >> - idev->channels = kx022a_channels; >> - idev->num_channels = ARRAY_SIZE(kx022a_channels); >> - idev->name = "kx022-accel"; > > Ah. Missed this naming in original driver review. We only normally > postfix with accel in devices that have multiple sensors with separate > drivers. Don't think that is true of the kx022a. Ouch. I am not 100% sure but may be you didn't miss it. It may be I just missed fixing this because your comment here sounds somewhat familiar to me! (Or then you commented on suffix in driver-name). > It's ABI so we are stuck with it, but avoid repeating that issue > for new devices. > >> >> +enum kx022a_device_type { >> + KX022A, >> +}; > > As below. I'd avoid using the enum unless needed. > That can make sense where a driver supports lots of devices but I don't think > it does here. Well, I know it is usually not too clever to be prepared for the future stuff too well. But - I don't think the enum and table are adding much of complexity? I am saying this as I think this driver could be extended to support also kx022 (without the A), kx023, kx122. I've also seen some references to model kx022A-120B (but I have no idea what's the story there or if that IC is publicly available). Maybe Mehdi would like to extend this driver further after the KX132 is done ;) >> -int kx022a_probe_internal(struct device *dev); >> -extern const struct regmap_config kx022a_regmap; >> +struct kx022a_chip_info { >> + const char *name; >> + enum kx022a_device_type type; >> + const struct regmap_config *regmap_config; >> + const struct iio_chan_spec *channels; >> + unsigned int num_channels; >> + unsigned int fifo_length; >> + u8 who; > Some of these are no immediately obvious so either rename the > field so it is more obvious what it is, or add comments. I would vote for adding a comment :) I like the who. Both the band and this member here :) Data-sheet has register named as "who_am_i" - so I don't think this name is too obfuscating - and what matters to me - it is short yet meaningful. >> + u8 id; >> + u8 cntl; >> + u8 cntl2; >> + u8 odcntl; >> + u8 buf_cntl1; >> + u8 buf_cntl2; >> + u8 buf_clear; >> + u8 buf_status1; >> + u16 buf_smp_lvl_mask; >> + u8 buf_read; >> + u8 inc1; >> + u8 inc4; >> + u8 inc5; >> + u8 inc6; >> + u8 xout_l; >> +}; >> + >> +struct kx022a_data { > > Why move this to the header? Unless there is a strong reason > I'd prefer this to stay down in the .c file. So would I. It's definitely nice to be able to see the struct in the same file where the code referencing it is. Yours, -- Matti
On 3/17/23 01:48, Mehdi Djait wrote: > Refactor the kx022a driver implementation to make it more > generic and extensible. > Add the chip_info structure will to the driver's private > data to hold all the device specific infos. > Move the enum, struct and constants definitions to the header > file. > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > --- > drivers/iio/accel/kionix-kx022a-i2c.c | 19 +- > drivers/iio/accel/kionix-kx022a-spi.c | 22 +- > drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++-------------- > drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++-- > 4 files changed, 274 insertions(+), 184 deletions(-) > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > index e6fd02d931b6..21c4c0ae1a68 100644 > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > @@ -15,23 +15,35 @@ > static int kx022a_i2c_probe(struct i2c_client *i2c) > { > struct device *dev = &i2c->dev; > + struct kx022a_chip_info *chip_info; > struct regmap *regmap; > + const struct i2c_device_id *id = i2c_client_get_device_id(i2c); > > if (!i2c->irq) { > dev_err(dev, "No IRQ configured\n"); > return -EINVAL; > } > > - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap); > + chip_info = device_get_match_data(&i2c->dev); > + if (!chip_info) > + 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"); Hm. I would like to pull the regmap_config out of the chip_info struct. As far as I see, the regmap_config is only needed in these bus specific files. On the other hand, the chip-info is only needed in the kionix-kx022a.c file, right? So, maybe you could here just get the regmap_config based on the chip-id (enum value you added - the data pointer in match tables could be just the enum value indicating the IC type). Then, you could pass this enum value to kx022a_probe_internal() - and the chip-info struct could be selected in the kionix-kx022a.c based on it. That way you would not need the struct chip-info here or regmap_config in kionix-kx022a.c. Same in the *-spi.c Something like: enum { KIONIX_IC_KX022A, KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */ }; static const struct of_device_id kx022a_of_match[] = { { .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A }, ... chip_id = device_get_match_data(&i2c->dev); regmap_cfg = kx022a_kx_regmap_cfg[chip_id]; regmap = devm_regmap_init_i2c(i2c, regmap_cfg); ... return kx022a_probe_internal(dev, chip_id); Do you think that would work? OTOH, to really benefit from this we should probably pull out the regmap-configs from the kionix-kx022a.c. I am not really sure where we should put it then though. Hence, if there is no good ideas how to split the config and chip-info so they are only available/used where needed - then I am also Ok with the current approach. > - > -struct kx022a_data { > - struct regmap *regmap; > - struct iio_trigger *trig; > - struct device *dev; > - struct iio_mount_matrix orientation; > - int64_t timestamp, old_timestamp; > - > - int irq; > - int inc_reg; > - int ien_reg; > - > - unsigned int state; > - unsigned int odr_ns; > - > - bool trigger_enabled; > - /* > - * Prevent toggling the sensor stby/active state (PC1 bit) in the > - * middle of a configuration, or when the fifo is enabled. Also, > - * protect the data stored/retrieved from this structure from > - * concurrent accesses. > - */ > - struct mutex mutex; > - u8 watermark; > - > - /* 3 x 16bit accel data + timestamp */ > - __le16 buffer[8] __aligned(IIO_DMA_MINALIGN); > - struct { > - __le16 channels[3]; > - s64 ts __aligned(8); > - } scan; > -}; As mentioned by Jonathan - It'd be better to keep this struct in C-file. > > +const struct kx022a_chip_info kx_chip_info[] = { > + [KX022A] = { > + .name = "kx022a", > + .type = KX022A, > + .regmap_config = &kx022a_regmap_config, As mentioned above, the regmap config is not really needed after the regmap is initialized. Id prefer this not being part of the chip info. > + .channels = kx022a_channels, > + .num_channels = ARRAY_SIZE(kx022a_channels), > + .fifo_length = KX022A_FIFO_LENGTH, > + .who = KX022A_REG_WHO, > + .id = KX022A_ID, > + .cntl = KX022A_REG_CNTL, > + .cntl2 = KX022A_REG_CNTL2, > + .odcntl = KX022A_REG_ODCNTL, > + .buf_cntl1 = KX022A_REG_BUF_CNTL1, > + .buf_cntl2 = KX022A_REG_BUF_CNTL2, > + .buf_clear = KX022A_REG_BUF_CLEAR, > + .buf_status1 = KX022A_REG_BUF_STATUS_1, > + .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL, > + .buf_read = KX022A_REG_BUF_READ, > + .inc1 = KX022A_REG_INC1, > + .inc4 = KX022A_REG_INC4, > + .inc5 = KX022A_REG_INC5, > + .inc6 = KX022A_REG_INC6, > + .xout_l = KX022A_REG_XOUT_L, > + }, > +}; > +EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A); > + > static int kx022a_read_avail(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > const int **vals, int *type, int *length, > @@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev, > } > } > > -#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC) > - > static void kx022a_reg2freq(unsigned int val, int *val1, int *val2) > { > - *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0]; > - *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1]; > + *val1 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][0]; > + *val2 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][1]; > } > As mentioned elsewhere, doing the renaming separately from the functional changes will ease the reviewing. > > +static int kx022a_get_fifo_bytes(struct kx022a_data *data) > +{ > + struct device *dev = regmap_get_device(data->regmap); > + __le16 buf_status; > + int ret, fifo_bytes; > + > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); > + if (ret) { > + dev_err(dev, "Error reading buffer status\n"); > + return ret; > + } > + > + buf_status &= data->chip_info->buf_smp_lvl_mask; > + fifo_bytes = le16_to_cpu(buf_status); > + > + /* > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note > + * is that full 258 bytes of data is indicated using the max value 255. > + */ > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) > + fifo_bytes = KX022A_FIFO_MAX_BYTES; > + > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > + > + return fifo_bytes; > +} I like adding this function. Here I agree with Jonathan - having a device specific functions would clarify this a bit. The KX022A "quirk" is a bit confusing. You could then get rid of the buf_smp_lvl_mask. > + > static int kx022a_drop_fifo_contents(struct kx022a_data *data) > { > /* > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data) > */ > data->timestamp = 0; > > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0); > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0); > } > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > bool irq) > { > struct kx022a_data *data = iio_priv(idev); > - struct device *dev = regmap_get_device(data->regmap); > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > + __le16 buffer[data->chip_info->fifo_length * 3]; I don't like this. Having the length of an array decided at run-time is not something I appreciate. Maybe you could just always reserve the memory so that the largest FIFO gets supported. I am just wondering how large arrays we can safely allocate from the stack? > @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data) > goto unlock_out; > > /* Enable buffer */ > - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, > - KX022A_MASK_BUF_EN); > + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, > + KX_MASK_BUF_EN); > if (ret) > goto unlock_out; > > - data->state |= KX022A_STATE_FIFO; > + data->state |= KX_STATE_FIFO; > ret = regmap_set_bits(data->regmap, data->ien_reg, > - KX022A_MASK_WMI); > + KX_MASK_WMI); I think this fits to one line now. (even on my screen) > if (ret) > goto unlock_out; > > -int kx022a_probe_internal(struct device *dev) > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) As mentioned elsewhere, this might also work if the chip-type enum was passed here as parameter. That way the bus specific part would not need to know about the struct chip_info... > { > static const char * const regulator_names[] = {"io-vdd", "vdd"}; > struct iio_trigger *indio_trig; > @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev) > return -ENOMEM; > > data = iio_priv(idev); > + data->chip_info = chip_info; ...Here you could then pick the correct chip_info based on the chip-type enum. In that case I'd like to get the regmap_config(s) in own file. Not sure how that would look like though. All in all, I like how this looks like. Nice job! Yours, -- Matti
On Mon, Mar 20, 2023 at 11:35:06AM +0200, Matti Vaittinen wrote: > On 3/17/23 01:48, Mehdi Djait wrote: > > Refactor the kx022a driver implementation to make it more > > generic and extensible. > > Add the chip_info structure will to the driver's private > > data to hold all the device specific infos. > > Move the enum, struct and constants definitions to the header > > file. ... > Something like: > > enum { > KIONIX_IC_KX022A, > KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */ > }; > > static const struct of_device_id kx022a_of_match[] = { > { .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A }, > ... > > chip_id = device_get_match_data(&i2c->dev); No, please avoid putting plain integers as pointers of driver_data. The problem you introduced with your suggestion is impossibility to distinguish 0 and NULL, beyond other not good things (like missing castings which are ugly).
On Mon, 20 Mar 2023 09:17:54 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hi Mehdi and Jonathan, > > Just my take on couple of comments from Jonathan :) I still have my own > review to do though... > > On 3/19/23 18:20, Jonathan Cameron wrote: > > On Fri, 17 Mar 2023 00:48:36 +0100 > > Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > > > >> Refactor the kx022a driver implementation to make it more > >> generic and extensible. > >> Add the chip_info structure will to the driver's private > >> data to hold all the device specific infos. > >> Move the enum, struct and constants definitions to the header > >> file. > > > > You also introduce an i2c_device_id table > > > > Without that I think module autoloading will be broken anyway so that's > > definitely a good thing to add. > > I am pretty sure the autoloading worked for OF-systems. But yes, adding > the i2c_device_id is probably a good idea. Thanks. Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692 > > > A few comments inline. Mostly around reducing the name changes. > > Wild cards (or simply shorted 'generic' prefixes like KX_) > > almost always bite back in the long run. Hence we generally just try > > to name things after the first device that they were relevant to. > > I must say I disagree on this with you Jonathan. I know wildcards tend > to get confusing - but I still like the idea of showing which of the > definitions are IC specific and which ones are generic or at least used > by more than one variant - especially as long as we only have two > supported ICs. I definitely like the macro naming added by Mehdi. This > approach has been very helpful for me for example in the BD718x7 > (BD71837/BD71847/BD71850) PMIC driver. My take on this is: > > 1) I like the generic KX_define. We already have other kionix drivers that don't use these defines. This is less of an issue if they are very local - so pushed down to the C file, but I still don't like the implication that they extend to a broad range of devices. > 2) I would not try adding wildcards like KX_X22 - to denote support for > 122 and 022 - while not supporting 132 - in my experience - that won't > scale. I think this already runs into this problem or at least sets the driver up to hit it very soon. The reality is that these definitions are shared by the 2 parts supported so far. 3rd part comes along and I'd be willing to place a bet that at least one of these definitions doesn't apply. So we end up with a mess converting it back to a specific name. I've gone down this path many times before and it very rarely works out. > 3) I definitely like the idea of using exact model number prefix for > 'stuff' which is intended to work only on one exact . When you have 2 devices it is easy to separate the 'generic' from the 'specific'. That breaks when you have 3. If we are sure there won't be a 3rd device supported by this driver then fair enough... > > Regarding the 3) - I am not so strict on how the register/mask defines > are handled - I _like_ the 1) 2) 3) approach above - but mask/register > defines tend to get set (correctly) once and not required to be looked > up after this. But. When the 'stuff' is functions - this gets very > useful as one is very often required to see which functions are executed > on which IC variant. Same goes to structs. Given they tend to be accessed via a function pointer, even functions are only set up the once. For these I'm fine with a nasty listing type approach with multiple part names in the function defintion. That doesn't scale great either as lots of parts get added but it at least calls out which function covers which parts. > > So, if we manage to convince Jonathan about the naming, then I like what > yoo had here! I would hovever do it in two steps. I would at first do > renaming patch where the generic defines were renamed - without any > functional changes - and only then add the kx132 stuff in a subsequent > patch. That would simplify seeing which changes are just renaming and > which are functional ones. > > But here, I must go with the wind - if subsystem maintainer says the > code should not have naming like this - then I have no say over it... :/ If we have truely universal defines - sometimes this happens for WHO AM I registers for example as they are the same over all devices from a manufacturer (more or less anyway) then the broad forms are fine. Otherwise it just tends to end up as a mess if lots of parts added. > > >> > >> +static const struct i2c_device_id kx022a_i2c_id[] = { > >> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] }, > > If there are a small set and we aren't ever going to index the chip_info structure > > we might be better off not bothering with the enum and instead using a separate > > instance of the structure for each chip. > > > > I kind of like also the table added by Mehdi. I admit I was at first > just thinking that we should have a pointer to the struct here without > any tables - but... After I took a peek in the kionix-kx022a.c - I kind > of liked the table and not exporting the struct names. So, I don't have > a strong opinion on this. > > I think it's worth noting that this driver could (maybe easily enough) > be extended to support also a few other kionix accelerometers. Maybe, if > we don't scare Mehdi away, we will see a few other variants supported as > well ;) This one wasn't a particularly important bit of feedback. I'm fine with the table, though seems slightly less readable to my eyes. > > >> data->regmap = regmap; > >> data->dev = dev; > >> data->irq = irq; > >> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS; > >> + data->odr_ns = KX_DEFAULT_PERIOD_NS; > >> mutex_init(&data->mutex); > >> > >> - idev->channels = kx022a_channels; > >> - idev->num_channels = ARRAY_SIZE(kx022a_channels); > >> - idev->name = "kx022-accel"; > > > > Ah. Missed this naming in original driver review. We only normally > > postfix with accel in devices that have multiple sensors with separate > > drivers. Don't think that is true of the kx022a. > > Ouch. I am not 100% sure but may be you didn't miss it. It may be I just > missed fixing this because your comment here sounds somewhat familiar to > me! (Or then you commented on suffix in driver-name). Meh. This stuff happens and at the end of the day it's a magic string that userspace can match against. No userspace knows all of them anyway so most likely it's just provided in a 'selection' box for a user or encoded in a custom script / config file. So not hugely important for it to have the simplest possible form. > > > It's ABI so we are stuck with it, but avoid repeating that issue > > for new devices. > > > >> > >> +enum kx022a_device_type { > >> + KX022A, > >> +}; > > > > As below. I'd avoid using the enum unless needed. > > That can make sense where a driver supports lots of devices but I don't think > > it does here. > > Well, I know it is usually not too clever to be prepared for the future > stuff too well. But - I don't think the enum and table are adding much > of complexity? I am saying this as I think this driver could be extended > to support also kx022 (without the A), kx023, kx122. I've also seen some > references to model kx022A-120B (but I have no idea what's the story > there or if that IC is publicly available). Maybe Mehdi would like to > extend this driver further after the KX132 is done ;) Not adding a lot, but you are going to end up with adding one line to an enum in the header for each new device, vs one extern line. So I'm not sure it saves anything either. > > >> -int kx022a_probe_internal(struct device *dev); > >> -extern const struct regmap_config kx022a_regmap; > >> +struct kx022a_chip_info { > >> + const char *name; > >> + enum kx022a_device_type type; > >> + const struct regmap_config *regmap_config; > >> + const struct iio_chan_spec *channels; > >> + unsigned int num_channels; > >> + unsigned int fifo_length; > >> + u8 who; > > Some of these are no immediately obvious so either rename the > > field so it is more obvious what it is, or add comments. > > I would vote for adding a comment :) I like the who. Both the band and > this member here :) Data-sheet has register named as "who_am_i" - so I > don't think this name is too obfuscating - and what matters to me - it > is short yet meaningful. > > >> + u8 id; > >> + u8 cntl; > >> + u8 cntl2; > >> + u8 odcntl; > >> + u8 buf_cntl1; > >> + u8 buf_cntl2; > >> + u8 buf_clear; > >> + u8 buf_status1; > >> + u16 buf_smp_lvl_mask; > >> + u8 buf_read; > >> + u8 inc1; > >> + u8 inc4; > >> + u8 inc5; > >> + u8 inc6; > >> + u8 xout_l; > >> +}; > >> + > >> +struct kx022a_data { > > > > Why move this to the header? Unless there is a strong reason > > I'd prefer this to stay down in the .c file. > > So would I. It's definitely nice to be able to see the struct in the > same file where the code referencing it is. > > > Yours, > -- Matti Thanks, Jonathan >
On Mon, 20 Mar 2023 11:35:06 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 3/17/23 01:48, Mehdi Djait wrote: > > Refactor the kx022a driver implementation to make it more > > generic and extensible. > > Add the chip_info structure will to the driver's private > > data to hold all the device specific infos. > > Move the enum, struct and constants definitions to the header > > file. > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > --- > > drivers/iio/accel/kionix-kx022a-i2c.c | 19 +- > > drivers/iio/accel/kionix-kx022a-spi.c | 22 +- > > drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++-------------- > > drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++-- > > 4 files changed, 274 insertions(+), 184 deletions(-) > > > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > > index e6fd02d931b6..21c4c0ae1a68 100644 > > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > > @@ -15,23 +15,35 @@ > > static int kx022a_i2c_probe(struct i2c_client *i2c) > > { > > struct device *dev = &i2c->dev; > > + struct kx022a_chip_info *chip_info; > > struct regmap *regmap; > > + const struct i2c_device_id *id = i2c_client_get_device_id(i2c); > > > > if (!i2c->irq) { > > dev_err(dev, "No IRQ configured\n"); > > return -EINVAL; > > } > > > > - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap); > > + chip_info = device_get_match_data(&i2c->dev); > > + if (!chip_info) > > + 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"); > > Hm. I would like to pull the regmap_config out of the chip_info struct. > As far as I see, the regmap_config is only needed in these bus specific > files. On the other hand, the chip-info is only needed in the > kionix-kx022a.c file, right? > I disagree. We've moved quite a few drivers away from the enum route because the indirection doesn't add anything useful and leads to nasty casting to enums. In particular, we have to avoid using enum value of 0 if we want to check if there is a match. For a pointer that's an easy check against NULL. The regmap is product specific so makes sense as part of the chip_info structure. > So, maybe you could here just get the regmap_config based on the chip-id > (enum value you added - the data pointer in match tables could be just > the enum value indicating the IC type). Then, you could pass this enum > value to kx022a_probe_internal() - and the chip-info struct could be > selected in the kionix-kx022a.c based on it. That way you would not need > the struct chip-info here or regmap_config in kionix-kx022a.c. Same in > the *-spi.c > > Something like: > > enum { > KIONIX_IC_KX022A, > KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */ > }; > > static const struct of_device_id kx022a_of_match[] = { > { .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A }, > ... > > chip_id = device_get_match_data(&i2c->dev); This fails for probes using the i2c_device_id table entries. So you need to check for invalid entry. Unfortunately that is a NULL return which you can't detect if your enum has a value of 0. > > regmap_cfg = kx022a_kx_regmap_cfg[chip_id]; > regmap = devm_regmap_init_i2c(i2c, regmap_cfg); > ... > return kx022a_probe_internal(dev, chip_id); > > Do you think that would work? It would work with the enum starting at 1, and it's a pattern that used to be common. Less so now because with multiple firmware types we want to be able to check trivially if we have a match. > > OTOH, to really benefit from this we should probably pull out the > regmap-configs from the kionix-kx022a.c. I am not really sure where we > should put it then though. Hence, if there is no good ideas how to split > the config and chip-info so they are only available/used where needed - > then I am also Ok with the current approach. Definitely stick to current approach. If I had the time I'd rip out all the code useing enums in match tables. It's bitten us a few times with nasty to track down bugs that only affect more obscure ways of binding the driver. ... > > > > > +static int kx022a_get_fifo_bytes(struct kx022a_data *data) > > +{ > > + struct device *dev = regmap_get_device(data->regmap); > > + __le16 buf_status; > > + int ret, fifo_bytes; > > + > > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); > > + if (ret) { > > + dev_err(dev, "Error reading buffer status\n"); > > + return ret; > > + } > > + > > + buf_status &= data->chip_info->buf_smp_lvl_mask; > > + fifo_bytes = le16_to_cpu(buf_status); > > + > > + /* > > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 > > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to > > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in > > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note > > + * is that full 258 bytes of data is indicated using the max value 255. > > + */ > > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) > > + fifo_bytes = KX022A_FIFO_MAX_BYTES; > > + > > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) > > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > > + > > + return fifo_bytes; > > +} > > I like adding this function. Here I agree with Jonathan - having a > device specific functions would clarify this a bit. The KX022A "quirk" > is a bit confusing. You could then get rid of the buf_smp_lvl_mask. I'd missed the type quirk. Good point, definitely have a callback. Get rid of that 'type' element of the chip_info. That is a bad design pattern as it doesn't scale to lots of devices as you end up with big switch statements. > > > + > > static int kx022a_drop_fifo_contents(struct kx022a_data *data) > > { > > /* > > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data) > > */ > > data->timestamp = 0; > > > > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0); > > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0); > > } > > > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > bool irq) > > { > > struct kx022a_data *data = iio_priv(idev); > > - struct device *dev = regmap_get_device(data->regmap); > > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > > + __le16 buffer[data->chip_info->fifo_length * 3]; > > I don't like this. Having the length of an array decided at run-time is > not something I appreciate. Maybe you could just always reserve the > memory so that the largest FIFO gets supported. I am just wondering how > large arrays we can safely allocate from the stack? I'd missed this as well. Definitely don't have a variable length array. Allocate it as a buffer accessed via a pointer in kx022a_data > > > > if (ret) > > goto unlock_out; > > > > > -int kx022a_probe_internal(struct device *dev) > > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) > > As mentioned elsewhere, this might also work if the chip-type enum was > passed here as parameter. That way the bus specific part would not need > to know about the struct chip_info... It only knows there is a pointer. Doesn't need to know more than that. + argument against as above. Jonathan
On 3/20/23 14:34, Jonathan Cameron wrote: > On Mon, 20 Mar 2023 11:35:06 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 3/17/23 01:48, Mehdi Djait wrote: >> >> OTOH, to really benefit from this we should probably pull out the >> regmap-configs from the kionix-kx022a.c. I am not really sure where we >> should put it then though. Hence, if there is no good ideas how to split >> the config and chip-info so they are only available/used where needed - >> then I am also Ok with the current approach. > > Definitely stick to current approach. If I had the time I'd > rip out all the code useing enums in match tables. It's bitten us > a few times with nasty to track down bugs that only affect more obscure > ways of binding the driver. > Seems like Jonathan has a strong opinion on this. Please follow his and Andy's guidance on this one and forget my comment. Yours, -- Matti
Hi Mehdi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on next-20230320]
[cannot apply to linus/master v6.3-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/3ddca10a4c03c3a64afb831cc9dd1e01fe89d305.1679009443.git.mehdi.djait.k%40gmail.com
patch subject: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
config: loongarch-randconfig-s032-20230319 (https://download.01.org/0day-ci/archive/20230321/202303210809.RAQ7nfl7-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/40c75341c42d0e5bea5d73961202978a4be41cd2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mehdi-Djait/dt-bindings-iio-Add-KX132-accelerometer/20230317-075056
git checkout 40c75341c42d0e5bea5d73961202978a4be41cd2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303210809.RAQ7nfl7-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct kx022a_chip_info *chip_info @@ got void const * @@
drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: expected struct kx022a_chip_info *chip_info
drivers/iio/accel/kionix-kx022a-spi.c:27:19: sparse: got void const *
>> drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: sparse: incorrect type in assignment (different modifiers) @@ expected struct kx022a_chip_info *chip_info @@ got struct kx022a_chip_info const * @@
drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: expected struct kx022a_chip_info *chip_info
drivers/iio/accel/kionix-kx022a-spi.c:29:27: sparse: got struct kx022a_chip_info const *
vim +27 drivers/iio/accel/kionix-kx022a-spi.c
14
15 static int kx022a_spi_probe(struct spi_device *spi)
16 {
17 struct device *dev = &spi->dev;
18 struct kx022a_chip_info *chip_info;
19 struct regmap *regmap;
20 const struct spi_device_id *id = spi_get_device_id(spi);
21
22 if (!spi->irq) {
23 dev_err(dev, "No IRQ configured\n");
24 return -EINVAL;
25 }
26
> 27 chip_info = device_get_match_data(&spi->dev);
28 if (!chip_info)
> 29 chip_info = (const struct kx022a_chip_info *) id->driver_data;
30
31 regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
32 if (IS_ERR(regmap))
33 return dev_err_probe(dev, PTR_ERR(regmap),
34 "Failed to initialize Regmap\n");
35
36 return kx022a_probe_internal(dev, chip_info);
37 }
38
Hello everyone, On Mon, Mar 20, 2023 at 12:24:47PM +0000, Jonathan Cameron wrote: > On Mon, 20 Mar 2023 09:17:54 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > Hi Mehdi and Jonathan, > > > > Just my take on couple of comments from Jonathan :) I still have my own > > review to do though... > > > > On 3/19/23 18:20, Jonathan Cameron wrote: > > > On Fri, 17 Mar 2023 00:48:36 +0100 > > > Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > > > > > >> Refactor the kx022a driver implementation to make it more > > >> generic and extensible. > > >> Add the chip_info structure will to the driver's private > > >> data to hold all the device specific infos. > > >> Move the enum, struct and constants definitions to the header > > >> file. > > > > > > You also introduce an i2c_device_id table > > > > > > Without that I think module autoloading will be broken anyway so that's > > > definitely a good thing to add. > > > > I am pretty sure the autoloading worked for OF-systems. But yes, adding > > the i2c_device_id is probably a good idea. Thanks. > > Ah. Maybe that issue only occurred for SPI - I'd assumed it was more general. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/?id=5fa6863ba692 > > > > > > A few comments inline. Mostly around reducing the name changes. > > > Wild cards (or simply shorted 'generic' prefixes like KX_) > > > almost always bite back in the long run. Hence we generally just try > > > to name things after the first device that they were relevant to. > > > > I must say I disagree on this with you Jonathan. I know wildcards tend > > to get confusing - but I still like the idea of showing which of the > > definitions are IC specific and which ones are generic or at least used > > by more than one variant - especially as long as we only have two > > supported ICs. I definitely like the macro naming added by Mehdi. This > > approach has been very helpful for me for example in the BD718x7 > > (BD71837/BD71847/BD71850) PMIC driver. My take on this is: > > > > 1) I like the generic KX_define. > > We already have other kionix drivers that don't use these defines. > This is less of an issue if they are very local - so pushed down to the C file, > but I still don't like the implication that they extend to a broad range of > devices. > > > 2) I would not try adding wildcards like KX_X22 - to denote support for > > 122 and 022 - while not supporting 132 - in my experience - that won't > > scale. > > I think this already runs into this problem or at least sets the driver > up to hit it very soon. The reality is that these definitions are shared > by the 2 parts supported so far. 3rd part comes along and I'd be willing > to place a bet that at least one of these definitions doesn't apply. > So we end up with a mess converting it back to a specific name. > > I've gone down this path many times before and it very rarely works out. > > > 3) I definitely like the idea of using exact model number prefix for > > 'stuff' which is intended to work only on one exact . > > When you have 2 devices it is easy to separate the 'generic' from the > 'specific'. That breaks when you have 3. If we are sure there won't be > a 3rd device supported by this driver then fair enough... > > > > > Regarding the 3) - I am not so strict on how the register/mask defines > > are handled - I _like_ the 1) 2) 3) approach above - but mask/register > > defines tend to get set (correctly) once and not required to be looked > > up after this. But. When the 'stuff' is functions - this gets very > > useful as one is very often required to see which functions are executed > > on which IC variant. Same goes to structs. > > Given they tend to be accessed via a function pointer, even functions > are only set up the once. For these I'm fine with a nasty listing > type approach with multiple part names in the function defintion. > That doesn't scale great either as lots of parts get added but it at least > calls out which function covers which parts. > > > > > So, if we manage to convince Jonathan about the naming, then I like what > > yoo had here! I would hovever do it in two steps. I would at first do > > renaming patch where the generic defines were renamed - without any > > functional changes - and only then add the kx132 stuff in a subsequent > > patch. That would simplify seeing which changes are just renaming and > > which are functional ones. > > > > But here, I must go with the wind - if subsystem maintainer says the > > code should not have naming like this - then I have no say over it... :/ > > If we have truely universal defines - sometimes this happens for WHO AM I > registers for example as they are the same over all devices from a manufacturer > (more or less anyway) then the broad forms are fine. Otherwise it just tends > to end up as a mess if lots of parts added. I will remove the generic defines. I also really liked the idea of seperating the IC specific stuff from the generic ones. Better play it safe here, I can also see it becoming a mess in the long run. > > > > >> > > >> +static const struct i2c_device_id kx022a_i2c_id[] = { > > >> + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] }, > > > If there are a small set and we aren't ever going to index the chip_info structure > > > we might be better off not bothering with the enum and instead using a separate > > > instance of the structure for each chip. > > > > > > > I kind of like also the table added by Mehdi. I admit I was at first > > just thinking that we should have a pointer to the struct here without > > any tables - but... After I took a peek in the kionix-kx022a.c - I kind > > of liked the table and not exporting the struct names. So, I don't have > > a strong opinion on this. > > > > I think it's worth noting that this driver could (maybe easily enough) > > be extended to support also a few other kionix accelerometers. Maybe, if > > we don't scare Mehdi away, we will see a few other variants supported as > > well ;) > > This one wasn't a particularly important bit of feedback. I'm fine with the > table, though seems slightly less readable to my eyes. My reasoning behind it: when supporting multiple devices, having a single array of chip_info and one single EXPORT_SYMBOL is more concise and readable. > > > > > >> data->regmap = regmap; > > >> data->dev = dev; > > >> data->irq = irq; > > >> - data->odr_ns = KX022A_DEFAULT_PERIOD_NS; > > >> + data->odr_ns = KX_DEFAULT_PERIOD_NS; > > >> mutex_init(&data->mutex); > > >> > > >> - idev->channels = kx022a_channels; > > >> - idev->num_channels = ARRAY_SIZE(kx022a_channels); > > >> - idev->name = "kx022-accel"; > > > > > > Ah. Missed this naming in original driver review. We only normally > > > postfix with accel in devices that have multiple sensors with separate > > > drivers. Don't think that is true of the kx022a. > > > > Ouch. I am not 100% sure but may be you didn't miss it. It may be I just > > missed fixing this because your comment here sounds somewhat familiar to > > me! (Or then you commented on suffix in driver-name). > > Meh. This stuff happens and at the end of the day it's a magic string > that userspace can match against. No userspace knows all of them anyway > so most likely it's just provided in a 'selection' box for a user or encoded > in a custom script / config file. So not hugely important for it to have > the simplest possible form. > > > > > > It's ABI so we are stuck with it, but avoid repeating that issue > > > for new devices. > I will change the name in the chip_info of kx022a back to "kx022-accel" I am already using "kx132" as name for the newly supported device > > > > >> > > >> +enum kx022a_device_type { > > >> + KX022A, > > >> +}; > > > > > > As below. I'd avoid using the enum unless needed. > > > That can make sense where a driver supports lots of devices but I don't think > > > it does here. > > > > Well, I know it is usually not too clever to be prepared for the future > > stuff too well. But - I don't think the enum and table are adding much > > of complexity? I am saying this as I think this driver could be extended > > to support also kx022 (without the A), kx023, kx122. I've also seen some > > references to model kx022A-120B (but I have no idea what's the story > > there or if that IC is publicly available). Maybe Mehdi would like to > > extend this driver further after the KX132 is done ;) Yes, my goal is to support more devices and I want to make as easy as possible to extend this driver :) > > Not adding a lot, but you are going to end up with adding one line > to an enum in the header for each new device, vs one > extern line. So I'm not sure it saves anything either. > Using a separate instance of the chip_info structure for each chip means also an extra symbol export > > > > >> -int kx022a_probe_internal(struct device *dev); > > >> -extern const struct regmap_config kx022a_regmap; > > >> +struct kx022a_chip_info { > > >> + const char *name; > > >> + enum kx022a_device_type type; > > >> + const struct regmap_config *regmap_config; > > >> + const struct iio_chan_spec *channels; > > >> + unsigned int num_channels; > > >> + unsigned int fifo_length; > > >> + u8 who; > > > Some of these are no immediately obvious so either rename the > > > field so it is more obvious what it is, or add comments. > > > > I would vote for adding a comment :) I like the who. Both the band and > > this member here :) Data-sheet has register named as "who_am_i" - so I > > don't think this name is too obfuscating - and what matters to me - it > > is short yet meaningful. > > > > >> + u8 id; > > >> + u8 cntl; > > >> + u8 cntl2; > > >> + u8 odcntl; > > >> + u8 buf_cntl1; > > >> + u8 buf_cntl2; > > >> + u8 buf_clear; > > >> + u8 buf_status1; > > >> + u16 buf_smp_lvl_mask; > > >> + u8 buf_read; > > >> + u8 inc1; > > >> + u8 inc4; > > >> + u8 inc5; > > >> + u8 inc6; > > >> + u8 xout_l; > > >> +}; > > >> + > > >> +struct kx022a_data { > > > > > > Why move this to the header? Unless there is a strong reason > > > I'd prefer this to stay down in the .c file. > > > > So would I. It's definitely nice to be able to see the struct in the > > same file where the code referencing it is. no strong reason, I will move it back to the .c file -- Kind Regards Mehdi Djait
Hello Matti, > > +static int kx022a_get_fifo_bytes(struct kx022a_data *data) > > +{ > > + struct device *dev = regmap_get_device(data->regmap); > > + __le16 buf_status; > > + int ret, fifo_bytes; > > + > > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); > > + if (ret) { > > + dev_err(dev, "Error reading buffer status\n"); > > + return ret; > > + } > > + > > + buf_status &= data->chip_info->buf_smp_lvl_mask; > > + fifo_bytes = le16_to_cpu(buf_status); > > + > > + /* > > + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 > > + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to > > + * 258 bytes of sample data. The quirk to know is that the amount of bytes in > > + * the FIFO is advertised via 8 bit register (max value 255). The thing to note > > + * is that full 258 bytes of data is indicated using the max value 255. > > + */ > > + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) > > + fifo_bytes = KX022A_FIFO_MAX_BYTES; > > + > > + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) > > + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > > + > > + return fifo_bytes; > > +} > > I like adding this function. Here I agree with Jonathan - having a device > specific functions would clarify this a bit. The KX022A "quirk" is a bit > confusing. You could then get rid of the buf_smp_lvl_mask. my bad here, I should have made a separate patch and explained more ... buf_smp_lvl_mask is essential because kionix products use different number of bits to report "the number of data bytes that have been stored in the sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2 kx022a: 8bits kx132: 10bits kx12x: 11bits kx126: 12bits I think this function is quite generic and can be used for different kionix devices: - It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific mask - It takes care of the quirk of kx022a which is just a simple if statement > > > + > > static int kx022a_drop_fifo_contents(struct kx022a_data *data) > > { > > /* > > @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data) > > */ > > data->timestamp = 0; > > - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0); > > + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0); > > } > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > bool irq) > > { > > struct kx022a_data *data = iio_priv(idev); > > - struct device *dev = regmap_get_device(data->regmap); > > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > > + __le16 buffer[data->chip_info->fifo_length * 3]; > > I don't like this. Having the length of an array decided at run-time is not > something I appreciate. Maybe you could just always reserve the memory so > that the largest FIFO gets supported. I am just wondering how large arrays > we can safely allocate from the stack? I was stupid enough to ignore the warnings... I will take care of it in the v2 > > > > @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data) > > goto unlock_out; > > /* Enable buffer */ > > - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, > > - KX022A_MASK_BUF_EN); > > + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, > > + KX_MASK_BUF_EN); > > if (ret) > > goto unlock_out; > > - data->state |= KX022A_STATE_FIFO; > > + data->state |= KX_STATE_FIFO; > > ret = regmap_set_bits(data->regmap, data->ien_reg, > > - KX022A_MASK_WMI); > > + KX_MASK_WMI); > > I think this fits to one line now. (even on my screen) > > > if (ret) > > goto unlock_out; > > > -int kx022a_probe_internal(struct device *dev) > > +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) > > As mentioned elsewhere, this might also work if the chip-type enum was > passed here as parameter. That way the bus specific part would not need to > know about the struct chip_info... > > > { > > static const char * const regulator_names[] = {"io-vdd", "vdd"}; > > struct iio_trigger *indio_trig; > > @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev) > > return -ENOMEM; > > data = iio_priv(idev); > > + data->chip_info = chip_info; > > ...Here you could then pick the correct chip_info based on the chip-type > enum. In that case I'd like to get the regmap_config(s) in own file. Not > sure how that would look like though. > > All in all, I like how this looks like. Nice job! Thank you for the feedback :) -- Kind Regards Mehdi Djait
On 3/21/23 17:56, Mehdi Djait wrote: > Hello Matti, > >>> +static int kx022a_get_fifo_bytes(struct kx022a_data *data) >>> +{ >>> + struct device *dev = regmap_get_device(data->regmap); >>> + __le16 buf_status; >>> + int ret, fifo_bytes; >>> + >>> + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); >>> + if (ret) { >>> + dev_err(dev, "Error reading buffer status\n"); >>> + return ret; >>> + } >>> + >>> + buf_status &= data->chip_info->buf_smp_lvl_mask; >>> + fifo_bytes = le16_to_cpu(buf_status); >>> + >>> + /* >>> + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 >>> + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to >>> + * 258 bytes of sample data. The quirk to know is that the amount of bytes in >>> + * the FIFO is advertised via 8 bit register (max value 255). The thing to note >>> + * is that full 258 bytes of data is indicated using the max value 255. >>> + */ >>> + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) >>> + fifo_bytes = KX022A_FIFO_MAX_BYTES; >>> + >>> + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) >>> + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); >>> + >>> + return fifo_bytes; >>> +} >> >> I like adding this function. Here I agree with Jonathan - having a device >> specific functions would clarify this a bit. The KX022A "quirk" is a bit >> confusing. You could then get rid of the buf_smp_lvl_mask. > > my bad here, I should have made a separate patch and explained more ... > buf_smp_lvl_mask is essential because kionix products use different > number of bits to report "the number of data bytes that have been stored in the > sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2 Yes, they have different size of FIFO, and the KX022A does also have the nasty "FIFO FULL" quirk. Due to this quirk and other differences I was suggesting you created own functions for kx022a and kx132. Eg something along the lines: static int kx022a_get_fifo_bytes(struct kx022a_data *data) { ... } static int kx132_get_fifo_bytes(struct kx022a_data *data) { ... } struct chip_info { ... int (*fifo_bytes)(struct kx022a_data *); }; and do the: fifo_bytes = kx022a_get_fifo_bytes; or fifo_bytes = kx132_get_fifo_bytes; in probe. That will also remove the need to check the IC variant for each FIFO read. If you did that you could remove the buf_smp_lvl_mask and maybe also the buf_statusX members from the chip_info struct (at least for now). You could also do regular read for KX022A and drop the endianess conversion for it. Bulk read is only needed for ICs with more than 8bits of FIFO status. Furthermore, the IC-type check could then go away and the above mentioned KX022A-specific handling would not be obfuscating the kx132 code. > > kx022a: 8bits > kx132: 10bits > kx12x: 11bits > kx126: 12bits > > I think this function is quite generic and can be used for different > kionix devices: > > - It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific > mask > - It takes care of the quirk of kx022a which is just a simple if statement Yes. Your function definitely works. And I do like the fact that you did own function for the "amount of data in fifo"-check. Still, the code would be little simpler and perform a tiny bit better if you did two functions instead of one. Yours, -- Matti
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c index e6fd02d931b6..21c4c0ae1a68 100644 --- a/drivers/iio/accel/kionix-kx022a-i2c.c +++ b/drivers/iio/accel/kionix-kx022a-i2c.c @@ -15,23 +15,35 @@ static int kx022a_i2c_probe(struct i2c_client *i2c) { struct device *dev = &i2c->dev; + struct kx022a_chip_info *chip_info; struct regmap *regmap; + const struct i2c_device_id *id = i2c_client_get_device_id(i2c); if (!i2c->irq) { dev_err(dev, "No IRQ configured\n"); return -EINVAL; } - regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap); + chip_info = device_get_match_data(&i2c->dev); + if (!chip_info) + 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)&kx_chip_info[KX022A] }, + { } +}; +MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id); + static const struct of_device_id kx022a_of_match[] = { - { .compatible = "kionix,kx022a", }, + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] }, { } }; MODULE_DEVICE_TABLE(of, kx022a_of_match); @@ -42,6 +54,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..ec076af0f261 100644 --- a/drivers/iio/accel/kionix-kx022a-spi.c +++ b/drivers/iio/accel/kionix-kx022a-spi.c @@ -15,40 +15,46 @@ static int kx022a_spi_probe(struct spi_device *spi) { struct device *dev = &spi->dev; + struct kx022a_chip_info *chip_info; struct regmap *regmap; + const struct spi_device_id *id = spi_get_device_id(spi); if (!spi->irq) { dev_err(dev, "No IRQ configured\n"); return -EINVAL; } - regmap = devm_regmap_init_spi(spi, &kx022a_regmap); + chip_info = device_get_match_data(&spi->dev); + if (!chip_info) + 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[] = { + { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx_chip_info[KX022A] }, { } }; -MODULE_DEVICE_TABLE(spi, kx022a_id); +MODULE_DEVICE_TABLE(spi, kx022a_spi_id); static const struct of_device_id kx022a_of_match[] = { - { .compatible = "kionix,kx022a", }, + { .compatible = "kionix,kx022a", .data = &kx_chip_info[KX022A] }, { } }; MODULE_DEVICE_TABLE(of, kx022a_of_match); static struct spi_driver kx022a_spi_driver = { .driver = { - .name = "kx022a-spi", + .name = "kx022a-spi", .of_match_table = kx022a_of_match, }, .probe = kx022a_spi_probe, - .id_table = kx022a_id, + .id_table = kx022a_spi_id, }; module_spi_driver(kx022a_spi_driver); diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c index 8dac0337c249..27e8642aa8f5 100644 --- a/drivers/iio/accel/kionix-kx022a.c +++ b/drivers/iio/accel/kionix-kx022a.c @@ -26,29 +26,7 @@ #include "kionix-kx022a.h" -/* - * The KX022A has FIFO which can store 43 samples of HiRes data from 2 - * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to - * 258 bytes of sample data. The quirk to know is that the amount of bytes in - * the FIFO is advertised via 8 bit register (max value 255). The thing to note - * is that full 258 bytes of data is indicated using the max value 255. - */ -#define KX022A_FIFO_LENGTH 43 -#define KX022A_FIFO_FULL_VALUE 255 -#define KX022A_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC) -#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC) - -/* 3 axis, 2 bytes of data for each of the axis */ -#define KX022A_FIFO_SAMPLES_SIZE_BYTES 6 -#define KX022A_FIFO_MAX_BYTES \ - (KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES) - -enum { - KX022A_STATE_SAMPLE, - KX022A_STATE_FIFO, -}; - -/* Regmap configs */ +/* kx022a Regmap configs */ static const struct regmap_range kx022a_volatile_ranges[] = { { .range_min = KX022A_REG_XHP_L, @@ -138,7 +116,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,39 +127,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; - struct iio_trigger *trig; - struct device *dev; - struct iio_mount_matrix orientation; - int64_t timestamp, old_timestamp; - - int irq; - int inc_reg; - int ien_reg; - - unsigned int state; - unsigned int odr_ns; - - bool trigger_enabled; - /* - * Prevent toggling the sensor stby/active state (PC1 bit) in the - * middle of a configuration, or when the fifo is enabled. Also, - * protect the data stored/retrieved from this structure from - * concurrent accesses. - */ - struct mutex mutex; - u8 watermark; - - /* 3 x 16bit accel data + timestamp */ - __le16 buffer[8] __aligned(IIO_DMA_MINALIGN); - struct { - __le16 channels[3]; - s64 ts __aligned(8); - } scan; -}; static const struct iio_mount_matrix * kx022a_get_mount_matrix(const struct iio_dev *idev, @@ -192,13 +137,6 @@ kx022a_get_mount_matrix(const struct iio_dev *idev, return &data->orientation; } -enum { - AXIS_X, - AXIS_Y, - AXIS_Z, - AXIS_MAX -}; - static const unsigned long kx022a_scan_masks[] = { BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z), 0 }; @@ -208,7 +146,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = { { } }; -#define KX022A_ACCEL_CHAN(axis, index) \ +#define KX022A_ACCEL_CHAN(axis, index, device) \ { \ .type = IIO_ACCEL, \ .modified = 1, \ @@ -220,7 +158,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 = device##_REG_##axis##OUT_L, \ .scan_index = index, \ .scan_type = { \ .sign = 's', \ @@ -231,9 +169,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, 0, KX022A), + KX022A_ACCEL_CHAN(Y, 1, KX022A), + KX022A_ACCEL_CHAN(Z, 2, KX022A), IIO_CHAN_SOFT_TIMESTAMP(3), }; @@ -286,6 +224,34 @@ static const int kx022a_scale_table[][2] = { { 4788, 403320 }, }; +const struct kx022a_chip_info kx_chip_info[] = { + [KX022A] = { + .name = "kx022a", + .type = KX022A, + .regmap_config = &kx022a_regmap_config, + .channels = kx022a_channels, + .num_channels = ARRAY_SIZE(kx022a_channels), + .fifo_length = KX022A_FIFO_LENGTH, + .who = KX022A_REG_WHO, + .id = KX022A_ID, + .cntl = KX022A_REG_CNTL, + .cntl2 = KX022A_REG_CNTL2, + .odcntl = KX022A_REG_ODCNTL, + .buf_cntl1 = KX022A_REG_BUF_CNTL1, + .buf_cntl2 = KX022A_REG_BUF_CNTL2, + .buf_clear = KX022A_REG_BUF_CLEAR, + .buf_status1 = KX022A_REG_BUF_STATUS_1, + .buf_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL, + .buf_read = KX022A_REG_BUF_READ, + .inc1 = KX022A_REG_INC1, + .inc4 = KX022A_REG_INC4, + .inc5 = KX022A_REG_INC5, + .inc6 = KX022A_REG_INC6, + .xout_l = KX022A_REG_XOUT_L, + }, +}; +EXPORT_SYMBOL_NS_GPL(kx_chip_info, IIO_KX022A); + static int kx022a_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, const int **vals, int *type, int *length, @@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev, } } -#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC) - static void kx022a_reg2freq(unsigned int val, int *val1, int *val2) { - *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0]; - *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1]; + *val1 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][0]; + *val2 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][1]; } static void kx022a_reg2scale(unsigned int val, unsigned int *val1, unsigned int *val2) { - val &= KX022A_MASK_GSEL; - val >>= KX022A_GSEL_SHIFT; + val &= KX_MASK_GSEL; + val >>= KX_GSEL_SHIFT; *val1 = kx022a_scale_table[val][0]; *val2 = kx022a_scale_table[val][1]; @@ -332,11 +296,11 @@ 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, - KX022A_MASK_PC1); + ret = regmap_set_bits(data->regmap, data->chip_info->cntl, + KX_MASK_PC1); else - ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL, - KX022A_MASK_PC1); + ret = regmap_clear_bits(data->regmap, data->chip_info->cntl, + KX_MASK_PC1); if (ret) dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret); @@ -403,8 +367,8 @@ static int kx022a_write_raw(struct iio_dev *idev, break; ret = regmap_update_bits(data->regmap, - KX022A_REG_ODCNTL, - KX022A_MASK_ODR, n); + data->chip_info->odcntl, + KX_MASK_ODR, n); data->odr_ns = kx022a_odrs[n]; kx022a_turn_on_unlock(data); break; @@ -424,9 +388,9 @@ static int kx022a_write_raw(struct iio_dev *idev, if (ret) break; - ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL, - KX022A_MASK_GSEL, - n << KX022A_GSEL_SHIFT); + ret = regmap_update_bits(data->regmap, data->chip_info->cntl, + KX_MASK_GSEL, + n << KX_GSEL_SHIFT); kx022a_turn_on_unlock(data); break; default: @@ -446,8 +410,8 @@ static int kx022a_fifo_set_wmi(struct kx022a_data *data) threshold = data->watermark; - return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1, - KX022A_MASK_WM_TH, threshold); + return regmap_update_bits(data->regmap, data->chip_info->buf_cntl1, + KX_MASK_WM_TH, threshold); } static int kx022a_get_axis(struct kx022a_data *data, @@ -489,11 +453,11 @@ static int kx022a_read_raw(struct iio_dev *idev, return ret; case IIO_CHAN_INFO_SAMP_FREQ: - ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val); + ret = regmap_read(data->regmap, data->chip_info->odcntl, ®val); if (ret) return ret; - if ((regval & KX022A_MASK_ODR) > + if ((regval & KX_MASK_ODR) > ARRAY_SIZE(kx022a_accel_samp_freq_table)) { dev_err(data->dev, "Invalid ODR\n"); return -EINVAL; @@ -504,7 +468,7 @@ static int kx022a_read_raw(struct iio_dev *idev, return IIO_VAL_INT_PLUS_MICRO; case IIO_CHAN_INFO_SCALE: - ret = regmap_read(data->regmap, KX022A_REG_CNTL, ®val); + ret = regmap_read(data->regmap, data->chip_info->cntl, ®val); if (ret < 0) return ret; @@ -531,8 +495,8 @@ static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val) { struct kx022a_data *data = iio_priv(idev); - if (val > KX022A_FIFO_LENGTH) - val = KX022A_FIFO_LENGTH; + if (val > data->chip_info->fifo_length) + val = data->chip_info->fifo_length; mutex_lock(&data->mutex); data->watermark = val; @@ -580,6 +544,37 @@ static const struct iio_dev_attr *kx022a_fifo_attributes[] = { NULL }; +static int kx022a_get_fifo_bytes(struct kx022a_data *data) +{ + struct device *dev = regmap_get_device(data->regmap); + __le16 buf_status; + int ret, fifo_bytes; + + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); + if (ret) { + dev_err(dev, "Error reading buffer status\n"); + return ret; + } + + buf_status &= data->chip_info->buf_smp_lvl_mask; + fifo_bytes = le16_to_cpu(buf_status); + + /* + * The KX022A has FIFO which can store 43 samples of HiRes data from 2 + * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to + * 258 bytes of sample data. The quirk to know is that the amount of bytes in + * the FIFO is advertised via 8 bit register (max value 255). The thing to note + * is that full 258 bytes of data is indicated using the max value 255. + */ + if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE) + fifo_bytes = KX022A_FIFO_MAX_BYTES; + + if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES) + dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); + + return fifo_bytes; +} + static int kx022a_drop_fifo_contents(struct kx022a_data *data) { /* @@ -593,35 +588,22 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data) */ data->timestamp = 0; - return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0); + return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0); } static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, bool irq) { struct kx022a_data *data = iio_priv(idev); - struct device *dev = regmap_get_device(data->regmap); - __le16 buffer[KX022A_FIFO_LENGTH * 3]; + __le16 buffer[data->chip_info->fifo_length * 3]; uint64_t sample_period; int count, fifo_bytes; bool renable = false; int64_t tstamp; int ret, i; - ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); - if (ret) { - dev_err(dev, "Error reading buffer status\n"); - return ret; - } - - /* Let's not overflow if we for some reason get bogus value from i2c */ - if (fifo_bytes == KX022A_FIFO_FULL_VALUE) - fifo_bytes = KX022A_FIFO_MAX_BYTES; - - if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) - dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); - - count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES; + fifo_bytes = kx022a_get_fifo_bytes(data); + count = fifo_bytes / KX_FIFO_SAMPLES_SIZE_BYTES; if (!count) return 0; @@ -679,8 +661,8 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, count = samples; } - fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES; - ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, + fifo_bytes = count * KX_FIFO_SAMPLES_SIZE_BYTES; + ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read, &buffer[0], fifo_bytes); if (ret) goto renable_out; @@ -733,20 +715,18 @@ static const struct iio_info kx022a_info = { static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en) { if (en) - return regmap_set_bits(data->regmap, KX022A_REG_CNTL, - KX022A_MASK_DRDY); + return regmap_set_bits(data->regmap, data->chip_info->cntl, + KX_MASK_DRDY); - return regmap_clear_bits(data->regmap, KX022A_REG_CNTL, - KX022A_MASK_DRDY); + return regmap_clear_bits(data->regmap, data->chip_info->cntl, + KX_MASK_DRDY); } static int kx022a_prepare_irq_pin(struct kx022a_data *data) { /* Enable IRQ1 pin. Set polarity to active low */ - int mask = KX022A_MASK_IEN | KX022A_MASK_IPOL | - KX022A_MASK_ITYP; - int val = KX022A_MASK_IEN | KX022A_IPOL_LOW | - KX022A_ITYP_LEVEL; + int mask = KX_MASK_IEN | KX_MASK_IPOL | KX_MASK_ITYP; + int val = KX_MASK_IEN | KX_IPOL_LOW | KX_ITYP_LEVEL; int ret; ret = regmap_update_bits(data->regmap, data->inc_reg, mask, val); @@ -754,7 +734,7 @@ static int kx022a_prepare_irq_pin(struct kx022a_data *data) return ret; /* We enable WMI to IRQ pin only at buffer_enable */ - mask = KX022A_MASK_INS2_DRDY; + mask = KX_MASK_INS2_DRDY; return regmap_set_bits(data->regmap, data->ien_reg, mask); } @@ -767,16 +747,16 @@ static int kx022a_fifo_disable(struct kx022a_data *data) if (ret) return ret; - ret = regmap_clear_bits(data->regmap, data->ien_reg, KX022A_MASK_WMI); + ret = regmap_clear_bits(data->regmap, data->ien_reg, KX_MASK_WMI); if (ret) goto unlock_out; - ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2, - KX022A_MASK_BUF_EN); + ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2, + KX_MASK_BUF_EN); if (ret) goto unlock_out; - data->state &= ~KX022A_STATE_FIFO; + data->state &= ~KX_STATE_FIFO; kx022a_drop_fifo_contents(data); @@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data) goto unlock_out; /* Enable buffer */ - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, - KX022A_MASK_BUF_EN); + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, + KX_MASK_BUF_EN); if (ret) goto unlock_out; - data->state |= KX022A_STATE_FIFO; + data->state |= KX_STATE_FIFO; ret = regmap_set_bits(data->regmap, data->ien_reg, - KX022A_MASK_WMI); + KX_MASK_WMI); if (ret) goto unlock_out; @@ -858,8 +838,8 @@ static irqreturn_t kx022a_trigger_handler(int irq, void *p) struct kx022a_data *data = iio_priv(idev); int ret; - ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer, - KX022A_FIFO_SAMPLES_SIZE_BYTES); + ret = regmap_bulk_read(data->regmap, data->chip_info->xout_l, data->buffer, + KX_FIFO_SAMPLES_SIZE_BYTES); if (ret < 0) goto err_read; @@ -879,7 +859,7 @@ static irqreturn_t kx022a_irq_handler(int irq, void *private) data->old_timestamp = data->timestamp; data->timestamp = iio_get_time_ns(idev); - if (data->state & KX022A_STATE_FIFO || data->trigger_enabled) + if (data->state & KX_STATE_FIFO || data->trigger_enabled) return IRQ_WAKE_THREAD; return IRQ_NONE; @@ -903,10 +883,10 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) ret = IRQ_HANDLED; } - if (data->state & KX022A_STATE_FIFO) { + if (data->state & KX_STATE_FIFO) { int ok; - ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true); + ok = __kx022a_fifo_flush(idev, data->chip_info->fifo_length, true); if (ok > 0) ret = IRQ_HANDLED; } @@ -927,7 +907,7 @@ static int kx022a_trigger_set_state(struct iio_trigger *trig, if (data->trigger_enabled == state) goto unlock_out; - if (data->state & KX022A_STATE_FIFO) { + if (data->state & KX_STATE_FIFO) { dev_warn(data->dev, "Can't set trigger when FIFO enabled\n"); ret = -EBUSY; goto unlock_out; @@ -959,7 +939,7 @@ static int kx022a_chip_init(struct kx022a_data *data) int ret, val; /* Reset the senor */ - ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST); + ret = regmap_write(data->regmap, data->chip_info->cntl2, KX_MASK_SRST); if (ret) return ret; @@ -969,25 +949,25 @@ static int kx022a_chip_init(struct kx022a_data *data) */ msleep(1); - ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val, - !(val & KX022A_MASK_SRST), - KX022A_SOFT_RESET_WAIT_TIME_US, - KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US); + ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val, + !(val & KX_MASK_SRST), + KX_SOFT_RESET_WAIT_TIME_US, + KX_SOFT_RESET_TOTAL_WAIT_TIME_US); if (ret) { dev_err(data->dev, "Sensor reset %s\n", - val & KX022A_MASK_SRST ? "timeout" : "fail#"); + val & KX_MASK_SRST ? "timeout" : "fail#"); return ret; } - ret = regmap_reinit_cache(data->regmap, &kx022a_regmap); + ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config); if (ret) { dev_err(data->dev, "Failed to reinit reg cache\n"); return ret; } /* set data res 16bit */ - ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, - KX022A_MASK_BRES16); + ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, + KX_MASK_BRES16); if (ret) { dev_err(data->dev, "Failed to set data resolution\n"); return ret; @@ -996,7 +976,7 @@ static int kx022a_chip_init(struct kx022a_data *data) return kx022a_prepare_irq_pin(data); } -int kx022a_probe_internal(struct device *dev) +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) { static const char * const regulator_names[] = {"io-vdd", "vdd"}; struct iio_trigger *indio_trig; @@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev) return -ENOMEM; data = iio_priv(idev); + data->chip_info = chip_info; /* * VDD is the analog and digital domain voltage supply and @@ -1033,37 +1014,37 @@ int kx022a_probe_internal(struct device *dev) if (ret && ret != -ENODEV) return dev_err_probe(dev, ret, "failed to enable regulator\n"); - ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id); + ret = regmap_read(regmap, data->chip_info->who, &chip_id); if (ret) return dev_err_probe(dev, ret, "Failed to access sensor\n"); - if (chip_id != KX022A_ID) { + if (chip_id != data->chip_info->id) { dev_err(dev, "unsupported device 0x%x\n", chip_id); return -EINVAL; } irq = fwnode_irq_get_byname(fwnode, "INT1"); if (irq > 0) { - data->inc_reg = KX022A_REG_INC1; - data->ien_reg = KX022A_REG_INC4; + data->inc_reg = data->chip_info->inc1; + data->ien_reg = data->chip_info->inc4; } else { irq = fwnode_irq_get_byname(fwnode, "INT2"); if (irq <= 0) return dev_err_probe(dev, irq, "No suitable IRQ\n"); - data->inc_reg = KX022A_REG_INC5; - data->ien_reg = KX022A_REG_INC6; + data->inc_reg = data->chip_info->inc5; + data->ien_reg = data->chip_info->inc6; } data->regmap = regmap; data->dev = dev; data->irq = irq; - data->odr_ns = KX022A_DEFAULT_PERIOD_NS; + data->odr_ns = KX_DEFAULT_PERIOD_NS; mutex_init(&data->mutex); - idev->channels = kx022a_channels; - idev->num_channels = ARRAY_SIZE(kx022a_channels); - idev->name = "kx022-accel"; + idev->channels = data->chip_info->channels; + idev->num_channels = data->chip_info->num_channels; + idev->name = data->chip_info->name; idev->info = &kx022a_info; idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; idev->available_scan_masks = kx022a_scan_masks; @@ -1112,7 +1093,7 @@ int kx022a_probe_internal(struct device *dev) * No need to check for NULL. request_threaded_irq() defaults to * dev_name() should the alloc fail. */ - name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a", + name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-accel", dev_name(data->dev)); ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler, diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h index 12424649d438..3bb40e9f5613 100644 --- a/drivers/iio/accel/kionix-kx022a.h +++ b/drivers/iio/accel/kionix-kx022a.h @@ -11,24 +11,48 @@ #include <linux/bits.h> #include <linux/regmap.h> +#include <linux/iio/iio.h> + +/* Common for all supported devices */ +#define KX_FIFO_SAMPLES_SIZE_BYTES 6 +#define KX_SOFT_RESET_WAIT_TIME_US (5 * USEC_PER_MSEC) +#define KX_SOFT_RESET_TOTAL_WAIT_TIME_US (500 * USEC_PER_MSEC) +#define KX_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC) +#define KX_MASK_GSEL GENMASK(4, 3) +#define KX_MASK_ODR GENMASK(3, 0) +#define KX_MASK_WM_TH GENMASK(6, 0) +#define KX_GSEL_SHIFT 3 +#define KX_MASK_IEN BIT(5) +#define KX_MASK_DRDY BIT(5) +#define KX_MASK_PC1 BIT(7) +#define KX_MASK_IPOL BIT(4) +#define KX_IPOL_LOW 0 +#define KX_MASK_ITYP BIT(3) +#define KX_ITYP_LEVEL 0 +#define KX_MASK_INS2_DRDY BIT(4) +#define KX_MASK_WMI BIT(5) +#define KX_MASK_BUF_EN BIT(7) +#define KX_MASK_SRST BIT(7) +#define KX_MASK_BRES16 BIT(6) + + #define KX022A_REG_WHO 0x0f #define KX022A_ID 0xc8 +#define KX022A_FIFO_LENGTH 43 +#define KX022A_FIFO_FULL_VALUE 255 +#define KX022A_FIFO_MAX_BYTES \ + (KX022A_FIFO_LENGTH * KX_FIFO_SAMPLES_SIZE_BYTES) + #define KX022A_REG_CNTL2 0x19 -#define KX022A_MASK_SRST BIT(7) #define KX022A_REG_CNTL 0x18 -#define KX022A_MASK_PC1 BIT(7) #define KX022A_MASK_RES BIT(6) -#define KX022A_MASK_DRDY BIT(5) -#define KX022A_MASK_GSEL GENMASK(4, 3) -#define KX022A_GSEL_SHIFT 3 #define KX022A_GSEL_2 0x0 #define KX022A_GSEL_4 BIT(3) #define KX022A_GSEL_8 BIT(4) #define KX022A_GSEL_16 GENMASK(4, 3) #define KX022A_REG_INS2 0x13 -#define KX022A_MASK_INS2_DRDY BIT(4) #define KX122_MASK_INS2_WMI BIT(5) #define KX022A_REG_XHP_L 0x0 @@ -45,38 +69,104 @@ #define KX022A_REG_MAN_WAKE 0x2c #define KX022A_REG_BUF_CNTL1 0x3a -#define KX022A_MASK_WM_TH GENMASK(6, 0) #define KX022A_REG_BUF_CNTL2 0x3b -#define KX022A_MASK_BUF_EN BIT(7) -#define KX022A_MASK_BRES16 BIT(6) #define KX022A_REG_BUF_STATUS_1 0x3c #define KX022A_REG_BUF_STATUS_2 0x3d +#define KX022A_MASK_BUF_SMP_LVL GENMASK(6, 0) #define KX022A_REG_BUF_CLEAR 0x3e #define KX022A_REG_BUF_READ 0x3f -#define KX022A_MASK_ODR GENMASK(3, 0) #define KX022A_ODR_SHIFT 3 #define KX022A_FIFO_MAX_WMI_TH 41 #define KX022A_REG_INC1 0x1c #define KX022A_REG_INC5 0x20 #define KX022A_REG_INC6 0x21 -#define KX022A_MASK_IEN BIT(5) -#define KX022A_MASK_IPOL BIT(4) #define KX022A_IPOL_LOW 0 -#define KX022A_IPOL_HIGH KX022A_MASK_IPOL1 -#define KX022A_MASK_ITYP BIT(3) -#define KX022A_ITYP_PULSE KX022A_MASK_ITYP -#define KX022A_ITYP_LEVEL 0 +#define KX022A_IPOL_HIGH KX_MASK_IPOL +#define KX022A_ITYP_PULSE KX_MASK_ITYP #define KX022A_REG_INC4 0x1f -#define KX022A_MASK_WMI BIT(5) #define KX022A_REG_SELF_TEST 0x60 #define KX022A_MAX_REGISTER 0x60 +enum kx022a_device_type { + KX022A, +}; + +enum { + KX_STATE_SAMPLE, + KX_STATE_FIFO, +}; + +enum { + AXIS_X, + AXIS_Y, + AXIS_Z, + AXIS_MAX +}; + struct device; -int kx022a_probe_internal(struct device *dev); -extern const struct regmap_config kx022a_regmap; +struct kx022a_chip_info { + const char *name; + enum kx022a_device_type type; + const struct regmap_config *regmap_config; + const struct iio_chan_spec *channels; + unsigned int num_channels; + unsigned int fifo_length; + u8 who; + u8 id; + u8 cntl; + u8 cntl2; + u8 odcntl; + u8 buf_cntl1; + u8 buf_cntl2; + u8 buf_clear; + u8 buf_status1; + u16 buf_smp_lvl_mask; + u8 buf_read; + u8 inc1; + u8 inc4; + u8 inc5; + u8 inc6; + u8 xout_l; +}; + +struct kx022a_data { + const struct kx022a_chip_info *chip_info; + struct regmap *regmap; + struct iio_trigger *trig; + struct device *dev; + struct iio_mount_matrix orientation; + int64_t timestamp, old_timestamp; + + int irq; + int inc_reg; + int ien_reg; + + unsigned int state; + unsigned int odr_ns; + + bool trigger_enabled; + /* + * Prevent toggling the sensor stby/active state (PC1 bit) in the + * middle of a configuration, or when the fifo is enabled. Also, + * protect the data stored/retrieved from this structure from + * concurrent accesses. + */ + struct mutex mutex; + u8 watermark; + + /* 3 x 16bit accel data + timestamp */ + __le16 buffer[8] __aligned(IIO_DMA_MINALIGN); + struct { + __le16 channels[3]; + s64 ts __aligned(8); + } scan; +}; + +int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info); +extern const struct kx022a_chip_info kx_chip_info[]; #endif
Refactor the kx022a driver implementation to make it more generic and extensible. Add the chip_info structure will to the driver's private data to hold all the device specific infos. Move the enum, struct and constants definitions to the header file. Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> --- drivers/iio/accel/kionix-kx022a-i2c.c | 19 +- drivers/iio/accel/kionix-kx022a-spi.c | 22 +- drivers/iio/accel/kionix-kx022a.c | 289 ++++++++++++-------------- drivers/iio/accel/kionix-kx022a.h | 128 ++++++++++-- 4 files changed, 274 insertions(+), 184 deletions(-)