Re: [Cocci] [PATCH v2 08/10] scripts: Coccinelle script for namespace dependencies.
On Tue, Aug 13, 2019 at 01:17:05PM +0100, Matthias Maennich wrote: > A script that uses the '.ns_deps' file generated by modpost to > automatically add the required symbol namespace dependencies to each > module. > > Usage: > 1) Move some symbols to a namespace with EXPORT_SYMBOL_NS() or define >DEFAULT_SYMBOL_NAMESPACE > 2) Run 'make' (or 'make modules') and get warnings about modules not >importing that namespace. > 3) Run 'make nsdeps' to automatically add required import statements >to said modules. > > This makes it easer for subsystem maintainers to introduce and maintain > symbol namespaces into their codebase. > > Co-developed-by: Martijn Coenen > Signed-off-by: Martijn Coenen > Signed-off-by: Matthias Maennich > --- [] > MAINTAINERS | 5 ++ > Makefile| 12 + > scripts/Makefile.modpost| 4 +- > scripts/coccinelle/misc/add_namespace.cocci | 23 + > scripts/nsdeps | 54 + > 5 files changed, 97 insertions(+), 1 deletion(-) > create mode 100644 scripts/coccinelle/misc/add_namespace.cocci > create mode 100644 scripts/nsdeps [] > +if [ "$SPATCH_VERSION_NUM" -lt "$SPATCH_REQ_VERSION_NUM" ] ; then > + echo 'spatch needs to be version 1.06 or higher' Nitpick: 1.0.6 > +exit 1 > +fi -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
[PATCH] iio: chemical: bme680: Fix pressure and humidity ABI mismatch
Standard ABI for reporting pressure is kilopascal and for
relative humidity it is millipercent.
What: /sys/bus/iio/devices/iio:deviceX/in_pressureY_input
What: /sys/bus/iio/devices/iio:deviceX/in_pressure_input
KernelVersion: 3.8
Contact:linux-...@vger.kernel.org
Description:
Scaled pressure measurement from channel Y, in kilopascal.
What: /sys/bus/iio/devices/iio:deviceX/in_humidityrelative_input
KernelVersion: 3.14
Contact:linux-...@vger.kernel.org
Description:
Scaled humidity measurement in milli percent.
Currently pressure is reported in hectopascal(hPa) and relative humidity
in percent. Hence fix this ABI mismatch conforming to the standard ABI.
Fixes: 1b3bd8592780 ("iio: chemical: Add support for Bosch BME680 sensor")
Signed-off-by: Himanshu Jha
---
While cleaning this mess I wonder about the gas channel and there
exists no `in_resistance_input` in standard ABI :-(
We only have:
What: /sys/bus/iio/devices/iio:deviceX/in_resistance_raw
What: /sys/bus/iio/devices/iio:deviceX/in_resistanceX_raw
What: /sys/bus/iio/devices/iio:deviceX/out_resistance_raw
What: /sys/bus/iio/devices/iio:deviceX/out_resistanceX_raw
KernelVersion: 4.3
Contact:linux-...@vger.kernel.org
Description:
Raw (unscaled no offset etc.) resistance reading that can be
processed
into an ohm value.
The sensor outputs processed value which is reported as is.
So, does it need a new ABI ?
drivers/iio/chemical/bme680_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/chemical/bme680_core.c
b/drivers/iio/chemical/bme680_core.c
index ccde4c65ff93..28cc96d1e3c8 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -670,7 +670,7 @@ static int bme680_read_press(struct bme680_data *data,
}
*val = bme680_compensate_press(data, adc_press);
- *val2 = 100;
+ *val2 = 1000;
return IIO_VAL_FRACTIONAL;
}
@@ -704,7 +704,7 @@ static int bme680_read_humid(struct bme680_data *data,
comp_humidity = bme680_compensate_humid(data, adc_humidity);
*val = comp_humidity;
- *val2 = 1000;
+ *val2 = 100;
return IIO_VAL_FRACTIONAL;
}
--
2.17.1
[PATCH v2] coccinelle: api: add devm_platform_ioremap_resource script
Use recently introduced devm_platform_ioremap_resource helper which wraps platform_get_resource() and devm_ioremap_resource() together. This helps produce much cleaner code and remove local `struct resource` declaration. Signed-off-by: Himanshu Jha Signed-off-by: Julia Lawall --- v2: - added SPDX License - used IORESOURCE_MEM instead of generic argument. - added Julia's SoB. .../api/devm_platform_ioremap_resource.cocci | 60 +++ 1 file changed, 60 insertions(+) create mode 100644 scripts/coccinelle/api/devm_platform_ioremap_resource.cocci diff --git a/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci new file mode 100644 index ..56a2e261d61d --- /dev/null +++ b/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: GPL-2.0 +/// Use devm_platform_ioremap_resource helper which wraps +/// platform_get_resource() and devm_ioremap_resource() together. +/// +// Confidence: High +// Copyright: (C) 2019 Himanshu Jha GPLv2. +// Copyright: (C) 2019 Julia Lawall, Inria/LIP6. GPLv2. +// Keywords: platform_get_resource, devm_ioremap_resource, +// Keywords: devm_platform_ioremap_resource + +virtual patch +virtual report + +@r depends on patch && !report@ +expression e1, e2, arg1, arg2, arg3; +identifier id; +@@ + +( +- id = platform_get_resource(arg1, IORESOURCE_MEM, arg2); +| +- struct resource *id = platform_get_resource(arg1, IORESOURCE_MEM, arg2); +) + ... when != id +- e1 = devm_ioremap_resource(arg3, id); ++ e1 = devm_platform_ioremap_resource(arg1, arg2); + ... when != id +? id = e2 + +@r1 depends on patch && !report@ +identifier r.id; +type T; +@@ + +- T *id; + ...when != id + +@r2 depends on report && !patch@ +identifier id; +expression e1, e2, arg1, arg2, arg3; +position j0; +@@ + +( + id = platform_get_resource(arg1, IORESOURCE_MEM, arg2); +| + struct resource *id = platform_get_resource(arg1, IORESOURCE_MEM, arg2); +) + ... when != id + e1@j0 = devm_ioremap_resource(arg3, id); + ... when != id +? id = e2 + +@script:python depends on report && !patch@ +e1 << r2.e1; +j0 << r2.j0; +@@ + +msg = "WARNING: Use devm_platform_ioremap_resource for %s" % (e1) +coccilib.report.print_report(j0[0], msg) -- 2.17.1
Re: [PATCH 1/2] staging:iio:accel:adis16203: add SPDX license identifier tag
On Wed, Apr 03, 2019 at 11:41:28AM +0200, Nicholas Mc Guire wrote: > Pop in the SPDX tag as the license is clearly indicated > as GPL V2 or later this should also be indicated with a SPDX license > identifier tag. > > Signed-off-by: Nicholas Mc Guire > --- > > chackpatch.pl was warning: > WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 > > Patch was compile tested with: X86_64_defconfig + SPI=y, IIO=m > STAGING=y, ADIS16203=m > > Patch is against 5.1-rc3 (localversion-next is next-20190403) > > drivers/staging/iio/accel/adis16203.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/iio/accel/adis16203.c > b/drivers/staging/iio/accel/adis16203.c > index 5cc96c80..cf9d41a 100644 > --- a/drivers/staging/iio/accel/adis16203.c > +++ b/drivers/staging/iio/accel/adis16203.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later SPDX-License-Identifier: GPL-2.0+ ? Documentation/process/license-rules.rst For 'GNU General Public License (GPL) version 2 or any later version' use: SPDX-License-Identifier: GPL-2.0+ The following should also be removed in this same patch: * Licensed under the GPL-2 or later. And lastly, MODULE_LICENSE() should also be checked, which appear to be correct in its current form. > /* > * ADIS16203 Programmable 360 Degrees Inclinometer > * > -- > 2.1.4 > -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface
On Sat, Mar 16, 2019 at 01:00:39PM +, Mike Looijmans wrote:
> On 16-03-19 11:24, Himanshu Jha wrote:
> > On Wed, Mar 06, 2019 at 08:31:48AM +0100, Mike Looijmans wrote:
> >> The SPI interface implementation was completely broken.
> >>
> >> When using the SPI interface, there are only 7 address bits, the upper bit
> >> is controlled by a page select register. The core needs access to both
> >> ranges, so implement register read/write for both regions. The regmap
> >> paging functionality didn't agree with a register that needs to be read
> >> and modified, so I implemented a custom paging algorithm.
> >>
> >> This fixes that the device wouldn't even probe in SPI mode.
> >>
> >> The SPI interface then isn't different from I2C, merged them into the core,
> >> and the I2C/SPI named registers are no longer needed.
> >>
> >> Implemented register value caching for the registers to reduce the I2C/SPI
> >> data transfers considerably.
> >>
> >> The calibration set reads as all zeroes until some undefined point in time,
> >> and I couldn't determine what makes it valid. The datasheet mentions these
> >> registers but does not provide any hints on when they become valid, and
> >> they
> >> aren't even enumerated in the memory map. So check the calibration and
> >> retry reading it from the device after each measurement until it provides
> >> something valid.
> >>
> >> Signed-off-by: Mike Looijmans
> >> ---
> >
> > I have been trying to test this patch in the past week and still it
> > failed everytime.
> >
> > First I used ACPI to enumerate the device in QEMU setup:
> >
> > Added some printks for debugging:
> >
> > [ 14.510198] bme680_spi spi-BME0680:00: Jumping to core driver now ...
> > [ 14.544528] bme680_spi spi-BME0680:00: Page setting done, on Page :0 now
> > [ 14.554363] bme680_spi spi-BME0680:00: bme680_regmap_spi_write: on Page
> > :0 now
> > [ 14.556151] bme680_spi spi-BME0680:00: bme680_regmap_spi_read: on Page
> > :0 now
> > [ 14.567815] bme680_spi spi-BME0680:00: Wrong chip ID, got ff expected 61
> >
>
> Looks like the SPI communication isn't working. At this point I'd check
> the wires using an osciloscope or analyzer or something.
OK. Will give it a try at university lab.
> > I also tried bypassing this by removing the following snippet and force
> > registration to see what happens next:
> >
> > > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> > > +BME680_CMD_SOFTRESET);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to reset chip\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regmap_read(regmap, BME680_REG_CHIP_ID, );
> > > + if (ret < 0) {
> > > + dev_err(dev, "Error reading chip ID\n");
> > > + return ret;
> > > + }
> > > +
> > > + if (val != BME680_CHIP_ID_VAL) {
> > > + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> > > + val, BME680_CHIP_ID_VAL);
> > > + return -ENODEV;
> > > + }
> > > +
> >
> > And it registered successfully, but all the bme680 attributes were
> > giving wrong values like temp was constant to 0.007, resistance
> > was resource busy due to insuffient target temperature error.
> > Pretty eveything was messed up at this stage.
>
> Makes perfect sense if it's unable to read the registers.
>
> If you cannot read the chip ID, nothing will work, no point skipping that.
Agreed! I was just trying to triage the issue.
> > Then I build and booted the kernel on BeagleBone Black Wireless with
> > DT matching this time:
> >
> > debian@beaglebone:~$ uname -a
> > Linux beaglebone 4.19.5-ti-r5 #1xross SMP PREEMPT Sat Mar 16 12:11:50 IST
> > 2019 armv7l GNU/Linux
> > debian@beaglebone:~$ dmesg | grep 'bme680'
> > [ 30.269207] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
> > [ 361.867410] bme680_core: disagrees about version of symbol module_layout
>
> Looks like a compilation problem with your kernel module?
No. I doesn't appear now.
I also build a more latest kernel but same error:
[ 519.741364] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
> > debian@beaglebone:~$ lsmod | grep 'bme'
> > bme680_spi 16384
Re: [PATCH v3 2/2] iio/chemical/bme680: Fix SPI read interface
On Wed, Mar 06, 2019 at 08:31:48AM +0100, Mike Looijmans wrote:
> The SPI interface implementation was completely broken.
>
> When using the SPI interface, there are only 7 address bits, the upper bit
> is controlled by a page select register. The core needs access to both
> ranges, so implement register read/write for both regions. The regmap
> paging functionality didn't agree with a register that needs to be read
> and modified, so I implemented a custom paging algorithm.
>
> This fixes that the device wouldn't even probe in SPI mode.
>
> The SPI interface then isn't different from I2C, merged them into the core,
> and the I2C/SPI named registers are no longer needed.
>
> Implemented register value caching for the registers to reduce the I2C/SPI
> data transfers considerably.
>
> The calibration set reads as all zeroes until some undefined point in time,
> and I couldn't determine what makes it valid. The datasheet mentions these
> registers but does not provide any hints on when they become valid, and they
> aren't even enumerated in the memory map. So check the calibration and
> retry reading it from the device after each measurement until it provides
> something valid.
>
> Signed-off-by: Mike Looijmans
> ---
I have been trying to test this patch in the past week and still it
failed everytime.
First I used ACPI to enumerate the device in QEMU setup:
Added some printks for debugging:
[ 14.510198] bme680_spi spi-BME0680:00: Jumping to core driver now ...
[ 14.544528] bme680_spi spi-BME0680:00: Page setting done, on Page :0 now
[ 14.554363] bme680_spi spi-BME0680:00: bme680_regmap_spi_write: on Page :0
now
[ 14.556151] bme680_spi spi-BME0680:00: bme680_regmap_spi_read: on Page :0 now
[ 14.567815] bme680_spi spi-BME0680:00: Wrong chip ID, got ff expected 61
I also tried bypassing this by removing the following snippet and force
registration to see what happens next:
> + ret = regmap_write(regmap, BME680_REG_SOFT_RESET,
> +BME680_CMD_SOFTRESET);
> + if (ret < 0) {
> + dev_err(dev, "Failed to reset chip\n");
> + return ret;
> + }
> +
> + ret = regmap_read(regmap, BME680_REG_CHIP_ID, );
> + if (ret < 0) {
> + dev_err(dev, "Error reading chip ID\n");
> + return ret;
> + }
> +
> + if (val != BME680_CHIP_ID_VAL) {
> + dev_err(dev, "Wrong chip ID, got %x expected %x\n",
> + val, BME680_CHIP_ID_VAL);
> + return -ENODEV;
> + }
> +
And it registered successfully, but all the bme680 attributes were
giving wrong values like temp was constant to 0.007, resistance
was resource busy due to insuffient target temperature error.
Pretty eveything was messed up at this stage.
Then I build and booted the kernel on BeagleBone Black Wireless with
DT matching this time:
debian@beaglebone:~$ uname -a
Linux beaglebone 4.19.5-ti-r5 #1xross SMP PREEMPT Sat Mar 16 12:11:50 IST 2019
armv7l GNU/Linux
debian@beaglebone:~$ dmesg | grep 'bme680'
[ 30.269207] bme680_spi spi0.0: Wrong chip ID, got ff expected 61
[ 361.867410] bme680_core: disagrees about version of symbol module_layout
debian@beaglebone:~$ lsmod | grep 'bme'
bme680_spi 16384 0
bme680_core20480 1 bme680_spi
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/
modaliasof_node/power/ statistics/ subsystem/ uevent
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/modalias
spi:bme680
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/
compatible name regspi-max-frequency
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/compatible
bme680
debian@beaglebone:~$ cat /sys/bus/spi/devices/spi0.0/of_node/name
bme680
debian@beaglebone:~$ dtc -f -I fs /proc/device-tree | grep -A3 'bme680'
bme680@0 {
compatible = "bme680";
reg = <0x0>;
spi-max-frequency = <0x989680>;
};
Same error again!
I really don't know where the problem is and how to rectify ?
OTOH, I2C works like a charm as it used to work before:
root@beaglebone:/sys/bus/iio/devices/iio:device1# cat name in_temp_input \
in_pressure_input in_humidityrelative_input in_resistance_input
bme680
26860 --> w/o your patch it used to be 26.86000 degC
990.87000
55.26500
10091
I'm still assuming that there is some problem on my side, as it works
flawless for you. But it is really difficult for me to figure out
exactly where the problem could be!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio/chemical/bme680: Fix SPI read interface
buf[1] |= BME680_SPI_MEM_PAGE_BIT;
> + else
> + buf[1] &= ~BME680_SPI_MEM_PAGE_BIT;
> +
> + ret = spi_write(spi, buf, 2);
> + if (ret < 0) {
> + dev_err(>dev, "failed to set page %u\n", page);
> + return ret;
> + }
> +
> + ctx->current_page = page;
> +
> + return 0;
> +}
I will take another look at this later as my head is spinning now ...
> static int bme680_regmap_spi_write(void *context, const void *data,
> size_t count)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> u8 buf[2];
>
> memcpy(buf, data, 2);
> +
> + ret = bme680_regmap_spi_select_page(ctx, buf[0]);
> + if (ret)
> + return ret;
> +
> /*
>* The SPI register address (= full register address without bit 7)
>* and the write command (bit7 = RW = '0')
>*/
> buf[0] &= ~0x80;
>
> - return spi_write_then_read(spi, buf, 2, NULL, 0);
> + return spi_write(spi, buf, 2);
> }
>
> static int bme680_regmap_spi_read(void *context, const void *reg,
> size_t reg_size, void *val, size_t val_size)
> {
> - struct spi_device *spi = context;
> + struct bme680_spi_bus_context *ctx = context;
> + struct spi_device *spi = ctx->spi;
> + int ret;
> + u8 addr = *(const u8 *)reg;
> + u8 addr7;
Unused variable.
> + ret = bme680_regmap_spi_select_page(ctx, addr);
> + if (ret)
> + return ret;
>
> - return spi_write_then_read(spi, reg, reg_size, val, val_size);
> + addr |= 0x80; /* bit7 = RW = '1' */
> +
> + return spi_write_then_read(spi, , 1, val, val_size);
> }
>
> static struct regmap_bus bme680_regmap_bus = {
> @@ -45,8 +111,8 @@ static int bme680_regmap_spi_read(void *context, const
> void *reg,
> static int bme680_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> + struct bme680_spi_bus_context *bus_context;
> struct regmap *regmap;
> - unsigned int val;
> int ret;
>
> spi->bits_per_word = 8;
> @@ -56,45 +122,21 @@ static int bme680_spi_probe(struct spi_device *spi)
> return ret;
> }
>
> + bus_context = devm_kzalloc(>dev, sizeof(*bus_context), GFP_KERNEL);
> + if (!bus_context)
> + return -ENOMEM;
> +
> + bus_context->spi = spi;
> + bus_context->current_page = 0xff; /* Undefined on warm boot */
OK. This is what you observed.
If this patch works as expected then I think a "Fixes:" tag should be
added while marking it for stable ?
Thanks for the patch Mike and apologies again for the whole mess.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] drivers: iio: industrialio-core: add check when kzalloc fails
On Fri, Jan 25, 2019 at 08:55:27AM +0200, Alexandru Ardelean wrote:
> On Thu, Jan 24, 2019 at 4:28 PM Bharath Vedartham
> wrote:
> >
> > add code to handle the case when kzalloc fails to allocate memory to dev
> >
> > Signed-off-by: Bharath Vedartham
> > ---
> > drivers/iio/industrialio-core.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/industrialio-core.c
> > b/drivers/iio/industrialio-core.c
> > index 4f5cd9f..93caa6b 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1475,6 +1475,8 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> > }
> > dev_set_name(>dev, "iio:device%d", dev->id);
> > INIT_LIST_HEAD(>buffer_list);
> > + } else {
> > + return NULL;
>
> I'd argue that this is a bit redundant, because `dev` is NULL, the
> return below (return dev) will also return NULL.
+1
Also, subject title is incorrect.
Always use:
$ git log --oneline drivers/iio/industrialio-core.c
Use the most frequently tag/subject from the output of above
command.
"iio: core: " would be fine.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3 1/2] iio: chemical: bme680: Add device-tree support
On Mon, Jan 14, 2019 at 03:19:13PM -0500, Sebastien Bourdelin wrote:
> This commit allow the driver to work with device-tree.
>
> Signed-off-by: Sebastien Bourdelin
> ---
Acked-by: Himanshu Jha
Thanks!
> v2 -> v3:
> - remove of_match_ptr: Suggested by Jonathan Cameron
> - minor style fixup
> - rebase on master
> v1 -> v2:
> - add missing of.h header in bme680_spi.c
> ---
> drivers/iio/chemical/bme680_i2c.c | 7 +++
> drivers/iio/chemical/bme680_spi.c | 8
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/iio/chemical/bme680_i2c.c
> b/drivers/iio/chemical/bme680_i2c.c
> index 06d4be539d2e..b2f805b6b36a 100644
> --- a/drivers/iio/chemical/bme680_i2c.c
> +++ b/drivers/iio/chemical/bme680_i2c.c
> @@ -70,10 +70,17 @@ static const struct acpi_device_id bme680_acpi_match[] = {
> };
> MODULE_DEVICE_TABLE(acpi, bme680_acpi_match);
>
> +static const struct of_device_id bme680_of_i2c_match[] = {
> + { .compatible = "bosch,bme680", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, bme680_of_i2c_match);
> +
> static struct i2c_driver bme680_i2c_driver = {
> .driver = {
> .name = "bme680_i2c",
> .acpi_match_table = ACPI_PTR(bme680_acpi_match),
> + .of_match_table = bme680_of_i2c_match,
> },
> .probe = bme680_i2c_probe,
> .id_table = bme680_i2c_id,
> diff --git a/drivers/iio/chemical/bme680_spi.c
> b/drivers/iio/chemical/bme680_spi.c
> index c9fb05e8d0b9..d0b7bdd3f066 100644
> --- a/drivers/iio/chemical/bme680_spi.c
> +++ b/drivers/iio/chemical/bme680_spi.c
> @@ -6,6 +6,7 @@
> */
> #include
> #include
> +#include
> #include
> #include
>
> @@ -110,10 +111,17 @@ static const struct acpi_device_id bme680_acpi_match[]
> = {
> };
> MODULE_DEVICE_TABLE(acpi, bme680_acpi_match);
>
> +static const struct of_device_id bme680_of_spi_match[] = {
> + { .compatible = "bosch,bme680", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, bme680_of_spi_match);
> +
> static struct spi_driver bme680_spi_driver = {
> .driver = {
> .name = "bme680_spi",
> .acpi_match_table = ACPI_PTR(bme680_acpi_match),
> + .of_match_table = bme680_of_spi_match,
> },
> .probe = bme680_spi_probe,
> .id_table = bme680_spi_id,
> --
> 2.20.1
>
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3 2/2] dt-bindings: iio: chemical: Add bindings for bme680
On Mon, Jan 14, 2019 at 03:19:14PM -0500, Sebastien Bourdelin wrote:
> BME680 is a pressure/temperature/humidity/voc sensor.
>
> Signed-off-by: Sebastien Bourdelin
> ---
> v2 -> v3:
> - change i2c address to 0x76 as it seems more reliable: Suggested by
> Himanshu Jha
> - rebase on master
> ---
> .../devicetree/bindings/iio/chemical/bme680.txt | 11 +++
> 1 file changed, 11 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/chemical/bme680.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/chemical/bme680.txt
> b/Documentation/devicetree/bindings/iio/chemical/bme680.txt
> new file mode 100644
> index ..7f3827cfb2ff
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/bme680.txt
> @@ -0,0 +1,11 @@
> +Bosch Sensortec BME680 pressure/temperature/humidity/voc sensors
> +
> +Required properties:
> +- compatible: must be "bosch,bme680"
> +
> +Example:
> +
> +bme680@76 {
> + compatible = "bosch,bme680";
> + reg = <0x76>;
> +};
Thanks,
Acked-by: Himanshu Jha
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2 1/2] iio: chemical: bme680: Add device-tree support
Hi Sebastien,
On Mon, Jan 14, 2019 at 03:00:41PM -0500, sebastien bourdelin wrote:
> Hi,
>
> On 1/12/19 4:42 AM, Himanshu Jha wrote:
> > On Fri, Jan 11, 2019 at 03:53:58PM -0500, Sebastien Bourdelin wrote:
> > > This commit allow the driver to work with device-tree.
> > >
> > > Signed-off-by: Sebastien Bourdelin
> > > ---
> > I get the following compilation failure:
> >
> > Below I have `allyesconfig` except 'BME680' configure as [M]
> > in case you wish to reproduce.
> >
> > himanshu@himanshu-Vostro-3559:~/linux-next$ grep -i -w
> > 'CONFIG_BME680\|CONFIG_ACPI\|CONFIG_OF' .config
> > CONFIG_ACPI=y
> > CONFIG_OF=y
> > CONFIG_BME680=m
> > himanshu@himanshu-Vostro-3559:~/linux-next$ sudo make
> > drivers/iio/chemical/bme680_spi.o
> > make[1]: Nothing to be done for 'all'.
> >CALLscripts/checksyscalls.sh
> >DESCEND objtool
> >CC [M] drivers/iio/chemical/bme680_spi.o
> > In file included from ./include/linux/acpi.h:41:0,
> > from drivers/iio/chemical/bme680_spi.c:7:
> > ./include/linux/module.h:213:1: error: expected ‘,’ or ‘;’ before ‘extern’
> > extern typeof(name) __mod_##type##__##name##_device_table \
> > ^
> > drivers/iio/chemical/bme680_spi.c:119:1: note: in expansion of macro
> > ‘MODULE_DEVICE_TABLE’
> > MODULE_DEVICE_TABLE(of, bme680_of_spi_match);
> > ^~~
> > scripts/Makefile.build:291: recipe for target
> > 'drivers/iio/chemical/bme680_spi.o' failed
> > make[1]: *** [drivers/iio/chemical/bme680_spi.o] Error 1
> > Makefile:1741: recipe for target 'drivers/iio/chemical/bme680_spi.o' failed
> > make: *** [drivers/iio/chemical/bme680_spi.o] Error 2
> Thanks for the test, this is bad, i will fix that!
> > BUT if:
> >
> > himanshu@himanshu-Vostro-3559:~/linux-next$ make allyesconfig
> > scripts/kconfig/conf --allyesconfig Kconfig
> > #
> > # configuration written to .config
> > #
> >
> > himanshu@himanshu-Vostro-3559:~/linux-next$ sudo make
> > drivers/iio/chemical/bme680_spi.o
> > scripts/kconfig/conf --syncconfig Kconfig
> > make[1]: Nothing to be done for 'all'.
> >HOSTCC scripts/dtc/dtc.o
> >HOSTCC scripts/dtc/flattree.o
> >HOSTCC scripts/dtc/fstree.o
> >HOSTCC scripts/dtc/data.o
> >HOSTCC scripts/dtc/livetree.o
> >HOSTCC scripts/dtc/treesource.o
> >HOSTCC scripts/dtc/srcpos.o
> >HOSTCC scripts/dtc/checks.o
> >HOSTCC scripts/dtc/util.o
> >LEX scripts/dtc/dtc-lexer.lex.c
> >YACCscripts/dtc/dtc-parser.tab.h
> >HOSTCC scripts/dtc/dtc-lexer.lex.o
> >YACCscripts/dtc/dtc-parser.tab.c
> >HOSTCC scripts/dtc/dtc-parser.tab.o
> >HOSTLD scripts/dtc/dtc
> >CC scripts/mod/empty.o
> >MKELF scripts/mod/elfconfig.h
> >HOSTCC scripts/mod/modpost.o
> >CC scripts/mod/devicetable-offsets.s
> >HOSTCC scripts/mod/file2alias.o
> >HOSTCC scripts/mod/sumversion.o
> >HOSTLD scripts/mod/modpost
> >CC kernel/bounds.s
> >CC arch/x86/kernel/asm-offsets.s
> >CALLscripts/checksyscalls.sh
> >DESCEND objtool
> >CC drivers/iio/chemical/bme680_spi.o
> >
> > Compiles without any issues.
> Hum, weird it compiles actually :s
I think this behavior is observed due to:
include/linux/module.h +212
#ifdef MODULE
/* Creates an alias so file2alias.c can find device table. */
#define MODULE_DEVICE_TABLE(type, name) \
extern typeof(name) __mod_##type##__##name##_device_table \
__attribute__ ((unused, alias(__stringify(name
#else /* !MODULE */
#define MODULE_DEVICE_TABLE(type, name)
#endif
So, when we build the driver as a module[M] then macro expansion
takes place giving us the compiler warning.
OTOH, if the driver is built as builtin[*] then marco expands
to nothing or simply goes away. And `;' completes the struct
declaration while silencing the warning.
static const struct of_device_id bme680_of_spi_match[] = {
{ .compatible = "bosch,bme680", },
{},
}
MODULE_DEVICE_TABLE(of, bme680_of_spi_match);
converts to:
static const struct of_device_id bme680_of_spi_match[] = {
{ .compatible = "bosch,bme680", },
{},
}
;
^^^
Amazing!
Correct me if I'm wrong somewhere, took me 2 hours to figure
that out :D
Also, I some additional interesting observations:
When buitin[*] -> no symbol tables in the RO segment of
Re: [PATCH v2 1/2] iio: chemical: bme680: Add device-tree support
On Fri, Jan 11, 2019 at 03:53:58PM -0500, Sebastien Bourdelin wrote:
> This commit allow the driver to work with device-tree.
>
> Signed-off-by: Sebastien Bourdelin
> ---
I get the following compilation failure:
Below I have `allyesconfig` except 'BME680' configure as [M]
in case you wish to reproduce.
himanshu@himanshu-Vostro-3559:~/linux-next$ grep -i -w
'CONFIG_BME680\|CONFIG_ACPI\|CONFIG_OF' .config
CONFIG_ACPI=y
CONFIG_OF=y
CONFIG_BME680=m
himanshu@himanshu-Vostro-3559:~/linux-next$ sudo make
drivers/iio/chemical/bme680_spi.o
make[1]: Nothing to be done for 'all'.
CALLscripts/checksyscalls.sh
DESCEND objtool
CC [M] drivers/iio/chemical/bme680_spi.o
In file included from ./include/linux/acpi.h:41:0,
from drivers/iio/chemical/bme680_spi.c:7:
./include/linux/module.h:213:1: error: expected ‘,’ or ‘;’ before ‘extern’
extern typeof(name) __mod_##type##__##name##_device_table \
^
drivers/iio/chemical/bme680_spi.c:119:1: note: in expansion of macro
‘MODULE_DEVICE_TABLE’
MODULE_DEVICE_TABLE(of, bme680_of_spi_match);
^~~
scripts/Makefile.build:291: recipe for target
'drivers/iio/chemical/bme680_spi.o' failed
make[1]: *** [drivers/iio/chemical/bme680_spi.o] Error 1
Makefile:1741: recipe for target 'drivers/iio/chemical/bme680_spi.o' failed
make: *** [drivers/iio/chemical/bme680_spi.o] Error 2
BUT if:
himanshu@himanshu-Vostro-3559:~/linux-next$ make allyesconfig
scripts/kconfig/conf --allyesconfig Kconfig
#
# configuration written to .config
#
himanshu@himanshu-Vostro-3559:~/linux-next$ sudo make
drivers/iio/chemical/bme680_spi.o
scripts/kconfig/conf --syncconfig Kconfig
make[1]: Nothing to be done for 'all'.
HOSTCC scripts/dtc/dtc.o
HOSTCC scripts/dtc/flattree.o
HOSTCC scripts/dtc/fstree.o
HOSTCC scripts/dtc/data.o
HOSTCC scripts/dtc/livetree.o
HOSTCC scripts/dtc/treesource.o
HOSTCC scripts/dtc/srcpos.o
HOSTCC scripts/dtc/checks.o
HOSTCC scripts/dtc/util.o
LEX scripts/dtc/dtc-lexer.lex.c
YACCscripts/dtc/dtc-parser.tab.h
HOSTCC scripts/dtc/dtc-lexer.lex.o
YACCscripts/dtc/dtc-parser.tab.c
HOSTCC scripts/dtc/dtc-parser.tab.o
HOSTLD scripts/dtc/dtc
CC scripts/mod/empty.o
MKELF scripts/mod/elfconfig.h
HOSTCC scripts/mod/modpost.o
CC scripts/mod/devicetable-offsets.s
HOSTCC scripts/mod/file2alias.o
HOSTCC scripts/mod/sumversion.o
HOSTLD scripts/mod/modpost
CC kernel/bounds.s
CC arch/x86/kernel/asm-offsets.s
CALLscripts/checksyscalls.sh
DESCEND objtool
CC drivers/iio/chemical/bme680_spi.o
Compiles without any issues.
Also, wondering when is 0x77 i2c address used, since I tested
this on 3 different boards with 0x76(when SDO is connected to GND)
And why do I connect SDO to ground everytime ?
It is because if SDO pin is left floating then I2C address will be
undefined as said in datasheet + I have observed this while testing.
Actallly, I don't understand what "VDIDO" is, as explained in the
datasheet.
Anyway, if the above compilation issue is not a problem, then
Acked-by: Himanshu Jha
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Wed, Jan 02, 2019 at 01:25:27PM -0800, Matt Ranostay wrote:
>
> > On Dec 24, 2018, at 01:58, Himanshu Jha wrote:
> >
> >> On Fri, Dec 21, 2018 at 03:26:26PM -0800, Amir Mahdi Ghorbanian wrote:
> >> Replaced bool in struct with unsigned int bitfield to conserve space and
> >> more clearly define size of varibales
>
> Important thing to note is depending on padding, alignment, and position of
> field it probably won’t save any space.
Well, then what do you say about the following results ;-)
Before:
--
himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data
drivers/staging/iio/adc/ad7192.o
struct ad7192_platform_data {
u16vref_mv; /* 0 2 */
u8 clock_source_sel; /* 2 1 */
/* XXX 1 byte hole, try to pack */
u32ext_clk_hz; /* 4 4 */
bool refin2_en;/* 8 1 */
bool rej60_en; /* 9 1 */
bool sinc3_en; /*10 1 */
bool chop_en; /*11 1 */
bool buf_en; /*12 1 */
bool unipolar_en; /*13 1 */
bool burnout_curr_en; /*14 1 */
/* size: 16, cachelines: 1, members: 10 */
/* sum members: 14, holes: 1, sum holes: 1 */
/* padding: 1 */
/* last cacheline: 16 bytes */
};
After:
-
himanshu@himanshu-Vostro-3559:~/gsoc/linux$ pahole -C ad7192_platform_data
drivers/staging/iio/adc/ad7192.o
struct ad7192_platform_data {
u16vref_mv; /* 0 2 */
u8 clock_source_sel; /* 2 1 */
/* XXX 1 byte hole, try to pack */
u32ext_clk_hz; /* 4 4 */
unsigned int refin2_en:1; /* 8:31 4 */
unsigned int rej60_en:1; /* 8:30 4 */
unsigned int sinc3_en:1; /* 8:29 4 */
unsigned int chop_en:1;/* 8:28 4 */
unsigned int buf_en:1; /* 8:27 4 */
unsigned int unipolar_en:1;/* 8:26 4 */
unsigned int burnout_curr_en:1;/* 8:25 4 */
/* size: 12, cachelines: 1, members: 10 */
/* sum members: 11, holes: 1, sum holes: 1 */
/* bit_padding: 25 bits */
/* last cacheline: 12 bytes */
};
> Also for internal unpacked structures it really makes little sense to save a
> few bytes of data. Don’t read into that packed structures are good.. they
> usually aren’t :)
Yes, agreed, but I often see patches to save few bytes by marking
array as static.
There is some effort in this direction:
https://lore.kernel.org/lkml/20181221235230.gc13...@ziepe.ca/
Very likely to get more patches if the above patch gets merged.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Staging: iio: ad7192: replaced bool in struct
On Fri, Dec 21, 2018 at 03:26:26PM -0800, Amir Mahdi Ghorbanian wrote:
> Replaced bool in struct with unsigned int bitfield to conserve space and
> more clearly define size of varibales
>
> Signed-off-by: Amir Mahdi Ghorbanian
> ---
There was some discussion on this at Outreachy list:
https://groups.google.com/d/msg/outreachy-kernel/xpQAl-Gn8HA/yqcQRG_qBgAJ
I think unless you post some statistics about 'conserving' space,
it is unlikely that maintainers will apply it.
This idea was originally given by Linus and that thread of discussion
is worth reading too.
> drivers/staging/iio/adc/ad7192.h | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7192.h
> b/drivers/staging/iio/adc/ad7192.h
> index 7433a43..7d3e62f 100644
> --- a/drivers/staging/iio/adc/ad7192.h
> +++ b/drivers/staging/iio/adc/ad7192.h
> @@ -35,13 +35,13 @@ struct ad7192_platform_data {
> u16 vref_mv;
> u8 clock_source_sel;
> u32 ext_clk_hz;
> - boolrefin2_en;
> - boolrej60_en;
> - boolsinc3_en;
> - boolchop_en;
> - boolbuf_en;
> - boolunipolar_en;
> - boolburnout_curr_en;
> + unsigned intrefin2_en : 1;
> + unsigned intrej60_en : 1;
> + unsigned intsinc3_en : 1;
> + unsigned intchop_en : 1;
> + unsigned intbuf_en : 1;
> + unsigned intunipolar_en : 1;
> + unsigned intburnout_curr_en : 1;
> };
>
> #endif /* IIO_ADC_AD7192_H_ */
> --
> 2.7.4
>
Goodluck!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/2] checkpatch: add Co-developed-by to signature tags
On Fri, Dec 14, 2018 at 12:16:34PM -0800, Andrew Morton wrote: > On Fri, 14 Dec 2018 18:35:28 +0100 Jorge Ramirez-Ortiz > wrote: > > > As per Documentation/process/submitting-patches, Co-developed-by is a > > valid signature. > > > > I'm with Joe - I find this tag kinda useless and duplicative. But whatever. I'm fine either way, just the problem is: Co-developed-by Vs Co-Developed-by with checkpatch spitting out that it is not a valid signature. In the end, I can completely remove the tag from docs if it is useless and duplicative. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] checkpatch: add Co-Developed-by to signature tags
On Fri, Dec 14, 2018 at 09:39:10AM -0800, Joe Perches wrote: > On Fri, 2018-12-14 at 22:58 +0530, Himanshu Jha wrote: > > On Fri, Dec 14, 2018 at 08:27:33AM -0800, Joe Perches wrote: > > > Is it really important to specify things like 75% / 25% > > > authorship crediting? > > > > IDK how that ratio came up into this discussion ? > > How does one tell when a co-developed-by: person > should be notated or blamed for a defective commit? > > git blame shows only 1 author. I would Cc all those who were involved in the defective commit log. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH 1/2] docs: fix Co-Developed-by docs
On Fri, Dec 14, 2018 at 06:35:27PM +0100, Jorge Ramirez-Ortiz wrote: > The accepted terminology will be Co-developed-by therefore losing the > capital letter from now on. > > Signed-off-by: Jorge Ramirez-Ortiz > --- Thanks. ACK! Finally, the discussion converged at somepoint. FYI, Cc Jonathan Cameron + IIO list. > Documentation/process/submitting-patches.rst | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/process/submitting-patches.rst > b/Documentation/process/submitting-patches.rst > index c091710..30dc00a 100644 > --- a/Documentation/process/submitting-patches.rst > +++ b/Documentation/process/submitting-patches.rst > @@ -510,7 +510,7 @@ tracking your trees, and to people trying to troubleshoot > bugs in your > tree. > > > -12) When to use Acked-by:, Cc:, and Co-Developed-by: > +12) When to use Acked-by:, Cc:, and Co-developed-by: > --- > > The Signed-off-by: tag indicates that the signer was involved in the > @@ -543,7 +543,7 @@ person it names - but it should indicate that this person > was copied on the > patch. This tag documents that potentially interested parties > have been included in the discussion. > > -A Co-Developed-by: states that the patch was also created by another > developer > +A Co-developed-by: states that the patch was also created by another > developer > along with the original author. This is useful at times when multiple people > work on a single patch. Note, this person also needs to have a > Signed-off-by: > line in the patch as well. > -- > 2.7.4 > -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] checkpatch: add Co-Developed-by to signature tags
On Fri, Dec 14, 2018 at 08:27:33AM -0800, Joe Perches wrote: > On Fri, 2018-12-14 at 21:46 +0530, Himanshu Jha wrote: > > On Fri, Dec 14, 2018 at 07:52:15AM -0800, Joe Perches wrote: > > > On Fri, 2018-12-14 at 14:01 +0100, Jorge Ramirez-Ortiz wrote: > > > > As per Documentation/process/submitting-patches, Co-developed-by is a > > > > valid signature. > > > > > > > > This commit removes the warning. > > > > > > Your commit message doesn't match your subject. > > > > > > A couple variants have been documented and only > > > one should actually be used. > > > > > > $ git grep -i co-developed-by Documentation/process/ > > > Documentation/process/5.Posting.rst: - Co-developed-by: states that the > > > patch was also created by another developer > > > Documentation/process/submitting-patches.rst:12) When to use Acked-by:, > > > Cc:, and Co-Developed-by: > > > Documentation/process/submitting-patches.rst:A Co-Developed-by: states > > > that the patch was also created by another developer > > > > > > $ git log --grep="co-developed-by:" -i | \ > > > grep -ohiP "co-developed-by:" | sort | uniq -c | sort -rn > > > 80 Co-developed-by: > > > 40 Co-Developed-by: > > > > > > So which should it be? > > > > > > btw: I prefer neither as I think Signed-off-by: is sufficient. > > > > OK, but does multiple Signed-off-by: in the commits imply that > > the patch was created by all those developers ? > > > > I don't think so, perhaps this was the reason to introduce > > Co-developed-by: tag. > > Perhaps, but a sign-off is also a recognition that the > patch was passed-through by individuals Yes, Agreed! > Effectively, there's no real difference. > > "Co-developed-by:" is just another word for "Authored-by:" > where multiple "Authorship" is the thing being notated. > > Is it really important to specify things like 75% / 25% > authorship crediting? IDK how that ratio came up into this discussion ? Anyway, I saw on IIO list that a bunch of students were involved in driver cleaning with the help of developers from Analog Devices Inc who intially wrote some code snippets. And that authorship crediting for Analog Devices folks would be helpful distinguishing that it was not just passed-through and rather they spent their time on it. > I don't really care about attribution so the concept is > not particularly valuable to me. Well, it might not be valuable to you but it is for others and I saw one such example in the past during my project. At least I do care about those developers who spent a considerable time on IIO list guiding students aside from their mainline work. FYI, IIO has already +1'd for "Co-developed-by:" -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] checkpatch: add Co-Developed-by to signature tags
On Fri, Dec 14, 2018 at 07:52:15AM -0800, Joe Perches wrote: > On Fri, 2018-12-14 at 14:01 +0100, Jorge Ramirez-Ortiz wrote: > > As per Documentation/process/submitting-patches, Co-developed-by is a > > valid signature. > > > > This commit removes the warning. > > Your commit message doesn't match your subject. > > A couple variants have been documented and only > one should actually be used. > > $ git grep -i co-developed-by Documentation/process/ > Documentation/process/5.Posting.rst: - Co-developed-by: states that the patch > was also created by another developer > Documentation/process/submitting-patches.rst:12) When to use Acked-by:, Cc:, > and Co-Developed-by: > Documentation/process/submitting-patches.rst:A Co-Developed-by: states that > the patch was also created by another developer > > $ git log --grep="co-developed-by:" -i | \ > grep -ohiP "co-developed-by:" | sort | uniq -c | sort -rn > 80 Co-developed-by: > 40 Co-Developed-by: > > So which should it be? > > btw: I prefer neither as I think Signed-off-by: is sufficient. OK, but does multiple Signed-off-by: in the commits imply that the patch was created by all those developers ? I don't think so, perhaps this was the reason to introduce Co-developed-by: tag. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [Cocci] [PATCH 0/2] scripts: coccinelle: Improve boolinit
+Cc Masahiro Yamada On Wed, Dec 12, 2018 at 12:55:55PM +0100, Julia Lawall wrote: > Reduce the scope of the rule and improve the warning messages. > > julia (2): > scripts: coccinelle: only suggest true/false in files that already use > them > scripts: coccinelle: Correct warning message > > scripts/coccinelle/misc/boolinit.cocci | 43 > +- > 1 file changed, 27 insertions(+), 16 deletions(-) > > -- > 1.9.1 > > ___ > Cocci mailing list > co...@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad5933: add binding doc for ad5933
On Sun, Dec 02, 2018 at 02:57:12PM -0200, Marcelo Schmitt wrote: > Add a devicetree documentation for the ad5933 and ad5934 impedance > converter, network analyzer. > > Co-Developed-by: Gabriel Capella checkpatch spits out: WARNING: Non-standard signature: Co-Developed-by: Co-developed-by Vs Co-Developed-by ? Documentation/process/5.Posting.rst: - Co-developed-by: states that the patch was also created by another developer Documentation/process/submitting-patches.rst:12) When to use Acked-by:, Cc:, and Co-Developed-by: Confusing! Don't know which one is correct. > Signed-off-by: Marcelo Schmitt > Signed-off-by: Gabriel Capella > --- Use `./scripts/get_maintainer.pl ` to list the DT maintainers and the relevant mailing list. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad5933: add binding doc for ad5933
On Sun, Dec 02, 2018 at 02:57:12PM -0200, Marcelo Schmitt wrote: > Add a devicetree documentation for the ad5933 and ad5934 impedance > converter, network analyzer. > > Co-Developed-by: Gabriel Capella checkpatch spits out: WARNING: Non-standard signature: Co-Developed-by: Co-developed-by Vs Co-Developed-by ? Documentation/process/5.Posting.rst: - Co-developed-by: states that the patch was also created by another developer Documentation/process/submitting-patches.rst:12) When to use Acked-by:, Cc:, and Co-Developed-by: Confusing! Don't know which one is correct. > Signed-off-by: Marcelo Schmitt > Signed-off-by: Gabriel Capella > --- Use `./scripts/get_maintainer.pl ` to list the DT maintainers and the relevant mailing list. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
]
> +static int sps30_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct sps30_state *state;
> + u8 buf[32] = { };
This is not valid in ISO-C but only in C++.
Instead,
u8 buf[32] = {0};
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + state = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + state->client = client;
> + indio_dev->dev.parent = >dev;
> + indio_dev->info = _info;
> + indio_dev->name = client->name;
> + indio_dev->channels = sps30_channels;
> + indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->available_scan_masks = sps30_scan_masks;
> +
> + mutex_init(>lock);
> + crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> +
> + /*
> + * Power-on-reset causes sensor to produce some glitch on i2c bus
> + * and some controllers end up in error state. Recover simply
> + * by placing something on the bus.
> + */
> + ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> + if (ret) {
> + dev_err(>dev, "failed to reset device\n");
> + return ret;
> + }
> + usleep_range(250, 350);
Isn't that range too high ?
msleep_interruptible ?
> + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> + ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> + if (ret) {
> + dev_err(>dev, "failed to read serial number\n");
> + return ret;
> + }
> + dev_info(>dev, "serial number: %s\n", buf);
> +
> + ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> + if (ret) {
> + dev_err(>dev, "failed to start measurement\n");
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(>dev, indio_dev, NULL,
> + sps30_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(>dev, indio_dev);
> +}
> +
> +static int sps30_remove(struct i2c_client *client)
Perfect candidate for `devm_add_action_or_reset()` ?
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct sps30_state *state = iio_priv(indio_dev);
> +
> + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> + return 0;
> +}
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/3] iio: chemical: add support for Sensirion SPS30 sensor
]
> +static int sps30_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct sps30_state *state;
> + u8 buf[32] = { };
This is not valid in ISO-C but only in C++.
Instead,
u8 buf[32] = {0};
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + state = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + state->client = client;
> + indio_dev->dev.parent = >dev;
> + indio_dev->info = _info;
> + indio_dev->name = client->name;
> + indio_dev->channels = sps30_channels;
> + indio_dev->num_channels = ARRAY_SIZE(sps30_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->available_scan_masks = sps30_scan_masks;
> +
> + mutex_init(>lock);
> + crc8_populate_msb(sps30_crc8_table, SPS30_CRC8_POLYNOMIAL);
> +
> + /*
> + * Power-on-reset causes sensor to produce some glitch on i2c bus
> + * and some controllers end up in error state. Recover simply
> + * by placing something on the bus.
> + */
> + ret = sps30_do_cmd(state, SPS30_RESET, NULL, 0);
> + if (ret) {
> + dev_err(>dev, "failed to reset device\n");
> + return ret;
> + }
> + usleep_range(250, 350);
Isn't that range too high ?
msleep_interruptible ?
> + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> + ret = sps30_do_cmd(state, SPS30_READ_SERIAL, buf, sizeof(buf));
> + if (ret) {
> + dev_err(>dev, "failed to read serial number\n");
> + return ret;
> + }
> + dev_info(>dev, "serial number: %s\n", buf);
> +
> + ret = sps30_do_cmd(state, SPS30_START_MEAS, NULL, 0);
> + if (ret) {
> + dev_err(>dev, "failed to start measurement\n");
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(>dev, indio_dev, NULL,
> + sps30_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(>dev, indio_dev);
> +}
> +
> +static int sps30_remove(struct i2c_client *client)
Perfect candidate for `devm_add_action_or_reset()` ?
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct sps30_state *state = iio_priv(indio_dev);
> +
> + sps30_do_cmd(state, SPS30_STOP_MEAS, NULL, 0);
> +
> + return 0;
> +}
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v7 3/3] staging: iio: ad2s1210: Add device tree table.
On Wed, Oct 31, 2018 at 09:30:36PM +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
>
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> b/drivers/staging/iio/resolver/ad2s1210.c
> index d3e7d5aad2c8..7c50def91a2b 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -701,6 +701,12 @@ static int ad2s1210_remove(struct spi_device *spi)
> return 0;
> }
>
> +static const struct of_device_id ad2s1210_of_match[] = {
> + { .compatible = "adi,ad2s1210", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
I believe this needs to be documented at:
Documentation/devicetree/bindings/iio/resolver/ad2s1210.txt
Cc'ed to devictree list + Rob(DT Maintainer).
Just wondering why didn't it came up till now from the IIO reviewers ? v7!!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v7 3/3] staging: iio: ad2s1210: Add device tree table.
On Wed, Oct 31, 2018 at 09:30:36PM +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
>
> Signed-off-by: Nishad Kamdar
> ---
> drivers/staging/iio/resolver/ad2s1210.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> b/drivers/staging/iio/resolver/ad2s1210.c
> index d3e7d5aad2c8..7c50def91a2b 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -701,6 +701,12 @@ static int ad2s1210_remove(struct spi_device *spi)
> return 0;
> }
>
> +static const struct of_device_id ad2s1210_of_match[] = {
> + { .compatible = "adi,ad2s1210", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad2s1210_of_match);
I believe this needs to be documented at:
Documentation/devicetree/bindings/iio/resolver/ad2s1210.txt
Cc'ed to devictree list + Rob(DT Maintainer).
Just wondering why didn't it came up till now from the IIO reviewers ? v7!!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > The "possible alignement issues" in CHECK report is difficult to figure
> > out by just doing a glance analysis. :)
> >
> > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
>
> If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> int? But my little experiments suggest that the size is the smallest that
> fits the requested bits and alignment chosen by the compiler, regardless of
> the type.
Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a
pointer.
It can be applied only when these bitfields are wrapped in a structure.
Testing:
#include
#include
struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};
int main(void)
{
printf("%zu\n", sizeof(struct S));
}
Output: 1
If I change all bool to unsigned int, output is: *4*.
So, conclusion is compiler doesn't squeeze the size less than
native size of the datatype i.e., if we changed all members to
unsigned int:1,
total width = 4 bits
padding = 4 bits
Therefore, total size should have been = 1 byte!
But since sizeof(unsigned int) == 4, it can't be squeezed to
less than it.
> bool x:1 has the advantage that anything that is not 0 is considered true.
Yes, implicit conversion rules for boolean.
> So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.
Well, int x:1 can either have 0..1 or -1..0 range due implementation
defined behavior as I said in the previous reply.
If you really want to consider negative values, then make it explicit
using `signed int x:1` which make range guaranteed to be -1..0
Regardless, integer conversion rules will kick in.
> But the :1 adds instructions, so at least for only one bool, where little
> space is saved, it is probably not worth it.
True, we should reply on a promised guideline rather than possibility.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > The "possible alignement issues" in CHECK report is difficult to figure
> > out by just doing a glance analysis. :)
> >
> > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
>
> If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> int? But my little experiments suggest that the size is the smallest that
> fits the requested bits and alignment chosen by the compiler, regardless of
> the type.
Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a
pointer.
It can be applied only when these bitfields are wrapped in a structure.
Testing:
#include
#include
struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};
int main(void)
{
printf("%zu\n", sizeof(struct S));
}
Output: 1
If I change all bool to unsigned int, output is: *4*.
So, conclusion is compiler doesn't squeeze the size less than
native size of the datatype i.e., if we changed all members to
unsigned int:1,
total width = 4 bits
padding = 4 bits
Therefore, total size should have been = 1 byte!
But since sizeof(unsigned int) == 4, it can't be squeezed to
less than it.
> bool x:1 has the advantage that anything that is not 0 is considered true.
Yes, implicit conversion rules for boolean.
> So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.
Well, int x:1 can either have 0..1 or -1..0 range due implementation
defined behavior as I said in the previous reply.
If you really want to consider negative values, then make it explicit
using `signed int x:1` which make range guaranteed to be -1..0
Regardless, integer conversion rules will kick in.
> But the :1 adds instructions, so at least for only one bool, where little
> space is saved, it is probably not worth it.
True, we should reply on a promised guideline rather than possibility.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi Sasha,
On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
Agreed!
FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207
As Steve says:
"Thus, changing:
int a : 1;
int b : 1;
int c : 1;
int d : 1;
to
bool a;
bool b;
bool c;
bool d;
at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."
In the above cases, we have all bitfields members and no non-bitfields
members.
But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9
Implementation Defined
--
* Whether a ''plain'' int bit-field is treated as a signed int
bit-field or as an unsigned int bit-field.
eg: `int foo:3` may have a range from 0..7 or -4..3
So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.
Also, such an change can also handle unsigned overflow
or better said "wrapping".
* Whether a bit-field can straddle a storage-unit boundary.
So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.
* The alignment of non-bit-field members of structures.
...
The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)
Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.
Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.
But if anyone is interested check this:
https://godbolt.org/
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi Sasha,
On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
Agreed!
FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207
As Steve says:
"Thus, changing:
int a : 1;
int b : 1;
int c : 1;
int d : 1;
to
bool a;
bool b;
bool c;
bool d;
at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."
In the above cases, we have all bitfields members and no non-bitfields
members.
But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9
Implementation Defined
--
* Whether a ''plain'' int bit-field is treated as a signed int
bit-field or as an unsigned int bit-field.
eg: `int foo:3` may have a range from 0..7 or -4..3
So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.
Also, such an change can also handle unsigned overflow
or better said "wrapping".
* Whether a bit-field can straddle a storage-unit boundary.
So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.
* The alignment of non-bit-field members of structures.
...
The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)
Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.
Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.
But if anyone is interested check this:
https://godbolt.org/
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3 2/3] iio: adc128s052: add ACPI _HID AANT1280
Hi Dan,
On Thu, Oct 25, 2018 at 04:35:41PM +0100, Dan O'Donovan wrote:
> From: Nicola Lunghi
>
> ACPI _HID AANT1280 matches an ADC124S101 present on E3940 SKUs of the UP
> Squared board.
>
> Add it to the driver.
>
> Signed-off-by: Nicola Lunghi
> [jav...@emutex.com: fix up commit message and one checkpatch warning]
> Signed-off-by: Javier Arteaga
> Signed-off-by: Dan O'Donovan
> ---
> drivers/iio/adc/ti-adc128s052.c | 18 +-
> 1 file changed, 17 insertions(+), 1 deletion(-)
[]
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id adc128_acpi_match[] = {
> + { "AANT1280", 2 }, /* ADC124S021 compatible ACPI ID */
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
> +#endif
I'm curious about the naming conventions used for selecting
an ACPI ID.
AFAIK, ACPI or PNP ID consists of *Vendor* ID + Product ID.
PNP ID: PNP Vendor IDs consist of 3 characters, each character being
an uppercase letter (A-Z).
ACPI ID: ACPI Vendor IDs consist of 4 characters, each character being
either an uppercase letter (A-Z) or a numeral (0-9).
In your case,
Vendor ID -> AANT (AAEON TECHNOLOGY INC. registered ACPI ID prefix)
http://www.uefi.org/ACPI_ID_List?search=AANT
Product ID -> 1280
How did you come up with 1280 ? And why AANT ? why not TXNW which is
registered ACPI prefix for TEXAS INSTRUMENTS ?
http://www.uefi.org/ACPI_ID_List?search=Texas+
Latest ACPI manuals says:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s Plug and Play
hardware ID.[1]
[1] "A Plug and Play ID or ACPI ID can be obtained by sending e-mail to
pn...@microsoft.com."
A _HID object evaluates to either a numeric 32-bit compressed EISA type ID or a
string. If a
string, the format must be an alphanumeric PNP or ACPI ID with no asterisk or
other leading
characters.
A valid PNP ID must be of the form "AAA" where A is an uppercase letter and
# is a hex
digit. A valid ACPI ID must be of the form "" where N is an uppercase
letter or a
digit ('0'-'9') and # is a hex digit. This specification reserves the string
"ACPI" for use only
with devices defined herein. It further reserves all strings representing 4 HEX
digits for
exclusive use with PCI-assigned Vendor IDs.
Next question:
There are a lot drivers in iio/ using ACPI for device enumeration using
these IDs. For now, let's take an example of Bosch Sensors:
drivers/iio/accel/bmc150-accel-i2c.c:static const struct acpi_device_id
bmc150_accel_acpi_match[] = {
drivers/iio/accel/bmc150-accel-i2c.c- {"BSBA0150",bmc150},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMC150A", bmc150},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMI055A", bmi055},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMA0255", bma255},
drivers/iio/gyro/bmg160_i2c.c:static const struct acpi_device_id
bmg160_acpi_match[] = {
drivers/iio/gyro/bmg160_i2c.c- {"BMG0160", 0},
drivers/iio/gyro/bmg160_i2c.c- {"BMI055B", 0},
drivers/iio/gyro/bmg160_i2c.c- {},
drivers/iio/gyro/bmg160_i2c.c-};
drivers/iio/imu/bmi160/bmi160_i2c.c:static const struct acpi_device_id
bmi160_acpi_match[] = {
drivers/iio/imu/bmi160/bmi160_i2c.c-{"BMI0160", 0},
drivers/iio/imu/bmi160/bmi160_i2c.c-{ },
drivers/iio/imu/bmi160/bmi160_i2c.c-};
drivers/iio/imu/bmi160/bmi160_i2c.c-MODULE_DEVICE_TABLE(acpi,
bmi160_acpi_match);
Bosch registered ACPI ID: BOSC
http://www.uefi.org/ACPI_ID_List?search=Bosch
Bosch registered PNP ID: BSG
http://www.uefi.org/PNP_ID_List?search=Bosch
So, how could we use PNP ID "BMI" which is registered prefix for
BENSON MEDICAL INSTRUMENTS COMPANY ?
http://www.uefi.org/PNP_ID_List?search=BMI
Product ID -> "160" is fine for bmi160 which uniquely identifies the
sensor device.
But shouldn't the prefix be "BSG0160" instead of "BMI0160" ?
When I wrote the driver for Bosch BME680, I followed the same guideline
as done everywhere else in the Bosch family:
drivers/iio/pressure/bmp280-i2c.c- {"BMP0280", BMP280_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BMP0180", BMP180_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BMP0085", BMP180_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BME0280", BME280_CHIP_ID },
Therefore, I used BME0680 for bosch bme680 sensor!
In OF matching, we use vendor prefixes and that looks more legimate:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/vendor-prefixes.txt
Dan,
These questions are not just for you but to rest of the community
members as well.
If there is something I misunderstood, then please let me know :)
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3 2/3] iio: adc128s052: add ACPI _HID AANT1280
Hi Dan,
On Thu, Oct 25, 2018 at 04:35:41PM +0100, Dan O'Donovan wrote:
> From: Nicola Lunghi
>
> ACPI _HID AANT1280 matches an ADC124S101 present on E3940 SKUs of the UP
> Squared board.
>
> Add it to the driver.
>
> Signed-off-by: Nicola Lunghi
> [jav...@emutex.com: fix up commit message and one checkpatch warning]
> Signed-off-by: Javier Arteaga
> Signed-off-by: Dan O'Donovan
> ---
> drivers/iio/adc/ti-adc128s052.c | 18 +-
> 1 file changed, 17 insertions(+), 1 deletion(-)
[]
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id adc128_acpi_match[] = {
> + { "AANT1280", 2 }, /* ADC124S021 compatible ACPI ID */
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
> +#endif
I'm curious about the naming conventions used for selecting
an ACPI ID.
AFAIK, ACPI or PNP ID consists of *Vendor* ID + Product ID.
PNP ID: PNP Vendor IDs consist of 3 characters, each character being
an uppercase letter (A-Z).
ACPI ID: ACPI Vendor IDs consist of 4 characters, each character being
either an uppercase letter (A-Z) or a numeral (0-9).
In your case,
Vendor ID -> AANT (AAEON TECHNOLOGY INC. registered ACPI ID prefix)
http://www.uefi.org/ACPI_ID_List?search=AANT
Product ID -> 1280
How did you come up with 1280 ? And why AANT ? why not TXNW which is
registered ACPI prefix for TEXAS INSTRUMENTS ?
http://www.uefi.org/ACPI_ID_List?search=Texas+
Latest ACPI manuals says:
6.1.5 _HID (Hardware ID)
This object is used to supply OSPM with the device’s Plug and Play
hardware ID.[1]
[1] "A Plug and Play ID or ACPI ID can be obtained by sending e-mail to
pn...@microsoft.com."
A _HID object evaluates to either a numeric 32-bit compressed EISA type ID or a
string. If a
string, the format must be an alphanumeric PNP or ACPI ID with no asterisk or
other leading
characters.
A valid PNP ID must be of the form "AAA" where A is an uppercase letter and
# is a hex
digit. A valid ACPI ID must be of the form "" where N is an uppercase
letter or a
digit ('0'-'9') and # is a hex digit. This specification reserves the string
"ACPI" for use only
with devices defined herein. It further reserves all strings representing 4 HEX
digits for
exclusive use with PCI-assigned Vendor IDs.
Next question:
There are a lot drivers in iio/ using ACPI for device enumeration using
these IDs. For now, let's take an example of Bosch Sensors:
drivers/iio/accel/bmc150-accel-i2c.c:static const struct acpi_device_id
bmc150_accel_acpi_match[] = {
drivers/iio/accel/bmc150-accel-i2c.c- {"BSBA0150",bmc150},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMC150A", bmc150},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMI055A", bmi055},
drivers/iio/accel/bmc150-accel-i2c.c- {"BMA0255", bma255},
drivers/iio/gyro/bmg160_i2c.c:static const struct acpi_device_id
bmg160_acpi_match[] = {
drivers/iio/gyro/bmg160_i2c.c- {"BMG0160", 0},
drivers/iio/gyro/bmg160_i2c.c- {"BMI055B", 0},
drivers/iio/gyro/bmg160_i2c.c- {},
drivers/iio/gyro/bmg160_i2c.c-};
drivers/iio/imu/bmi160/bmi160_i2c.c:static const struct acpi_device_id
bmi160_acpi_match[] = {
drivers/iio/imu/bmi160/bmi160_i2c.c-{"BMI0160", 0},
drivers/iio/imu/bmi160/bmi160_i2c.c-{ },
drivers/iio/imu/bmi160/bmi160_i2c.c-};
drivers/iio/imu/bmi160/bmi160_i2c.c-MODULE_DEVICE_TABLE(acpi,
bmi160_acpi_match);
Bosch registered ACPI ID: BOSC
http://www.uefi.org/ACPI_ID_List?search=Bosch
Bosch registered PNP ID: BSG
http://www.uefi.org/PNP_ID_List?search=Bosch
So, how could we use PNP ID "BMI" which is registered prefix for
BENSON MEDICAL INSTRUMENTS COMPANY ?
http://www.uefi.org/PNP_ID_List?search=BMI
Product ID -> "160" is fine for bmi160 which uniquely identifies the
sensor device.
But shouldn't the prefix be "BSG0160" instead of "BMI0160" ?
When I wrote the driver for Bosch BME680, I followed the same guideline
as done everywhere else in the Bosch family:
drivers/iio/pressure/bmp280-i2c.c- {"BMP0280", BMP280_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BMP0180", BMP180_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BMP0085", BMP180_CHIP_ID },
drivers/iio/pressure/bmp280-i2c.c- {"BME0280", BME280_CHIP_ID },
Therefore, I used BME0680 for bosch bme680 sensor!
In OF matching, we use vendor prefixes and that looks more legimate:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/vendor-prefixes.txt
Dan,
These questions are not just for you but to rest of the community
members as well.
If there is something I misunderstood, then please let me know :)
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad7780: update voltage on read
On Thu, Oct 25, 2018 at 11:26:36AM -0300, Renato Lui Geh wrote: > Hi, > > Thanks for the quick review. :) > > > But please create one patch per issue and do not put unrelated changes into > > the same patch. > > Should I resend this patch as a patchset containing the two changes? Yes! "One patch per change policy" > > Also your mail client seems to have replaced tabs in the patch with spaces, > > this means the patch will not apply cleanly. Check the > > Documentation/email-clients.txt file for some hints how to configure your > > mail client so it will not break patches. > > From my end my original email patch appears to have tabs instead of > spaces. I redownloaded my email and vim shows that the indentation has > the ^I tab characters. But when downloading your reply it was converted > to spaces. Am I missing something? Your patch applies fine. I think the problem is on Lars end due to Thunderbird. In the meantime, you can verify the patch using: $ git am Also, you can try to use `git send-email` to send patches. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad7780: update voltage on read
On Thu, Oct 25, 2018 at 11:26:36AM -0300, Renato Lui Geh wrote: > Hi, > > Thanks for the quick review. :) > > > But please create one patch per issue and do not put unrelated changes into > > the same patch. > > Should I resend this patch as a patchset containing the two changes? Yes! "One patch per change policy" > > Also your mail client seems to have replaced tabs in the patch with spaces, > > this means the patch will not apply cleanly. Check the > > Documentation/email-clients.txt file for some hints how to configure your > > mail client so it will not break patches. > > From my end my original email patch appears to have tabs instead of > spaces. I redownloaded my email and vim shows that the indentation has > the ^I tab characters. But when downloading your reply it was converted > to spaces. Am I missing something? Your patch applies fine. I think the problem is on Lars end due to Thunderbird. In the meantime, you can verify the patch using: $ git am Also, you can try to use `git send-email` to send patches. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
>
>
> On 2018年10月12日 15:35, Song Qiang wrote:
> > PNI RM3100 is a high resolution, large signal immunity magnetometer,
> > composed of 3 single sensors and a processing chip with a MagI2C
> > interface.
> >
> ...
> > +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + unsigned long scan_mask = *indio_dev->active_scan_mask;
> > + unsigned int mask_len = indio_dev->masklength;
> > + struct rm3100_data *data = iio_priv(indio_dev);
> > + struct regmap *regmap = data->regmap;
> > + int ret, i, bit;
> > +
> > + mutex_lock(>lock);
> > + switch (scan_mask) {
> > + case BIT(0) | BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(0) | BIT(1):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> Hi Jonathan,
>
> I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
>
>
> On 2018年10月12日 15:35, Song Qiang wrote:
> > PNI RM3100 is a high resolution, large signal immunity magnetometer,
> > composed of 3 single sensors and a processing chip with a MagI2C
> > interface.
> >
> ...
> > +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + unsigned long scan_mask = *indio_dev->active_scan_mask;
> > + unsigned int mask_len = indio_dev->masklength;
> > + struct rm3100_data *data = iio_priv(indio_dev);
> > + struct regmap *regmap = data->regmap;
> > + int ret, i, bit;
> > +
> > + mutex_lock(>lock);
> > + switch (scan_mask) {
> > + case BIT(0) | BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(0) | BIT(1):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> Hi Jonathan,
>
> I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad2s1210: fix 'assignment operator' style checks
On Fri, Oct 05, 2018 at 10:24:26PM -0300, Matheus Tavares Bernardino wrote: > This patch fixes all "Assignment operator '=' should be on the previous > line" checks found in ad2s1210.c by checkpatch.pl. > > Signed-off-by: Matheus Tavares > --- As already pointed out tabs -> whitespace issue. I assume you attached or copied the patch into your email client and sent it. Usually, these clients wrap the message which leads to patch corruption and hence it would apply to the maintainer's git tree cleanly. Therefore, I would suggest using `git send-email`. Also, as a safety measure you may use the `--dry-run` flag of `git send-email` to see how it would look like when you send the patch. Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: iio: ad2s1210: fix 'assignment operator' style checks
On Fri, Oct 05, 2018 at 10:24:26PM -0300, Matheus Tavares Bernardino wrote: > This patch fixes all "Assignment operator '=' should be on the previous > line" checks found in ad2s1210.c by checkpatch.pl. > > Signed-off-by: Matheus Tavares > --- As already pointed out tabs -> whitespace issue. I assume you attached or copied the patch into your email client and sent it. Usually, these clients wrap the message which leads to patch corruption and hence it would apply to the maintainer's git tree cleanly. Therefore, I would suggest using `git send-email`. Also, as a safety measure you may use the `--dry-run` flag of `git send-email` to see how it would look like when you send the patch. Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100
00_dt_match,
> + },
> + .probe_new = rm3100_probe,
> +};
> +module_i2c_driver(rm3100_driver);
> +
> +MODULE_AUTHOR("Song Qiang ");
> +MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer i2c driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/magnetometer/rm3100-spi.c
> b/drivers/iio/magnetometer/rm3100-spi.c
> new file mode 100644
> index ..1a7a69fb9872
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100-spi.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for PNI RM3100 3-axis geomagnetic sensor a spi bus.
> + *
> + * Copyright (C) 2018 Song Qiang
> + */
> +
> +#include
> +#include
> +
> +#include "rm3100.h"
> +
> +static const struct regmap_config rm3100_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .rd_table = _readable_table,
> + .wr_table = _writable_table,
> + .volatile_table = _volatile_table,
> +
> + .read_flag_mask = 0x80,
> +
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int rm3100_probe(struct spi_device *spi)
> +{
> + struct regmap *regmap;
> + int ret;
> +
> + /* Actually this device supports both mode 0 and mode 3. */
> + spi->mode = SPI_MODE_0;
> + /* Data rates cannot exceed 1Mbits. */
> + spi->max_speed_hz = 100;
> + spi->bits_per_word = 8;
> + ret = spi_setup(spi);
> + if (ret)
> + return ret;
> +
> + regmap = devm_regmap_init_spi(spi, _regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return rm3100_common_probe(>dev, regmap, spi->irq);
> +}
> +
> +static const struct of_device_id rm3100_dt_match[] = {
> + { .compatible = "pni,rm3100", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rm3100_dt_match);
> +
> +static struct spi_driver rm3100_driver = {
> + .driver = {
> + .name = "rm3100-spi",
> + .of_match_table = rm3100_dt_match,
> + },
> + .probe = rm3100_probe,
> +};
> +module_spi_driver(rm3100_driver);
> +
> +MODULE_AUTHOR("Song Qiang ");
> +MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer spi driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/magnetometer/rm3100.h
> b/drivers/iio/magnetometer/rm3100.h
> new file mode 100644
> index ..673647574add
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for PNI RM3100 driver
Remove this bogus line.
> + * Copyright (C) 2018 Song Qiang
> + */
> +
> +#ifndef RM3100_CORE_H
> +#define RM3100_CORE_H
> +
> +#include
> +
> +#define RM3100_REG_REV_ID0x36
Does this ID needs to be checked and validated during
intialisation with default state ID val ?
> +
> +/* Cycle Count Registers. */
> +#define RM3100_REG_CC_X 0x05
> +#define RM3100_REG_CC_Y 0x07
> +#define RM3100_REG_CC_Z 0x09
> +
> +/* Continues Measurement Mode register. */
Is it continuous ?
> +#define RM3100_REG_CMM 0x01
> +#define RM3100_CMM_START BIT(0)
> +#define RM3100_CMM_X BIT(4)
> +#define RM3100_CMM_Y BIT(5)
> +#define RM3100_CMM_Z BIT(6)
> +
> +/* TiMe Rate Configuration register. */
Time!
> +#define RM3100_REG_TMRC 0x0B
> +#define RM3100_TMRC_OFFSET 0x92
> +
> +/* Result Status register. */
> +#define RM3100_REG_STATUS0x34
> +#define RM3100_STATUS_DRDY BIT(7)
If this status bit is a field of status register then
align this as:
#define RM3100_REG_STATUS 0x34
#define RM3100_STATUS_DRDY BIT(7)
> +/* Measurement result registers. */
> +#define RM3100_REG_MX2 0x24
> +#define RM3100_REG_MY2 0x27
> +#define RM3100_REG_MZ2 0x2a
> +
> +#define RM3100_W_REG_START RM3100_REG_CMM
> +#define RM3100_W_REG_END RM3100_REG_REV_ID
> +#define RM3100_R_REG_START RM3100_REG_CMM
> +#define RM3100_R_REG_END RM3100_REG_STATUS
> +#define RM3100_V_REG_START RM3100_REG_MX2
> +#define RM3100_V_REG_END RM3100_REG_STATUS
> +
> +#define RM3100_SCAN_BYTES24
> +
> +struct rm3100_data {
> + struct device *dev;
Better remove this ?
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100
00_dt_match,
> + },
> + .probe_new = rm3100_probe,
> +};
> +module_i2c_driver(rm3100_driver);
> +
> +MODULE_AUTHOR("Song Qiang ");
> +MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer i2c driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/magnetometer/rm3100-spi.c
> b/drivers/iio/magnetometer/rm3100-spi.c
> new file mode 100644
> index ..1a7a69fb9872
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100-spi.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Support for PNI RM3100 3-axis geomagnetic sensor a spi bus.
> + *
> + * Copyright (C) 2018 Song Qiang
> + */
> +
> +#include
> +#include
> +
> +#include "rm3100.h"
> +
> +static const struct regmap_config rm3100_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .rd_table = _readable_table,
> + .wr_table = _writable_table,
> + .volatile_table = _volatile_table,
> +
> + .read_flag_mask = 0x80,
> +
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int rm3100_probe(struct spi_device *spi)
> +{
> + struct regmap *regmap;
> + int ret;
> +
> + /* Actually this device supports both mode 0 and mode 3. */
> + spi->mode = SPI_MODE_0;
> + /* Data rates cannot exceed 1Mbits. */
> + spi->max_speed_hz = 100;
> + spi->bits_per_word = 8;
> + ret = spi_setup(spi);
> + if (ret)
> + return ret;
> +
> + regmap = devm_regmap_init_spi(spi, _regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return rm3100_common_probe(>dev, regmap, spi->irq);
> +}
> +
> +static const struct of_device_id rm3100_dt_match[] = {
> + { .compatible = "pni,rm3100", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, rm3100_dt_match);
> +
> +static struct spi_driver rm3100_driver = {
> + .driver = {
> + .name = "rm3100-spi",
> + .of_match_table = rm3100_dt_match,
> + },
> + .probe = rm3100_probe,
> +};
> +module_spi_driver(rm3100_driver);
> +
> +MODULE_AUTHOR("Song Qiang ");
> +MODULE_DESCRIPTION("PNI RM3100 3-axis magnetometer spi driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/magnetometer/rm3100.h
> b/drivers/iio/magnetometer/rm3100.h
> new file mode 100644
> index ..673647574add
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for PNI RM3100 driver
Remove this bogus line.
> + * Copyright (C) 2018 Song Qiang
> + */
> +
> +#ifndef RM3100_CORE_H
> +#define RM3100_CORE_H
> +
> +#include
> +
> +#define RM3100_REG_REV_ID0x36
Does this ID needs to be checked and validated during
intialisation with default state ID val ?
> +
> +/* Cycle Count Registers. */
> +#define RM3100_REG_CC_X 0x05
> +#define RM3100_REG_CC_Y 0x07
> +#define RM3100_REG_CC_Z 0x09
> +
> +/* Continues Measurement Mode register. */
Is it continuous ?
> +#define RM3100_REG_CMM 0x01
> +#define RM3100_CMM_START BIT(0)
> +#define RM3100_CMM_X BIT(4)
> +#define RM3100_CMM_Y BIT(5)
> +#define RM3100_CMM_Z BIT(6)
> +
> +/* TiMe Rate Configuration register. */
Time!
> +#define RM3100_REG_TMRC 0x0B
> +#define RM3100_TMRC_OFFSET 0x92
> +
> +/* Result Status register. */
> +#define RM3100_REG_STATUS0x34
> +#define RM3100_STATUS_DRDY BIT(7)
If this status bit is a field of status register then
align this as:
#define RM3100_REG_STATUS 0x34
#define RM3100_STATUS_DRDY BIT(7)
> +/* Measurement result registers. */
> +#define RM3100_REG_MX2 0x24
> +#define RM3100_REG_MY2 0x27
> +#define RM3100_REG_MZ2 0x2a
> +
> +#define RM3100_W_REG_START RM3100_REG_CMM
> +#define RM3100_W_REG_END RM3100_REG_REV_ID
> +#define RM3100_R_REG_START RM3100_REG_CMM
> +#define RM3100_R_REG_END RM3100_REG_STATUS
> +#define RM3100_V_REG_START RM3100_REG_MX2
> +#define RM3100_V_REG_END RM3100_REG_STATUS
> +
> +#define RM3100_SCAN_BYTES24
> +
> +struct rm3100_data {
> + struct device *dev;
Better remove this ?
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: adc: Fix potential integer overflow
Hi Gustavo,
On Tue, Sep 18, 2018 at 07:53:14AM -0500, Gustavo A. R. Silva wrote:
> Cast factor to s64 in order to give the compiler complete information
> about the proper arithmetic to use and avoid a potential integer
> overflow. Notice that such variable is being used in a context
> that expects an expression of type s64 (64 bits, signed).
>
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/iio/adc/qcom-vadc-common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/qcom-vadc-common.c
> b/drivers/iio/adc/qcom-vadc-common.c
> index dcd7fb5..e360e27 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16
> adc_code,
> voltage = div64_s64(voltage, data->full_scale_code_volt);
> if (voltage > 0) {
> voltage *= prescale->den;
> - temp = prescale->num * factor;
> + temp = prescale->num * (s64)factor;
As Jonathan pointed it is a false positive, let me share some more
insight on this particular set of warnings.
`num` is u32 and `factor` is unsigned int(u32 on most implementations).
So, if multiplication b/w them exceeds UNIT_MAX then that is perfectly
defined behavior in C. And often called "wrapping".
https://port70.net/~nsz/c/c11/n1570.html#6.2.5p9
And *if* it exceeds UNIT_MAX, then it is certainly wrong arthimetic
implementation by the author.
On the other hand, if it were the case signed int overflow then
certainly it is undefined behavior and called "overflow".
And here `temp` is guaranteed to not overflow!
But I don't understand what issue are you trying to resolve here and I'm
interested in this particular set of warnings because I too get coverity
scan reports on the same although I only search for IIO drivers issues.
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: adc: Fix potential integer overflow
Hi Gustavo,
On Tue, Sep 18, 2018 at 07:53:14AM -0500, Gustavo A. R. Silva wrote:
> Cast factor to s64 in order to give the compiler complete information
> about the proper arithmetic to use and avoid a potential integer
> overflow. Notice that such variable is being used in a context
> that expects an expression of type s64 (64 bits, signed).
>
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/iio/adc/qcom-vadc-common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/qcom-vadc-common.c
> b/drivers/iio/adc/qcom-vadc-common.c
> index dcd7fb5..e360e27 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16
> adc_code,
> voltage = div64_s64(voltage, data->full_scale_code_volt);
> if (voltage > 0) {
> voltage *= prescale->den;
> - temp = prescale->num * factor;
> + temp = prescale->num * (s64)factor;
As Jonathan pointed it is a false positive, let me share some more
insight on this particular set of warnings.
`num` is u32 and `factor` is unsigned int(u32 on most implementations).
So, if multiplication b/w them exceeds UNIT_MAX then that is perfectly
defined behavior in C. And often called "wrapping".
https://port70.net/~nsz/c/c11/n1570.html#6.2.5p9
And *if* it exceeds UNIT_MAX, then it is certainly wrong arthimetic
implementation by the author.
On the other hand, if it were the case signed int overflow then
certainly it is undefined behavior and called "overflow".
And here `temp` is guaranteed to not overflow!
But I don't understand what issue are you trying to resolve here and I'm
interested in this particular set of warnings because I too get coverity
scan reports on the same although I only search for IIO drivers issues.
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v6 1/2] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 16:24:21 +0800
> Song Qiang wrote:
>
> > This driver was originally written by ST in 2016 as a misc input device
> > driver, and hasn't been maintained for a long time. I grabbed some code
> > from it's API and reformed it into an iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > It can be tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> >
> > Signed-off-by: Song Qiang
> There are a few bits and bobs in here, but as they are all minor and
> one at least was me giving you wrong advise, I've fixed it up.
>
> Please check I haven't made a mess of it!
>
> Applied with changes to the togreg branch of iio.git and pushed out
> as testing (where it should be visible now) for the autobuilders to
> play with it.
The SPDX license identifier is inconsistent!
> +// SPDX-License-Identifier: GPL-2.0+
> +MODULE_LICENSE("GPL v2");
216 For 'GNU General Public License (GPL) version 2 only' use:
217 SPDX-License-Identifier: GPL-2.0
218 For 'GNU General Public License (GPL) version 2 or any later
version' use:
219 SPDX-License-Identifier: GPL-2.0+
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v6 1/2] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
On Sat, Sep 22, 2018 at 03:46:58PM +0100, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 16:24:21 +0800
> Song Qiang wrote:
>
> > This driver was originally written by ST in 2016 as a misc input device
> > driver, and hasn't been maintained for a long time. I grabbed some code
> > from it's API and reformed it into an iio proximity device driver.
> > This version of driver uses i2c bus to talk to the sensor and
> > polling for measuring completes, so no irq line is needed.
> > It can be tested with reading from
> > /sys/bus/iio/devices/iio:deviceX/in_distance_input
> >
> > Signed-off-by: Song Qiang
> There are a few bits and bobs in here, but as they are all minor and
> one at least was me giving you wrong advise, I've fixed it up.
>
> Please check I haven't made a mess of it!
>
> Applied with changes to the togreg branch of iio.git and pushed out
> as testing (where it should be visible now) for the autobuilders to
> play with it.
The SPDX license identifier is inconsistent!
> +// SPDX-License-Identifier: GPL-2.0+
> +MODULE_LICENSE("GPL v2");
216 For 'GNU General Public License (GPL) version 2 only' use:
217 SPDX-License-Identifier: GPL-2.0
218 For 'GNU General Public License (GPL) version 2 or any later
version' use:
219 SPDX-License-Identifier: GPL-2.0+
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode
On Sat, Sep 22, 2018 at 11:36:16AM +0100, Jonathan Cameron wrote: > On Fri, 21 Sep 2018 15:26:03 +0800 > kbuild test robot wrote: > > > Hi Eugen, > This one is leaving me stumped... > > Anyone care to point out what I'm missing that is wrong here? > > Also Eugen, please don't cc stable on a patch directly. It is fine to send > a backport request once a patch has hit mainline, but before that it's just > adding noise to a list as they won't take it directly anyway. Song is correct on this one. So, just replace the declaration to: struct iio_chan_spec const *chan; and then warning goes away... Another thing, its better not to reply the automated bot and instead to the author by changing "From" address. :) That is what I do often when replying to that thread. https://lore.kernel.org/lkml/20180921182616.GA2077@himanshu-Vostro-3559/ And it nests properly in the given thread avoiding any noise to kbuild bot. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH 2/2] iio: adc: at91: fix wrong channel number in triggered buffer mode
On Sat, Sep 22, 2018 at 11:36:16AM +0100, Jonathan Cameron wrote: > On Fri, 21 Sep 2018 15:26:03 +0800 > kbuild test robot wrote: > > > Hi Eugen, > This one is leaving me stumped... > > Anyone care to point out what I'm missing that is wrong here? > > Also Eugen, please don't cc stable on a patch directly. It is fine to send > a backport request once a patch has hit mainline, but before that it's just > adding noise to a list as they won't take it directly anyway. Song is correct on this one. So, just replace the declaration to: struct iio_chan_spec const *chan; and then warning goes away... Another thing, its better not to reply the automated bot and instead to the author by changing "From" address. :) That is what I do often when replying to that thread. https://lore.kernel.org/lkml/20180921182616.GA2077@himanshu-Vostro-3559/ And it nests properly in the given thread avoiding any noise to kbuild bot. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: magnetometer: hmc5843: Clean up redundant code.
Hi Song, On Fri, Sep 21, 2018 at 04:10:16PM +0800, kbuild test robot wrote: > Hi Song, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc4 next-20180920] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: x86_64-randconfig-u0-09211331 (attached as .config) > compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > > >> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! > >> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! > >> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! You would need to export these above symbols using EXPORT_SYMBOL() to be used by i2c/spi modules. But on the other hand, exporting too many symbols is a bad idea since it is only used for this driver and not at any other place in IIO. So, in my opinion drop this patch and leave the code as-is. https://lkml.org/lkml/2018/7/16/566 --> worth reading Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: magnetometer: hmc5843: Clean up redundant code.
Hi Song, On Fri, Sep 21, 2018 at 04:10:16PM +0800, kbuild test robot wrote: > Hi Song, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc4 next-20180920] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Song-Qiang/iio-magnetometer-hmc5843-Clean-up-redundant-code/20180921-091239 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: x86_64-randconfig-u0-09211331 (attached as .config) > compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > > >> ERROR: "hmc5843_volatile_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! > >> ERROR: "hmc5843_readable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! > >> ERROR: "hmc5843_writable_table" [drivers/iio/magnetometer/hmc5843_i2c.ko] > >> undefined! You would need to export these above symbols using EXPORT_SYMBOL() to be used by i2c/spi modules. But on the other hand, exporting too many symbols is a bad idea since it is only used for this driver and not at any other place in IIO. So, in my opinion drop this patch and leave the code as-is. https://lkml.org/lkml/2018/7/16/566 --> worth reading Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH -next] iio: potentiometer: mcp4531: merge calls to of_match_device and of_device_get_match_data
On Sat, Sep 15, 2018 at 06:54:30PM +0800, YueHaibing wrote: > Drop call to of_match_device, which is subsumed by the subsequent > call to of_device_get_match_data. The code becomes simpler, and a > temporary variable can be dropped. > > Found by coccinelle. > > Signed-off-by: YueHaibing Already applied! https://lore.kernel.org/lkml/20180819201752.2280be76@archlinux/ Anyway, if you want to work on making similar changes, you may refer the following cocci script: I tested[1] the changes through 0-day and had few false positives and regressions. Therefore, check the results and compile test the changes before sending patches. Julia, and I had tried to make the confidence "High" but few cases were difficult to resolve, and later neither of us had time to look back at it again. Not all maintainers will acknowledge such a change, but IIRC Rob Herring gave a thumbs up on few cases on the dt-mailing list. [1] https://github.com/himanshujha199640/linux-next/commit/efb7ed923bd00c86fe0a4e67e2ddb636ff0e0ff4 - /// Use of_device_get_match_data() to get matched data in an OF driver //# of_device_get_match_data() returns const * and therefore the left //# argument of assignment should also be a const * for compatible //# types. /// // Confidence: Moderate // Copyright: (C) 2018 Himanshu Jha, GPLv2. // Copyright: (C) 2018 Julia Lawall, INRIA/LIP6. GPLv2. // Keywords: of_match_device, of_device_get_match_data virtual patch virtual context virtual org virtual report @r1 depends on patch && !context && !org && !report@ expression x,y,z; identifier match; type T; statement S; @@ - match = of_match_device(x, y); ( - if(match==NULL) S - z = match->data; + z = of_device_get_match_data(y); | - if(match==NULL) S - z = (T *)match->data; + z = of_device_get_match_data(y); | - if(match==NULL) S - z = (T)match->data; + z = (T)of_device_get_match_data(y); | - if(match!= NULL) - z = match->data; + z = of_device_get_match_data(y); | - if(match!= NULL) - z = (T *)match->data; + z = of_device_get_match_data(y); | - if(match!= NULL) - z = (T)match->data; + z = (T)of_device_get_match_data(y); ) ... when != match @r2 depends on r1 && patch && !context && !org && !report@ type T1; expression e; identifier r1.match; @@ ( - T1 match = e; | - T1 match; ) // @r1_context depends on !patch && (context || org || report)@ type T; identifier match; statement S; expression x, y, z; position j0, j1; @@ * match@j0 = of_match_device(x, y); ( * if(match==NULL) S * z = match@j1->data; | * if(match==NULL) S * z = (T *)match@j1->data; | * if(match==NULL) S * z = (T)match@j1->data; | * if(match!= NULL) * z = match@j1->data; | * if(match!= NULL) * z = (T *)match@j1->data; | * if(match!= NULL) * z = (T)match@j1->data; ) ... when != match @r2_context depends on r1 && !patch && (context || org || report)@ type T1; identifier r1_context.match; expression e; position j0; @@ ( * T1 match@j0 = e; | * T1 match@j0; ) // @script:python r1_org depends on org@ j0 << r1_context.j0; j1 << r1_context.j1; @@ msg = "WARNING: opportunity for of_device_get_match_data." coccilib.org.print_todo(j0[0], msg) coccilib.org.print_link(j1[0], "") // @script:python r1_report depends on report@ j0 << r1_context.j0; j1 << r1_context.j1; @@ msg = "WARNING: opportunity for of_device_get_match_data around line %s." % (j1[0].line) coccilib.report.print_report(j0[0], msg) - Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH -next] iio: potentiometer: mcp4531: merge calls to of_match_device and of_device_get_match_data
On Sat, Sep 15, 2018 at 06:54:30PM +0800, YueHaibing wrote: > Drop call to of_match_device, which is subsumed by the subsequent > call to of_device_get_match_data. The code becomes simpler, and a > temporary variable can be dropped. > > Found by coccinelle. > > Signed-off-by: YueHaibing Already applied! https://lore.kernel.org/lkml/20180819201752.2280be76@archlinux/ Anyway, if you want to work on making similar changes, you may refer the following cocci script: I tested[1] the changes through 0-day and had few false positives and regressions. Therefore, check the results and compile test the changes before sending patches. Julia, and I had tried to make the confidence "High" but few cases were difficult to resolve, and later neither of us had time to look back at it again. Not all maintainers will acknowledge such a change, but IIRC Rob Herring gave a thumbs up on few cases on the dt-mailing list. [1] https://github.com/himanshujha199640/linux-next/commit/efb7ed923bd00c86fe0a4e67e2ddb636ff0e0ff4 - /// Use of_device_get_match_data() to get matched data in an OF driver //# of_device_get_match_data() returns const * and therefore the left //# argument of assignment should also be a const * for compatible //# types. /// // Confidence: Moderate // Copyright: (C) 2018 Himanshu Jha, GPLv2. // Copyright: (C) 2018 Julia Lawall, INRIA/LIP6. GPLv2. // Keywords: of_match_device, of_device_get_match_data virtual patch virtual context virtual org virtual report @r1 depends on patch && !context && !org && !report@ expression x,y,z; identifier match; type T; statement S; @@ - match = of_match_device(x, y); ( - if(match==NULL) S - z = match->data; + z = of_device_get_match_data(y); | - if(match==NULL) S - z = (T *)match->data; + z = of_device_get_match_data(y); | - if(match==NULL) S - z = (T)match->data; + z = (T)of_device_get_match_data(y); | - if(match!= NULL) - z = match->data; + z = of_device_get_match_data(y); | - if(match!= NULL) - z = (T *)match->data; + z = of_device_get_match_data(y); | - if(match!= NULL) - z = (T)match->data; + z = (T)of_device_get_match_data(y); ) ... when != match @r2 depends on r1 && patch && !context && !org && !report@ type T1; expression e; identifier r1.match; @@ ( - T1 match = e; | - T1 match; ) // @r1_context depends on !patch && (context || org || report)@ type T; identifier match; statement S; expression x, y, z; position j0, j1; @@ * match@j0 = of_match_device(x, y); ( * if(match==NULL) S * z = match@j1->data; | * if(match==NULL) S * z = (T *)match@j1->data; | * if(match==NULL) S * z = (T)match@j1->data; | * if(match!= NULL) * z = match@j1->data; | * if(match!= NULL) * z = (T *)match@j1->data; | * if(match!= NULL) * z = (T)match@j1->data; ) ... when != match @r2_context depends on r1 && !patch && (context || org || report)@ type T1; identifier r1_context.match; expression e; position j0; @@ ( * T1 match@j0 = e; | * T1 match@j0; ) // @script:python r1_org depends on org@ j0 << r1_context.j0; j1 << r1_context.j1; @@ msg = "WARNING: opportunity for of_device_get_match_data." coccilib.org.print_todo(j0[0], msg) coccilib.org.print_link(j1[0], "") // @script:python r1_report depends on report@ j0 << r1_context.j0; j1 << r1_context.j1; @@ msg = "WARNING: opportunity for of_device_get_match_data around line %s." % (j1[0].line) coccilib.report.print_report(j0[0], msg) - Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
Hi Song,
On Thu, Sep 13, 2018 at 10:22:29AM +0800, Song Qiang wrote:
> This driver was originally written by ST in 2016 as a misc input device
> driver, and hasn't been maintained for a long time. I grabbed some code
> from it's API and reformed it into a iio proximity device driver.
> This version of driver uses i2c bus to talk to the sensor and
> polling for measuring completes, so no irq line is needed.
> This version of driver supports only one-shot mode, and it can be
> tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_raw
>
> Signed-off-by: Song Qiang
I'm sorry but you probably need another revision.
And don't feel embarrassed, I mean this is your first patch
in the kernel and that's an IIO driver!!
I used to fix spelling mistakes, align parameters, ... an year ago
and even those got rejected several times ;)
> ---
> +ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> +M: Song Qiang
> +L: linux-...@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/proximity/vl53l0x-i2c.c
> +F: Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
Obligations!
Just don't go "Keyser Söze" once you're done with your major
at University. You would need to test/review patches in future.
Not a big deal though..
> +
> +static const struct of_device_id st_vl53l0x_dt_match[] = {
> + { .compatible = "st,vl53l0x-i2c", },
> + { }
> +};
MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match); ?
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
Hi Song,
On Thu, Sep 13, 2018 at 10:22:29AM +0800, Song Qiang wrote:
> This driver was originally written by ST in 2016 as a misc input device
> driver, and hasn't been maintained for a long time. I grabbed some code
> from it's API and reformed it into a iio proximity device driver.
> This version of driver uses i2c bus to talk to the sensor and
> polling for measuring completes, so no irq line is needed.
> This version of driver supports only one-shot mode, and it can be
> tested with reading from
> /sys/bus/iio/devices/iio:deviceX/in_distance_raw
>
> Signed-off-by: Song Qiang
I'm sorry but you probably need another revision.
And don't feel embarrassed, I mean this is your first patch
in the kernel and that's an IIO driver!!
I used to fix spelling mistakes, align parameters, ... an year ago
and even those got rejected several times ;)
> ---
> +ST VL53L0X ToF RANGER(I2C) IIO DRIVER
> +M: Song Qiang
> +L: linux-...@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/proximity/vl53l0x-i2c.c
> +F: Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
Obligations!
Just don't go "Keyser Söze" once you're done with your major
at University. You would need to test/review patches in future.
Not a big deal though..
> +
> +static const struct of_device_id st_vl53l0x_dt_match[] = {
> + { .compatible = "st,vl53l0x-i2c", },
> + { }
> +};
MODULE_DEVICE_TABLE(of, st_vl53l0x_dt_match); ?
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 4/5] iio: fxas21002c: add ODR/Scale support
On Fri, Sep 14, 2018 at 04:26:44PM +0100, Afonso Bordado wrote: > Hi, > > Thanks for your help with this. > > > And I suspect it may be originating from your code snippet: > > > > #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> > > (scale))) > > > > and looking at the implementation: > > > > include/linux/iio/iio.h > > /** > > * IIO_DEGREE_TO_RAD() - Convert degree to rad > > * @deg: A value in degree > > * > > * Returns the given value converted from degree to rad > > */ > > #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 900ULL) / > > 1800ULL) > > > > This '/' operator might be the culprit! > > > > Just for checking that the error, remove the macro declaration > > `FXAS21002C_SCALE` > > plus its usage and re-cross compile using `make ARCH=i386`. > > > > In my case I used the `div64_s64` function handles builds for both > > 32/64 > > arch accordingly. > > Yes, this is indeed the culprit. If `div64_s64` works the same way, I > wonder if the best option is to change the macro definition. "works the same way" ? Let us assume that the problem arises due to the 64 bit division, in which gcc places the __divdi3() runtime function to promote the "freestanding" environment implementation. And then linking fails due to unavailability of definitions/declarations of the aforementioned function. With `div64_s64` usgae the linker binds the definition present at lib/div64.c and the build completes successfully whether building for 32/64 bit environment. But then why didn't this error showed up in the past, in the rest of the drivers ? I see its wide usage in IIO without bug reports: himanshu@himanshu-Vostro-3559:~/linux-next$ git grep -w "IIO_DEGREE_TO_RAD" drivers/iio/ | wc -l 34 And that concludes, that there is some problem within your code! In the meantime, you can try to look the disassembly of the function where this macro is actually used and search for __divdi3/__udivdi3 function referenced in the plt. I might be wrong though... Wait a while for the experts to join in! -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 4/5] iio: fxas21002c: add ODR/Scale support
On Fri, Sep 14, 2018 at 04:26:44PM +0100, Afonso Bordado wrote: > Hi, > > Thanks for your help with this. > > > And I suspect it may be originating from your code snippet: > > > > #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> > > (scale))) > > > > and looking at the implementation: > > > > include/linux/iio/iio.h > > /** > > * IIO_DEGREE_TO_RAD() - Convert degree to rad > > * @deg: A value in degree > > * > > * Returns the given value converted from degree to rad > > */ > > #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 900ULL) / > > 1800ULL) > > > > This '/' operator might be the culprit! > > > > Just for checking that the error, remove the macro declaration > > `FXAS21002C_SCALE` > > plus its usage and re-cross compile using `make ARCH=i386`. > > > > In my case I used the `div64_s64` function handles builds for both > > 32/64 > > arch accordingly. > > Yes, this is indeed the culprit. If `div64_s64` works the same way, I > wonder if the best option is to change the macro definition. "works the same way" ? Let us assume that the problem arises due to the 64 bit division, in which gcc places the __divdi3() runtime function to promote the "freestanding" environment implementation. And then linking fails due to unavailability of definitions/declarations of the aforementioned function. With `div64_s64` usgae the linker binds the definition present at lib/div64.c and the build completes successfully whether building for 32/64 bit environment. But then why didn't this error showed up in the past, in the rest of the drivers ? I see its wide usage in IIO without bug reports: himanshu@himanshu-Vostro-3559:~/linux-next$ git grep -w "IIO_DEGREE_TO_RAD" drivers/iio/ | wc -l 34 And that concludes, that there is some problem within your code! In the meantime, you can try to look the disassembly of the function where this macro is actually used and search for __divdi3/__udivdi3 function referenced in the plt. I might be wrong though... Wait a while for the experts to join in! -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: lidar-v2: replace i2c block access method with the one already implemented.
On Thu, Sep 13, 2018 at 11:51:45AM +0800, Song Qiang wrote: > This driver tries to access a block of data on a i2c bus and it tries > to manually make a device command frame and a consecutively read frame, > then uses i2c_transfer() to read data. But this has already been > implemented in i2c_smbus_read_i2c_block_data(). > Sorry for not having this device by my hand, which is a little expansive > for me, but I have another i2c device and tested with both i2c_transfer() > and i2c_smbus_read_i2c_block_data() and they all ends the same. > I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer() > function is the same as i2c_smbus_read_block_data()? The original code > is commented with something I'm not sure, but I think if it's a standard > SMBus, it should be able to use in here. > Hoping for someone to explain. This is not how you write a commit message. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > Signed-off-by: Song Qiang > --- Your doubts/suggestions/rants all should be here just below the `---` line, so that if maintainer applies the patch then your queries doesn't get included in the commit message. And no need to reply an explicit "thanks". That is implicit ;) We already have a ~30k email traffic on LKML... https://marc.info/?l=linux-kernel=1=2 Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: lidar-v2: replace i2c block access method with the one already implemented.
On Thu, Sep 13, 2018 at 11:51:45AM +0800, Song Qiang wrote: > This driver tries to access a block of data on a i2c bus and it tries > to manually make a device command frame and a consecutively read frame, > then uses i2c_transfer() to read data. But this has already been > implemented in i2c_smbus_read_i2c_block_data(). > Sorry for not having this device by my hand, which is a little expansive > for me, but I have another i2c device and tested with both i2c_transfer() > and i2c_smbus_read_i2c_block_data() and they all ends the same. > I'm not familiar with the SMBus, don't know if the lidar_smbus_xfer() > function is the same as i2c_smbus_read_block_data()? The original code > is commented with something I'm not sure, but I think if it's a standard > SMBus, it should be able to use in here. > Hoping for someone to explain. This is not how you write a commit message. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > Signed-off-by: Song Qiang > --- Your doubts/suggestions/rants all should be here just below the `---` line, so that if maintainer applies the patch then your queries doesn't get included in the commit message. And no need to reply an explicit "thanks". That is implicit ;) We already have a ~30k email traffic on LKML... https://marc.info/?l=linux-kernel=1=2 Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2 3/4] iio: adc: Add Xilinx AMS driver
Hi Manish,
On Fri, Sep 14, 2018 at 12:48:29PM +0530, Manish Narani wrote:
> The AMS includes an ADC as well as on-chip sensors that can be used to
> sample external voltages and monitor on-die operating conditions, such
> as temperature and supply voltage levels. The AMS has two SYSMON blocks.
> PL-SYSMON block is capable of monitoring off chip voltage and
> temperature.
> PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> from external master. Out of these interface currently only DRP is
> supported.
> Other block PS-SYSMON is memory mapped to PS.
> The AMS can use internal channels to monitor voltage and temperature as
> well as one primary and up to 16 auxiliary channels for measuring
> external voltages.
> The voltage and temperature monitoring channels also have event
> capability which allows to generate an interrupt when their value falls
> below or raises above a set threshold.
>
> Signed-off-by: Manish Narani
> ---
[]
> +// SPDX-License-Identifier: GPL-2.0
License Identifier seems inconsistent as below you
mentioned "GPL" and not "GPLv2".
Please check once.
Documentation/process/license-rules.rst
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + switch (chan->address) {
> + case AMS_SUPPLY1:
/* fall through */
> + case AMS_SUPPLY2:
/* fall through */
Similarly to others as well.
There is a plan to enable "-Wimplicit-fallthrough" gcc flag
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Also, Gustavo nearly cleaned all the cases and would save
his effort of doing it again :)
https://lore.kernel.org/lkml/20180903183618.ga6...@embeddedor.com/
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Xilinx, Inc.");
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2 3/4] iio: adc: Add Xilinx AMS driver
Hi Manish,
On Fri, Sep 14, 2018 at 12:48:29PM +0530, Manish Narani wrote:
> The AMS includes an ADC as well as on-chip sensors that can be used to
> sample external voltages and monitor on-die operating conditions, such
> as temperature and supply voltage levels. The AMS has two SYSMON blocks.
> PL-SYSMON block is capable of monitoring off chip voltage and
> temperature.
> PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> from external master. Out of these interface currently only DRP is
> supported.
> Other block PS-SYSMON is memory mapped to PS.
> The AMS can use internal channels to monitor voltage and temperature as
> well as one primary and up to 16 auxiliary channels for measuring
> external voltages.
> The voltage and temperature monitoring channels also have event
> capability which allows to generate an interrupt when their value falls
> below or raises above a set threshold.
>
> Signed-off-by: Manish Narani
> ---
[]
> +// SPDX-License-Identifier: GPL-2.0
License Identifier seems inconsistent as below you
mentioned "GPL" and not "GPLv2".
Please check once.
Documentation/process/license-rules.rst
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + switch (chan->address) {
> + case AMS_SUPPLY1:
/* fall through */
> + case AMS_SUPPLY2:
/* fall through */
Similarly to others as well.
There is a plan to enable "-Wimplicit-fallthrough" gcc flag
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Also, Gustavo nearly cleaned all the cases and would save
his effort of doing it again :)
https://lore.kernel.org/lkml/20180903183618.ga6...@embeddedor.com/
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Xilinx, Inc.");
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
> +/*
> + * Support for ST's VL53L0X FlightSense ToF Ranger Sensor on a i2c bus.
> + *
> + * Copyright (C) 2016 STMicroelectronics Imaging Division.
> + * Copyright (C) 2018 Song Qiang
Datasheet link + i2c slave address can be put here.
Look at other iio drivers.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define VL53L0X_DRV_NAME "vl53l0x-i2c"
> +
> +#define VL_REG_SYSRANGE_MODE_MASKGENMASK(3, 0)
> +#define VL_REG_SYSRANGE_START0x00
> +#define VL_REG_SYSRANGE_MODE_SINGLESHOT 0x00
> +#define VL_REG_SYSRANGE_MODE_START_STOP BIT(0)
> +#define VL_REG_SYSRANGE_MODE_BACKTOBACK BIT(1)
> +#define VL_REG_SYSRANGE_MODE_TIMED BIT(2)
> +#define VL_REG_SYSRANGE_MODE_HISTOGRAM BIT(3)
> +
> +#define VL_REG_SYS_SEQUENCE_CFG BIT(0)
> +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD BIT(2)
> +#define VL_REG_SYS_RANGE_CFG 0x09
> +
> +#define VL_REG_SYS_INT_GPIO_DISABLED 0x00
> +#define VL_REG_SYS_INT_GPIO_LEVEL_LOWBIT(0)
> +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH BIT(1)
> +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW0x03
> +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY BIT(2)
> +#define VL_REG_SYS_INT_CFG_GPIO 0x0A
> +#define VL_REG_SYS_INT_CLEAR 0x0B
> +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH 0x84
> +
> +#define VL_REG_RESULT_INT_STATUS 0x13
> +#define VL_REG_RESULT_RANGE_STATUS 0x14
> +#define VL_REG_RESULT_RANGE_SATTUS_COMPLETE BIT(0)
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS 0x8a
> +
> +#define VL_REG_IDENTIFICATION_MODEL_ID 0xc0
> +#define VL_REG_IDENTIFICATION_REVISION_ID0xc2
Make all capitals: 0xC2, 0xC0, 0x8A ...
> +struct vl53l0x_data {
> + struct i2c_client *client;
> +};
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> + struct i2c_client *client = data->client;
> + unsigned int tries = 20;
> + struct i2c_msg msg[2];
> + u8 buffer[12];
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + do {
> + ret = i2c_smbus_read_byte_data(data->client,
> +VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & VL_REG_RESULT_RANGE_SATTUS_COMPLETE)
> + break;
> +
> + usleep_range(1000, 5000);
> + } while (tries--);
> + if (!tries)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0)
> + return ret;
> + else if (ret != 2)
> + dev_err(>client->dev, "vl53l0x: consecutively read error.
> ");
> +
> + *val = le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
Not required since no buffer/trigger support!
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v3] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
> +/*
> + * Support for ST's VL53L0X FlightSense ToF Ranger Sensor on a i2c bus.
> + *
> + * Copyright (C) 2016 STMicroelectronics Imaging Division.
> + * Copyright (C) 2018 Song Qiang
Datasheet link + i2c slave address can be put here.
Look at other iio drivers.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define VL53L0X_DRV_NAME "vl53l0x-i2c"
> +
> +#define VL_REG_SYSRANGE_MODE_MASKGENMASK(3, 0)
> +#define VL_REG_SYSRANGE_START0x00
> +#define VL_REG_SYSRANGE_MODE_SINGLESHOT 0x00
> +#define VL_REG_SYSRANGE_MODE_START_STOP BIT(0)
> +#define VL_REG_SYSRANGE_MODE_BACKTOBACK BIT(1)
> +#define VL_REG_SYSRANGE_MODE_TIMED BIT(2)
> +#define VL_REG_SYSRANGE_MODE_HISTOGRAM BIT(3)
> +
> +#define VL_REG_SYS_SEQUENCE_CFG BIT(0)
> +#define VL_REG_SYS_INTERMEASUREMENT_PERIOD BIT(2)
> +#define VL_REG_SYS_RANGE_CFG 0x09
> +
> +#define VL_REG_SYS_INT_GPIO_DISABLED 0x00
> +#define VL_REG_SYS_INT_GPIO_LEVEL_LOWBIT(0)
> +#define VL_REG_SYS_INT_GPIO_LEVEL_HIGH BIT(1)
> +#define VL_REG_SYS_INT_GPIO_OUT_OF_WINDOW0x03
> +#define VL_REG_SYS_INT_GPIO_NEW_SAMPLE_READY BIT(2)
> +#define VL_REG_SYS_INT_CFG_GPIO 0x0A
> +#define VL_REG_SYS_INT_CLEAR 0x0B
> +#define VL_REG_GPIO_HV_MUX_ACTIVE_HIGH 0x84
> +
> +#define VL_REG_RESULT_INT_STATUS 0x13
> +#define VL_REG_RESULT_RANGE_STATUS 0x14
> +#define VL_REG_RESULT_RANGE_SATTUS_COMPLETE BIT(0)
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS 0x8a
> +
> +#define VL_REG_IDENTIFICATION_MODEL_ID 0xc0
> +#define VL_REG_IDENTIFICATION_REVISION_ID0xc2
Make all capitals: 0xC2, 0xC0, 0x8A ...
> +struct vl53l0x_data {
> + struct i2c_client *client;
> +};
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> + struct i2c_client *client = data->client;
> + unsigned int tries = 20;
> + struct i2c_msg msg[2];
> + u8 buffer[12];
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + do {
> + ret = i2c_smbus_read_byte_data(data->client,
> +VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & VL_REG_RESULT_RANGE_SATTUS_COMPLETE)
> + break;
> +
> + usleep_range(1000, 5000);
> + } while (tries--);
> + if (!tries)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0)
> + return ret;
> + else if (ret != 2)
> + dev_err(>client->dev, "vl53l0x: consecutively read error.
> ");
> +
> + *val = le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
Not required since no buffer/trigger support!
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 4/5] iio: fxas21002c: add ODR/Scale support
Hello Afonso, On Wed, Sep 12, 2018 at 05:26:01PM +0800, kbuild test robot wrote: > Hi Afonso, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc3 next-20180912] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Afonso-Bordado/iio-gyro-add-support-for-fxas21002c/20180912-084443 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: i386-allmodconfig (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All errors (new ones prefixed by >>): > > >> ERROR: "__divdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! > >> ERROR: "__udivdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! Hmm. This is nasty error that had hit me back and occurs when you do 64 bit arithmetic in your code and assume it will also build for 32 bit environment(i386). https://lists.01.org/pipermail/kbuild-all/2018-July/050481.html But looking at the code seems like there is no such 64 bit division which is why 0-day didn't inform you the exact line of error unlike my case in above link. And I suspect it may be originating from your code snippet: #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> (scale))) and looking at the implementation: include/linux/iio/iio.h /** * IIO_DEGREE_TO_RAD() - Convert degree to rad * @deg: A value in degree * * Returns the given value converted from degree to rad */ #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 900ULL) / 1800ULL) This '/' operator might be the culprit! Just for checking that the error, remove the macro declaration `FXAS21002C_SCALE` plus its usage and re-cross compile using `make ARCH=i386`. In my case I used the `div64_s64` function handles builds for both 32/64 arch accordingly. Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 4/5] iio: fxas21002c: add ODR/Scale support
Hello Afonso, On Wed, Sep 12, 2018 at 05:26:01PM +0800, kbuild test robot wrote: > Hi Afonso, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on iio/togreg] > [also build test ERROR on v4.19-rc3 next-20180912] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Afonso-Bordado/iio-gyro-add-support-for-fxas21002c/20180912-084443 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > config: i386-allmodconfig (attached as .config) > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > All errors (new ones prefixed by >>): > > >> ERROR: "__divdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! > >> ERROR: "__udivdi3" [drivers/iio/gyro/fxas21002c.ko] undefined! Hmm. This is nasty error that had hit me back and occurs when you do 64 bit arithmetic in your code and assume it will also build for 32 bit environment(i386). https://lists.01.org/pipermail/kbuild-all/2018-July/050481.html But looking at the code seems like there is no such 64 bit division which is why 0-day didn't inform you the exact line of error unlike my case in above link. And I suspect it may be originating from your code snippet: #define FXAS21002C_SCALE(scale) (IIO_DEGREE_TO_RAD(62500U >> (scale))) and looking at the implementation: include/linux/iio/iio.h /** * IIO_DEGREE_TO_RAD() - Convert degree to rad * @deg: A value in degree * * Returns the given value converted from degree to rad */ #define IIO_DEGREE_TO_RAD(deg) (((deg) * 314159ULL + 900ULL) / 1800ULL) This '/' operator might be the culprit! Just for checking that the error, remove the macro declaration `FXAS21002C_SCALE` plus its usage and re-cross compile using `make ARCH=i386`. In my case I used the `div64_s64` function handles builds for both 32/64 arch accordingly. Thanks -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
_GPIO_HV_MUX_ACTIVE_HIGH 0x84
> +#define VL_REG_SYS_INT_CLEAR 0x0B
> +
> +#define VL_REG_RESULT_INT_STATUS 0x13
> +#define VL_REG_RESULT_RANGE_STATUS 0x14
> +#define VL_REG_RESULT_RANGE_SATTUS_COMPLETE BIT(0)
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS 0x8a
> +
> +#define VL_REG_IDENTIFICATION_MODEL_ID 0xc0
> +#define VL_REG_IDENTIFICATION_REVISION_ID0xc2
> +
> +struct vl53l0x_data {
> + struct i2c_client *client;
> + struct iio_dev *indio_dev;
This is not required in this private struct.
Not sure though...
> +};
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> + struct i2c_client *client = data->client;
> + unsigned int tries = 20;
> + struct i2c_msg msg[2];
> + u8 buffer[12];
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + do {
> + ret = i2c_smbus_read_byte_data(data->client,
> +VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & VL_REG_RESULT_RANGE_SATTUS_COMPLETE)
> + break;
> +
> + usleep_range(1000, 5000);
> + } while (tries--);
> + if (!tries)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0)
> + return ret;
> + else if (ret != 2)
> + dev_err(>client->dev, "vl53l0x: consecutively read error.
> ");
> +
> + *val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
IIRC __le/be _* shouldn't be directly used and instead use
le16_to_cpu(). Let it decide the byte order unwrapping to
be done.
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_DISTANCE) {
> + dev_err(>client->dev, "vl53l0x: iio type error");
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
You don't have any other buffer/trigger support for now and I believe
there is no need to claim direct mode.
> + if (ret)
> + return ret;
> + ret = vl53l0x_read_proximity(data, chan, val);
> + if (ret < 0)
> + dev_err(>client->dev,
> + "vl53l0x: raw value read error with %d", ret);
> +
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + dev_err(>client->dev, "vl53l0x: IIO_CHAN_* not
> recognzed.");
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info vl53l0x_info = {
> + .read_raw = vl53l0x_read_raw,
> +};
> +
> +static int vl53l0x_probe(struct i2c_client *client)
> +{
> + struct vl53l0x_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + data->indio_dev = indio_dev;
Is this required ?
> + i2c_set_clientdata(client, indio_dev);
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev->dev.parent = >dev;
> + indio_dev->name = VL53L0X_DRV_NAME;
> + indio_dev->info = _info;
> + indio_dev->channels = vl53l0x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = devm_iio_device_register(>dev, indio_dev);
return devm_iio_device_register(>dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
Do you have the sensor and are these patches tested ?
Just curious!
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
_GPIO_HV_MUX_ACTIVE_HIGH 0x84
> +#define VL_REG_SYS_INT_CLEAR 0x0B
> +
> +#define VL_REG_RESULT_INT_STATUS 0x13
> +#define VL_REG_RESULT_RANGE_STATUS 0x14
> +#define VL_REG_RESULT_RANGE_SATTUS_COMPLETE BIT(0)
> +
> +#define VL_REG_I2C_SLAVE_DEVICE_ADDRESS 0x8a
> +
> +#define VL_REG_IDENTIFICATION_MODEL_ID 0xc0
> +#define VL_REG_IDENTIFICATION_REVISION_ID0xc2
> +
> +struct vl53l0x_data {
> + struct i2c_client *client;
> + struct iio_dev *indio_dev;
This is not required in this private struct.
Not sure though...
> +};
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> + struct i2c_client *client = data->client;
> + unsigned int tries = 20;
> + struct i2c_msg msg[2];
> + u8 buffer[12];
> + int ret;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + do {
> + ret = i2c_smbus_read_byte_data(data->client,
> +VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & VL_REG_RESULT_RANGE_SATTUS_COMPLETE)
> + break;
> +
> + usleep_range(1000, 5000);
> + } while (tries--);
> + if (!tries)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> + if (ret < 0)
> + return ret;
> + else if (ret != 2)
> + dev_err(>client->dev, "vl53l0x: consecutively read error.
> ");
> +
> + *val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
IIRC __le/be _* shouldn't be directly used and instead use
le16_to_cpu(). Let it decide the byte order unwrapping to
be done.
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_DISTANCE) {
> + dev_err(>client->dev, "vl53l0x: iio type error");
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
You don't have any other buffer/trigger support for now and I believe
there is no need to claim direct mode.
> + if (ret)
> + return ret;
> + ret = vl53l0x_read_proximity(data, chan, val);
> + if (ret < 0)
> + dev_err(>client->dev,
> + "vl53l0x: raw value read error with %d", ret);
> +
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + dev_err(>client->dev, "vl53l0x: IIO_CHAN_* not
> recognzed.");
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info vl53l0x_info = {
> + .read_raw = vl53l0x_read_raw,
> +};
> +
> +static int vl53l0x_probe(struct i2c_client *client)
> +{
> + struct vl53l0x_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + data->indio_dev = indio_dev;
Is this required ?
> + i2c_set_clientdata(client, indio_dev);
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + indio_dev->dev.parent = >dev;
> + indio_dev->name = VL53L0X_DRV_NAME;
> + indio_dev->info = _info;
> + indio_dev->channels = vl53l0x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = devm_iio_device_register(>dev, indio_dev);
return devm_iio_device_register(>dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
Do you have the sensor and are these patches tested ?
Just curious!
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
On Tue, Sep 11, 2018 at 02:46:38PM +0800, Song Qiang wrote:
> On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> > On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > > This driver was originally written by ST in 2016 as a misc input device,
> > > and hasn't been maintained for a long time. I grabbed some code from
> > > it's API and reformed it to a iio proximity device driver.
> > > This version of driver uses i2c bus to talk to the sensor and
> > > polling for measuring completes, so no irq line is needed.
> > > This version of driver supports only one-shot mode, and it can be
> > > tested with reading from
> > > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > >
> > > Signed-off-by: Song Qiang
> > > ---
> >
> > The Cc list contains developers who might not be relevant
> > for the discussion.
> >
> > So, copy only those people listed by:
> >
> > $./scripts/get_maintainer.pl
> >
> > Don't know why Kate & Greg are cc'ed ?
> >
> > > .../bindings/iio/proximity/vl53l0x.txt| 12 +
> > > drivers/iio/proximity/Kconfig | 13 +
> > > drivers/iio/proximity/Makefile| 2 +
> > > drivers/iio/proximity/vl53l0x-i2c.c | 295 ++
> > > 4 files changed, 322 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > new file mode 100644
> > > index ..64b69442f08e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > @@ -0,0 +1,12 @@
> > > +ST's VL53L0X ToF ranging sensor
> > > +
> > > +Required properties:
> > > + - compatible: must be "st,vl53l0x-i2c"
> > > + - reg: i2c address where to find the device
> > > +
> > > +Example:
> > > +
> > > +vl53l0x@29 {
> > > + compatible = "st,vl53l0x-i2c";
> > > + reg = <0x29>;
> > > +};
> > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > > index f726f9427602..1563a5f9144d 100644
> > > --- a/drivers/iio/proximity/Kconfig
> > > +++ b/drivers/iio/proximity/Kconfig
> > > @@ -79,4 +79,17 @@ config SRF08
> > > To compile this driver as a module, choose M here: the
> > > module will be called srf08.
> > >
> > > +config VL53L0X_I2C
> > > + tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGERED_BUFFER
> >
> > I don't see any buffer/trigger support, so better to remove these
> > two options.
> >
> > > + depends on I2C
> > > + help
> > > + Say Y here to build a driver for STMicroelectronics VL53L0X
> > > + ToF ranger sensors with i2c interface.
> > > + This driver can be used to measure the distance of objects.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called vl53l0x-i2c.
> >
> > `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> > is not used to probe the driver.
> >
> > > endmenu
> > > diff --git a/drivers/iio/proximity/Makefile
> > > b/drivers/iio/proximity/Makefile
> > > index 4f4ed45e87ef..7cb771665c8b 100644
> > > --- a/drivers/iio/proximity/Makefile
> > > +++ b/drivers/iio/proximity/Makefile
> > > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
> > > obj-$(CONFIG_SRF04) += srf04.o
> > > obj-$(CONFIG_SRF08) += srf08.o
> > > obj-$(CONFIG_SX9500) += sx9500.o
> > > +obj-$(CONFIG_VL53L0X_I2C)+= vl53l0x-i2c.o
> > > +
> > > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c
> > > b/drivers/iio/proximity/vl53l0x-i2c.c
> > > new file mode 100644
> > > index ..c00713041d30
> > > --- /dev/null
> > > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > > @@ -0,0 +1,295 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > > + *
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
On Tue, Sep 11, 2018 at 02:46:38PM +0800, Song Qiang wrote:
> On Mon, Sep 10, 2018 at 11:27:47PM +0530, Himanshu Jha wrote:
> > On Mon, Sep 10, 2018 at 10:42:59PM +0800, Song Qiang wrote:
> > > This driver was originally written by ST in 2016 as a misc input device,
> > > and hasn't been maintained for a long time. I grabbed some code from
> > > it's API and reformed it to a iio proximity device driver.
> > > This version of driver uses i2c bus to talk to the sensor and
> > > polling for measuring completes, so no irq line is needed.
> > > This version of driver supports only one-shot mode, and it can be
> > > tested with reading from
> > > /sys/bus/iio/devices/iio:deviceX/in_distance_raw
> > >
> > > Signed-off-by: Song Qiang
> > > ---
> >
> > The Cc list contains developers who might not be relevant
> > for the discussion.
> >
> > So, copy only those people listed by:
> >
> > $./scripts/get_maintainer.pl
> >
> > Don't know why Kate & Greg are cc'ed ?
> >
> > > .../bindings/iio/proximity/vl53l0x.txt| 12 +
> > > drivers/iio/proximity/Kconfig | 13 +
> > > drivers/iio/proximity/Makefile| 2 +
> > > drivers/iio/proximity/vl53l0x-i2c.c | 295 ++
> > > 4 files changed, 322 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > create mode 100644 drivers/iio/proximity/vl53l0x-i2c.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > new file mode 100644
> > > index ..64b69442f08e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/vl53l0x.txt
> > > @@ -0,0 +1,12 @@
> > > +ST's VL53L0X ToF ranging sensor
> > > +
> > > +Required properties:
> > > + - compatible: must be "st,vl53l0x-i2c"
> > > + - reg: i2c address where to find the device
> > > +
> > > +Example:
> > > +
> > > +vl53l0x@29 {
> > > + compatible = "st,vl53l0x-i2c";
> > > + reg = <0x29>;
> > > +};
> > > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > > index f726f9427602..1563a5f9144d 100644
> > > --- a/drivers/iio/proximity/Kconfig
> > > +++ b/drivers/iio/proximity/Kconfig
> > > @@ -79,4 +79,17 @@ config SRF08
> > > To compile this driver as a module, choose M here: the
> > > module will be called srf08.
> > >
> > > +config VL53L0X_I2C
> > > + tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> > > + select IIO_BUFFER
> > > + select IIO_TRIGGERED_BUFFER
> >
> > I don't see any buffer/trigger support, so better to remove these
> > two options.
> >
> > > + depends on I2C
> > > + help
> > > + Say Y here to build a driver for STMicroelectronics VL53L0X
> > > + ToF ranger sensors with i2c interface.
> > > + This driver can be used to measure the distance of objects.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called vl53l0x-i2c.
> >
> > `name` attribute will be VL53L0X_DRV_NAME(vl53l0x) if OF matching
> > is not used to probe the driver.
> >
> > > endmenu
> > > diff --git a/drivers/iio/proximity/Makefile
> > > b/drivers/iio/proximity/Makefile
> > > index 4f4ed45e87ef..7cb771665c8b 100644
> > > --- a/drivers/iio/proximity/Makefile
> > > +++ b/drivers/iio/proximity/Makefile
> > > @@ -10,3 +10,5 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
> > > obj-$(CONFIG_SRF04) += srf04.o
> > > obj-$(CONFIG_SRF08) += srf08.o
> > > obj-$(CONFIG_SX9500) += sx9500.o
> > > +obj-$(CONFIG_VL53L0X_I2C)+= vl53l0x-i2c.o
> > > +
> > > diff --git a/drivers/iio/proximity/vl53l0x-i2c.c
> > > b/drivers/iio/proximity/vl53l0x-i2c.c
> > > new file mode 100644
> > > index ..c00713041d30
> > > --- /dev/null
> > > +++ b/drivers/iio/proximity/vl53l0x-i2c.c
> > > @@ -0,0 +1,295 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * vl53l0x-i2c.c - Support for STM VL53L0X FlightSense TOF
> > > + *
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
2997
> +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV0x0089
> +#define VL_REG_ALGO_PHASECAL_LIM 0x0030 /* 0x130 */
> +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT 0x0030
> +
> +struct vl53l0x_data {
> + struct i2c_client *client;
> + struct mutex lock;
This lock needs a comment to explain its purpose.
> + int useLongRange;
Weird spacing.
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
Align all these functions to match open parentheses with mix of
tabs + whitespaces(as required):
static int vl53l0x_read_proximity(struct vl53l0x_data *data,
const struct iio_chan_spec *chan,
int *val)
> + int ret;
> + struct i2c_client *client = data->client;
> + int tries = 20;
> + u8 buffer[12];
> + struct i2c_msg msg[2];
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + while (tries-- > 0) {
> + ret = i2c_smbus_read_byte_data(data->client,
> + VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & 0x01)
> + break;
> + usleep_range(1000, 5000);
> + }
> +
> + if (tries < 0)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> +
> + if (ret != 2) {
> + pr_err("vl53l0x: consecutively read error. ");
> + return ret;
> + }
> +
> + *val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_DISTANCE) {
> + pr_err("vl53l0x: iio type error");
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = vl53l0x_read_proximity(data, chan, val);
> + if (ret < 0)
> + pr_err("vl53l0x: raw value read error with %d", ret);
> +
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info vl53l0x_info = {
> + .read_raw = vl53l0x_read_raw,
> +};
> +
> +static int vl53l0x_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{
> + int ret;
> + struct vl53l0x_data *data;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + i2c_set_clientdata(client, indio_dev);
> + mutex_init(>lock);
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EOPNOTSUPP;
> +
> + indio_dev->dev.parent = >dev;
> + indio_dev->name = VL53L0X_DRV_NAME;
> + indio_dev->info = _info;
> + indio_dev->channels = vl53l0x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + //probe 0xc0 if the value is 0xEE.
> +
> + ret = iio_device_register(indio_dev);
Would better be better to use resource managened functions since
I don't see any point of using vl53l0x_remove() function below.
Better use devm_iio_device_register()!
> + if (ret)
> + return ret;
> +
> + dev_set_drvdata(>dev, data);
You already setted it up above using:
i2c_set_clientdata(client, indio_dev);
> + return 0;
> +}
> +
> +static int vl53l0x_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + kfree(data);
> +
> + return 0;
> +}
Plus all other comments addressed by Andy.
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: proximity: Add driver support for ST's VL53L0X ToF ranging sensor.
2997
> +#define VL_REG_VHV_CFG_PAD_SCL_SDA__EXTSUP_HV0x0089
> +#define VL_REG_ALGO_PHASECAL_LIM 0x0030 /* 0x130 */
> +#define VL_REG_ALGO_PHASECAL_CFG_TIMEOUT 0x0030
> +
> +struct vl53l0x_data {
> + struct i2c_client *client;
> + struct mutex lock;
This lock needs a comment to explain its purpose.
> + int useLongRange;
Weird spacing.
> +
> +static int vl53l0x_read_proximity(struct vl53l0x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
Align all these functions to match open parentheses with mix of
tabs + whitespaces(as required):
static int vl53l0x_read_proximity(struct vl53l0x_data *data,
const struct iio_chan_spec *chan,
int *val)
> + int ret;
> + struct i2c_client *client = data->client;
> + int tries = 20;
> + u8 buffer[12];
> + struct i2c_msg msg[2];
> + u8 write_command = VL_REG_RESULT_RANGE_STATUS;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + VL_REG_SYSRANGE_START, 1);
> + if (ret < 0)
> + return ret;
> +
> + while (tries-- > 0) {
> + ret = i2c_smbus_read_byte_data(data->client,
> + VL_REG_RESULT_RANGE_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & 0x01)
> + break;
> + usleep_range(1000, 5000);
> + }
> +
> + if (tries < 0)
> + return -ETIMEDOUT;
> +
> + msg[0].addr = client->addr;
> + msg[0].buf = _command;
> + msg[0].len = 1;
> + msg[0].flags = client->flags | I2C_M_STOP;
> +
> + msg[1].addr = client->addr;
> + msg[1].buf = buffer;
> + msg[1].len = 12;
> + msg[1].flags = client->flags | I2C_M_RD;
> +
> + ret = i2c_transfer(client->adapter, msg, 2);
> +
> + if (ret != 2) {
> + pr_err("vl53l0x: consecutively read error. ");
> + return ret;
> + }
> +
> + *val = __le16_to_cpu((buffer[10] << 8) + buffer[11]);
> +
> + return 0;
> +}
> +
> +static const struct iio_chan_spec vl53l0x_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static int vl53l0x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_DISTANCE) {
> + pr_err("vl53l0x: iio type error");
> + return -EINVAL;
> + }
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = vl53l0x_read_proximity(data, chan, val);
> + if (ret < 0)
> + pr_err("vl53l0x: raw value read error with %d", ret);
> +
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + pr_err("vl53l0x: IIO_CHAN_* not recognzed.");
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info vl53l0x_info = {
> + .read_raw = vl53l0x_read_raw,
> +};
> +
> +static int vl53l0x_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{
> + int ret;
> + struct vl53l0x_data *data;
> + struct iio_dev *indio_dev;
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + i2c_set_clientdata(client, indio_dev);
> + mutex_init(>lock);
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE))
> + return -EOPNOTSUPP;
> +
> + indio_dev->dev.parent = >dev;
> + indio_dev->name = VL53L0X_DRV_NAME;
> + indio_dev->info = _info;
> + indio_dev->channels = vl53l0x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(vl53l0x_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + //probe 0xc0 if the value is 0xEE.
> +
> + ret = iio_device_register(indio_dev);
Would better be better to use resource managened functions since
I don't see any point of using vl53l0x_remove() function below.
Better use devm_iio_device_register()!
> + if (ret)
> + return ret;
> +
> + dev_set_drvdata(>dev, data);
You already setted it up above using:
i2c_set_clientdata(client, indio_dev);
> + return 0;
> +}
> +
> +static int vl53l0x_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct vl53l0x_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + kfree(data);
> +
> + return 0;
> +}
Plus all other comments addressed by Andy.
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: remove unnecessary condition judgment in am2315_trigger_handler
On Sat, Sep 08, 2018 at 06:57:36PM +0800, zhong jiang wrote:
> The iterator in for_each_set_bit is never null, therefore, remove
> the redundant conditional judgment.
>
> Signed-off-by: zhong jiang
> ---
> drivers/iio/humidity/am2315.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/iio/humidity/am2315.c b/drivers/iio/humidity/am2315.c
> index 7d8669d..dc12e37 100644
> --- a/drivers/iio/humidity/am2315.c
> +++ b/drivers/iio/humidity/am2315.c
> @@ -176,8 +176,7 @@ static irqreturn_t am2315_trigger_handler(int irq, void
> *p)
> i = 0;
> for_each_set_bit(bit, indio_dev->active_scan_mask,
>indio_dev->masklength) {
> - data->buffer[i] = (bit ? sensor_data.temp_data :
> - sensor_data.hum_data);
> + data->buffer[i] = sensor_data.temp_data;
No, this seems wrong!
We have buffer support to either take both readings(temp & humid)
simultaneously, or only single channel using specified scan mask.
Patch title should be:
"iio: humidity: am2315: .. "
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: remove unnecessary condition judgment in am2315_trigger_handler
On Sat, Sep 08, 2018 at 06:57:36PM +0800, zhong jiang wrote:
> The iterator in for_each_set_bit is never null, therefore, remove
> the redundant conditional judgment.
>
> Signed-off-by: zhong jiang
> ---
> drivers/iio/humidity/am2315.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/iio/humidity/am2315.c b/drivers/iio/humidity/am2315.c
> index 7d8669d..dc12e37 100644
> --- a/drivers/iio/humidity/am2315.c
> +++ b/drivers/iio/humidity/am2315.c
> @@ -176,8 +176,7 @@ static irqreturn_t am2315_trigger_handler(int irq, void
> *p)
> i = 0;
> for_each_set_bit(bit, indio_dev->active_scan_mask,
>indio_dev->masklength) {
> - data->buffer[i] = (bit ? sensor_data.temp_data :
> - sensor_data.hum_data);
> + data->buffer[i] = sensor_data.temp_data;
No, this seems wrong!
We have buffer support to either take both readings(temp & humid)
simultaneously, or only single channel using specified scan mask.
Patch title should be:
"iio: humidity: am2315: .. "
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2 1/4] iio: gyro: add support for fxas21002c
p */
> + msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
> +
> + return 0;
> +}
> +
> +static int fxas21002c_verify_chip(struct fxas21002c_data *data)
> +{
> + int ret;
> + int chip_id;
> +
> + ret = regmap_read(data->regmap, FXAS21002C_REG_WHO_AM_I, _id);
Is this correct ?
regmap_read() needs unsigned int * as a the third agrument. This warning is
usually prompted on compilation IIRC and build shall fail!
> + if (ret) {
> + dev_err(>client->dev, "could not read device id\n");
> + return ret;
> + }
> +
> + if (chip_id != FXAS21002C_CHIP_ID) {
> + dev_err(>client->dev,
> + "unsupported chip id %02x\n", chip_id);
^ %02d ?
I have been skimming through the kernel source for a few a while and
have observed often that we ignore "-Wformat-signedness" warnings ?
-Wformat-signedness:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
"If -Wformat is specified, also warn if the format string requires
an unsigned argument and the argument is signed and vice versa."
And the ISO-C11 Standard says:
https://port70.net/~nsz/c/c11/n1570.html#7.21.6.1p9
"If a conversion specification is invalid, the behavior is undefined.
If any argument is not the correct type for the corresponding
conversion specification, the behavior is *undefined*."
Undefined Behavior:
--
In the C community, undefined behavior may be humorously referred to as "nasal
demons",
after a comp.std.c post that explained undefined behavior as allowing the
compiler to
do _anything_ it chooses, even "to make demons fly out of your nose"
And Paul E. McKenney comments on compiler writers:
https://lwn.net/Articles/508999/
"I have seen the glint in their eyes when they discuss optimization
techniques that you would not want your children to know about!"
You may try and check the compiler warning by building source with
"-Wformat-signedness" flag.
Otherwise, you can avoid my comment and call me a language-lawyer ;)
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
> +struct iio_chan_spec const *chan, int *val)
Similar alignment here.
> + int ret;
> + __be16 bulk_raw;
> +
> + switch (chan->type) {
> + case IIO_ANGL_VEL:
> + ret = regmap_bulk_read(data->regmap, chan->address,
> +_raw, sizeof(bulk_raw));
> + if (ret)
> + return ret;
> +
> + *val = sign_extend32(be16_to_cpu(bulk_raw), 15);
> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + ret = regmap_read(data->regmap, chan->address, val);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int fxas21002c_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan, int *val,
> +int *val2, long mask)
Similarly here.
static int fxas21002c_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v2 1/4] iio: gyro: add support for fxas21002c
p */
> + msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
> +
> + return 0;
> +}
> +
> +static int fxas21002c_verify_chip(struct fxas21002c_data *data)
> +{
> + int ret;
> + int chip_id;
> +
> + ret = regmap_read(data->regmap, FXAS21002C_REG_WHO_AM_I, _id);
Is this correct ?
regmap_read() needs unsigned int * as a the third agrument. This warning is
usually prompted on compilation IIRC and build shall fail!
> + if (ret) {
> + dev_err(>client->dev, "could not read device id\n");
> + return ret;
> + }
> +
> + if (chip_id != FXAS21002C_CHIP_ID) {
> + dev_err(>client->dev,
> + "unsupported chip id %02x\n", chip_id);
^ %02d ?
I have been skimming through the kernel source for a few a while and
have observed often that we ignore "-Wformat-signedness" warnings ?
-Wformat-signedness:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
"If -Wformat is specified, also warn if the format string requires
an unsigned argument and the argument is signed and vice versa."
And the ISO-C11 Standard says:
https://port70.net/~nsz/c/c11/n1570.html#7.21.6.1p9
"If a conversion specification is invalid, the behavior is undefined.
If any argument is not the correct type for the corresponding
conversion specification, the behavior is *undefined*."
Undefined Behavior:
--
In the C community, undefined behavior may be humorously referred to as "nasal
demons",
after a comp.std.c post that explained undefined behavior as allowing the
compiler to
do _anything_ it chooses, even "to make demons fly out of your nose"
And Paul E. McKenney comments on compiler writers:
https://lwn.net/Articles/508999/
"I have seen the glint in their eyes when they discuss optimization
techniques that you would not want your children to know about!"
You may try and check the compiler warning by building source with
"-Wformat-signedness" flag.
Otherwise, you can avoid my comment and call me a language-lawyer ;)
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
> +struct iio_chan_spec const *chan, int *val)
Similar alignment here.
> + int ret;
> + __be16 bulk_raw;
> +
> + switch (chan->type) {
> + case IIO_ANGL_VEL:
> + ret = regmap_bulk_read(data->regmap, chan->address,
> +_raw, sizeof(bulk_raw));
> + if (ret)
> + return ret;
> +
> + *val = sign_extend32(be16_to_cpu(bulk_raw), 15);
> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + ret = regmap_read(data->regmap, chan->address, val);
> + if (ret)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int fxas21002c_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan, int *val,
> +int *val2, long mask)
Similarly here.
static int fxas21002c_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
Thanks
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: chemical: bme680: Add check for val2 in the write_raw function
On Sun, Aug 19, 2018 at 05:25:14PM +0100, Jonathan Cameron wrote: > On Sat, 11 Aug 2018 15:56:36 +0530 > Himanshu Jha wrote: > > > val2 is responsible for the floating part of the number to be > > written to the device. We don't need the floating part > > while writing the oversampling ratio for BME680 since the > > available oversampling ratios are pure natural numbers. > > > > So, add a sanity check to make sure val2 is 0. > > > > Signed-off-by: Himanshu Jha > > As discussed in David's patch series v3, I think this is still relevant > but now needs an update to cover the new code. I already had informed David to reabse his series on top of my patch and he rebased this patch series. So, it should apply cleanly without any further effort. If it doesn't, then ping me. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] iio: chemical: bme680: Add check for val2 in the write_raw function
On Sun, Aug 19, 2018 at 05:25:14PM +0100, Jonathan Cameron wrote: > On Sat, 11 Aug 2018 15:56:36 +0530 > Himanshu Jha wrote: > > > val2 is responsible for the floating part of the number to be > > written to the device. We don't need the floating part > > while writing the oversampling ratio for BME680 since the > > available oversampling ratios are pure natural numbers. > > > > So, add a sanity check to make sure val2 is 0. > > > > Signed-off-by: Himanshu Jha > > As discussed in David's patch series v3, I think this is still relevant > but now needs an update to cover the new code. I already had informed David to reabse his series on top of my patch and he rebased this patch series. So, it should apply cleanly without any further effort. If it doesn't, then ping me. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Coccinelle: remove pci_alloc_consistent semantic to dectect in zalloc-simple.cocci
On Sat, Aug 18, 2018 at 10:10:26PM +0800, zhong jiang wrote: > On 2018/8/18 22:01, Julia Lawall wrote: > > > > On Sat, 18 Aug 2018, zhong jiang wrote: > > > >> On 2018/8/18 20:52, Himanshu Jha wrote: > >>> On Sat, Aug 18, 2018 at 08:01:40PM +0800, zhong jiang wrote: > >>>> Because pci_alloc_consistent has been deprecated. We prefer to use > >>>> dam_alloc_coherent directly. Therefore, we should remove > >>>> pci_alloc_consistent > >>> ^^^ typo "dma" > >>> > >>> Also, typo in the patch subject "dectect" -> "detect" > >> Thanks, will fix it in v2. > >>> Otherwise, > >>> > >>> Acked-by: Himanshu Jha > >>> > >>> Thanks for cleaning up, we still have few more *_alloc/memset > >>> in the mainline, especially in the scsi subsystem, even after > >>> I cleaned up with two long patch series in the past. > >>> > >>I post same patches like *_alloc/memset in the scsi subsystem. > >> Unfortunately, > >>Maintainer maybe do not care about the change. therefore, None of my > >> patches > >> are received and rarely feedback. > > Maybe they will be picked up later. > Hope so. It's been more than a month since I post the patches. Maybe I > should give a kindly ping. :-) Heh! The problem is Martin only when picks up patches if the driver maintainer Acks them, and it happens very rarely until you have a *real* bugfix. But since these patches are simple, so Martin should pick it up even if the patch doesn't receive any feedback. If you ping, then certainly Martin will give you the same feedback. Maybe you can make them happy by sending a syzbot bugfix ;) -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Coccinelle: remove pci_alloc_consistent semantic to dectect in zalloc-simple.cocci
On Sat, Aug 18, 2018 at 10:10:26PM +0800, zhong jiang wrote: > On 2018/8/18 22:01, Julia Lawall wrote: > > > > On Sat, 18 Aug 2018, zhong jiang wrote: > > > >> On 2018/8/18 20:52, Himanshu Jha wrote: > >>> On Sat, Aug 18, 2018 at 08:01:40PM +0800, zhong jiang wrote: > >>>> Because pci_alloc_consistent has been deprecated. We prefer to use > >>>> dam_alloc_coherent directly. Therefore, we should remove > >>>> pci_alloc_consistent > >>> ^^^ typo "dma" > >>> > >>> Also, typo in the patch subject "dectect" -> "detect" > >> Thanks, will fix it in v2. > >>> Otherwise, > >>> > >>> Acked-by: Himanshu Jha > >>> > >>> Thanks for cleaning up, we still have few more *_alloc/memset > >>> in the mainline, especially in the scsi subsystem, even after > >>> I cleaned up with two long patch series in the past. > >>> > >>I post same patches like *_alloc/memset in the scsi subsystem. > >> Unfortunately, > >>Maintainer maybe do not care about the change. therefore, None of my > >> patches > >> are received and rarely feedback. > > Maybe they will be picked up later. > Hope so. It's been more than a month since I post the patches. Maybe I > should give a kindly ping. :-) Heh! The problem is Martin only when picks up patches if the driver maintainer Acks them, and it happens very rarely until you have a *real* bugfix. But since these patches are simple, so Martin should pick it up even if the patch doesn't receive any feedback. If you ping, then certainly Martin will give you the same feedback. Maybe you can make them happy by sending a syzbot bugfix ;) -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Coccinelle: remove pci_alloc_consistent semantic to dectect in zalloc-simple.cocci
On Sat, Aug 18, 2018 at 08:01:40PM +0800, zhong jiang wrote: > Because pci_alloc_consistent has been deprecated. We prefer to use > dam_alloc_coherent directly. Therefore, we should remove pci_alloc_consistent ^^^ typo "dma" Also, typo in the patch subject "dectect" -> "detect" Otherwise, Acked-by: Himanshu Jha Thanks for cleaning up, we still have few more *_alloc/memset in the mainline, especially in the scsi subsystem, even after I cleaned up with two long patch series in the past. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] Coccinelle: remove pci_alloc_consistent semantic to dectect in zalloc-simple.cocci
On Sat, Aug 18, 2018 at 08:01:40PM +0800, zhong jiang wrote: > Because pci_alloc_consistent has been deprecated. We prefer to use > dam_alloc_coherent directly. Therefore, we should remove pci_alloc_consistent ^^^ typo "dma" Also, typo in the patch subject "dectect" -> "detect" Otherwise, Acked-by: Himanshu Jha Thanks for cleaning up, we still have few more *_alloc/memset in the mainline, especially in the scsi subsystem, even after I cleaned up with two long patch series in the past. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: gasket: remove null ptr check before kfree
On Sun, Aug 12, 2018 at 12:28:37PM +0530, Sumit Kumar wrote: > Remove null ptr check before kfree because kfree is null ptr safe. > Issue found by checkpatch. > > Signed-off-by: Sumit Kumar Hmm. Victim of copy-paste I guess. No worries. Next time just use: $ git commit -s To understand what -s flag does, refer `man git commit`. > --- Now, this where you should have put versions just below the '---': v2: - corrected email address. And yes, the subject should be: "Subject: [PATCH v2] staging: gasket: remove null ptr check before kfree" https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format Hope that helps! -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] staging: gasket: remove null ptr check before kfree
On Sun, Aug 12, 2018 at 12:28:37PM +0530, Sumit Kumar wrote: > Remove null ptr check before kfree because kfree is null ptr safe. > Issue found by checkpatch. > > Signed-off-by: Sumit Kumar Hmm. Victim of copy-paste I guess. No worries. Next time just use: $ git commit -s To understand what -s flag does, refer `man git commit`. > --- Now, this where you should have put versions just below the '---': v2: - corrected email address. And yes, the subject should be: "Subject: [PATCH v2] staging: gasket: remove null ptr check before kfree" https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format Hope that helps! -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
[PATCH] iio: chemical: bme680: Add check for val2 in the write_raw function
val2 is responsible for the floating part of the number to be
written to the device. We don't need the floating part
while writing the oversampling ratio for BME680 since the
available oversampling ratios are pure natural numbers.
So, add a sanity check to make sure val2 is 0.
Signed-off-by: Himanshu Jha
---
drivers/iio/chemical/bme680_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/chemical/bme680_core.c
b/drivers/iio/chemical/bme680_core.c
index 7d9bb62baa3f..9d5a05e054d1 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -852,6 +852,9 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
{
struct bme680_data *data = iio_priv(indio_dev);
+ if (val2 != 0)
+ return -EINVAL;
+
switch (mask) {
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
switch (chan->type) {
--
2.17.1
[PATCH] iio: chemical: bme680: Add check for val2 in the write_raw function
val2 is responsible for the floating part of the number to be
written to the device. We don't need the floating part
while writing the oversampling ratio for BME680 since the
available oversampling ratios are pure natural numbers.
So, add a sanity check to make sure val2 is 0.
Signed-off-by: Himanshu Jha
---
drivers/iio/chemical/bme680_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/chemical/bme680_core.c
b/drivers/iio/chemical/bme680_core.c
index 7d9bb62baa3f..9d5a05e054d1 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -852,6 +852,9 @@ static int bme680_write_raw(struct iio_dev *indio_dev,
{
struct bme680_data *data = iio_priv(indio_dev);
+ if (val2 != 0)
+ return -EINVAL;
+
switch (mask) {
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
switch (chan->type) {
--
2.17.1
Re: [PATCH] coccicheck: return proper error code on check fail
On Fri, Aug 10, 2018 at 05:45:46PM +0300, Denis Efremov wrote: > My mistake. Initially, I thought that this line signals about errors > in the code, but now I see that this is about the tool's internal > error. However, this doesn't change the fact that coccicheck returns > the improper error code. > > I will reformulate the commit message and send the v2 patch with the > same diff. Thank you for clarifying things. I would also request to use the latest source from https://github.com/coccinelle/coccinelle Because some distribution supplied coccinelle packages are obsolete and likely more prone to be disfunctional. For instance: https://systeme.lip6.fr/pipermail/cocci/2017-December/004799.html Thanks. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH] coccicheck: return proper error code on check fail
On Fri, Aug 10, 2018 at 05:45:46PM +0300, Denis Efremov wrote: > My mistake. Initially, I thought that this line signals about errors > in the code, but now I see that this is about the tool's internal > error. However, this doesn't change the fact that coccicheck returns > the improper error code. > > I will reformulate the commit message and send the v2 patch with the > same diff. Thank you for clarifying things. I would also request to use the latest source from https://github.com/coccinelle/coccinelle Because some distribution supplied coccinelle packages are obsolete and likely more prone to be disfunctional. For instance: https://systeme.lip6.fr/pipermail/cocci/2017-December/004799.html Thanks. -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH][iio-next] iio: chemical: fix spelling mistake "failted" -> "failed"
On Fri, Aug 03, 2018 at 12:55:51PM +0100, Colin King wrote: > From: Colin Ian King > > fix spelling mistake in dev_err error message text > > Signed-off-by: Colin Ian King Oops! Thanks for the patch Colin. Reviewed-by: Himanshu Jha -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
Re: [PATCH][iio-next] iio: chemical: fix spelling mistake "failted" -> "failed"
On Fri, Aug 03, 2018 at 12:55:51PM +0100, Colin King wrote: > From: Colin Ian King > > fix spelling mistake in dev_err error message text > > Signed-off-by: Colin Ian King Oops! Thanks for the patch Colin. Reviewed-by: Himanshu Jha -- Himanshu Jha Undergraduate Student Department of Electronics & Communication Guru Tegh Bahadur Institute of Technology
[PATCH v5] iio: chemical: Add support for Bosch BME680 sensor
Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v5: -removed unnecessary casts from compensate functions. -used devm_iio_register for now. -used bitshifting near approximation instead of complex multiplication in bme680_compensate_gas(). -used div64_s64() for signed 64 bit divison to avoid build error on 32 bit architecture. [undefined reference to `__divdi3'] -fixed spurious spacing in a macro. -marked lookuptable array as const. -fixed comment indenting. -added dev_err() for soft reset failure. v4: -Added Bosch API links and datasheet link. -explained with comments about the compensate functions. -fixed SPI ID register address. -Used FIELD_PREP macro to avoid unnecessary hard coding. -Simplified Kconfig to remove unnecessary parentheses. v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 23 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 96 drivers/iio/chemical/bme680_core.c | 958 + drivers/iio/chemical/bme680_i2c.c | 85 drivers/iio/chemical/bme680_spi.c | 125 + 6 files changed, 1290 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..b8e005b 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,29 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if I2C + select BME680_SPI if SPI + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on I2C && BME680 + select REGMAP_I2C + +config BME680_SPI + tristate + depends on SPI && BME680 + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -4,6 +4,9 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o +obj-$(CONFIG_BME680) += bme680_core.o +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o +obj-$(CONFIG_BME680_SPI) += bme680_spi.o obj-$(CONFIG_CCS811) += ccs811.o obj-$(CONFIG_IAQCORE) += ams-iaq-core.o o
[PATCH v5] iio: chemical: Add support for Bosch BME680 sensor
Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v5: -removed unnecessary casts from compensate functions. -used devm_iio_register for now. -used bitshifting near approximation instead of complex multiplication in bme680_compensate_gas(). -used div64_s64() for signed 64 bit divison to avoid build error on 32 bit architecture. [undefined reference to `__divdi3'] -fixed spurious spacing in a macro. -marked lookuptable array as const. -fixed comment indenting. -added dev_err() for soft reset failure. v4: -Added Bosch API links and datasheet link. -explained with comments about the compensate functions. -fixed SPI ID register address. -Used FIELD_PREP macro to avoid unnecessary hard coding. -Simplified Kconfig to remove unnecessary parentheses. v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 23 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 96 drivers/iio/chemical/bme680_core.c | 958 + drivers/iio/chemical/bme680_i2c.c | 85 drivers/iio/chemical/bme680_spi.c | 125 + 6 files changed, 1290 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..b8e005b 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,29 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if I2C + select BME680_SPI if SPI + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on I2C && BME680 + select REGMAP_I2C + +config BME680_SPI + tristate + depends on SPI && BME680 + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -4,6 +4,9 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o +obj-$(CONFIG_BME680) += bme680_core.o +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o +obj-$(CONFIG_BME680_SPI) += bme680_spi.o obj-$(CONFIG_CCS811) += ccs811.o obj-$(CONFIG_IAQCORE) += ams-iaq-core.o o
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Mon, Jul 23, 2018 at 03:16:10PM -0700, David Frey wrote:
> On 7/22/2018 3:21 PM, Himanshu Jha wrote:
> >On Sat, Jul 21, 2018 at 08:45:34PM +0300, Daniel Baluta wrote:
> >>On Sat, Jul 21, 2018 at 6:43 PM, Andy Shevchenko
> >> wrote:
> >>>On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> >>> wrote:
> >>>
> >>>>>>+ /* Look up table 1 for the possible gas range values */
> >>>>>>+ u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >>>>>>+ 2147483647u, 2147483647u, 2126008810u,
> >>>>>>+ 2147483647u, 2130303777u, 2147483647u,
> >>>>>>+ 2147483647u, 2143188679u, 2136746228u,
> >>>>>>+ 2147483647u, 2126008810u, 2147483647u,
> >>>>>>+ 2147483647u};
> >>>
> >>>This one needs perhaps a bit of though, but...
> >>>
> >>>>>>+ /* Look up table 2 for the possible gas range values */
> >>>>>>+ u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >>>>>>+ 51200u, 255744255u, 127110228u,
> >>>>>>6400u,
> >>>>>>+ 32258064u, 16016016u, 800u, 400u,
> >>>>>>+ 200u, 100u, 50u, 25u,
> >>>>>>125000u};
> >>>
> >>>...this one obviously just a not needed one. You may replace it with a
> >>>one constant and simple calculation to get either value (index from
> >>>value, or value from index).
> >>
> >>Indeed this can be reduce to:
> >>
> >>125.000 << (15 - idx).
> >>
> >>The real question here is if we approximate 255.744.255u to 256.00.00u how
> >>much different is the result. Being a gas sensor I think it is very
> >>hard to appreciate.
> >>
> >>We can go with this formula + adding a comment with the table with the
> >>exact coefficients.
> >
> >So, I have planned to use this 125000 << (15 - idx) equation with
> >approximating the array members.
> >
> >About the difference in results we would get after approximating isn't
> >much of a problem IMHO because gas sensor is primarily used for IAQ, and
> >IAQ is relative to the resistance reading.
> >
> >For eg:
> >
> >Resistance(ohm) IAQ
> >value < 30K Very bad
> >30k < value < 50kworse
> >50k < value < 70kbad
> >...
> >..
> >so on..
> >
> >So, what I simply imply is the scale will be adjusted and nothing else
> >changes, unlike if it had been pressure, temperature, humidity.
> >
> >The IAQ implementation is userspace application suggesting
> >good/bad/ugly air quality.
> >
> >And since we know David Frey is planning to use this sensor in his
> >product mangOH board.
> >
> >So, David, how are you planning to use the gas sensing part in your
> >product ? RGB leds, buzzer, alarm ?
> >
> >Thanks Andy for the suggestion :)
> >
>
> My understanding is that the Bosch BSEC (Bosch Sensortec Environmental
> Cluster - https://www.bosch-sensortec.com/bst/products/all_products/bsec)
> software calculates the indoor air quality (IAQ) which is presented in
> the range of 0 to 500. BSEC is proprietary, pre-compiled static
> library. I don't know how they derive the IAQ, but it seems that it
> could be based on smoothing outlying gas resistance values and integrating
> other values such as temperature, humidity and pressure.
> Unless this driver can somehow produce IAQ values of equal or greater
> reliability to the BSEC library, then I would prefer that it just
> present the raw gas resistance value so that a user can write a program
> to feed the sensor data into BSEC.
>
> mangOH isn't really a traditional product. It's an open hardware board
> designed around Sierra Wireless cellular modules that run Linux. So I
> don't have any specific use case in mind, but I want to enable our users
> (and thus future products) to make use of air quality measurements.
I tested the gas sensor with the new bitshifting equation and didn't find any
discrepancy in the resistance readings.
Even I don't know how Bosch implements IAQ algorithm, but one thing I
got around testing the sensor was that the air quality is inversely
proportional to resistance value.
[You can try it yourself, by spraying deodrant/aerosol sprays around the sensor
;)]
But we shouldn't be worried about such issues since these should be handled
fine with the userspace application and depends on the implementation.
We only need to focus on exporting data.
Anyway, thanks again for your time!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Mon, Jul 23, 2018 at 03:16:10PM -0700, David Frey wrote:
> On 7/22/2018 3:21 PM, Himanshu Jha wrote:
> >On Sat, Jul 21, 2018 at 08:45:34PM +0300, Daniel Baluta wrote:
> >>On Sat, Jul 21, 2018 at 6:43 PM, Andy Shevchenko
> >> wrote:
> >>>On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> >>> wrote:
> >>>
> >>>>>>+ /* Look up table 1 for the possible gas range values */
> >>>>>>+ u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >>>>>>+ 2147483647u, 2147483647u, 2126008810u,
> >>>>>>+ 2147483647u, 2130303777u, 2147483647u,
> >>>>>>+ 2147483647u, 2143188679u, 2136746228u,
> >>>>>>+ 2147483647u, 2126008810u, 2147483647u,
> >>>>>>+ 2147483647u};
> >>>
> >>>This one needs perhaps a bit of though, but...
> >>>
> >>>>>>+ /* Look up table 2 for the possible gas range values */
> >>>>>>+ u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >>>>>>+ 51200u, 255744255u, 127110228u,
> >>>>>>6400u,
> >>>>>>+ 32258064u, 16016016u, 800u, 400u,
> >>>>>>+ 200u, 100u, 50u, 25u,
> >>>>>>125000u};
> >>>
> >>>...this one obviously just a not needed one. You may replace it with a
> >>>one constant and simple calculation to get either value (index from
> >>>value, or value from index).
> >>
> >>Indeed this can be reduce to:
> >>
> >>125.000 << (15 - idx).
> >>
> >>The real question here is if we approximate 255.744.255u to 256.00.00u how
> >>much different is the result. Being a gas sensor I think it is very
> >>hard to appreciate.
> >>
> >>We can go with this formula + adding a comment with the table with the
> >>exact coefficients.
> >
> >So, I have planned to use this 125000 << (15 - idx) equation with
> >approximating the array members.
> >
> >About the difference in results we would get after approximating isn't
> >much of a problem IMHO because gas sensor is primarily used for IAQ, and
> >IAQ is relative to the resistance reading.
> >
> >For eg:
> >
> >Resistance(ohm) IAQ
> >value < 30K Very bad
> >30k < value < 50kworse
> >50k < value < 70kbad
> >...
> >..
> >so on..
> >
> >So, what I simply imply is the scale will be adjusted and nothing else
> >changes, unlike if it had been pressure, temperature, humidity.
> >
> >The IAQ implementation is userspace application suggesting
> >good/bad/ugly air quality.
> >
> >And since we know David Frey is planning to use this sensor in his
> >product mangOH board.
> >
> >So, David, how are you planning to use the gas sensing part in your
> >product ? RGB leds, buzzer, alarm ?
> >
> >Thanks Andy for the suggestion :)
> >
>
> My understanding is that the Bosch BSEC (Bosch Sensortec Environmental
> Cluster - https://www.bosch-sensortec.com/bst/products/all_products/bsec)
> software calculates the indoor air quality (IAQ) which is presented in
> the range of 0 to 500. BSEC is proprietary, pre-compiled static
> library. I don't know how they derive the IAQ, but it seems that it
> could be based on smoothing outlying gas resistance values and integrating
> other values such as temperature, humidity and pressure.
> Unless this driver can somehow produce IAQ values of equal or greater
> reliability to the BSEC library, then I would prefer that it just
> present the raw gas resistance value so that a user can write a program
> to feed the sensor data into BSEC.
>
> mangOH isn't really a traditional product. It's an open hardware board
> designed around Sierra Wireless cellular modules that run Linux. So I
> don't have any specific use case in mind, but I want to enable our users
> (and thus future products) to make use of air quality measurements.
I tested the gas sensor with the new bitshifting equation and didn't find any
discrepancy in the resistance readings.
Even I don't know how Bosch implements IAQ algorithm, but one thing I
got around testing the sensor was that the air quality is inversely
proportional to resistance value.
[You can try it yourself, by spraying deodrant/aerosol sprays around the sensor
;)]
But we shouldn't be worried about such issues since these should be handled
fine with the userspace application and depends on the implementation.
We only need to focus on exporting data.
Anyway, thanks again for your time!
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Sat, Jul 21, 2018 at 08:45:34PM +0300, Daniel Baluta wrote:
> On Sat, Jul 21, 2018 at 6:43 PM, Andy Shevchenko
> wrote:
> > On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> > wrote:
> >
> >>> > + /* Look up table 1 for the possible gas range values */
> >>> > + u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >>> > + 2147483647u, 2147483647u, 2126008810u,
> >>> > + 2147483647u, 2130303777u, 2147483647u,
> >>> > + 2147483647u, 2143188679u, 2136746228u,
> >>> > + 2147483647u, 2126008810u, 2147483647u,
> >>> > + 2147483647u};
> >
> > This one needs perhaps a bit of though, but...
> >
> >>> > + /* Look up table 2 for the possible gas range values */
> >>> > + u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >>> > + 51200u, 255744255u, 127110228u,
> >>> > 6400u,
> >>> > + 32258064u, 16016016u, 800u, 400u,
> >>> > + 200u, 100u, 50u, 25u,
> >>> > 125000u};
> >
> > ...this one obviously just a not needed one. You may replace it with a
> > one constant and simple calculation to get either value (index from
> > value, or value from index).
>
> Indeed this can be reduce to:
>
> 125.000 << (15 - idx).
>
> The real question here is if we approximate 255.744.255u to 256.00.00u how
> much different is the result. Being a gas sensor I think it is very
> hard to appreciate.
>
> We can go with this formula + adding a comment with the table with the
> exact coefficients.
So, I have planned to use this 125000 << (15 - idx) equation with
approximating the array members.
About the difference in results we would get after approximating isn't
much of a problem IMHO because gas sensor is primarily used for IAQ, and
IAQ is relative to the resistance reading.
For eg:
Resistance(ohm) IAQ
value < 30K Very bad
30k < value < 50k worse
50k < value < 70k bad
...
..
so on..
So, what I simply imply is the scale will be adjusted and nothing else
changes, unlike if it had been pressure, temperature, humidity.
The IAQ implementation is userspace application suggesting
good/bad/ugly air quality.
And since we know David Frey is planning to use this sensor in his
product mangOH board.
So, David, how are you planning to use the gas sensing part in your
product ? RGB leds, buzzer, alarm ?
Thanks Andy for the suggestion :)
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Sat, Jul 21, 2018 at 08:45:34PM +0300, Daniel Baluta wrote:
> On Sat, Jul 21, 2018 at 6:43 PM, Andy Shevchenko
> wrote:
> > On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> > wrote:
> >
> >>> > + /* Look up table 1 for the possible gas range values */
> >>> > + u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >>> > + 2147483647u, 2147483647u, 2126008810u,
> >>> > + 2147483647u, 2130303777u, 2147483647u,
> >>> > + 2147483647u, 2143188679u, 2136746228u,
> >>> > + 2147483647u, 2126008810u, 2147483647u,
> >>> > + 2147483647u};
> >
> > This one needs perhaps a bit of though, but...
> >
> >>> > + /* Look up table 2 for the possible gas range values */
> >>> > + u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >>> > + 51200u, 255744255u, 127110228u,
> >>> > 6400u,
> >>> > + 32258064u, 16016016u, 800u, 400u,
> >>> > + 200u, 100u, 50u, 25u,
> >>> > 125000u};
> >
> > ...this one obviously just a not needed one. You may replace it with a
> > one constant and simple calculation to get either value (index from
> > value, or value from index).
>
> Indeed this can be reduce to:
>
> 125.000 << (15 - idx).
>
> The real question here is if we approximate 255.744.255u to 256.00.00u how
> much different is the result. Being a gas sensor I think it is very
> hard to appreciate.
>
> We can go with this formula + adding a comment with the table with the
> exact coefficients.
So, I have planned to use this 125000 << (15 - idx) equation with
approximating the array members.
About the difference in results we would get after approximating isn't
much of a problem IMHO because gas sensor is primarily used for IAQ, and
IAQ is relative to the resistance reading.
For eg:
Resistance(ohm) IAQ
value < 30K Very bad
30k < value < 50k worse
50k < value < 70k bad
...
..
so on..
So, what I simply imply is the scale will be adjusted and nothing else
changes, unlike if it had been pressure, temperature, humidity.
The IAQ implementation is userspace application suggesting
good/bad/ugly air quality.
And since we know David Frey is planning to use this sensor in his
product mangOH board.
So, David, how are you planning to use the gas sensing part in your
product ? RGB leds, buzzer, alarm ?
Thanks Andy for the suggestion :)
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Sat, Jul 21, 2018 at 06:43:51PM +0300, Andy Shevchenko wrote:
> On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> wrote:
>
> >> > + /* Look up table 1 for the possible gas range values */
> >> > + u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >> > + 2147483647u, 2147483647u, 2126008810u,
> >> > + 2147483647u, 2130303777u, 2147483647u,
> >> > + 2147483647u, 2143188679u, 2136746228u,
> >> > + 2147483647u, 2126008810u, 2147483647u,
> >> > + 2147483647u};
>
> This one needs perhaps a bit of though, but...
>
> >> > + /* Look up table 2 for the possible gas range values */
> >> > + u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >> > + 51200u, 255744255u, 127110228u,
> >> > 6400u,
> >> > + 32258064u, 16016016u, 800u, 400u,
> >> > + 200u, 100u, 50u, 25u,
> >> > 125000u};
>
> ...this one obviously just a not needed one. You may replace it with a
> one constant and simple calculation to get either value (index from
> value, or value from index).
Not sure to understand your opinion!?
Anything related to increase to .ro segment ?
I think adding is better to let compiler optimise the code a bit
further.
> --
> With Best Regards,
> Andy Shevchenko
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
On Sat, Jul 21, 2018 at 06:43:51PM +0300, Andy Shevchenko wrote:
> On Sat, Jul 21, 2018 at 6:36 PM, Himanshu Jha
> wrote:
>
> >> > + /* Look up table 1 for the possible gas range values */
> >> > + u32 lookupTable1[16] = {2147483647u, 2147483647u, 2147483647u,
> >> > + 2147483647u, 2147483647u, 2126008810u,
> >> > + 2147483647u, 2130303777u, 2147483647u,
> >> > + 2147483647u, 2143188679u, 2136746228u,
> >> > + 2147483647u, 2126008810u, 2147483647u,
> >> > + 2147483647u};
>
> This one needs perhaps a bit of though, but...
>
> >> > + /* Look up table 2 for the possible gas range values */
> >> > + u32 lookupTable2[16] = {409600u, 204800u, 102400u,
> >> > + 51200u, 255744255u, 127110228u,
> >> > 6400u,
> >> > + 32258064u, 16016016u, 800u, 400u,
> >> > + 200u, 100u, 50u, 25u,
> >> > 125000u};
>
> ...this one obviously just a not needed one. You may replace it with a
> one constant and simple calculation to get either value (index from
> value, or value from index).
Not sure to understand your opinion!?
Anything related to increase to .ro segment ?
I think adding is better to let compiler optimise the code a bit
further.
> --
> With Best Regards,
> Andy Shevchenko
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
upport 32 bit
> platforms? Also, why does var 2 want to be cast to a 64 bit?
> You'll need to go back to 32 bit anyway to use do_div.
I will change to do_div().
> > +
> > + if ((check & BME680_GAS_STAB_BIT) == 0) {
> > + /*
> > +* occurs if either the gas heating duration was insuffient
> > +* to reach the target heater temperature or the target
> > +* heater temperature was too high for the heater sink to
> > +* reach.
> > +*/
>
> Odd comment indenting. I would move it before the if to make it
> more 'natural'. Still clearly applies to this block without looking 'odd'.
Will do.
> > +static int bme680_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct bme680_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + return bme680_read_temp(data, val, val2);
> > + case IIO_PRESSURE:
> > + return bme680_read_press(data, val, val2);
> > + case IIO_HUMIDITYRELATIVE:
> > + return bme680_read_humid(data, val, val2);
> > + case IIO_RESISTANCE:
> > + return bme680_read_gas(data, val);
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + *val = 1 << data->oversampling_temp;
> > + return IIO_VAL_INT;
> > + case IIO_PRESSURE:
> > + *val = 1 << data->oversampling_press;
> > + return IIO_VAL_INT;
> > + case IIO_HUMIDITYRELATIVE:
> > + *val = 1 << data->oversampling_humid;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
>
> Don't think you can get here so this code should not be here.
Ok.
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
>
> You can't get here so no point in having this return. I'll delete it.
OK.
> > + ret = devm_add_action(dev, bme680_core_remove, indio_dev);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to register remove action\n");
> > + return ret;
> > + }
>
> I think this is logically in the wrong place. The things it's actually
> doing is unwinding the register below, but at this stage you haven't
> performed that registration.
>
> If this is all I fine, I 'might' move it and apply anyway.
>
> I'm assuming that there will shortly be more in there than a simple
> unregister (otherwise you could just have used devm_iio_device_register...
>
> I'd prefer that for now you just use devm_iio_device_register
> and drop this until you need it.
OK.
> > +
> > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> > + BME680_CMD_SOFTRESET);
> > + if (ret < 0)
>
> It's not something I'm that bothered by, but you are a little random
> in whether a given error outputs a debug message or not...
I missed this.
Will fix.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
upport 32 bit
> platforms? Also, why does var 2 want to be cast to a 64 bit?
> You'll need to go back to 32 bit anyway to use do_div.
I will change to do_div().
> > +
> > + if ((check & BME680_GAS_STAB_BIT) == 0) {
> > + /*
> > +* occurs if either the gas heating duration was insuffient
> > +* to reach the target heater temperature or the target
> > +* heater temperature was too high for the heater sink to
> > +* reach.
> > +*/
>
> Odd comment indenting. I would move it before the if to make it
> more 'natural'. Still clearly applies to this block without looking 'odd'.
Will do.
> > +static int bme680_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct bme680_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + return bme680_read_temp(data, val, val2);
> > + case IIO_PRESSURE:
> > + return bme680_read_press(data, val, val2);
> > + case IIO_HUMIDITYRELATIVE:
> > + return bme680_read_humid(data, val, val2);
> > + case IIO_RESISTANCE:
> > + return bme680_read_gas(data, val);
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + *val = 1 << data->oversampling_temp;
> > + return IIO_VAL_INT;
> > + case IIO_PRESSURE:
> > + *val = 1 << data->oversampling_press;
> > + return IIO_VAL_INT;
> > + case IIO_HUMIDITYRELATIVE:
> > + *val = 1 << data->oversampling_humid;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
>
> Don't think you can get here so this code should not be here.
Ok.
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +
>
> You can't get here so no point in having this return. I'll delete it.
OK.
> > + ret = devm_add_action(dev, bme680_core_remove, indio_dev);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to register remove action\n");
> > + return ret;
> > + }
>
> I think this is logically in the wrong place. The things it's actually
> doing is unwinding the register below, but at this stage you haven't
> performed that registration.
>
> If this is all I fine, I 'might' move it and apply anyway.
>
> I'm assuming that there will shortly be more in there than a simple
> unregister (otherwise you could just have used devm_iio_device_register...
>
> I'd prefer that for now you just use devm_iio_device_register
> and drop this until you need it.
OK.
> > +
> > + ret = regmap_write(regmap, BME680_REG_SOFT_RESET_I2C,
> > + BME680_CMD_SOFTRESET);
> > + if (ret < 0)
>
> It's not something I'm that bothered by, but you are a little random
> in whether a given error outputs a debug message or not...
I missed this.
Will fix.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
[PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v4: -Added Bosch API links and datasheet link. -explained with comments about the compensate functions. -fixed SPI ID register address. -Used FIELD_PREP macro to avoid unnecessary hard coding. -Simplified Kconfig to remove unnecessary parentheses. v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 23 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 96 drivers/iio/chemical/bme680_core.c | 976 + drivers/iio/chemical/bme680_i2c.c | 83 drivers/iio/chemical/bme680_spi.c | 123 + 6 files changed, 1304 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..b8e005b 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,29 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if I2C + select BME680_SPI if SPI + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on I2C && BME680 + select REGMAP_I2C + +config BME680_SPI + tristate + depends on SPI && BME680 + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -4,6 +4,9 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o +obj-$(CONFIG_BME680) += bme680_core.o +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o +obj-$(CONFIG_BME680_SPI) += bme680_spi.o obj-$(CONFIG_CCS811) += ccs811.o obj-$(CONFIG_IAQCORE) += ams-iaq-core.o obj-$(CONFIG_VZ89X)+= vz89x.o diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h new file mode 100644 index 000..b872f66 --- /dev/null +++ b/drivers/iio/chemical/bme680.h @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef BME680_H_ +#define BME680_H_ + +#define BME680_REG_CHIP_I2C_ID 0xD0 +#define BME680_REG_CHIP_SPI_ID 0x50 +#define BME680_CHIP_ID_VAL 0x61 +#define BME680_REG_S
[PATCH v4] iio: chemical: Add support for Bosch BME680 sensor
Bosch BME680 is a 4-in-1 sensor with temperature, pressure, humidity and gas sensing capability. It supports both I2C and SPI communication protocol for effective data communication. The device supports two modes: 1. Sleep mode 2. Forced mode The measurements only takes place when forced mode is triggered and a single TPHG cycle is performed by the sensor. The sensor automatically goes to sleep after afterwards. The device has various calibration constants/parameters programmed into devices' non-volatile memory(NVM) during production and can't be altered by the user. These constants are used in the compensation functions to get the required compensated readings along with the raw data. The compensation functions/algorithms are provided by Bosch Sensortec GmbH via their API[1]. As these don't change during the measurement cycle, therefore we read and store them at the probe. The default configs supplied by Bosch are also set at probe. 0-day tested with build success. GSoC-2018: https://summerofcode.withgoogle.com/projects/#6691473790074880 Mentor: Daniel Baluta [1] https://github.com/BoschSensortec/BME680_driver Datasheet: https://ae-bst.resource.bosch.com/media/_tech/media/datasheets/BST-BME680-DS001-00.pdf Cc: Daniel Baluta Signed-off-by: Himanshu Jha --- v4: -Added Bosch API links and datasheet link. -explained with comments about the compensate functions. -fixed SPI ID register address. -Used FIELD_PREP macro to avoid unnecessary hard coding. -Simplified Kconfig to remove unnecessary parentheses. v3: -moved files to chemical directory instead of a dedicated directory. -read calibration parameters serially with endian conversions. -drop some return ret. -removed few unnecessary casts safely. -added 'u' suffix to explicitly specify unsigned for large values and thereby fixing comiler warning. -left aligned all comments. -added a comment explaining heater stability failure. v2: -Used devm_add_action() to add a generic remove method for both I2C & SPI driver. -Introduction of compensation functions. -chip initialisation routines moved to respective I2C and SPI driver. -Introduction of gas sensing rountines. -Simplified Kconfig to reduce various options. drivers/iio/chemical/Kconfig | 23 + drivers/iio/chemical/Makefile | 3 + drivers/iio/chemical/bme680.h | 96 drivers/iio/chemical/bme680_core.c | 976 + drivers/iio/chemical/bme680_i2c.c | 83 drivers/iio/chemical/bme680_spi.c | 123 + 6 files changed, 1304 insertions(+) create mode 100644 drivers/iio/chemical/bme680.h create mode 100644 drivers/iio/chemical/bme680_core.c create mode 100644 drivers/iio/chemical/bme680_i2c.c create mode 100644 drivers/iio/chemical/bme680_spi.c diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig index 5cb5be7..b8e005b 100644 --- a/drivers/iio/chemical/Kconfig +++ b/drivers/iio/chemical/Kconfig @@ -21,6 +21,29 @@ config ATLAS_PH_SENSOR To compile this driver as module, choose M here: the module will be called atlas-ph-sensor. +config BME680 + tristate "Bosch Sensortec BME680 sensor driver" + depends on (I2C || SPI) + select REGMAP + select BME680_I2C if I2C + select BME680_SPI if SPI + help + Say yes here to build support for Bosch Sensortec BME680 sensor with + temperature, pressure, humidity and gas sensing capability. + + This driver can also be built as a module. If so, the module for I2C + would be called bme680_i2c and bme680_spi for SPI support. + +config BME680_I2C + tristate + depends on I2C && BME680 + select REGMAP_I2C + +config BME680_SPI + tristate + depends on SPI && BME680 + select REGMAP_SPI + config CCS811 tristate "AMS CCS811 VOC sensor" depends on I2C diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile index a629b29..2f4c4ba 100644 --- a/drivers/iio/chemical/Makefile +++ b/drivers/iio/chemical/Makefile @@ -4,6 +4,9 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_ATLAS_PH_SENSOR) += atlas-ph-sensor.o +obj-$(CONFIG_BME680) += bme680_core.o +obj-$(CONFIG_BME680_I2C) += bme680_i2c.o +obj-$(CONFIG_BME680_SPI) += bme680_spi.o obj-$(CONFIG_CCS811) += ccs811.o obj-$(CONFIG_IAQCORE) += ams-iaq-core.o obj-$(CONFIG_VZ89X)+= vz89x.o diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h new file mode 100644 index 000..b872f66 --- /dev/null +++ b/drivers/iio/chemical/bme680.h @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef BME680_H_ +#define BME680_H_ + +#define BME680_REG_CHIP_I2C_ID 0xD0 +#define BME680_REG_CHIP_SPI_ID 0x50 +#define BME680_CHIP_ID_VAL 0x61 +#define BME680_REG_S
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Jonathan, On Sun, Jul 15, 2018 at 10:10:36AM +0100, Jonathan Cameron wrote: > On Sat, 14 Jul 2018 13:03:42 +0530 > Himanshu Jha wrote: > > > Hi David, > > > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > > Hi Himanshu Jha, > > > > > > First a bit of background. I'm working on a device which will contain a > > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > > for the chip a little while ago. The (WIP) driver can be found here: > > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > > > Great! > > > > > This driver is written targeting an older kernel (3.18.x) because that's > > > the > > > kernel we're stuck on for now. Rather than writing the driver from > > > scratch, > > > what we did was write the Linux kernel driver as a wrapper around the > > > Bosch > > > code. My theory at the time was that Bosch made the chip, so they > > > probably > > > know what they're doing when it comes to writing a driver library. After > > > having looked at the code in more detail, I'm less confident that our > > > approach was the best one. I'm not attempting to upstream the driver > > > built > > > by my colleague and I'm not trying to request review of this code either. > > > I > > > simply want to make you aware of it so that you can look at it to get some > > > ideas. > > > > Thanks for taking your time to review. > > > > > I have included a number of comments on your driver below. Keep up the > > > good > > > work! > > > > > > >+++ b/drivers/iio/chemical/bme680.h > > > >@@ -0,0 +1,99 @@ > > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > > >+#ifndef BME680_H_ > > > >+#define BME680_H_ > > > >+ > > > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > > > >+#define BME680_REG_CHIP_SPI_ID 0x50 > > > >+#define BME680_CHIP_ID_VAL 0x61 > > > Try to be consistent with the indenting of the defines. I think this > > > would > > > be clearest: > > > #define BME680_REG_X 0x00 > > > #define BME680_X_FOO_EN_MASKBIT(0) > > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > > #define BME680_BAR_VAL1 3 > > > #define BME680_BAR_VAL2 7 > > > > > > This way the register, field definition and field values are all visually > > > distinctive. > > > > I have used this pattern everywhere where applicable. But not applied > > for *_VAL, would definitely follow this up. > > > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > > The datasheet says that the soft reset register differs for I2C and SPI. > > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > > > That's really a stupid mistake :( > > I have exported these individual initialization code in the I2C & SPI > > drivers respectively but it slipped my mind somehow. This device has > > peculiar characteristics in register addressing. > > > > I will correct this in next version. > > > > > >+#define BME680_CMD_SOFTRESET0xB6 > > > >+#define BME680_REG_STATUS 0x73 > > > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > > > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > > > >+ > > > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > > You could use the FIELD_PREP macro from to eliminate > > > the > > > need for these macros. For example: > > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > > > Ah, yes! I didn't knew about these magic macros. It will remove some > > log2() computation hacks from my code. > > > > > >+ > > > >+#define BME680_REG_TEMP_MSB 0x22 > > > >+#define BME680_REG_PRESS_MSB0x1F > > > >+#define BM6880_REG_HUMIDITY_MSB
Re: [PATCH v3] iio: chemical: Add support for Bosch BME680 sensor
Hi Jonathan, On Sun, Jul 15, 2018 at 10:10:36AM +0100, Jonathan Cameron wrote: > On Sat, 14 Jul 2018 13:03:42 +0530 > Himanshu Jha wrote: > > > Hi David, > > > > On Fri, Jul 13, 2018 at 01:42:35PM -0700, David Frey wrote: > > > Hi Himanshu Jha, > > > > > > First a bit of background. I'm working on a device which will contain a > > > bme680 sensor. A colleague of mine started work on a Linux kernel driver > > > for the chip a little while ago. The (WIP) driver can be found here: > > > https://github.com/mangOH/mangOH/tree/master/linux_kernel_modules/bme680 > > > > Great! > > > > > This driver is written targeting an older kernel (3.18.x) because that's > > > the > > > kernel we're stuck on for now. Rather than writing the driver from > > > scratch, > > > what we did was write the Linux kernel driver as a wrapper around the > > > Bosch > > > code. My theory at the time was that Bosch made the chip, so they > > > probably > > > know what they're doing when it comes to writing a driver library. After > > > having looked at the code in more detail, I'm less confident that our > > > approach was the best one. I'm not attempting to upstream the driver > > > built > > > by my colleague and I'm not trying to request review of this code either. > > > I > > > simply want to make you aware of it so that you can look at it to get some > > > ideas. > > > > Thanks for taking your time to review. > > > > > I have included a number of comments on your driver below. Keep up the > > > good > > > work! > > > > > > >+++ b/drivers/iio/chemical/bme680.h > > > >@@ -0,0 +1,99 @@ > > > >+/* SPDX-License-Identifier: GPL-2.0 */ > > > >+#ifndef BME680_H_ > > > >+#define BME680_H_ > > > >+ > > > >+#define BME680_REG_CHIP_I2C_ID 0xD0 > > > >+#define BME680_REG_CHIP_SPI_ID 0x50 > > > >+#define BME680_CHIP_ID_VAL 0x61 > > > Try to be consistent with the indenting of the defines. I think this > > > would > > > be clearest: > > > #define BME680_REG_X 0x00 > > > #define BME680_X_FOO_EN_MASKBIT(0) > > > #define BME680_X_BAR_MASK GENMASK(3, 1) > > > #define BME680_BAR_VAL1 3 > > > #define BME680_BAR_VAL2 7 > > > > > > This way the register, field definition and field values are all visually > > > distinctive. > > > > I have used this pattern everywhere where applicable. But not applied > > for *_VAL, would definitely follow this up. > > > > > >+#define BME680_REG_SOFT_RESET 0xE0 > > > The datasheet says that the soft reset register differs for I2C and SPI. > > > For I2C it is 0xE0 and for SPI it is 0x60 when page 0 is selected. > > > > That's really a stupid mistake :( > > I have exported these individual initialization code in the I2C & SPI > > drivers respectively but it slipped my mind somehow. This device has > > peculiar characteristics in register addressing. > > > > I will correct this in next version. > > > > > >+#define BME680_CMD_SOFTRESET0xB6 > > > >+#define BME680_REG_STATUS 0x73 > > > >+#define BME680_SPI_MEM_PAGE_BIT BIT(4) > > > >+#define BME680_SPI_MEM_PAGE_1_VAL 1 > > > >+ > > > >+#define BME680_OSRS_TEMP_X(osrs_t) ((osrs_t) << 5) > > > >+#define BME680_OSRS_PRESS_X(osrs_p) ((osrs_p) << 2) > > > >+#define BME680_OSRS_HUMID_X(osrs_h) ((osrs_h) << 0) > > > You could use the FIELD_PREP macro from to eliminate > > > the > > > need for these macros. For example: > > > ctrl_meas_reg = FIELD_PREP(BME680_OSRS_TEMP_MASK, temp_val) | > > > FIELD_PREP(BME680_OSRS_PRESS_MASK, press_val) | > > > FIELD_PREP(BME880_MODE_MASK, mode_val); > > > > Ah, yes! I didn't knew about these magic macros. It will remove some > > log2() computation hacks from my code. > > > > > >+ > > > >+#define BME680_REG_TEMP_MSB 0x22 > > > >+#define BME680_REG_PRESS_MSB0x1F > > > >+#define BM6880_REG_HUMIDITY_MSB
