| Message ID | 20211028101840.24632-11-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Bosch BNO055 IMU | expand |
On Thu, 28 Oct 2021 12:18:40 +0200 Andrea Merello <andrea.merello@gmail.com> wrote: > This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus > and it enables the BNO055 core driver to work in this scenario. > > Signed-off-by: Andrea Merello <andrea.merello@iit.it> Hi Andrea, A few minor things inline. Jonathan > --- > drivers/iio/imu/bno055/Kconfig | 6 ++++ > drivers/iio/imu/bno055/Makefile | 1 + > drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ > 3 files changed, 61 insertions(+) > create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c > > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig > index 941e43f0368d..87200787d548 100644 > --- a/drivers/iio/imu/bno055/Kconfig > +++ b/drivers/iio/imu/bno055/Kconfig > @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL > tristate "Bosh BNO055 attached via serial bus" > depends on SERIAL_DEV_BUS > select BOSH_BNO055_IIO > + > +config BOSH_BNO055_I2C > + tristate "Bosh BNO055 attached via I2C bus" > + depends on I2C > + select REGMAP_I2C > + select BOSH_BNO055_IIO > diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile > index 7285ade2f4b9..eaf24018cb28 100644 > --- a/drivers/iio/imu/bno055/Makefile > +++ b/drivers/iio/imu/bno055/Makefile > @@ -2,3 +2,4 @@ > > obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o > obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o > +obj-$(CONFIG_BOSH_BNO055_I2C) += bno055_i2c.o > diff --git a/drivers/iio/imu/bno055/bno055_i2c.c b/drivers/iio/imu/bno055/bno055_i2c.c > new file mode 100644 > index 000000000000..eea0daa6a61d > --- /dev/null > +++ b/drivers/iio/imu/bno055/bno055_i2c.c > @@ -0,0 +1,54 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * I2C interface for Bosh BNO055 IMU. > + * This file implements I2C communication up to the register read/write > + * level. Not really. It just uses regmap, so I'd drop this comment. > + * > + * Copyright (C) 2021 Istituto Italiano di Tecnologia > + * Electronic Design Laboratory > + * Written by Andrea Merello <andrea.merello@iit.it> > + */ > + > +#include <linux/i2c.h> Why? I'm not seeing an i2c specific calls in here. > +#include <linux/regmap.h> > +#include <linux/module.h> mod_devicetable.h for struct i2c_device_id > + > +#include "bno055.h" > + > +#define BNO055_I2C_XFER_BURST_BREAK_THRESHOLD 3 /* FIXME */ > + > +static int bno055_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct regmap *regmap = > + devm_regmap_init_i2c(client, &bno055_regmap_config); > + > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "Unable to init register map"); > + return PTR_ERR(regmap); > + } > + > + return bno055_probe(&client->dev, regmap, > + BNO055_I2C_XFER_BURST_BREAK_THRESHOLD); > + > + return 0; ? > +} > + > +static const struct i2c_device_id bno055_i2c_id[] = { > + {"bno055", 0}, > + { }, It's at terminator, so don't put a comma as we'll never add entries after this. > +}; > +MODULE_DEVICE_TABLE(i2c, bno055_i2c_id); > + > +static struct i2c_driver bno055_driver = { > + .driver = { > + .name = "bno055-i2c", > + }, > + .probe = bno055_i2c_probe, > + .id_table = bno055_i2c_id > +}; > +module_i2c_driver(bno055_driver); > + > +MODULE_AUTHOR("Andrea Merello"); > +MODULE_DESCRIPTION("Bosch BNO055 I2C interface"); > +MODULE_LICENSE("GPL v2");
On 10/28/21 3:18 AM, Andrea Merello wrote: > This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus > and it enables the BNO055 core driver to work in this scenario. > > Signed-off-by: Andrea Merello <andrea.merello@iit.it> > --- > drivers/iio/imu/bno055/Kconfig | 6 ++++ > drivers/iio/imu/bno055/Makefile | 1 + > drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ > 3 files changed, 61 insertions(+) > create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c > > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig > index 941e43f0368d..87200787d548 100644 > --- a/drivers/iio/imu/bno055/Kconfig > +++ b/drivers/iio/imu/bno055/Kconfig > @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL > tristate "Bosh BNO055 attached via serial bus" > depends on SERIAL_DEV_BUS > select BOSH_BNO055_IIO > + > +config BOSH_BNO055_I2C > + tristate "Bosh BNO055 attached via I2C bus" > + depends on I2C > + select REGMAP_I2C > + select BOSH_BNO055_IIO Hi, The config entries that have user prompt strings should also have help text. scripts/checkpatch.pl should have told you about that...
Hi Andrea,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.15-rc7]
[cannot apply to jic23-iio/togreg next-20211029]
[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]
url: https://github.com/0day-ci/linux/commits/Andrea-Merello/utils_macro-introduce-find_closest_unsorted/20211028-191851
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2f111a6fd5b5297b4e92f53798ca086f7c7d33a4
config: alpha-randconfig-r014-20211029 (attached as .config)
compiler: alpha-linux-gcc (GCC) 11.2.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/0day-ci/linux/commit/fab64510c8234ae8acfbc853cc8c433ae831f7b3
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrea-Merello/utils_macro-introduce-find_closest_unsorted/20211028-191851
git checkout fab64510c8234ae8acfbc853cc8c433ae831f7b3
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/iio/imu/bno055/bno055.c:234:5: warning: no previous prototype for 'bno055_calibration_load' [-Wmissing-prototypes]
234 | int bno055_calibration_load(struct bno055_priv *priv, const struct firmware *fw)
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/printk.h:555,
from include/linux/kernel.h:19,
from include/linux/clk.h:13,
from drivers/iio/imu/bno055/bno055.c:18:
drivers/iio/imu/bno055/bno055.c: In function 'bno055_calibration_load':
>> drivers/iio/imu/bno055/bno055.c:237:36: warning: format '%d' expects argument of type 'int', but argument 4 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
237 | dev_dbg(priv->dev, "Invalid calibration file size %d (expected %d)",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
134 | func(&id, ##__VA_ARGS__); \
| ^~~~~~~~~~~
include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
166 | _dynamic_func_call(fmt,__dynamic_dev_dbg, \
| ^~~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
155 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~
include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
155 | dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~
drivers/iio/imu/bno055/bno055.c:237:17: note: in expansion of macro 'dev_dbg'
237 | dev_dbg(priv->dev, "Invalid calibration file size %d (expected %d)",
| ^~~~~~~
drivers/iio/imu/bno055/bno055.c:237:68: note: format string is defined here
237 | dev_dbg(priv->dev, "Invalid calibration file size %d (expected %d)",
| ~^
| |
| int
| %ld
drivers/iio/imu/bno055/bno055.c: In function 'bno055_fusion_enable_store':
drivers/iio/imu/bno055/bno055.c:917:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
917 | int ret = 0;
| ^~~
drivers/iio/imu/bno055/bno055.c: At top level:
drivers/iio/imu/bno055/bno055.c:1130:5: warning: no previous prototype for 'bno055_debugfs_reg_access' [-Wmissing-prototypes]
1130 | int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/imu/bno055/bno055.c: In function 'bno055_trigger_handler':
drivers/iio/imu/bno055/bno055.c:1330:9: error: implicit declaration of function 'for_each_set_bitrange'; did you mean 'for_each_set_bit'? [-Werror=implicit-function-declaration]
1330 | for_each_set_bitrange(start, end, iio_dev->active_scan_mask,
| ^~~~~~~~~~~~~~~~~~~~~
| for_each_set_bit
drivers/iio/imu/bno055/bno055.c:1331:51: error: expected ';' before '{' token
1331 | iio_dev->masklength) {
| ^~
| ;
drivers/iio/imu/bno055/bno055.c:1364:1: warning: label 'done' defined but not used [-Wunused-label]
1364 | done:
| ^~~~
drivers/iio/imu/bno055/bno055.c:1327:13: warning: unused variable 'ret' [-Wunused-variable]
1327 | int ret;
| ^~~
drivers/iio/imu/bno055/bno055.c:1326:13: warning: unused variable 'quat' [-Wunused-variable]
1326 | int quat;
| ^~~~
drivers/iio/imu/bno055/bno055.c:1325:14: warning: unused variable 'thr_hit' [-Wunused-variable]
1325 | bool thr_hit;
| ^~~~~~~
drivers/iio/imu/bno055/bno055.c:1324:13: warning: unused variable 'buf_idx' [-Wunused-variable]
1324 | int buf_idx = 0;
| ^~~~~~~
drivers/iio/imu/bno055/bno055.c:1323:23: warning: unused variable 'mask' [-Wunused-variable]
1323 | unsigned long mask;
| ^~~~
drivers/iio/imu/bno055/bno055.c:1322:14: warning: unused variable 'first' [-Wunused-variable]
1322 | bool first = true;
| ^~~~~
drivers/iio/imu/bno055/bno055.c:1321:14: warning: unused variable 'xfer_pending' [-Wunused-variable]
1321 | bool xfer_pending = false;
| ^~~~~~~~~~~~
drivers/iio/imu/bno055/bno055.c:1320:37: warning: unused variable 'prev_end' [-Wunused-variable]
1320 | int xfer_start, start, end, prev_end;
| ^~~~~~~~
drivers/iio/imu/bno055/bno055.c:1320:13: warning: unused variable 'xfer_start' [-Wunused-variable]
1320 | int xfer_start, start, end, prev_end;
| ^~~~~~~~~~
At top level:
drivers/iio/imu/bno055/bno055.c:1262:12: warning: 'bno055_scan_xfer' defined but not used [-Wunused-function]
1262 | static int bno055_scan_xfer(struct bno055_priv *priv,
| ^~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +237 drivers/iio/imu/bno055/bno055.c
734efd9783b7759 Andrea Merello 2021-10-28 232
734efd9783b7759 Andrea Merello 2021-10-28 233 /* must be called in configuration mode */
734efd9783b7759 Andrea Merello 2021-10-28 @234 int bno055_calibration_load(struct bno055_priv *priv, const struct firmware *fw)
734efd9783b7759 Andrea Merello 2021-10-28 235 {
734efd9783b7759 Andrea Merello 2021-10-28 236 if (fw->size != BNO055_CALDATA_LEN) {
734efd9783b7759 Andrea Merello 2021-10-28 @237 dev_dbg(priv->dev, "Invalid calibration file size %d (expected %d)",
734efd9783b7759 Andrea Merello 2021-10-28 238 fw->size, BNO055_CALDATA_LEN);
734efd9783b7759 Andrea Merello 2021-10-28 239 return -EINVAL;
734efd9783b7759 Andrea Merello 2021-10-28 240 }
734efd9783b7759 Andrea Merello 2021-10-28 241
734efd9783b7759 Andrea Merello 2021-10-28 242 dev_dbg(priv->dev, "loading cal data: %*ph", BNO055_CALDATA_LEN, fw->data);
734efd9783b7759 Andrea Merello 2021-10-28 243 return regmap_bulk_write(priv->regmap, BNO055_CALDATA_START,
734efd9783b7759 Andrea Merello 2021-10-28 244 fw->data, BNO055_CALDATA_LEN);
734efd9783b7759 Andrea Merello 2021-10-28 245 }
734efd9783b7759 Andrea Merello 2021-10-28 246
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Il giorno ven 29 ott 2021 alle ore 00:04 Randy Dunlap <rdunlap@infradead.org> ha scritto: > > On 10/28/21 3:18 AM, Andrea Merello wrote: > > This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus > > and it enables the BNO055 core driver to work in this scenario. > > > > Signed-off-by: Andrea Merello <andrea.merello@iit.it> > > --- > > drivers/iio/imu/bno055/Kconfig | 6 ++++ > > drivers/iio/imu/bno055/Makefile | 1 + > > drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ > > 3 files changed, 61 insertions(+) > > create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c > > > > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig > > index 941e43f0368d..87200787d548 100644 > > --- a/drivers/iio/imu/bno055/Kconfig > > +++ b/drivers/iio/imu/bno055/Kconfig > > @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL > > tristate "Bosh BNO055 attached via serial bus" > > depends on SERIAL_DEV_BUS > > select BOSH_BNO055_IIO > > + > > +config BOSH_BNO055_I2C > > + tristate "Bosh BNO055 attached via I2C bus" > > + depends on I2C > > + select REGMAP_I2C > > + select BOSH_BNO055_IIO > > Hi, > > The config entries that have user prompt strings should also > have help text. scripts/checkpatch.pl should have told you > about that... I'll add it, thanks. But FYI checkpatch doesn't complain about that here. > -- > ~Randy
On 11/9/21 3:56 AM, Andrea Merello wrote: > Il giorno ven 29 ott 2021 alle ore 00:04 Randy Dunlap > <rdunlap@infradead.org> ha scritto: >> >> On 10/28/21 3:18 AM, Andrea Merello wrote: >>> This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus >>> and it enables the BNO055 core driver to work in this scenario. >>> >>> Signed-off-by: Andrea Merello <andrea.merello@iit.it> >>> --- >>> drivers/iio/imu/bno055/Kconfig | 6 ++++ >>> drivers/iio/imu/bno055/Makefile | 1 + >>> drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ >>> 3 files changed, 61 insertions(+) >>> create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c >>> >>> diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig >>> index 941e43f0368d..87200787d548 100644 >>> --- a/drivers/iio/imu/bno055/Kconfig >>> +++ b/drivers/iio/imu/bno055/Kconfig >>> @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL >>> tristate "Bosh BNO055 attached via serial bus" >>> depends on SERIAL_DEV_BUS >>> select BOSH_BNO055_IIO >>> + >>> +config BOSH_BNO055_I2C >>> + tristate "Bosh BNO055 attached via I2C bus" >>> + depends on I2C >>> + select REGMAP_I2C >>> + select BOSH_BNO055_IIO >> >> Hi, >> >> The config entries that have user prompt strings should also >> have help text. scripts/checkpatch.pl should have told you >> about that... > > I'll add it, thanks. But FYI checkpatch doesn't complain about that here. Hm, thanks for adding it and telling me about that. checkpatch.pl does have some code for checking that but I confirmed that it does not catch this simple case. Joe, can you identify why checkpatch does not detect missing Kconfig help text is this simple case? Thanks.
(cc'ing Andi Kleen, who wrote this code a decade ago) On Tue, 2021-11-09 at 07:47 -0800, Randy Dunlap wrote: > On 11/9/21 3:56 AM, Andrea Merello wrote: > > Il giorno ven 29 ott 2021 alle ore 00:04 Randy Dunlap <rdunlap@infradead.org> ha scritto: > > > On 10/28/21 3:18 AM, Andrea Merello wrote: > > > > This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus > > > > and it enables the BNO055 core driver to work in this scenario. > > > > > > > > Signed-off-by: Andrea Merello <andrea.merello@iit.it> > > > > --- > > > > drivers/iio/imu/bno055/Kconfig | 6 ++++ > > > > drivers/iio/imu/bno055/Makefile | 1 + [] > > > > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig [] > > > > @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL > > > > tristate "Bosh BNO055 attached via serial bus" > > > > depends on SERIAL_DEV_BUS > > > > select BOSH_BNO055_IIO > > > > + > > > > +config BOSH_BNO055_I2C > > > > + tristate "Bosh BNO055 attached via I2C bus" > > > > + depends on I2C > > > > + select REGMAP_I2C > > > > + select BOSH_BNO055_IIO [] > > > The config entries that have user prompt strings should also > > > have help text. scripts/checkpatch.pl should have told you > > > about that... > > > > I'll add it, thanks. But FYI checkpatch doesn't complain about that here. > > Hm, thanks for adding it and telling me about that. > > checkpatch.pl does have some code for checking that but I confirmed > that it does not catch this simple case. > > Joe, can you identify why checkpatch does not detect missing Kconfig > help text is this simple case? Original patch here: https://lore.kernel.org/all/20211028101840.24632-11-andrea.merello@gmail.com/raw checkpatch is counting the diff header lines that follow the config entry. Maybe this is clearer (better?) code: --- scripts/checkpatch.pl | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1784921c645da..b3ce8e04d7df7 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -3483,20 +3483,22 @@ sub process { my $cnt = $realcnt; my $ln = $linenr + 1; my $f; - my $is_start = 0; - my $is_end = 0; + my $needs_help = 0; + my $has_help = 0; for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) { $f = $lines[$ln - 1]; - $cnt-- if ($lines[$ln - 1] !~ /^-/); - $is_end = $lines[$ln - 1] =~ /^\+/; + $cnt-- if ($f !~ /^-/); next if ($f =~ /^-/); - last if (!$file && $f =~ /^\@\@/); + last if (!$file && $f =~ /^(?:\@\@|diff )/); - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { - $is_start = 1; - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { - $length = -1; + if ($f =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { + $needs_help = 1; + next; + } elsif ($f =~ /^\+\s*help\s*$/) { + $length = 0; + $has_help = 1; + next; } $f =~ s/^.//; @@ -3510,16 +3512,16 @@ sub process { # common words in help texts if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice| if|endif|menu|endmenu|source)\b/x) { - $is_end = 1; last; } - $length++; + $length++ if ($has_help); } - if ($is_start && $is_end && $length < $min_conf_desc_length) { + if ($needs_help && + (!$has_help || + ($has_help && $length < $min_conf_desc_length))) { WARN("CONFIG_DESCRIPTION", "please write a paragraph that describes the config symbol fully\n" . $herecurr); } - #print "is_start<$is_start> is_end<$is_end> length<$length>\n"; } # check MAINTAINERS entries
On 11/9/21 10:21 AM, Joe Perches wrote: > (cc'ing Andi Kleen, who wrote this code a decade ago) > > On Tue, 2021-11-09 at 07:47 -0800, Randy Dunlap wrote: >> On 11/9/21 3:56 AM, Andrea Merello wrote: >>> Il giorno ven 29 ott 2021 alle ore 00:04 Randy Dunlap <rdunlap@infradead.org> ha scritto: >>>> On 10/28/21 3:18 AM, Andrea Merello wrote: >>>>> This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus >>>>> and it enables the BNO055 core driver to work in this scenario. >>>>> >>>>> Signed-off-by: Andrea Merello <andrea.merello@iit.it> >>>>> --- >>>>> drivers/iio/imu/bno055/Kconfig | 6 ++++ >>>>> drivers/iio/imu/bno055/Makefile | 1 + > [] >>>>> diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig > [] >>>>> @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL >>>>> tristate "Bosh BNO055 attached via serial bus" >>>>> depends on SERIAL_DEV_BUS >>>>> select BOSH_BNO055_IIO >>>>> + >>>>> +config BOSH_BNO055_I2C >>>>> + tristate "Bosh BNO055 attached via I2C bus" >>>>> + depends on I2C >>>>> + select REGMAP_I2C >>>>> + select BOSH_BNO055_IIO > [] >>>> The config entries that have user prompt strings should also >>>> have help text. scripts/checkpatch.pl should have told you >>>> about that... >>> >>> I'll add it, thanks. But FYI checkpatch doesn't complain about that here. >> >> Hm, thanks for adding it and telling me about that. >> >> checkpatch.pl does have some code for checking that but I confirmed >> that it does not catch this simple case. >> >> Joe, can you identify why checkpatch does not detect missing Kconfig >> help text is this simple case? > > Original patch here: https://lore.kernel.org/all/20211028101840.24632-11-andrea.merello@gmail.com/raw > > checkpatch is counting the diff header lines that follow the config entry. > Maybe this is clearer (better?) code: > --- > scripts/checkpatch.pl | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 1784921c645da..b3ce8e04d7df7 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -3483,20 +3483,22 @@ sub process { > my $cnt = $realcnt; > my $ln = $linenr + 1; > my $f; > - my $is_start = 0; > - my $is_end = 0; > + my $needs_help = 0; > + my $has_help = 0; > for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) { > $f = $lines[$ln - 1]; > - $cnt-- if ($lines[$ln - 1] !~ /^-/); > - $is_end = $lines[$ln - 1] =~ /^\+/; > + $cnt-- if ($f !~ /^-/); > > next if ($f =~ /^-/); > - last if (!$file && $f =~ /^\@\@/); > + last if (!$file && $f =~ /^(?:\@\@|diff )/); > > - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { > - $is_start = 1; > - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { > - $length = -1; > + if ($f =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { > + $needs_help = 1; > + next; > + } elsif ($f =~ /^\+\s*help\s*$/) { > + $length = 0; > + $has_help = 1; > + next; > } > > $f =~ s/^.//; > @@ -3510,16 +3512,16 @@ sub process { > # common words in help texts > if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice| > if|endif|menu|endmenu|source)\b/x) { > - $is_end = 1; > last; > } > - $length++; > + $length++ if ($has_help); > } > - if ($is_start && $is_end && $length < $min_conf_desc_length) { > + if ($needs_help && > + (!$has_help || > + ($has_help && $length < $min_conf_desc_length))) { > WARN("CONFIG_DESCRIPTION", > "please write a paragraph that describes the config symbol fully\n" . $herecurr); > } > - #print "is_start<$is_start> is_end<$is_end> length<$length>\n"; > } > > # check MAINTAINERS entries > > Which now says: WARNING: please write a paragraph that describes the config symbol fully #16: FILE: drivers/iio/Kconfig:101: +config BOSH_BNO055_I2C (This is for a dummy entry that I made for testing, not from Andrea's patch.) Thanks, Joe. Tested-by: Randy Dunlap <rdunlap@infradead.org> Acked-by: Randy Dunlap <rdunlap@infradead.org>
On Tue, 2021-11-09 at 11:11 -0800, Randy Dunlap wrote: > On 11/9/21 10:21 AM, Joe Perches wrote: > > (cc'ing Andi Kleen, who wrote this code a decade ago) > > > Joe, can you identify why checkpatch does not detect missing Kconfig > > > help text is this simple case? > > > > Original patch here: https://lore.kernel.org/all/20211028101840.24632-11-andrea.merello@gmail.com/raw > > > > checkpatch is counting the diff header lines that follow the config entry. > > Maybe this is clearer (better?) code: > > --- > Tested-by: Randy Dunlap <rdunlap@infradead.org> > Acked-by: Randy Dunlap <rdunlap@infradead.org> Hey Randy/Andi. I like this patch below a bit more. It shows the Kconfig context block in the output message and documents the code a bit more. Care to test it again? --- scripts/checkpatch.pl | 53 +++++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1784921c645da..0b5c0363119ff 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -3480,46 +3480,49 @@ sub process { # (\b) rather than a whitespace character (\s) $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) { my $length = 0; - my $cnt = $realcnt; - my $ln = $linenr + 1; - my $f; - my $is_start = 0; - my $is_end = 0; - for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) { - $f = $lines[$ln - 1]; - $cnt-- if ($lines[$ln - 1] !~ /^-/); - $is_end = $lines[$ln - 1] =~ /^\+/; + my $ln = $linenr; + my $needs_help = 0; + my $has_help = 0; + my $herecfg = $herecurr; + while (defined $lines[$ln]) { + my $f = $lines[$ln++]; next if ($f =~ /^-/); - last if (!$file && $f =~ /^\@\@/); + last if (!$file && $f =~ /^(?:\@\@|diff )/); - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { - $is_start = 1; - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { - $length = -1; + $herecfg .= $rawlines[$ln - 1] . "\n"; + if ($f =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { + $needs_help = 1; + next; + } + if ($f =~ /^\+\s*help\s*$/) { + $has_help = 1; + next; } - $f =~ s/^.//; - $f =~ s/#.*//; - $f =~ s/^\s+//; - next if ($f =~ /^$/); + $f =~ s/^.//; # strip patch context [+ ] + $f =~ s/#.*//; # strip # directives + $f =~ s/^\s+//; # strip leading blanks + next if ($f =~ /^$/); # skip blank lines + # At the end of this Kconfig block: # This only checks context lines in the patch # and so hopefully shouldn't trigger false # positives, even though some of these are # common words in help texts - if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice| - if|endif|menu|endmenu|source)\b/x) { - $is_end = 1; + if ($f =~ /^(?:config|menuconfig|choice|endchoice| + if|endif|menu|endmenu|source)\b/x) { + $herecfg =~ s/.*\n\Z//; # strip last line last; } - $length++; + $length++ if ($has_help); } - if ($is_start && $is_end && $length < $min_conf_desc_length) { + if ($needs_help && + (!$has_help || + ($has_help && $length < $min_conf_desc_length))) { WARN("CONFIG_DESCRIPTION", - "please write a paragraph that describes the config symbol fully\n" . $herecurr); + "please write a help paragraph that fully describes the config symbol\n" . $herecfg); } - #print "is_start<$is_start> is_end<$is_end> length<$length>\n"; } # check MAINTAINERS entries
On 11/9/21 12:46 PM, Joe Perches wrote: > On Tue, 2021-11-09 at 11:11 -0800, Randy Dunlap wrote: >> On 11/9/21 10:21 AM, Joe Perches wrote: >>> (cc'ing Andi Kleen, who wrote this code a decade ago) >>>> Joe, can you identify why checkpatch does not detect missing Kconfig >>>> help text is this simple case? >>> >>> Original patch here: https://lore.kernel.org/all/20211028101840.24632-11-andrea.merello@gmail.com/raw >>> >>> checkpatch is counting the diff header lines that follow the config entry. >>> Maybe this is clearer (better?) code: >>> --- >> Tested-by: Randy Dunlap <rdunlap@infradead.org> >> Acked-by: Randy Dunlap <rdunlap@infradead.org> > > Hey Randy/Andi. > > I like this patch below a bit more. > > It shows the Kconfig context block in the output message and > documents the code a bit more. > > Care to test it again? > --- > scripts/checkpatch.pl | 53 +++++++++++++++++++++++++++------------------------ > 1 file changed, 28 insertions(+), 25 deletions(-) > Same tags from me, better output. Thanks! Tested-by: Randy Dunlap <rdunlap@infradead.org> Acked-by: Randy Dunlap <rdunlap@infradead.org>
Just an inline comment; OK for the rest. Il giorno gio 28 ott 2021 alle ore 13:05 Jonathan Cameron <jic23@kernel.org> ha scritto: > > On Thu, 28 Oct 2021 12:18:40 +0200 > Andrea Merello <andrea.merello@gmail.com> wrote: > > > This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus > > and it enables the BNO055 core driver to work in this scenario. > > > > Signed-off-by: Andrea Merello <andrea.merello@iit.it> > Hi Andrea, > > A few minor things inline. > > Jonathan > > > --- > > drivers/iio/imu/bno055/Kconfig | 6 ++++ > > drivers/iio/imu/bno055/Makefile | 1 + > > drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ > > 3 files changed, 61 insertions(+) > > create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c > > > > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig > > index 941e43f0368d..87200787d548 100644 > > --- a/drivers/iio/imu/bno055/Kconfig > > +++ b/drivers/iio/imu/bno055/Kconfig > > @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL > > tristate "Bosh BNO055 attached via serial bus" > > depends on SERIAL_DEV_BUS > > select BOSH_BNO055_IIO > > + > > +config BOSH_BNO055_I2C > > + tristate "Bosh BNO055 attached via I2C bus" > > + depends on I2C > > + select REGMAP_I2C > > + select BOSH_BNO055_IIO > > diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile > > index 7285ade2f4b9..eaf24018cb28 100644 > > --- a/drivers/iio/imu/bno055/Makefile > > +++ b/drivers/iio/imu/bno055/Makefile > > @@ -2,3 +2,4 @@ > > > > obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o > > obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o > > +obj-$(CONFIG_BOSH_BNO055_I2C) += bno055_i2c.o > > diff --git a/drivers/iio/imu/bno055/bno055_i2c.c b/drivers/iio/imu/bno055/bno055_i2c.c > > new file mode 100644 > > index 000000000000..eea0daa6a61d > > --- /dev/null > > +++ b/drivers/iio/imu/bno055/bno055_i2c.c > > @@ -0,0 +1,54 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * I2C interface for Bosh BNO055 IMU. > > + * This file implements I2C communication up to the register read/write > > + * level. > > Not really. It just uses regmap, so I'd drop this comment. > > > + * > > + * Copyright (C) 2021 Istituto Italiano di Tecnologia > > + * Electronic Design Laboratory > > + * Written by Andrea Merello <andrea.merello@iit.it> > > + */ > > + > > +#include <linux/i2c.h> > > Why? I'm not seeing an i2c specific calls in here. Because of the definition of struct i2c_client, that is being accessed in lines like this dev_err(&client->dev, "Unable to init register map"); > > +#include <linux/regmap.h> > > +#include <linux/module.h> > > mod_devicetable.h for struct i2c_device_id > > > + > > +#include "bno055.h" > > + > > +#define BNO055_I2C_XFER_BURST_BREAK_THRESHOLD 3 /* FIXME */ > > + > > +static int bno055_i2c_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct regmap *regmap = > > + devm_regmap_init_i2c(client, &bno055_regmap_config); > > + > > + if (IS_ERR(regmap)) { > > + dev_err(&client->dev, "Unable to init register map"); > > + return PTR_ERR(regmap); > > + } > > + > > + return bno055_probe(&client->dev, regmap, > > + BNO055_I2C_XFER_BURST_BREAK_THRESHOLD); > > + > > + return 0; > > ? > > > +} > > + > > +static const struct i2c_device_id bno055_i2c_id[] = { > > + {"bno055", 0}, > > + { }, > > It's at terminator, so don't put a comma as we'll never add entries after this. > > > +}; > > +MODULE_DEVICE_TABLE(i2c, bno055_i2c_id); > > + > > +static struct i2c_driver bno055_driver = { > > + .driver = { > > + .name = "bno055-i2c", > > + }, > > + .probe = bno055_i2c_probe, > > + .id_table = bno055_i2c_id > > +}; > > +module_i2c_driver(bno055_driver); > > + > > +MODULE_AUTHOR("Andrea Merello"); > > +MODULE_DESCRIPTION("Bosch BNO055 I2C interface"); > > +MODULE_LICENSE("GPL v2"); >
On Thu, 11 Nov 2021 11:12:58 +0100 Andrea Merello <andrea.merello@gmail.com> wrote: > Just an inline comment; OK for the rest. > > > +#include <linux/i2c.h> > > > > Why? I'm not seeing an i2c specific calls in here. > > Because of the definition of struct i2c_client, that is being accessed > in lines like this > dev_err(&client->dev, "Unable to init register map"); doh. I was clearly being a bit unobservant that day.
diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig index 941e43f0368d..87200787d548 100644 --- a/drivers/iio/imu/bno055/Kconfig +++ b/drivers/iio/imu/bno055/Kconfig @@ -7,3 +7,9 @@ config BOSH_BNO055_SERIAL tristate "Bosh BNO055 attached via serial bus" depends on SERIAL_DEV_BUS select BOSH_BNO055_IIO + +config BOSH_BNO055_I2C + tristate "Bosh BNO055 attached via I2C bus" + depends on I2C + select REGMAP_I2C + select BOSH_BNO055_IIO diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile index 7285ade2f4b9..eaf24018cb28 100644 --- a/drivers/iio/imu/bno055/Makefile +++ b/drivers/iio/imu/bno055/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o +obj-$(CONFIG_BOSH_BNO055_I2C) += bno055_i2c.o diff --git a/drivers/iio/imu/bno055/bno055_i2c.c b/drivers/iio/imu/bno055/bno055_i2c.c new file mode 100644 index 000000000000..eea0daa6a61d --- /dev/null +++ b/drivers/iio/imu/bno055/bno055_i2c.c @@ -0,0 +1,54 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * I2C interface for Bosh BNO055 IMU. + * This file implements I2C communication up to the register read/write + * level. + * + * Copyright (C) 2021 Istituto Italiano di Tecnologia + * Electronic Design Laboratory + * Written by Andrea Merello <andrea.merello@iit.it> + */ + +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/module.h> + +#include "bno055.h" + +#define BNO055_I2C_XFER_BURST_BREAK_THRESHOLD 3 /* FIXME */ + +static int bno055_i2c_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct regmap *regmap = + devm_regmap_init_i2c(client, &bno055_regmap_config); + + if (IS_ERR(regmap)) { + dev_err(&client->dev, "Unable to init register map"); + return PTR_ERR(regmap); + } + + return bno055_probe(&client->dev, regmap, + BNO055_I2C_XFER_BURST_BREAK_THRESHOLD); + + return 0; +} + +static const struct i2c_device_id bno055_i2c_id[] = { + {"bno055", 0}, + { }, +}; +MODULE_DEVICE_TABLE(i2c, bno055_i2c_id); + +static struct i2c_driver bno055_driver = { + .driver = { + .name = "bno055-i2c", + }, + .probe = bno055_i2c_probe, + .id_table = bno055_i2c_id +}; +module_i2c_driver(bno055_driver); + +MODULE_AUTHOR("Andrea Merello"); +MODULE_DESCRIPTION("Bosch BNO055 I2C interface"); +MODULE_LICENSE("GPL v2");
This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus and it enables the BNO055 core driver to work in this scenario. Signed-off-by: Andrea Merello <andrea.merello@iit.it> --- drivers/iio/imu/bno055/Kconfig | 6 ++++ drivers/iio/imu/bno055/Makefile | 1 + drivers/iio/imu/bno055/bno055_i2c.c | 54 +++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c