[PATCH v2 2/3] media: dvbsky: Remove duplicate error reporting for dvbsky_usb_generic_rw
Errors are already reported by the common code in dvb_usb_v2_generic_io
(which dvbsky_usb_generic_rw is a wrapper of), so there is no reason
report the error again.
Signed-off-by: Stefan Brüns
---
drivers/media/usb/dvb-usb-v2/dvbsky.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index ae0814dd202a..3ff9833597e5 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -100,8 +100,6 @@ static int dvbsky_gpio_ctrl(struct dvb_usb_device *d, u8
gport, u8 value)
obuf[1] = gport;
obuf[2] = value;
ret = dvbsky_usb_generic_rw(d, obuf, 3, ibuf, 1);
- if (ret)
- dev_err(>udev->dev, "failed=%d\n", ret);
return ret;
}
@@ -139,8 +137,6 @@ static int dvbsky_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msg[],
obuf[3] = msg[0].addr;
ret = dvbsky_usb_generic_rw(d, obuf, 4,
ibuf, msg[0].len + 1);
- if (ret)
- dev_err(>udev->dev, "failed=%d\n", ret);
if (!ret)
memcpy(msg[0].buf, [1], msg[0].len);
} else {
@@ -151,8 +147,6 @@ static int dvbsky_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msg[],
memcpy([3], msg[0].buf, msg[0].len);
ret = dvbsky_usb_generic_rw(d, obuf,
msg[0].len + 3, ibuf, 1);
- if (ret)
- dev_err(>udev->dev, "failed=%d\n", ret);
}
} else {
if ((msg[0].len > 60) || (msg[1].len > 60)) {
@@ -170,9 +164,6 @@ static int dvbsky_i2c_xfer(struct i2c_adapter *adap, struct
i2c_msg msg[],
memcpy([4], msg[0].buf, msg[0].len);
ret = dvbsky_usb_generic_rw(d, obuf,
msg[0].len + 4, ibuf, msg[1].len + 1);
- if (ret)
- dev_err(>udev->dev, "failed=%d\n", ret);
-
if (!ret)
memcpy(msg[1].buf, [1], msg[1].len);
}
@@ -201,8 +192,6 @@ static int dvbsky_rc_query(struct dvb_usb_device *d)
obuf[0] = 0x10;
ret = dvbsky_usb_generic_rw(d, obuf, 1, ibuf, 2);
- if (ret)
- dev_err(>udev->dev, "failed=%d\n", ret);
if (ret == 0)
code = (ibuf[0] << 8) | ibuf[1];
if (code != 0x) {
--
2.21.0
[PATCH v2 1/3] media: dvb-usb-v2: Report error on all error paths
actual_length != wlen is the only error path which does not generate an
error message. Adding an error message here allows to report a more
specific error and to remove the error reporting from the call sites.
Also clean up the error paths - in case of an error, the remaining
code is skipped, and ret is returned. Skip setting ret and return
immediately (no cleanup necessary).
Signed-off-by: Stefan Brüns
---
drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c
b/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c
index 5bafeb6486be..5b32d159f968 100644
--- a/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c
+++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c
@@ -37,14 +37,19 @@ static int dvb_usb_v2_generic_io(struct dvb_usb_device *d,
ret = usb_bulk_msg(d->udev, usb_sndbulkpipe(d->udev,
d->props->generic_bulk_ctrl_endpoint), wbuf, wlen,
_length, 2000);
- if (ret < 0)
+ if (ret) {
dev_err(>udev->dev, "%s: usb_bulk_msg() failed=%d\n",
KBUILD_MODNAME, ret);
- else
- ret = actual_length != wlen ? -EIO : 0;
+ return ret;
+ }
+ if (actual_length != wlen) {
+ dev_err(>udev->dev, "%s: usb_bulk_msg() write length=%d,
actual=%d\n",
+ KBUILD_MODNAME, wlen, actual_length);
+ return -EIO;
+ }
- /* an answer is expected, and no error before */
- if (!ret && rbuf && rlen) {
+ /* an answer is expected */
+ if (rbuf && rlen) {
if (d->props->generic_bulk_ctrl_delay)
usleep_range(d->props->generic_bulk_ctrl_delay,
d->props->generic_bulk_ctrl_delay
--
2.21.0
[PATCH v2 0/3] Cleanup error reporting
Almost all error path in dvb_usb_v2_generic_io report a specific error using dev_err(), only the "writelen != actuallen" case was omitted. Add an error message. Remove any extra error reporting from the call sites which was only required for this case. Only affected drivers are dvbsky and af9035. v2: Add Signed-off-by Stefan Brüns (3): media: dvb-usb-v2: Report error on all error paths media: dvbsky: Remove duplicate error reporting for dvbsky_usb_generic_rw media: af9035: Remove duplicate error reporting for dvbsky_usb_generic_rw drivers/media/usb/dvb-usb-v2/af9035.c | 2 -- drivers/media/usb/dvb-usb-v2/dvb_usb_urb.c | 15 ++- drivers/media/usb/dvb-usb-v2/dvbsky.c | 11 --- 3 files changed, 10 insertions(+), 18 deletions(-) -- 2.21.0
[PATCH v2 3/3] media: af9035: Remove duplicate error reporting for dvbsky_usb_generic_rw
All error cases inside the function already report errors via dev_err(), and dvb_usb_v2_generic_rw also reports all error cases, so there is no silent code path when an error has occured. Signed-off-by: Stefan Brüns --- drivers/media/usb/dvb-usb-v2/af9035.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c index 1b7f1af399fb..15643e2f9395 100644 --- a/drivers/media/usb/dvb-usb-v2/af9035.c +++ b/drivers/media/usb/dvb-usb-v2/af9035.c @@ -120,8 +120,6 @@ static int af9035_ctrl_msg(struct dvb_usb_device *d, struct usb_req *req) memcpy(req->rbuf, >buf[ACK_HDR_LEN], req->rlen); exit: mutex_unlock(>usb_mutex); - if (ret < 0) - dev_dbg(>dev, "failed=%d\n", ret); return ret; } -- 2.21.0
[PATCH] media: dvbsky: Avoid leaking dvb frontend
Commit 14f4eaeddabc ("media: dvbsky: fix driver unregister logic") fixed
a use-after-free by removing the reference to the frontend after deleting
the backing i2c device.
This has the unfortunate side effect the frontend device is never freed
in the dvb core leaving a dangling device, leading to errors when the
dvb core tries to register the frontend after e.g. a replug as reported
here: https://www.spinics.net/lists/linux-media/msg138181.html
media: dvbsky: issues with DVBSky T680CI
===
[ 561.119145] sp2 8-0040: CIMaX SP2 successfully attached
[ 561.119161] usb 2-3: DVB: registering adapter 0 frontend 0 (Silicon Labs
Si2168)...
[ 561.119174] sysfs: cannot create duplicate filename '/class/dvb/
dvb0.frontend0'
===
The use after free happened as dvb_usbv2_disconnect calls in this order:
- dvb_usb_device::props->exit(...)
- dvb_usbv2_adapter_frontend_exit(...)
+ if (fe) dvb_unregister_frontend(fe)
+ dvb_usb_device::props->frontend_detach(...)
Moving the release of the i2c device from exit() to frontend_detach()
avoids the dangling pointer access and allows the core to unregister
the frontend.
This was originally reported for a DVBSky T680CI, but it also affects
the MyGica T230C. As all supported devices structure the registration/
unregistration identically, apply the change for all device types.
Signed-off-by: Stefan Brüns
---
drivers/media/usb/dvb-usb-v2/dvbsky.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c
b/drivers/media/usb/dvb-usb-v2/dvbsky.c
index dffcadd8c834..3ff9833597e5 100644
--- a/drivers/media/usb/dvb-usb-v2/dvbsky.c
+++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c
@@ -604,16 +604,18 @@ static int dvbsky_init(struct dvb_usb_device *d)
return 0;
}
-static void dvbsky_exit(struct dvb_usb_device *d)
+static int dvbsky_frontend_detach(struct dvb_usb_adapter *adap)
{
+ struct dvb_usb_device *d = adap_to_d(adap);
struct dvbsky_state *state = d_to_priv(d);
- struct dvb_usb_adapter *adap = >adapter[0];
+
+ dev_dbg(>udev->dev, "%s: adap=%d\n", __func__, adap->id);
dvb_module_release(state->i2c_client_tuner);
dvb_module_release(state->i2c_client_demod);
dvb_module_release(state->i2c_client_ci);
- adap->fe[0] = NULL;
+ return 0;
}
/* DVB USB Driver stuff */
@@ -629,11 +631,11 @@ static struct dvb_usb_device_properties dvbsky_s960_props
= {
.i2c_algo = _i2c_algo,
.frontend_attach = dvbsky_s960_attach,
+ .frontend_detach = dvbsky_frontend_detach,
.init = dvbsky_init,
.get_rc_config= dvbsky_get_rc_config,
.streaming_ctrl = dvbsky_streaming_ctrl,
.identify_state = dvbsky_identify_state,
- .exit = dvbsky_exit,
.read_mac_address = dvbsky_read_mac_addr,
.num_adapters = 1,
@@ -656,11 +658,11 @@ static struct dvb_usb_device_properties
dvbsky_s960c_props = {
.i2c_algo = _i2c_algo,
.frontend_attach = dvbsky_s960c_attach,
+ .frontend_detach = dvbsky_frontend_detach,
.init = dvbsky_init,
.get_rc_config= dvbsky_get_rc_config,
.streaming_ctrl = dvbsky_streaming_ctrl,
.identify_state = dvbsky_identify_state,
- .exit = dvbsky_exit,
.read_mac_address = dvbsky_read_mac_addr,
.num_adapters = 1,
@@ -683,11 +685,11 @@ static struct dvb_usb_device_properties
dvbsky_t680c_props = {
.i2c_algo = _i2c_algo,
.frontend_attach = dvbsky_t680c_attach,
+ .frontend_detach = dvbsky_frontend_detach,
.init = dvbsky_init,
.get_rc_config= dvbsky_get_rc_config,
.streaming_ctrl = dvbsky_streaming_ctrl,
.identify_state = dvbsky_identify_state,
- .exit = dvbsky_exit,
.read_mac_address = dvbsky_read_mac_addr,
.num_adapters = 1,
@@ -710,11 +712,11 @@ static struct dvb_usb_device_properties dvbsky_t330_props
= {
.i2c_algo = _i2c_algo,
.frontend_attach = dvbsky_t330_attach,
+ .frontend_detach = dvbsky_frontend_detach,
.init = dvbsky_init,
.get_rc_config= dvbsky_get_rc_config,
.streaming_ctrl = dvbsky_streaming_ctrl,
.identify_state = dvbsky_identify_state,
- .exit = dvbsky_exit,
.read_mac_address = dvbsky_read_mac_addr,
.num_adapters = 1,
@@ -737,11 +739,11 @@ static struct dvb_usb_device_properties
mygica_t230c_props = {
.i2c_algo = _i2c_algo,
.frontend_attach = dvbsky_mygica_t230c_attach,
+ .frontend_detach = dvbsky_frontend_detach,
.init = dvbsky_init,
.get_rc_config= dvbsky_get_rc_config,
.streaming_ctrl = dvbsky_streaming_ctrl,
.identify_state = dvbsky_ident
Re: [PATCH][V2] iio: adc: ina2xx: add in early -EINVAL returns in case statements
On Dienstag, 16. Oktober 2018 18:14:18 CEST Colin King wrote:
> From: Colin Ian King
>
> Static analysis with CoverityScan is throwing warnings that specific
> case statements are missing breaks. Rather than adding breaks, add
> return -EINVAL to the specific case statements to clarify the
> error return paths. Fix also saves 50 bytes.
>
> Before:
>text data bss dec hex filename
> 21418 4936 128 264826772 drivers/iio/adc/ina2xx-adc.o
>
> After:
> dec hex filename
> 21370 4936 128 264346742 drivers/iio/adc/ina2xx-adc.o
>
> (gcc 8.2, x86-64)
>
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")
>
> ---
>
> V2: use returns instead of break statements to keep with the
> current style used in the switch statement.
>
> Signed-off-by: Colin Ian King
Reviewed-by: Stefan Brüns
> ---
> drivers/iio/adc/ina2xx-adc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index d1239624187d..bdd7cba6f6b0 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -250,6 +250,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val2 = chip->shunt_resistor_uohm;
> return IIO_VAL_FRACTIONAL;
> }
> + return -EINVAL;
>
> case IIO_CHAN_INFO_HARDWAREGAIN:
> switch (chan->address) {
> @@ -262,6 +263,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val = chip->range_vbus == 32 ? 1 : 2;
> return IIO_VAL_INT;
> }
> + return -EINVAL;
> }
>
> return -EINVAL;
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH][V2] iio: adc: ina2xx: add in early -EINVAL returns in case statements
On Dienstag, 16. Oktober 2018 18:14:18 CEST Colin King wrote:
> From: Colin Ian King
>
> Static analysis with CoverityScan is throwing warnings that specific
> case statements are missing breaks. Rather than adding breaks, add
> return -EINVAL to the specific case statements to clarify the
> error return paths. Fix also saves 50 bytes.
>
> Before:
>text data bss dec hex filename
> 21418 4936 128 264826772 drivers/iio/adc/ina2xx-adc.o
>
> After:
> dec hex filename
> 21370 4936 128 264346742 drivers/iio/adc/ina2xx-adc.o
>
> (gcc 8.2, x86-64)
>
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")
>
> ---
>
> V2: use returns instead of break statements to keep with the
> current style used in the switch statement.
>
> Signed-off-by: Colin Ian King
Reviewed-by: Stefan Brüns
> ---
> drivers/iio/adc/ina2xx-adc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index d1239624187d..bdd7cba6f6b0 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -250,6 +250,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val2 = chip->shunt_resistor_uohm;
> return IIO_VAL_FRACTIONAL;
> }
> + return -EINVAL;
>
> case IIO_CHAN_INFO_HARDWAREGAIN:
> switch (chan->address) {
> @@ -262,6 +263,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val = chip->range_vbus == 32 ? 1 : 2;
> return IIO_VAL_INT;
> }
> + return -EINVAL;
> }
>
> return -EINVAL;
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] iio: adc: ina2xx: fix missing break statement
On Montag, 8. Oktober 2018 23:09:04 CEST Colin King wrote:
> From: Colin Ian King
>
> The IIO_CHAN_INFO_SCALE case is missing a break statement and in
> the unlikely event that chan->address is not matched in the nested
> switch statement then the code falls through to the following
> IIO_CHAN_INFO_HARDWAREGAIN case. Fix this by adding the missing
> break. While we are fixing this, it's probably a good idea to
> add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
> too (this is a moot point).
>
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")
Although it is good for code clarity to add a break statement, the code can
never return anything but -EINVAL in case chan->address is not handled in
IIO_CHAN_INFO_SCALE:
-
switch (mask) {
case IIO_CHAN_INFO_SCALE:
switch (chan->address) {
case INA2XX_SHUNT_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_BUS_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_CURRENT:
... return IIO_VAL_FRACTIONAL;
case INA2XX_POWER:
... return IIO_VAL_FRACTIONAL;
}
case IIO_CHAN_INFO_HARDWAREGAIN:
switch (chan->address) {
case INA2XX_SHUNT_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_BUS_VOLTAGE:
... return IIO_VAL_INT;
}
}
return -EINVAL;
-
The addresses handled in INFO_HARDWAREGAIN is a subset of the ones in
INFO_SCALE.
I would prefer an early "return -EINVAL" here, as it matches better with the
other "switch (mask)" cases above.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] iio: adc: ina2xx: fix missing break statement
On Montag, 8. Oktober 2018 23:09:04 CEST Colin King wrote:
> From: Colin Ian King
>
> The IIO_CHAN_INFO_SCALE case is missing a break statement and in
> the unlikely event that chan->address is not matched in the nested
> switch statement then the code falls through to the following
> IIO_CHAN_INFO_HARDWAREGAIN case. Fix this by adding the missing
> break. While we are fixing this, it's probably a good idea to
> add in a break statement to the IIO_CHAN_INFO_HARDWAREGAIN case
> too (this is a moot point).
>
> Detected by CoverityScan, CID#1462408 ("Missing break in switch")
Although it is good for code clarity to add a break statement, the code can
never return anything but -EINVAL in case chan->address is not handled in
IIO_CHAN_INFO_SCALE:
-
switch (mask) {
case IIO_CHAN_INFO_SCALE:
switch (chan->address) {
case INA2XX_SHUNT_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_BUS_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_CURRENT:
... return IIO_VAL_FRACTIONAL;
case INA2XX_POWER:
... return IIO_VAL_FRACTIONAL;
}
case IIO_CHAN_INFO_HARDWAREGAIN:
switch (chan->address) {
case INA2XX_SHUNT_VOLTAGE:
... return IIO_VAL_FRACTIONAL;
case INA2XX_BUS_VOLTAGE:
... return IIO_VAL_INT;
}
}
return -EINVAL;
-
The addresses handled in INFO_HARDWAREGAIN is a subset of the ones in
INFO_SCALE.
I would prefer an early "return -EINVAL" here, as it matches better with the
other "switch (mask)" cases above.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v1 0/2] Remove duplicate driver for MyGica T230C
On Wednesday, 10 January 2018 00:33:37 CET Stefan Brüns wrote: > In 2017-02, two drivers for the T230C where submitted, but until now > only the one based on the older dvb-usb/cxusb.c driver has been part > of the mainline kernel. As a dvb-usb-v2 driver is preferable, remove > the other driver. > > The cxusb.c patch also contained an unrelated change for the T230, > i.e. a correction of the RC model. As this change apparently is > correct, restore it. This has not been tested due to lack of hardware. > > > Evgeny Plehov (1): > Revert "[media] dvb-usb-cxusb: Geniatech T230C support" > > Stefan Brüns (1): > [media] cxusb: restore RC_MAP for MyGica T230 > > drivers/media/usb/dvb-usb/cxusb.c | 137 > ------ 1 file changed, 137 deletions(-) Ping! -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 0/2] Remove duplicate driver for MyGica T230C
On Wednesday, 10 January 2018 00:33:37 CET Stefan Brüns wrote: > In 2017-02, two drivers for the T230C where submitted, but until now > only the one based on the older dvb-usb/cxusb.c driver has been part > of the mainline kernel. As a dvb-usb-v2 driver is preferable, remove > the other driver. > > The cxusb.c patch also contained an unrelated change for the T230, > i.e. a correction of the RC model. As this change apparently is > correct, restore it. This has not been tested due to lack of hardware. > > > Evgeny Plehov (1): > Revert "[media] dvb-usb-cxusb: Geniatech T230C support" > > Stefan Brüns (1): > [media] cxusb: restore RC_MAP for MyGica T230 > > drivers/media/usb/dvb-usb/cxusb.c | 137 > ------ 1 file changed, 137 deletions(-) Ping! -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
[PATCH v1 0/2] Remove duplicate driver for MyGica T230C
In 2017-02, two drivers for the T230C where submitted, but until now only the one based on the older dvb-usb/cxusb.c driver has been part of the mainline kernel. As a dvb-usb-v2 driver is preferable, remove the other driver. The cxusb.c patch also contained an unrelated change for the T230, i.e. a correction of the RC model. As this change apparently is correct, restore it. This has not been tested due to lack of hardware. Evgeny Plehov (1): Revert "[media] dvb-usb-cxusb: Geniatech T230C support" Stefan Brüns (1): [media] cxusb: restore RC_MAP for MyGica T230 drivers/media/usb/dvb-usb/cxusb.c | 137 -- 1 file changed, 137 deletions(-) -- 2.15.1
[PATCH v1 0/2] Remove duplicate driver for MyGica T230C
In 2017-02, two drivers for the T230C where submitted, but until now only the one based on the older dvb-usb/cxusb.c driver has been part of the mainline kernel. As a dvb-usb-v2 driver is preferable, remove the other driver. The cxusb.c patch also contained an unrelated change for the T230, i.e. a correction of the RC model. As this change apparently is correct, restore it. This has not been tested due to lack of hardware. Evgeny Plehov (1): Revert "[media] dvb-usb-cxusb: Geniatech T230C support" Stefan Brüns (1): [media] cxusb: restore RC_MAP for MyGica T230 drivers/media/usb/dvb-usb/cxusb.c | 137 -- 1 file changed, 137 deletions(-) -- 2.15.1
[PATCH v1 1/2] Revert "[media] dvb-usb-cxusb: Geniatech T230C support"
From: Evgeny Plehov <evgenyple...@ukr.net>
This reverts commit f8585ce655e9cdeabc38e8e2580b05735110e4a5.
The T230C is handled by the dvb-usb-v2/dvbsky.c driver, which should
be preferred over a dvb-usb (v1) driver.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/media/usb/dvb-usb/cxusb.c | 139 +-
1 file changed, 1 insertion(+), 138 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index 37dea0adc695..edb7cd2e43d9 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -1239,82 +1239,6 @@ static int cxusb_mygica_t230_frontend_attach(struct
dvb_usb_adapter *adap)
return 0;
}
-static int cxusb_mygica_t230c_frontend_attach(struct dvb_usb_adapter *adap)
-{
- struct dvb_usb_device *d = adap->dev;
- struct cxusb_state *st = d->priv;
- struct i2c_adapter *adapter;
- struct i2c_client *client_demod;
- struct i2c_client *client_tuner;
- struct i2c_board_info info;
- struct si2168_config si2168_config;
- struct si2157_config si2157_config;
-
- /* Select required USB configuration */
- if (usb_set_interface(d->udev, 0, 0) < 0)
- err("set interface failed");
-
- /* Unblock all USB pipes */
- usb_clear_halt(d->udev,
- usb_sndbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint));
- usb_clear_halt(d->udev,
- usb_rcvbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint));
- usb_clear_halt(d->udev,
- usb_rcvbulkpipe(d->udev,
d->props.adapter[0].fe[0].stream.endpoint));
-
- /* attach frontend */
- memset(_config, 0, sizeof(si2168_config));
- si2168_config.i2c_adapter =
- si2168_config.fe = >fe_adap[0].fe;
- si2168_config.ts_mode = SI2168_TS_PARALLEL;
- si2168_config.ts_clock_inv = 1;
- memset(, 0, sizeof(struct i2c_board_info));
- strlcpy(info.type, "si2168", I2C_NAME_SIZE);
- info.addr = 0x64;
- info.platform_data = _config;
- request_module(info.type);
- client_demod = i2c_new_device(>i2c_adap, );
- if (client_demod == NULL || client_demod->dev.driver == NULL)
- return -ENODEV;
-
- if (!try_module_get(client_demod->dev.driver->owner)) {
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
-
- /* attach tuner */
- memset(_config, 0, sizeof(si2157_config));
- si2157_config.fe = adap->fe_adap[0].fe;
- memset(, 0, sizeof(struct i2c_board_info));
- strlcpy(info.type, "si2141", I2C_NAME_SIZE);
- info.addr = 0x60;
- info.platform_data = _config;
- request_module("si2157");
- client_tuner = i2c_new_device(adapter, );
- if (client_tuner == NULL || client_tuner->dev.driver == NULL) {
- module_put(client_demod->dev.driver->owner);
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
- if (!try_module_get(client_tuner->dev.driver->owner)) {
- i2c_unregister_device(client_tuner);
- module_put(client_demod->dev.driver->owner);
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
-
- st->i2c_client_demod = client_demod;
- st->i2c_client_tuner = client_tuner;
-
- /* hook fe: need to resync the slave fifo when signal locks. */
- mutex_init(>stream_mutex);
- st->last_lock = 0;
- st->fe_read_status = adap->fe_adap[0].fe->ops.read_status;
- adap->fe_adap[0].fe->ops.read_status = cxusb_read_status;
-
- return 0;
-}
-
/*
* DViCO has shipped two devices with the same USB ID, but only one of them
* needs a firmware download. Check the device class details to see if they
@@ -1397,7 +1321,6 @@ static struct dvb_usb_device_properties
cxusb_aver_a868r_properties;
static struct dvb_usb_device_properties cxusb_d680_dmb_properties;
static struct dvb_usb_device_properties cxusb_mygica_d689_properties;
static struct dvb_usb_device_properties cxusb_mygica_t230_properties;
-static struct dvb_usb_device_properties cxusb_mygica_t230c_properties;
static int cxusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
@@ -1430,8 +1353,6 @@ static int cxusb_probe(struct usb_interface *intf,
THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, _mygica_t230_properties,
THIS_MODULE, NULL, adapter_nr) ||
- 0 == dvb_usb_device_init(intf, _mygica_t230c_properties,
-THIS_MODULE, NULL, adapter_nr) ||
0)
return 0;
@@ -1483,7
[PATCH v1 1/2] Revert "[media] dvb-usb-cxusb: Geniatech T230C support"
From: Evgeny Plehov
This reverts commit f8585ce655e9cdeabc38e8e2580b05735110e4a5.
The T230C is handled by the dvb-usb-v2/dvbsky.c driver, which should
be preferred over a dvb-usb (v1) driver.
Signed-off-by: Stefan Brüns
---
drivers/media/usb/dvb-usb/cxusb.c | 139 +-
1 file changed, 1 insertion(+), 138 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index 37dea0adc695..edb7cd2e43d9 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -1239,82 +1239,6 @@ static int cxusb_mygica_t230_frontend_attach(struct
dvb_usb_adapter *adap)
return 0;
}
-static int cxusb_mygica_t230c_frontend_attach(struct dvb_usb_adapter *adap)
-{
- struct dvb_usb_device *d = adap->dev;
- struct cxusb_state *st = d->priv;
- struct i2c_adapter *adapter;
- struct i2c_client *client_demod;
- struct i2c_client *client_tuner;
- struct i2c_board_info info;
- struct si2168_config si2168_config;
- struct si2157_config si2157_config;
-
- /* Select required USB configuration */
- if (usb_set_interface(d->udev, 0, 0) < 0)
- err("set interface failed");
-
- /* Unblock all USB pipes */
- usb_clear_halt(d->udev,
- usb_sndbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint));
- usb_clear_halt(d->udev,
- usb_rcvbulkpipe(d->udev, d->props.generic_bulk_ctrl_endpoint));
- usb_clear_halt(d->udev,
- usb_rcvbulkpipe(d->udev,
d->props.adapter[0].fe[0].stream.endpoint));
-
- /* attach frontend */
- memset(_config, 0, sizeof(si2168_config));
- si2168_config.i2c_adapter =
- si2168_config.fe = >fe_adap[0].fe;
- si2168_config.ts_mode = SI2168_TS_PARALLEL;
- si2168_config.ts_clock_inv = 1;
- memset(, 0, sizeof(struct i2c_board_info));
- strlcpy(info.type, "si2168", I2C_NAME_SIZE);
- info.addr = 0x64;
- info.platform_data = _config;
- request_module(info.type);
- client_demod = i2c_new_device(>i2c_adap, );
- if (client_demod == NULL || client_demod->dev.driver == NULL)
- return -ENODEV;
-
- if (!try_module_get(client_demod->dev.driver->owner)) {
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
-
- /* attach tuner */
- memset(_config, 0, sizeof(si2157_config));
- si2157_config.fe = adap->fe_adap[0].fe;
- memset(, 0, sizeof(struct i2c_board_info));
- strlcpy(info.type, "si2141", I2C_NAME_SIZE);
- info.addr = 0x60;
- info.platform_data = _config;
- request_module("si2157");
- client_tuner = i2c_new_device(adapter, );
- if (client_tuner == NULL || client_tuner->dev.driver == NULL) {
- module_put(client_demod->dev.driver->owner);
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
- if (!try_module_get(client_tuner->dev.driver->owner)) {
- i2c_unregister_device(client_tuner);
- module_put(client_demod->dev.driver->owner);
- i2c_unregister_device(client_demod);
- return -ENODEV;
- }
-
- st->i2c_client_demod = client_demod;
- st->i2c_client_tuner = client_tuner;
-
- /* hook fe: need to resync the slave fifo when signal locks. */
- mutex_init(>stream_mutex);
- st->last_lock = 0;
- st->fe_read_status = adap->fe_adap[0].fe->ops.read_status;
- adap->fe_adap[0].fe->ops.read_status = cxusb_read_status;
-
- return 0;
-}
-
/*
* DViCO has shipped two devices with the same USB ID, but only one of them
* needs a firmware download. Check the device class details to see if they
@@ -1397,7 +1321,6 @@ static struct dvb_usb_device_properties
cxusb_aver_a868r_properties;
static struct dvb_usb_device_properties cxusb_d680_dmb_properties;
static struct dvb_usb_device_properties cxusb_mygica_d689_properties;
static struct dvb_usb_device_properties cxusb_mygica_t230_properties;
-static struct dvb_usb_device_properties cxusb_mygica_t230c_properties;
static int cxusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
@@ -1430,8 +1353,6 @@ static int cxusb_probe(struct usb_interface *intf,
THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, _mygica_t230_properties,
THIS_MODULE, NULL, adapter_nr) ||
- 0 == dvb_usb_device_init(intf, _mygica_t230c_properties,
-THIS_MODULE, NULL, adapter_nr) ||
0)
return 0;
@@ -1483,7 +1404,6 @@ enum cxusb_table_index {
CONEXANT_D680_DMB,
[PATCH v1 2/2] [media] cxusb: restore RC_MAP for MyGica T230
Commit f8585ce655e9cdeabc38e8e2580b05735110e4a5 ("[media] dvb-usb-cxusb:
Geniatech T230C support") sneaked in an unrelated change for the older
T230 (not C) model. As the commit was reverted this change was reverted
too, although likely correct.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/media/usb/dvb-usb/cxusb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index edb7cd2e43d9..75f44b534007 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -2165,7 +2165,7 @@ static struct dvb_usb_device_properties
cxusb_mygica_t230_properties = {
.rc.core = {
.rc_interval= 100,
- .rc_codes = RC_MAP_D680_DMB,
+ .rc_codes = RC_MAP_TOTAL_MEDIA_IN_HAND_02,
.module_name= KBUILD_MODNAME,
.rc_query = cxusb_d680_dmb_rc_query,
.allowed_protos = RC_PROTO_BIT_UNKNOWN,
--
2.15.1
[PATCH v1 2/2] [media] cxusb: restore RC_MAP for MyGica T230
Commit f8585ce655e9cdeabc38e8e2580b05735110e4a5 ("[media] dvb-usb-cxusb:
Geniatech T230C support") sneaked in an unrelated change for the older
T230 (not C) model. As the commit was reverted this change was reverted
too, although likely correct.
Signed-off-by: Stefan Brüns
---
drivers/media/usb/dvb-usb/cxusb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/dvb-usb/cxusb.c
b/drivers/media/usb/dvb-usb/cxusb.c
index edb7cd2e43d9..75f44b534007 100644
--- a/drivers/media/usb/dvb-usb/cxusb.c
+++ b/drivers/media/usb/dvb-usb/cxusb.c
@@ -2165,7 +2165,7 @@ static struct dvb_usb_device_properties
cxusb_mygica_t230_properties = {
.rc.core = {
.rc_interval= 100,
- .rc_codes = RC_MAP_D680_DMB,
+ .rc_codes = RC_MAP_TOTAL_MEDIA_IN_HAND_02,
.module_name= KBUILD_MODNAME,
.rc_query = cxusb_d680_dmb_rc_query,
.allowed_protos = RC_PROTO_BIT_UNKNOWN,
--
2.15.1
Re: [Intel-gfx] [PATCH v2] drm/i915: Try EDID bitbanging on HDMI after failed read
On Wednesday, January 3, 2018 8:14:47 AM CET Jani Nikula wrote:
> On Tue, 02 Jan 2018, Chris Wilson <ch...@chris-wilson.co.uk> wrote:
> > Quoting Rodrigo Vivi (2018-01-02 19:12:18)
> >
> >> On Sun, Dec 31, 2017 at 10:34:54PM +, Stefan Brüns wrote:
> >> > + edid = drm_get_edid(connector, i2c);
> >> > +
> >> > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
> >> > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using
> >> > GPIO bit-banging\n"); + intel_gmbus_force_bit(i2c, true);
> >> > + edid = drm_get_edid(connector, i2c);
> >> > + intel_gmbus_force_bit(i2c, false);
> >> > + }
> >>
> >> Approach seems fine for this case.
> >> I just wonder what would be the risks of forcing this bit and edid read
> >> when nothing is present on the other end?>
> > Should be no more risky than using GMBUS as the bit-banging is the
> > underlying HW protocol; it should just be adding an extra delay to
> > the disconnected probe. Offset against the chance that it fixes
> > detection of borderline devices.
> >
> > I would say that given the explanation above, the question is why not
> > apply it universally? (Bonus points for including the explanation as
> > comments.)
>
> I'm wondering, is gmbus too fast for the adapters, does gmbus generally
> have different timing for the ack/nak as described in the commit message
> than bit banging, or are the adapters just plain buggy? Do we have any
> control over gmbus timings (don't have the time to peruse the bpsec just
> now)?
I have seen two different behaviours, one on the ~2009 GM965, the other on the
~2013 Haswell. The Haswell provides a 250..500ns hold time, the other does
not.
There is a flag in the GMBUS0 register, GMBUS_HOLD_EXT, "300ns hold time, rsvd
on Pineview". The driver does not set this flag. Possibly it is always set/
implied on the Haswell (which is post-Pineview), and should be set for
anything older than Pineview.
There is another odd fact with the GM965, according to the register setting it
should run at 100 kBit/s, but it only runs at 30 kBit/s. The Haswell runs at
100 kBit/s, as specified. As there are also idle periods ever 8 bytes, the
EDID read takes 270ms before it fails.
The bitbanging code, running at 45 kBit/s (2 * 20us per clock cycle plus
overhead) on the other hand just needs 58 ms, but keeps one core busy
(udelay).
Unfortunately I currently have no older system than the Haswell available, so
I can not check if the GMBUS_HOLD_EXT flag has any effect.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [Intel-gfx] [PATCH v2] drm/i915: Try EDID bitbanging on HDMI after failed read
On Wednesday, January 3, 2018 8:14:47 AM CET Jani Nikula wrote:
> On Tue, 02 Jan 2018, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2018-01-02 19:12:18)
> >
> >> On Sun, Dec 31, 2017 at 10:34:54PM +, Stefan Brüns wrote:
> >> > + edid = drm_get_edid(connector, i2c);
> >> > +
> >> > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
> >> > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using
> >> > GPIO bit-banging\n"); + intel_gmbus_force_bit(i2c, true);
> >> > + edid = drm_get_edid(connector, i2c);
> >> > + intel_gmbus_force_bit(i2c, false);
> >> > + }
> >>
> >> Approach seems fine for this case.
> >> I just wonder what would be the risks of forcing this bit and edid read
> >> when nothing is present on the other end?>
> > Should be no more risky than using GMBUS as the bit-banging is the
> > underlying HW protocol; it should just be adding an extra delay to
> > the disconnected probe. Offset against the chance that it fixes
> > detection of borderline devices.
> >
> > I would say that given the explanation above, the question is why not
> > apply it universally? (Bonus points for including the explanation as
> > comments.)
>
> I'm wondering, is gmbus too fast for the adapters, does gmbus generally
> have different timing for the ack/nak as described in the commit message
> than bit banging, or are the adapters just plain buggy? Do we have any
> control over gmbus timings (don't have the time to peruse the bpsec just
> now)?
I have seen two different behaviours, one on the ~2009 GM965, the other on the
~2013 Haswell. The Haswell provides a 250..500ns hold time, the other does
not.
There is a flag in the GMBUS0 register, GMBUS_HOLD_EXT, "300ns hold time, rsvd
on Pineview". The driver does not set this flag. Possibly it is always set/
implied on the Haswell (which is post-Pineview), and should be set for
anything older than Pineview.
There is another odd fact with the GM965, according to the register setting it
should run at 100 kBit/s, but it only runs at 30 kBit/s. The Haswell runs at
100 kBit/s, as specified. As there are also idle periods ever 8 bytes, the
EDID read takes 270ms before it fails.
The bitbanging code, running at 45 kBit/s (2 * 20us per clock cycle plus
overhead) on the other hand just needs 58 ms, but keeps one core busy
(udelay).
Unfortunately I currently have no older system than the Haswell available, so
I can not check if the GMBUS_HOLD_EXT flag has any effect.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
[PATCH v4 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v4:
- Typo, div_s64, not s64_div
Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..b55b8bd1427b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now)
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = div_s64(timespec64_to_ns(), 1000);
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v4 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns
---
Changes in v4:
- Typo, div_s64, not s64_div
Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..b55b8bd1427b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now)
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = div_s64(timespec64_to_ns(), 1000);
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v2] drm/i915: Try EDID bitbanging on HDMI after failed read
The ACK/NACK implementation as found in e.g. the G965 has the falling
clock edge and the release of the data line after the ACK for the received
byte happen at the same time.
This is conformant with the I2C specification, which allows a zero hold
time, see footnote [3]: "A device must internally provide a hold time of
at least 300 ns for the SDA signal (with respect to the V IH(min) of the
SCL signal) to bridge the undefined region of the falling edge of SCL."
Some HDMI-to-VGA converters apparently fail to adhere to this requirement
and latch SDA at the falling clock edge, so instead of an ACK
sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the
transfer.
The bitbanging releases the data line for the ACK only 1/4 bit time after
the falling clock edge, so a slave will see the correct value no matter
if it samples at the rising or the falling clock edge or in the center.
Fallback to bitbanging is already done for the CRT connector.
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2:
- Fix/enhance commit message, no code changes
drivers/gpu/drm/i915/intel_hdmi.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 4dea833f9d1b..847cda4c017c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector)
struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
struct edid *edid;
bool connected = false;
+ struct i2c_adapter *i2c;
intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
- edid = drm_get_edid(connector,
- intel_gmbus_get_adapter(dev_priv,
- intel_hdmi->ddc_bus));
+ i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+ edid = drm_get_edid(connector, i2c);
+
+ if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
+ DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO
bit-banging\n");
+ intel_gmbus_force_bit(i2c, true);
+ edid = drm_get_edid(connector, i2c);
+ intel_gmbus_force_bit(i2c, false);
+ }
intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
--
2.15.1
[PATCH v2] drm/i915: Try EDID bitbanging on HDMI after failed read
The ACK/NACK implementation as found in e.g. the G965 has the falling
clock edge and the release of the data line after the ACK for the received
byte happen at the same time.
This is conformant with the I2C specification, which allows a zero hold
time, see footnote [3]: "A device must internally provide a hold time of
at least 300 ns for the SDA signal (with respect to the V IH(min) of the
SCL signal) to bridge the undefined region of the falling edge of SCL."
Some HDMI-to-VGA converters apparently fail to adhere to this requirement
and latch SDA at the falling clock edge, so instead of an ACK
sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the
transfer.
The bitbanging releases the data line for the ACK only 1/4 bit time after
the falling clock edge, so a slave will see the correct value no matter
if it samples at the rising or the falling clock edge or in the center.
Fallback to bitbanging is already done for the CRT connector.
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685
Signed-off-by: Stefan Brüns
---
Changes in v2:
- Fix/enhance commit message, no code changes
drivers/gpu/drm/i915/intel_hdmi.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 4dea833f9d1b..847cda4c017c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector)
struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
struct edid *edid;
bool connected = false;
+ struct i2c_adapter *i2c;
intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
- edid = drm_get_edid(connector,
- intel_gmbus_get_adapter(dev_priv,
- intel_hdmi->ddc_bus));
+ i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+ edid = drm_get_edid(connector, i2c);
+
+ if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
+ DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO
bit-banging\n");
+ intel_gmbus_force_bit(i2c, true);
+ edid = drm_get_edid(connector, i2c);
+ intel_gmbus_force_bit(i2c, false);
+ }
intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
--
2.15.1
[PATCH v3 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..64e9e8e1ad70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now)
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = s64_div(timespec64_to_ns(), 1000);
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v3 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns
---
Changes in v3:
- use s64_div for 64 bit integer division to fix compiles on 32 bit archs
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..64e9e8e1ad70 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now)
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = s64_div(timespec64_to_ns(), 1000);
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v1] drm/i915: Try EDID bitbanging on HDMI after failed read
The ACK/NACK implementation as found in e.g. the G965 has the falling
clock edge and the release of the data line to ACK the received byte
happen at the same time.
Some HDMI-to-VGA converters apparently read the ACK not in the middle of
the clock high phase, but at the rising clock edge, so instead of an ACK
sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the
transfer.
The bitbanging releases the data line for the ACK only 1/4 bit time after
the falling clock edge, so a slave will see the correct value no matter
if is samples at the rising or the falling clock edge or in the center.
Fallback to bitbanging is already done for the CRT connector.
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/gpu/drm/i915/intel_hdmi.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 4dea833f9d1b..847cda4c017c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector)
struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
struct edid *edid;
bool connected = false;
+ struct i2c_adapter *i2c;
intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
- edid = drm_get_edid(connector,
- intel_gmbus_get_adapter(dev_priv,
- intel_hdmi->ddc_bus));
+ i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+ edid = drm_get_edid(connector, i2c);
+
+ if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
+ DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO
bit-banging\n");
+ intel_gmbus_force_bit(i2c, true);
+ edid = drm_get_edid(connector, i2c);
+ intel_gmbus_force_bit(i2c, false);
+ }
intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
--
2.15.1
[PATCH v1] drm/i915: Try EDID bitbanging on HDMI after failed read
The ACK/NACK implementation as found in e.g. the G965 has the falling
clock edge and the release of the data line to ACK the received byte
happen at the same time.
Some HDMI-to-VGA converters apparently read the ACK not in the middle of
the clock high phase, but at the rising clock edge, so instead of an ACK
sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the
transfer.
The bitbanging releases the data line for the ACK only 1/4 bit time after
the falling clock edge, so a slave will see the correct value no matter
if is samples at the rising or the falling clock edge or in the center.
Fallback to bitbanging is already done for the CRT connector.
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685
Signed-off-by: Stefan Brüns
---
drivers/gpu/drm/i915/intel_hdmi.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 4dea833f9d1b..847cda4c017c 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector)
struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
struct edid *edid;
bool connected = false;
+ struct i2c_adapter *i2c;
intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
- edid = drm_get_edid(connector,
- intel_gmbus_get_adapter(dev_priv,
- intel_hdmi->ddc_bus));
+ i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+
+ edid = drm_get_edid(connector, i2c);
+
+ if (!edid && !intel_gmbus_is_forced_bit(i2c)) {
+ DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO
bit-banging\n");
+ intel_gmbus_force_bit(i2c, true);
+ edid = drm_get_edid(connector, i2c);
+ intel_gmbus_force_bit(i2c, false);
+ }
intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
--
2.15.1
[PATCH v2 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.
The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
- int cnvr_need_clear = 0;
time_a = iio_get_time_ns(indio_dev);
@@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
ret = regmap_read(chip->regmap,
INA2XX_BUS_VOLTAGE, );
alert &= INA219_CNVR;
- cnvr_need_clear = alert;
}
if (ret < 0)
@@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
return ret;
data[i++] = val;
-
- if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
- cnvr_need_clear = 0;
- }
-
- /* Dummy read on INA219 power register to clear CNVR flag */
- if (cnvr_need_clear && chip->config->chip_id == ina219) {
- unsigned int val;
-
- ret = regmap_read(chip->regmap, INA2XX_POWER, );
- if (ret < 0)
- return ret;
}
time_b = iio_get_time_ns(indio_dev);
--
2.15.1
[PATCH v2 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.
The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282
Signed-off-by: Stefan Brüns
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
- int cnvr_need_clear = 0;
time_a = iio_get_time_ns(indio_dev);
@@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
ret = regmap_read(chip->regmap,
INA2XX_BUS_VOLTAGE, );
alert &= INA219_CNVR;
- cnvr_need_clear = alert;
}
if (ret < 0)
@@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
return ret;
data[i++] = val;
-
- if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
- cnvr_need_clear = 0;
- }
-
- /* Dummy read on INA219 power register to clear CNVR flag */
- if (cnvr_need_clear && chip->config->chip_id == ina219) {
- unsigned int val;
-
- ret = regmap_read(chip->regmap, INA2XX_POWER, );
- if (ret < 0)
- return ret;
}
time_b = iio_get_time_ns(indio_dev);
--
2.15.1
[PATCH v2 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
Currently, the INA2xx driver may end up causing 100% load on a single core and fully loading the I2C bus, which is caused by two different issues: The code uses a udelay to bridge the gab between two subsequent samples. As the sampling interval may be up to 16 seconds, the CPU is busy waiting most of the time. The second issue manifests when using the (default) "synchronous" mode. The code polls for a set conversion ready flag, but fails to align the sampling interval to the raising flag. The polling interval is (rightfully) slighly shorter than the sampling interval, so after some samples the sampling thread is continously polling. The patch series fixes both issues: Patch 1 and 2 are just some small cosmetic changes. Patch 3 removes an unnecessary read. According to the datasheet, the CNVR flag is only cleared by reading the power register, but is cleared by reading any of the measurement registers, thus the dummy read can be skipped. This behaviour has been confirmed by TI technical support. Patch 4 replaces the udelay with usleep_range. Patch 5 reworks the delay logic. Previously the IIO timestamp clock was used to capture entry and exit times of the work function. The timestamp clock is user selectable and may be non-monotonic. Also, any time spent outside the work function is not accounted for. Patch 6 moves the timestap capture to the end of the conversion ready status poll. Patch 7 addresses the alignment issue. Every time an unset flag is seen on poll loop entry, the reference timestamp is readjusted. Both old and fixed behaviour has been verified using a logic analyzer. In synchrounous mode, every few samples a double read of the status register can be observed, showing the raising status flag, the other samples are evenly spaced at sampling intervals inbetween. Changes in v2: - add a comment mentioning skipping samples on overrun - Describe old behaviour in commit message more clearly No real code changes, but added a comment in patch 5, and clarified commit messages of patch 5 and 7. Stefan Brüns (7): iio: adc: ina2xx: Remove bogus cast for data argument iio: adc: ina2xx: Clarify size requirement for data buffer iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag iio: adc: ina2xx: Do not udelay for several seconds iio: adc: ina2xx: Use a monotonic clock for delay calculation iio: adc: ina2xx: Align timestamp with conversion ready flag iio: adc: ina2xx: Actually align the loop with the conversion ready flag drivers/iio/adc/ina2xx-adc.c | 110 +-- 1 file changed, 65 insertions(+), 45 deletions(-) -- 2.15.1
[PATCH v2 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
Currently, the INA2xx driver may end up causing 100% load on a single core and fully loading the I2C bus, which is caused by two different issues: The code uses a udelay to bridge the gab between two subsequent samples. As the sampling interval may be up to 16 seconds, the CPU is busy waiting most of the time. The second issue manifests when using the (default) "synchronous" mode. The code polls for a set conversion ready flag, but fails to align the sampling interval to the raising flag. The polling interval is (rightfully) slighly shorter than the sampling interval, so after some samples the sampling thread is continously polling. The patch series fixes both issues: Patch 1 and 2 are just some small cosmetic changes. Patch 3 removes an unnecessary read. According to the datasheet, the CNVR flag is only cleared by reading the power register, but is cleared by reading any of the measurement registers, thus the dummy read can be skipped. This behaviour has been confirmed by TI technical support. Patch 4 replaces the udelay with usleep_range. Patch 5 reworks the delay logic. Previously the IIO timestamp clock was used to capture entry and exit times of the work function. The timestamp clock is user selectable and may be non-monotonic. Also, any time spent outside the work function is not accounted for. Patch 6 moves the timestap capture to the end of the conversion ready status poll. Patch 7 addresses the alignment issue. Every time an unset flag is seen on poll loop entry, the reference timestamp is readjusted. Both old and fixed behaviour has been verified using a logic analyzer. In synchrounous mode, every few samples a double read of the status register can be observed, showing the raising status flag, the other samples are evenly spaced at sampling intervals inbetween. Changes in v2: - add a comment mentioning skipping samples on overrun - Describe old behaviour in commit message more clearly No real code changes, but added a comment in patch 5, and clarified commit messages of patch 5 and 7. Stefan Brüns (7): iio: adc: ina2xx: Remove bogus cast for data argument iio: adc: ina2xx: Clarify size requirement for data buffer iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag iio: adc: ina2xx: Do not udelay for several seconds iio: adc: ina2xx: Use a monotonic clock for delay calculation iio: adc: ina2xx: Align timestamp with conversion ready flag iio: adc: ina2xx: Actually align the loop with the conversion ready flag drivers/iio/adc/ina2xx-adc.c | 110 +-- 1 file changed, 65 insertions(+), 45 deletions(-) -- 2.15.1
[PATCH v2 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer
The timestamp is inserted into the buffer after the sample data by
iio_push_to_buffers_with_timestamp, document the space requirement for
the timestamp.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3195f8754c3b..8c8120406f52 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -700,7 +700,8 @@ static const struct iio_chan_spec ina219_channels[] = {
static int ina2xx_work_buffer(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- unsigned short data[8];
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
--
2.15.1
[PATCH v2 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer
The timestamp is inserted into the buffer after the sample data by
iio_push_to_buffers_with_timestamp, document the space requirement for
the timestamp.
Signed-off-by: Stefan Brüns
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3195f8754c3b..8c8120406f52 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -700,7 +700,8 @@ static const struct iio_chan_spec ina219_channels[] = {
static int ina2xx_work_buffer(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- unsigned short data[8];
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
--
2.15.1
[PATCH v2 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..415e53b5c0a6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now).
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = timespec64_to_ns() / 1000;
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v2 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Use a monotonic timestamp to track the time for the next poll iteration.
The timestamp is advanced by the sampling interval each iteration. In case
the conversion overrruns the register readout (i.e. fast sampling combined
with a slow bus), one or multiple samples will be dropped.
Signed-off-by: Stefan Brüns
---
Changes in v2:
- add a comment mentioning skipping samples on overrun
drivers/iio/adc/ina2xx-adc.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..415e53b5c0a6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,28 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ /*
+* Advance the timestamp for the next poll by one sampling
+* interval, and sleep for the remainder (next - now).
+* In case "next" has already passed, the interval is added
+* multiple times, i.e. samples are dropped.
+*/
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = timespec64_to_ns() / 1000;
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v2 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag
As the timestamp is no longer (ab-)used to measure the function run time, it can be taken at the correct time, i.e. when the conversion has finished. Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> --- Changes in v2: None drivers/iio/adc/ina2xx-adc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 415e53b5c0a6..b7407ac91b59 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) s64 time; unsigned int alert; - time = iio_get_time_ns(indio_dev); - /* * Because the timer thread and the chip conversion clock * are asynchronous, the period difference will eventually @@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) } while (!alert); + time = iio_get_time_ns(indio_dev); + /* * Single register reads: bulk_read will not work with ina226/219 * as there is no auto-increment of the register pointer. -- 2.15.1
[PATCH v2 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag
As the timestamp is no longer (ab-)used to measure the function run time, it can be taken at the correct time, i.e. when the conversion has finished. Signed-off-by: Stefan Brüns --- Changes in v2: None drivers/iio/adc/ina2xx-adc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 415e53b5c0a6..b7407ac91b59 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) s64 time; unsigned int alert; - time = iio_get_time_ns(indio_dev); - /* * Because the timer thread and the chip conversion clock * are asynchronous, the period difference will eventually @@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) } while (!alert); + time = iio_get_time_ns(indio_dev); + /* * Single register reads: bulk_read will not work with ina226/219 * as there is no auto-increment of the register pointer. -- 2.15.1
[PATCH v2 4/7] iio: adc: ina2xx: Do not udelay for several seconds
The conversion time can be up to 16 seconds (8 ms per channel, 2 channels,
1024 times averaging).
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b027d485398b..2621a34ee5c6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -764,7 +764,7 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us;
+ int buffer_us, delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -778,8 +778,10 @@ static int ina2xx_capture_thread(void *data)
if (buffer_us < 0)
return buffer_us;
- if (sampling_us > buffer_us)
- udelay(sampling_us - buffer_us);
+ if (sampling_us > buffer_us) {
+ delay_us = sampling_us - buffer_us;
+ usleep_range(delay_us, (delay_us * 3) >> 1);
+ }
} while (!kthread_should_stop());
--
2.15.1
[PATCH v2 4/7] iio: adc: ina2xx: Do not udelay for several seconds
The conversion time can be up to 16 seconds (8 ms per channel, 2 channels,
1024 times averaging).
Signed-off-by: Stefan Brüns
---
Changes in v2: None
drivers/iio/adc/ina2xx-adc.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b027d485398b..2621a34ee5c6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -764,7 +764,7 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us;
+ int buffer_us, delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -778,8 +778,10 @@ static int ina2xx_capture_thread(void *data)
if (buffer_us < 0)
return buffer_us;
- if (sampling_us > buffer_us)
- udelay(sampling_us - buffer_us);
+ if (sampling_us > buffer_us) {
+ delay_us = sampling_us - buffer_us;
+ usleep_range(delay_us, (delay_us * 3) >> 1);
+ }
} while (!kthread_should_stop());
--
2.15.1
[PATCH v2 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag
Currently, the registers are read out once per conversion interval. If
the reading is delayed as the conversion has not yet finished, this extra
time is treated as being part of the readout, although it should delay
the start of the poll interval. This results in the interval starting
slightly earlier in each iteration, until all time between reads is
spent polling the status registers instead of sleeping.
To fix this, the delay has to account for the state of the conversion
ready flag. Whenever the conversion is already finished, schedule the next
read on the regular interval, otherwise schedule it one interval after the
flag bit has been set.
Split the work function in two functions, one for the status poll and one
for reading the values, to be able to note down the time when the flag
bit is raised.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
Changes in v2:
- Describe old behaviour in commit message more clearly
drivers/iio/adc/ina2xx-adc.c | 57 +---
1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b7407ac91b59..80af0d2322a3 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(4),
};
-static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- /* data buffer needs space for channel data and timestap */
- unsigned short data[4 + sizeof(s64)/sizeof(short)];
- int bit, ret, i = 0;
- s64 time;
+ int ret;
unsigned int alert;
/*
@@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
* For now, we do an extra read of the MASK_ENABLE register (INA226)
* resp. the BUS_VOLTAGE register (INA219).
*/
- if (!chip->allow_async_readout)
- do {
- if (chip->config->chip_id == ina226) {
- ret = regmap_read(chip->regmap,
- INA226_MASK_ENABLE, );
- alert &= INA226_CVRF;
- } else {
- ret = regmap_read(chip->regmap,
- INA2XX_BUS_VOLTAGE, );
- alert &= INA219_CNVR;
- }
+ if (chip->config->chip_id == ina226) {
+ ret = regmap_read(chip->regmap,
+ INA226_MASK_ENABLE, );
+ alert &= INA226_CVRF;
+ } else {
+ ret = regmap_read(chip->regmap,
+ INA2XX_BUS_VOLTAGE, );
+ alert &= INA219_CNVR;
+ }
- if (ret < 0)
- return ret;
+ if (ret < 0)
+ return ret;
- } while (!alert);
+ return !!alert;
+}
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+ struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
+ int bit, ret, i = 0;
+ s64 time;
time = iio_get_time_ns(indio_dev);
@@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
ktime_get_ts64();
do {
+ while (!chip->allow_async_readout) {
+ ret = ina2xx_conversion_ready(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ /*
+* If the conversion was not yet finished,
+* reset the reference timestamp.
+*/
+ if (ret == 0)
+ ktime_get_ts64();
+ else
+ break;
+ }
+
ret = ina2xx_work_buffer(indio_dev);
if (ret < 0)
return ret;
--
2.15.1
[PATCH v2 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag
Currently, the registers are read out once per conversion interval. If
the reading is delayed as the conversion has not yet finished, this extra
time is treated as being part of the readout, although it should delay
the start of the poll interval. This results in the interval starting
slightly earlier in each iteration, until all time between reads is
spent polling the status registers instead of sleeping.
To fix this, the delay has to account for the state of the conversion
ready flag. Whenever the conversion is already finished, schedule the next
read on the regular interval, otherwise schedule it one interval after the
flag bit has been set.
Split the work function in two functions, one for the status poll and one
for reading the values, to be able to note down the time when the flag
bit is raised.
Signed-off-by: Stefan Brüns
---
Changes in v2:
- Describe old behaviour in commit message more clearly
drivers/iio/adc/ina2xx-adc.c | 57 +---
1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b7407ac91b59..80af0d2322a3 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(4),
};
-static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- /* data buffer needs space for channel data and timestap */
- unsigned short data[4 + sizeof(s64)/sizeof(short)];
- int bit, ret, i = 0;
- s64 time;
+ int ret;
unsigned int alert;
/*
@@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
* For now, we do an extra read of the MASK_ENABLE register (INA226)
* resp. the BUS_VOLTAGE register (INA219).
*/
- if (!chip->allow_async_readout)
- do {
- if (chip->config->chip_id == ina226) {
- ret = regmap_read(chip->regmap,
- INA226_MASK_ENABLE, );
- alert &= INA226_CVRF;
- } else {
- ret = regmap_read(chip->regmap,
- INA2XX_BUS_VOLTAGE, );
- alert &= INA219_CNVR;
- }
+ if (chip->config->chip_id == ina226) {
+ ret = regmap_read(chip->regmap,
+ INA226_MASK_ENABLE, );
+ alert &= INA226_CVRF;
+ } else {
+ ret = regmap_read(chip->regmap,
+ INA2XX_BUS_VOLTAGE, );
+ alert &= INA219_CNVR;
+ }
- if (ret < 0)
- return ret;
+ if (ret < 0)
+ return ret;
- } while (!alert);
+ return !!alert;
+}
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+ struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
+ int bit, ret, i = 0;
+ s64 time;
time = iio_get_time_ns(indio_dev);
@@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
ktime_get_ts64();
do {
+ while (!chip->allow_async_readout) {
+ ret = ina2xx_conversion_ready(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ /*
+* If the conversion was not yet finished,
+* reset the reference timestamp.
+*/
+ if (ret == 0)
+ ktime_get_ts64();
+ else
+ break;
+ }
+
ret = ina2xx_work_buffer(indio_dev);
if (ret < 0)
return ret;
--
2.15.1
[PATCH v2 1/7] iio: adc: ina2xx: Remove bogus cast for data argument
iio_push_to_buffers_with_timestamp expects a void pointer, so the cast is both unnecessary and misleading. Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> --- Changes in v2: None drivers/iio/adc/ina2xx-adc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index ddf878163bf9..3195f8754c3b 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) time_b = iio_get_time_ns(indio_dev); - iio_push_to_buffers_with_timestamp(indio_dev, - (unsigned int *)data, time_a); + iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); return (unsigned long)(time_b - time_a) / 1000; }; -- 2.15.1
[PATCH v2 1/7] iio: adc: ina2xx: Remove bogus cast for data argument
iio_push_to_buffers_with_timestamp expects a void pointer, so the cast is both unnecessary and misleading. Signed-off-by: Stefan Brüns --- Changes in v2: None drivers/iio/adc/ina2xx-adc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index ddf878163bf9..3195f8754c3b 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) time_b = iio_get_time_ns(indio_dev); - iio_push_to_buffers_with_timestamp(indio_dev, - (unsigned int *)data, time_a); + iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); return (unsigned long)(time_b - time_a) / 1000; }; -- 2.15.1
Re: [PATCH v4] iio: adc: ina2xx: Make calibration register value fixed
On Monday, December 18, 2017 9:43:58 AM CET Maciej Purski wrote:
> Calibration register is used for calculating current register in
> hardware according to datasheet:
> current = shunt_volt * calib_register / 2048 (ina 226)
> current = shunt_volt * calib_register / 4096 (ina 219)
>
> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in
> order to avoid truncation error and provide best precision allowed
> by shunt_voltage measurement. Make current scale value follow changes
> of shunt_resistor from sysfs as calib_register value is now fixed.
>
> Power_lsb value should also follow shunt_resistor changes as stated in
> datasheet:
> power_lsb = 25 * current_lsb (ina 226)
> power_lsb = 20 * current_lsb (ina 219)
>
> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394
>
> Signed-off-by: Maciej Purski <m.pur...@samsung.com>
Reviewed-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> ---
> Changes in v4:
> - fix comments
>
> Changes in v3:
> - remove variable current_lsb and calculate it on each read of current
> and power scale value
> - update comments
>
> Changes in v2:
> - rebase on top of the latest next
> - remove a redundant variable - power_lsb_uW
> - fix comments
> ---
> drivers/iio/adc/ina2xx-adc.c | 64
> +++- 1 file changed, 33
> insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index ddf8781..9e8ca12 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -124,11 +124,12 @@ enum ina2xx_ids { ina219, ina226 };
>
> struct ina2xx_config {
> u16 config_default;
> - int calibration_factor;
> + int calibration_value;
> int shunt_voltage_lsb; /* nV */
> int bus_voltage_shift; /* position of lsb */
> int bus_voltage_lsb;/* uV */
> - int power_lsb; /* uW */
> + /* fixed relation between current and power lsb, uW/uA */
> + int power_lsb_factor;
> enum ina2xx_ids chip_id;
> };
>
> @@ -149,20 +150,20 @@ struct ina2xx_chip_info {
> static const struct ina2xx_config ina2xx_config[] = {
> [ina219] = {
> .config_default = INA219_CONFIG_DEFAULT,
> - .calibration_factor = 4096,
> + .calibration_value = 4096,
> .shunt_voltage_lsb = 1,
> .bus_voltage_shift = INA219_BUS_VOLTAGE_SHIFT,
> .bus_voltage_lsb = 4000,
> - .power_lsb = 2,
> + .power_lsb_factor = 20,
> .chip_id = ina219,
> },
> [ina226] = {
> .config_default = INA226_CONFIG_DEFAULT,
> - .calibration_factor = 512,
> + .calibration_value = 2048,
> .shunt_voltage_lsb = 2500,
> .bus_voltage_shift = 0,
> .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> + .power_lsb_factor = 25,
> .chip_id = ina226,
> },
> };
> @@ -227,16 +228,26 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val2 = 1000;
> return IIO_VAL_FRACTIONAL;
>
> - case INA2XX_POWER:
> - /* processed (mW) = raw*lsb (uW) / 1000 */
> - *val = chip->config->power_lsb;
> - *val2 = 1000;
> + case INA2XX_CURRENT:
> + /*
> + * processed (mA) = raw * current_lsb (mA)
> + * current_lsb (mA) = shunt_voltage_lsb (nV) /
> + *shunt_resistor (uOhm)
> + */
> + *val = chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
> return IIO_VAL_FRACTIONAL;
>
> - case INA2XX_CURRENT:
> - /* processed (mA) = raw (mA) */
> - *val = 1;
> - return IIO_VAL_INT;
> + case INA2XX_POWER:
> + /*
> + * processed (mW) = raw * power_lsb (mW)
> + * power_lsb (mW) = power_lsb_factor (mW/mA) *
> + * current_lsb (mA)
> + */
> + *val = chip->config->power_lsb_factor *
> +chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
> + return IIO_VAL_FRACTIONAL;
> }
>
> case IIO_CHAN_INFO_HARDW
Re: [PATCH v4] iio: adc: ina2xx: Make calibration register value fixed
On Monday, December 18, 2017 9:43:58 AM CET Maciej Purski wrote:
> Calibration register is used for calculating current register in
> hardware according to datasheet:
> current = shunt_volt * calib_register / 2048 (ina 226)
> current = shunt_volt * calib_register / 4096 (ina 219)
>
> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in
> order to avoid truncation error and provide best precision allowed
> by shunt_voltage measurement. Make current scale value follow changes
> of shunt_resistor from sysfs as calib_register value is now fixed.
>
> Power_lsb value should also follow shunt_resistor changes as stated in
> datasheet:
> power_lsb = 25 * current_lsb (ina 226)
> power_lsb = 20 * current_lsb (ina 219)
>
> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394
>
> Signed-off-by: Maciej Purski
Reviewed-by: Stefan Brüns
> ---
> Changes in v4:
> - fix comments
>
> Changes in v3:
> - remove variable current_lsb and calculate it on each read of current
> and power scale value
> - update comments
>
> Changes in v2:
> - rebase on top of the latest next
> - remove a redundant variable - power_lsb_uW
> - fix comments
> ---
> drivers/iio/adc/ina2xx-adc.c | 64
> +++- 1 file changed, 33
> insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index ddf8781..9e8ca12 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -124,11 +124,12 @@ enum ina2xx_ids { ina219, ina226 };
>
> struct ina2xx_config {
> u16 config_default;
> - int calibration_factor;
> + int calibration_value;
> int shunt_voltage_lsb; /* nV */
> int bus_voltage_shift; /* position of lsb */
> int bus_voltage_lsb;/* uV */
> - int power_lsb; /* uW */
> + /* fixed relation between current and power lsb, uW/uA */
> + int power_lsb_factor;
> enum ina2xx_ids chip_id;
> };
>
> @@ -149,20 +150,20 @@ struct ina2xx_chip_info {
> static const struct ina2xx_config ina2xx_config[] = {
> [ina219] = {
> .config_default = INA219_CONFIG_DEFAULT,
> - .calibration_factor = 4096,
> + .calibration_value = 4096,
> .shunt_voltage_lsb = 1,
> .bus_voltage_shift = INA219_BUS_VOLTAGE_SHIFT,
> .bus_voltage_lsb = 4000,
> - .power_lsb = 2,
> + .power_lsb_factor = 20,
> .chip_id = ina219,
> },
> [ina226] = {
> .config_default = INA226_CONFIG_DEFAULT,
> - .calibration_factor = 512,
> + .calibration_value = 2048,
> .shunt_voltage_lsb = 2500,
> .bus_voltage_shift = 0,
> .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> + .power_lsb_factor = 25,
> .chip_id = ina226,
> },
> };
> @@ -227,16 +228,26 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> *val2 = 1000;
> return IIO_VAL_FRACTIONAL;
>
> - case INA2XX_POWER:
> - /* processed (mW) = raw*lsb (uW) / 1000 */
> - *val = chip->config->power_lsb;
> - *val2 = 1000;
> + case INA2XX_CURRENT:
> + /*
> + * processed (mA) = raw * current_lsb (mA)
> + * current_lsb (mA) = shunt_voltage_lsb (nV) /
> + *shunt_resistor (uOhm)
> + */
> + *val = chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
> return IIO_VAL_FRACTIONAL;
>
> - case INA2XX_CURRENT:
> - /* processed (mA) = raw (mA) */
> - *val = 1;
> - return IIO_VAL_INT;
> + case INA2XX_POWER:
> + /*
> + * processed (mW) = raw * power_lsb (mW)
> + * power_lsb (mW) = power_lsb_factor (mW/mA) *
> + * current_lsb (mA)
> + */
> + *val = chip->config->power_lsb_factor *
> +chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
> + return IIO_VAL_FRACTIONAL;
> }
>
> case IIO_CHAN_INFO_HARDWAREGAIN:
> @@ -541,25 +552,21 @@ static ssize_t ina2xx_allow_async_re
Re: [PATCH v3] iio: adc: ina2xx: Make calibration register value fixed
2xx_allow_async_readout_store(struct
> device *dev, }
>
> /*
> - * Set current LSB to 1mA, shunt is in uOhms
> - * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-3. The only remaining parameter is RShunt.
> - * There is no need to expose the CALIBRATION register
> - * to the user for now. But we need to reset this register
> - * if the user updates RShunt after driver init, e.g upon
> - * reading an EEPROM/Probe-type value.
> + * Calibration register is set to the best value, which eliminates
> + * truncation errors on calculating current register in hardware.
> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best values
> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as
> + * calibration_value.
> */
> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> {
> - u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -chip->shunt_resistor_uohm);
> -
> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> + return regmap_write(chip->regmap, INA2XX_CALIBRATION,
> + chip->config->calibration_value);
> }
>
> +/*
> + * As the calibration register is fixed, the product of current_lsb
> + * and shunt_resistor should also be fixed and equal
> + * to shunt_voltage_lsb. Current_lsb will be calculated in read_raw()
> + */
I think this comment is misplaced here. Neither current_lsb nor
shunt_voltage_lsb are used in the remaining code, the resistor value is just
stored.
> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int
> val) {
> - if (val <= 0 || val > chip->config->calibration_factor)
> + if (val == 0 || val > INT_MAX)
> return -EINVAL;
>
> chip->shunt_resistor_uohm = val;
> @@ -592,11 +595,6 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> device *dev, if (ret)
> return ret;
>
> - /* Update the Calibration register */
> - ret = ina2xx_set_calibration(chip);
> - if (ret)
> - return ret;
> -
> return len;
> }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v3] iio: adc: ina2xx: Make calibration register value fixed
out_store(struct
> device *dev, }
>
> /*
> - * Set current LSB to 1mA, shunt is in uOhms
> - * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-3. The only remaining parameter is RShunt.
> - * There is no need to expose the CALIBRATION register
> - * to the user for now. But we need to reset this register
> - * if the user updates RShunt after driver init, e.g upon
> - * reading an EEPROM/Probe-type value.
> + * Calibration register is set to the best value, which eliminates
> + * truncation errors on calculating current register in hardware.
> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best values
> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as
> + * calibration_value.
> */
> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> {
> - u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -chip->shunt_resistor_uohm);
> -
> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> + return regmap_write(chip->regmap, INA2XX_CALIBRATION,
> + chip->config->calibration_value);
> }
>
> +/*
> + * As the calibration register is fixed, the product of current_lsb
> + * and shunt_resistor should also be fixed and equal
> + * to shunt_voltage_lsb. Current_lsb will be calculated in read_raw()
> + */
I think this comment is misplaced here. Neither current_lsb nor
shunt_voltage_lsb are used in the remaining code, the resistor value is just
stored.
> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int
> val) {
> - if (val <= 0 || val > chip->config->calibration_factor)
> + if (val == 0 || val > INT_MAX)
> return -EINVAL;
>
> chip->shunt_resistor_uohm = val;
> @@ -592,11 +595,6 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> device *dev, if (ret)
> return ret;
>
> - /* Update the Calibration register */
> - ret = ina2xx_set_calibration(chip);
> - if (ret)
> - return ret;
> -
> return len;
> }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote: > On Sun, 10 Dec 2017 21:53:42 +0100 > > Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote: > > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote: > > > On Fri, 8 Dec 2017 18:41:48 +0100 > > > > > > Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote: > > > > Although the datasheet states the CNVR flag is cleared by reading the > > > > BUS_VOLTAGE register, it is actually cleared by reading any of the > > > > voltage/current/power registers. > > > > > > > > The behaviour has been confirmed by TI support: > > > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/64 > > > > 7053 > > > > /2378282 > > > > > > > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> > > > > > > I haven't checked the code thoroughly so there may well be something > > > stopping it but have you checked the case where the only channel enabled > > > is > > > the timestamp? > > > > > > Obviously it makes little sense, but IIRC there is nothing in the core > > > preventing that happening. > > > > The timestamp is completely unrelated to the status register, so I fail to > > understand your question. Can you please clarify? > > If you only have a timestamp, the trigger will still fire (I think) > but you'll do no reading at all from the device. If configured in this, > admittedly odd, way you should just get a stream of timestamps with no > data. If there are reads depends on the mode - if running asynchronously, it will just stream out 64 bits of timestamp each interval. In synchronous mode, the driver will read the status register (low bits of bus voltage register for INA219, msk register for INA226), which implicitly clears the CNVR flag. > > This only removes a redundant read. > > The question is whether it is redundant if we have no non timestamp > registers enabled. According to the documentation, INA219 and 226 had to be treated differently. As it turned out, both actually behave the same way regarding the CNVR flag, so we just poll the status register, which for both devices clears the flag. Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
On Tuesday, December 12, 2017 9:15:30 PM CET Jonathan Cameron wrote: > On Sun, 10 Dec 2017 21:53:42 +0100 > > Stefan Brüns wrote: > > On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote: > > > On Fri, 8 Dec 2017 18:41:48 +0100 > > > > > > Stefan Brüns wrote: > > > > Although the datasheet states the CNVR flag is cleared by reading the > > > > BUS_VOLTAGE register, it is actually cleared by reading any of the > > > > voltage/current/power registers. > > > > > > > > The behaviour has been confirmed by TI support: > > > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/64 > > > > 7053 > > > > /2378282 > > > > > > > > Signed-off-by: Stefan Brüns > > > > > > I haven't checked the code thoroughly so there may well be something > > > stopping it but have you checked the case where the only channel enabled > > > is > > > the timestamp? > > > > > > Obviously it makes little sense, but IIRC there is nothing in the core > > > preventing that happening. > > > > The timestamp is completely unrelated to the status register, so I fail to > > understand your question. Can you please clarify? > > If you only have a timestamp, the trigger will still fire (I think) > but you'll do no reading at all from the device. If configured in this, > admittedly odd, way you should just get a stream of timestamps with no > data. If there are reads depends on the mode - if running asynchronously, it will just stream out 64 bits of timestamp each interval. In synchronous mode, the driver will read the status register (low bits of bus voltage register for INA219, msk register for INA226), which implicitly clears the CNVR flag. > > This only removes a redundant read. > > The question is whether it is redundant if we have no non timestamp > registers enabled. According to the documentation, INA219 and 226 had to be treated differently. As it turned out, both actually behave the same way regarding the CNVR flag, so we just poll the status register, which for both devices clears the flag. Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH] platform/x86: intel-vbtn: Simplify autorelease logic
On Saturday, December 9, 2017 12:07:08 AM CET Darren Hart (VMware) wrote: > The new notify_handler logic determining if autorelease should be used or > not is a bit awkward, and can result in more than one call to > sparse_keymap_report_event for the same event (scancode). The nesting > and long lines also made it difficult to read. > > Simplify the logic by eliminating a level of nesting with a goto and > always calculate autorelease and val so we can make a single call to > sparse_keymap_report_event. > > Signed-off-by: Darren Hart (VMware) <dvh...@infradead.org> > Cc: Stefan Brüns <stefan.bru...@rwth-aachen.de> > Cc: AceLan Kao <acelan@canonical.com> > --- > Note: This is based on top of Stefan's v2 patch series for intel-vbtn, > currently in my review-dvhart branch. > > drivers/platform/x86/intel-vbtn.c | 25 +---- > 1 file changed, 13 insertions(+), 12 deletions(-) Reviewed-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> Tested-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH] platform/x86: intel-vbtn: Simplify autorelease logic
On Saturday, December 9, 2017 12:07:08 AM CET Darren Hart (VMware) wrote: > The new notify_handler logic determining if autorelease should be used or > not is a bit awkward, and can result in more than one call to > sparse_keymap_report_event for the same event (scancode). The nesting > and long lines also made it difficult to read. > > Simplify the logic by eliminating a level of nesting with a goto and > always calculate autorelease and val so we can make a single call to > sparse_keymap_report_event. > > Signed-off-by: Darren Hart (VMware) > Cc: Stefan Brüns > Cc: AceLan Kao > --- > Note: This is based on top of Stefan's v2 patch series for intel-vbtn, > currently in my review-dvhart branch. > > drivers/platform/x86/intel-vbtn.c | 25 + > 1 file changed, 13 insertions(+), 12 deletions(-) Reviewed-by: Stefan Brüns Tested-by: Stefan Brüns Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:48 +0100 > > Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote: > > Although the datasheet states the CNVR flag is cleared by reading the > > BUS_VOLTAGE register, it is actually cleared by reading any of the > > voltage/current/power registers. > > > > The behaviour has been confirmed by TI support: > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053 > > /2378282 > > > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> > > I haven't checked the code thoroughly so there may well be something > stopping it but have you checked the case where the only channel enabled is > the timestamp? > > Obviously it makes little sense, but IIRC there is nothing in the core > preventing that happening. The timestamp is completely unrelated to the status register, so I fail to understand your question. Can you please clarify? This only removes a redundant read. All channel combinations (w/ and w/o timestamp) work, but combinations not including the power register use less bus time now. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
On Sunday, December 10, 2017 6:27:33 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:48 +0100 > > Stefan Brüns wrote: > > Although the datasheet states the CNVR flag is cleared by reading the > > BUS_VOLTAGE register, it is actually cleared by reading any of the > > voltage/current/power registers. > > > > The behaviour has been confirmed by TI support: > > http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053 > > /2378282 > > > > Signed-off-by: Stefan Brüns > > I haven't checked the code thoroughly so there may well be something > stopping it but have you checked the case where the only channel enabled is > the timestamp? > > Obviously it makes little sense, but IIRC there is nothing in the core > preventing that happening. The timestamp is completely unrelated to the status register, so I fail to understand your question. Can you please clarify? This only removes a redundant read. All channel combinations (w/ and w/o timestamp) work, but combinations not including the power register use less bus time now. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:50 +0100
>
> Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote:
> > The iio timestamp clock is user selectable and may be non-monotonic. Also,
> > only part of the acquisition time is measured, thus the delay was longer
> > than intended.
> >
> > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > ---
> >
> > drivers/iio/adc/ina2xx-adc.c | 35 +--
> > 1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 2621a34ee5c6..65bd9e69faf2 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev
> > *indio_dev)>
> > /* data buffer needs space for channel data and timestap */
> > unsigned short data[4 + sizeof(s64)/sizeof(short)];
> > int bit, ret, i = 0;
> >
> > - s64 time_a, time_b;
> > + s64 time;
> >
> > unsigned int alert;
> >
> > - time_a = iio_get_time_ns(indio_dev);
> > + time = iio_get_time_ns(indio_dev);
> >
> > /*
> >
> > * Because the timer thread and the chip conversion clock
> >
> > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev
> > *indio_dev)>
> > data[i++] = val;
> >
> > }
> >
> > - time_b = iio_get_time_ns(indio_dev);
> > + iio_push_to_buffers_with_timestamp(indio_dev, data, time);
> >
> > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
> > -
> > - return (unsigned long)(time_b - time_a) / 1000;
> > + return 0;
> >
> > };
> >
> > static int ina2xx_capture_thread(void *data)
> >
> > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
> >
> > struct iio_dev *indio_dev = data;
> > struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> > int sampling_us = SAMPLING_PERIOD(chip);
> >
> > - int buffer_us, delay_us;
> > + int ret;
> > + struct timespec64 next, now, delta;
> > + s64 delay_us;
> >
> > /*
> >
> > * Poll a bit faster than the chip internal Fs, in case
> >
> > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data)
> >
> > if (!chip->allow_async_readout)
> >
> > sampling_us -= 200;
> >
> > + ktime_get_ts64();
> > +
> >
> > do {
> >
> > - buffer_us = ina2xx_work_buffer(indio_dev);
> > - if (buffer_us < 0)
> > - return buffer_us;
> > + ret = ina2xx_work_buffer(indio_dev);
> > + if (ret < 0)
> > + return ret;
> >
> > - if (sampling_us > buffer_us) {
> > - delay_us = sampling_us - buffer_us;
> > - usleep_range(delay_us, (delay_us * 3) >> 1);
> > - }
> > + ktime_get_ts64();
> > +
> > + do {
> > + timespec64_add_ns(, 1000 * sampling_us);
> > + delta = timespec64_sub(next, now);
> > + delay_us = timespec64_to_ns() / 1000;
> > + } while (delay_us <= 0);
>
> Umm. I'm lost, what is the purpose of the above dance?
> A comment perhaps.
next is the timestamp for the next read to happen, now is the current time.
Obviously we have to sleep for the remainder.
Each sampling interval the "next" timestamp is pushed back by sampling_us.
Normally this happens exactly once per read, i.e. we schedule the reads to
happen exactly each sampling interval.
The sampling inteval is *only* added multiple times if it is faster than the
bus can deliver the data (at 100 kBits/s, each register read takes about 400
us, so sampling faster than every ~1 ms is not possible.
The old code measured the time spent for reading the registers and slept for
the remainder of the interval. This way the sampling drifts, as there is some
time not accounted for - usleep_range, function call overhead, kthread
interrupted.
Using a timestamp avoids the drift. It also allows simple readjustment of the
"next" sampling time when polling the status register.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:50 +0100
>
> Stefan Brüns wrote:
> > The iio timestamp clock is user selectable and may be non-monotonic. Also,
> > only part of the acquisition time is measured, thus the delay was longer
> > than intended.
> >
> > Signed-off-by: Stefan Brüns
> > ---
> >
> > drivers/iio/adc/ina2xx-adc.c | 35 +--
> > 1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> > index 2621a34ee5c6..65bd9e69faf2 100644
> > --- a/drivers/iio/adc/ina2xx-adc.c
> > +++ b/drivers/iio/adc/ina2xx-adc.c
> > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev
> > *indio_dev)>
> > /* data buffer needs space for channel data and timestap */
> > unsigned short data[4 + sizeof(s64)/sizeof(short)];
> > int bit, ret, i = 0;
> >
> > - s64 time_a, time_b;
> > + s64 time;
> >
> > unsigned int alert;
> >
> > - time_a = iio_get_time_ns(indio_dev);
> > + time = iio_get_time_ns(indio_dev);
> >
> > /*
> >
> > * Because the timer thread and the chip conversion clock
> >
> > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev
> > *indio_dev)>
> > data[i++] = val;
> >
> > }
> >
> > - time_b = iio_get_time_ns(indio_dev);
> > + iio_push_to_buffers_with_timestamp(indio_dev, data, time);
> >
> > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
> > -
> > - return (unsigned long)(time_b - time_a) / 1000;
> > + return 0;
> >
> > };
> >
> > static int ina2xx_capture_thread(void *data)
> >
> > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
> >
> > struct iio_dev *indio_dev = data;
> > struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> > int sampling_us = SAMPLING_PERIOD(chip);
> >
> > - int buffer_us, delay_us;
> > + int ret;
> > + struct timespec64 next, now, delta;
> > + s64 delay_us;
> >
> > /*
> >
> > * Poll a bit faster than the chip internal Fs, in case
> >
> > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data)
> >
> > if (!chip->allow_async_readout)
> >
> > sampling_us -= 200;
> >
> > + ktime_get_ts64();
> > +
> >
> > do {
> >
> > - buffer_us = ina2xx_work_buffer(indio_dev);
> > - if (buffer_us < 0)
> > - return buffer_us;
> > + ret = ina2xx_work_buffer(indio_dev);
> > + if (ret < 0)
> > + return ret;
> >
> > - if (sampling_us > buffer_us) {
> > - delay_us = sampling_us - buffer_us;
> > - usleep_range(delay_us, (delay_us * 3) >> 1);
> > - }
> > + ktime_get_ts64();
> > +
> > + do {
> > + timespec64_add_ns(, 1000 * sampling_us);
> > + delta = timespec64_sub(next, now);
> > + delay_us = timespec64_to_ns() / 1000;
> > + } while (delay_us <= 0);
>
> Umm. I'm lost, what is the purpose of the above dance?
> A comment perhaps.
next is the timestamp for the next read to happen, now is the current time.
Obviously we have to sleep for the remainder.
Each sampling interval the "next" timestamp is pushed back by sampling_us.
Normally this happens exactly once per read, i.e. we schedule the reads to
happen exactly each sampling interval.
The sampling inteval is *only* added multiple times if it is faster than the
bus can deliver the data (at 100 kBits/s, each register read takes about 400
us, so sampling faster than every ~1 ms is not possible.
The old code measured the time spent for reading the registers and slept for
the remainder of the interval. This way the sampling drifts, as there is some
time not accounted for - usleep_range, function call overhead, kthread
interrupted.
Using a timestamp avoids the drift. It also allows simple readjustment of the
"next" sampling time when polling the status register.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
On Sunday, December 10, 2017 6:36:54 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:45 +0100 > > Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote: > > Currently, the INA2xx driver may end up causing 100% load on a single core > > and fully loading the I2C bus, which is caused by two different issues: > > > > The code uses a udelay to bridge the gab between two subsequent samples. > > As the sampling interval may be up to 16 seconds, the CPU is busy > > waiting most of the time. > > > > The second issue manifests when using the (default) "synchronous" mode. > > The code polls for a set conversion ready flag, but fails to align the > > sampling interval to the raising flag. The polling interval is > > (rightfully) slighly shorter than the sampling interval, so after some > > samples the sampling thread is continously polling. > > I'm confused. Would you mind doing an asci art example perhaps? Lets assume the conversion interval is set to 2 ms. If the polling is done at the sampling frequency, it might be slightly to long due to differences between the host and device clocks, so the polling has to run somewhat faster. Somewhat faster means 200 us, this is kept unchanged. In case the CNVR flag is not set, the status register is read again until the flag raises. The time instant the flag raises is the reference for later reads. The following shows the timing for the fixed code. Each character corresponds to 200us, first row: real time (ms), second row: conversion finished by device, third row: register read s: status, CNVR not set S: status, CNVR set S: value register, e.g. shunt voltage _: bus busy (each reads needs 400 us @ 100 kBit/s) __0123456789 ...C.C.C.C.C ..s_S_V_.S_V_..S_V_s_S_V_..S_V_. At 0 ms, the conversion has not yet finished, but a 0.4 it has. Further reads are done at 0.4 + n * (2 - 0.2) ms. The next read happens at 2.2 ms, the third should at 4.0, but happens slightly late at 4.2. The read at 5.8 gets an unset CNVR flag, so the sampling is readjusted to happen at 6.2 + n' (2 - 0.2) ms. The old code does the following: __0123456789 ...C.C.C.C.C ..s_S_V_...s_S_V_...s_s_S_V_.s_s_S_V_.s_s_s_S_V_ The first read happens at 0 ms, it measures the time for the reading (1.2 ms), sleeps for the remainder (0.6 ms) and reads again. The third read takes 1.6, so sleep for 0.2 ms. The old code does not differentiate between time spent in the status poll and time spent for reading. Time spent in the status poll should not be subtracted from the delay until the next read (well, halfway, only the time spent with the poll returning !CNVR). Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
On Sunday, December 10, 2017 6:36:54 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:45 +0100 > > Stefan Brüns wrote: > > Currently, the INA2xx driver may end up causing 100% load on a single core > > and fully loading the I2C bus, which is caused by two different issues: > > > > The code uses a udelay to bridge the gab between two subsequent samples. > > As the sampling interval may be up to 16 seconds, the CPU is busy > > waiting most of the time. > > > > The second issue manifests when using the (default) "synchronous" mode. > > The code polls for a set conversion ready flag, but fails to align the > > sampling interval to the raising flag. The polling interval is > > (rightfully) slighly shorter than the sampling interval, so after some > > samples the sampling thread is continously polling. > > I'm confused. Would you mind doing an asci art example perhaps? Lets assume the conversion interval is set to 2 ms. If the polling is done at the sampling frequency, it might be slightly to long due to differences between the host and device clocks, so the polling has to run somewhat faster. Somewhat faster means 200 us, this is kept unchanged. In case the CNVR flag is not set, the status register is read again until the flag raises. The time instant the flag raises is the reference for later reads. The following shows the timing for the fixed code. Each character corresponds to 200us, first row: real time (ms), second row: conversion finished by device, third row: register read s: status, CNVR not set S: status, CNVR set S: value register, e.g. shunt voltage _: bus busy (each reads needs 400 us @ 100 kBit/s) __0123456789 ...C.C.C.C.C ..s_S_V_.S_V_..S_V_s_S_V_..S_V_. At 0 ms, the conversion has not yet finished, but a 0.4 it has. Further reads are done at 0.4 + n * (2 - 0.2) ms. The next read happens at 2.2 ms, the third should at 4.0, but happens slightly late at 4.2. The read at 5.8 gets an unset CNVR flag, so the sampling is readjusted to happen at 6.2 + n' (2 - 0.2) ms. The old code does the following: __0123456789 ...C.C.C.C.C ..s_S_V_...s_S_V_...s_s_S_V_.s_s_S_V_.s_s_s_S_V_ The first read happens at 0 ms, it measures the time for the reading (1.2 ms), sleeps for the remainder (0.6 ms) and reads again. The third read takes 1.6, so sleep for 0.2 ms. The old code does not differentiate between time spent in the status poll and time spent for reading. Time spent in the status poll should not be subtracted from the delay until the next read (well, halfway, only the time spent with the poll returning !CNVR). Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH] platform/x86: intel-vbtn: Simplify autorelease logic
On Saturday, December 9, 2017 12:07:08 AM CET Darren Hart (VMware) wrote: > The new notify_handler logic determining if autorelease should be used or > not is a bit awkward, and can result in more than one call to > sparse_keymap_report_event for the same event (scancode). The nesting > and long lines also made it difficult to read. > > Simplify the logic by eliminating a level of nesting with a goto and > always calculate autorelease and val so we can make a single call to > sparse_keymap_report_event. > > Signed-off-by: Darren Hart (VMware) <dvh...@infradead.org> > Cc: Stefan Brüns <stefan.bru...@rwth-aachen.de> > Cc: AceLan Kao <acelan@canonical.com> > --- > Note: This is based on top of Stefan's v2 patch series for intel-vbtn, > currently in my review-dvhart branch. Hi Darren, is this tree publically available? Thanks, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH] platform/x86: intel-vbtn: Simplify autorelease logic
On Saturday, December 9, 2017 12:07:08 AM CET Darren Hart (VMware) wrote: > The new notify_handler logic determining if autorelease should be used or > not is a bit awkward, and can result in more than one call to > sparse_keymap_report_event for the same event (scancode). The nesting > and long lines also made it difficult to read. > > Simplify the logic by eliminating a level of nesting with a goto and > always calculate autorelease and val so we can make a single call to > sparse_keymap_report_event. > > Signed-off-by: Darren Hart (VMware) > Cc: Stefan Brüns > Cc: AceLan Kao > --- > Note: This is based on top of Stefan's v2 patch series for intel-vbtn, > currently in my review-dvhart branch. Hi Darren, is this tree publically available? Thanks, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 3:15:55 AM CET Darren Hart wrote:
> On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote:
> > Currently all key events use autorelease, but this forbids use as a
> > modifier key.
> >
> > As all event codes come in even/odd pairs, we can lookup the key type
> > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently
> > handled key down event. If the key up is ignored, we keep setting the
> > autorelease flag for the key down.
> >
> > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
>
> +Dmitry, curious for your take on this approach.
Any response would be appreciated. This is required to make Volume keys work
properly.
> > ---
> >
> > Changes in v2:
> > - New patch, add support for seperate key up/down in intel-vbtn
> >
> > drivers/platform/x86/intel-vbtn.c | 17 +++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel-vbtn.c
> > b/drivers/platform/x86/intel-vbtn.c index ae55be91a64b..e3f6375af85c
> > 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -76,14 +76,27 @@ static void notify_handler(acpi_handle handle, u32
> > event, void *context)>
> > {
> >
> > struct platform_device *device = context;
> > struct intel_vbtn_priv *priv = dev_get_drvdata(>dev);
> >
> > + const struct key_entry *ke_rel;
> > + bool autorelease;
> >
> > if (priv->wakeup_mode) {
> >
> > if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) {
> >
> > pm_wakeup_hard_event(>dev);
> > return;
> >
> > }
> >
> > - } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true))
> > {
> > - return;
> > + } else {
> > + /* Use the fact press/release come in even/odd pairs */
> > + if ((event & 1) && sparse_keymap_report_event(priv->input_dev,
> > + event, 0, false))
> > + return;
> > +
> > + ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev,
> > + event | 1);
> > + autorelease = !ke_rel || ke_rel->type == KE_IGNORE;
> > +
> > + if (sparse_keymap_report_event(priv->input_dev, event, 1,
> > + autorelease))
> > + return;
> >
> > }
> > dev_dbg(>dev, "unknown event index 0x%x\n", event);
> >
> > }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 3:15:55 AM CET Darren Hart wrote:
> On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote:
> > Currently all key events use autorelease, but this forbids use as a
> > modifier key.
> >
> > As all event codes come in even/odd pairs, we can lookup the key type
> > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently
> > handled key down event. If the key up is ignored, we keep setting the
> > autorelease flag for the key down.
> >
> > Signed-off-by: Stefan Brüns
>
> +Dmitry, curious for your take on this approach.
Any response would be appreciated. This is required to make Volume keys work
properly.
> > ---
> >
> > Changes in v2:
> > - New patch, add support for seperate key up/down in intel-vbtn
> >
> > drivers/platform/x86/intel-vbtn.c | 17 +++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel-vbtn.c
> > b/drivers/platform/x86/intel-vbtn.c index ae55be91a64b..e3f6375af85c
> > 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -76,14 +76,27 @@ static void notify_handler(acpi_handle handle, u32
> > event, void *context)>
> > {
> >
> > struct platform_device *device = context;
> > struct intel_vbtn_priv *priv = dev_get_drvdata(>dev);
> >
> > + const struct key_entry *ke_rel;
> > + bool autorelease;
> >
> > if (priv->wakeup_mode) {
> >
> > if (sparse_keymap_entry_from_scancode(priv->input_dev, event)) {
> >
> > pm_wakeup_hard_event(>dev);
> > return;
> >
> > }
> >
> > - } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true))
> > {
> > - return;
> > + } else {
> > + /* Use the fact press/release come in even/odd pairs */
> > + if ((event & 1) && sparse_keymap_report_event(priv->input_dev,
> > + event, 0, false))
> > + return;
> > +
> > + ke_rel = sparse_keymap_entry_from_scancode(priv->input_dev,
> > + event | 1);
> > + autorelease = !ke_rel || ke_rel->type == KE_IGNORE;
> > +
> > + if (sparse_keymap_report_event(priv->input_dev, event, 1,
> > + autorelease))
> > + return;
> >
> > }
> > dev_dbg(>dev, "unknown event index 0x%x\n", event);
> >
> > }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
[PATCH v1 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag
Currently, the logic just shifts the interval duration between the delay
and the polling, but never aligns to the conversion ready flag.
Whenever the conversion is already finished, schedule the next read on
the regular interval, otherwise schedule it one interval after the flag
bit has been set.
Split the work function in two functions, one for the status poll and one
for reading the values, to be able to note down the time when the flag
bit is raised.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/iio/adc/ina2xx-adc.c | 57 +---
1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 0a6745e15a5d..728cc109a61c 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(4),
};
-static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- /* data buffer needs space for channel data and timestap */
- unsigned short data[4 + sizeof(s64)/sizeof(short)];
- int bit, ret, i = 0;
- s64 time;
+ int ret;
unsigned int alert;
/*
@@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
* For now, we do an extra read of the MASK_ENABLE register (INA226)
* resp. the BUS_VOLTAGE register (INA219).
*/
- if (!chip->allow_async_readout)
- do {
- if (chip->config->chip_id == ina226) {
- ret = regmap_read(chip->regmap,
- INA226_MASK_ENABLE, );
- alert &= INA226_CVRF;
- } else {
- ret = regmap_read(chip->regmap,
- INA2XX_BUS_VOLTAGE, );
- alert &= INA219_CNVR;
- }
+ if (chip->config->chip_id == ina226) {
+ ret = regmap_read(chip->regmap,
+ INA226_MASK_ENABLE, );
+ alert &= INA226_CVRF;
+ } else {
+ ret = regmap_read(chip->regmap,
+ INA2XX_BUS_VOLTAGE, );
+ alert &= INA219_CNVR;
+ }
- if (ret < 0)
- return ret;
+ if (ret < 0)
+ return ret;
- } while (!alert);
+ return !!alert;
+}
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+ struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
+ int bit, ret, i = 0;
+ s64 time;
time = iio_get_time_ns(indio_dev);
@@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
ktime_get_ts64();
do {
+ while (!chip->allow_async_readout) {
+ ret = ina2xx_conversion_ready(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ /*
+* If the conversion was not yet finished,
+* reset the reference timestamp.
+*/
+ if (ret == 0)
+ ktime_get_ts64();
+ else
+ break;
+ }
+
ret = ina2xx_work_buffer(indio_dev);
if (ret < 0)
return ret;
--
2.15.1
[PATCH v1 7/7] iio: adc: ina2xx: Actually align the loop with the conversion ready flag
Currently, the logic just shifts the interval duration between the delay
and the polling, but never aligns to the conversion ready flag.
Whenever the conversion is already finished, schedule the next read on
the regular interval, otherwise schedule it one interval after the flag
bit has been set.
Split the work function in two functions, one for the status poll and one
for reading the values, to be able to note down the time when the flag
bit is raised.
Signed-off-by: Stefan Brüns
---
drivers/iio/adc/ina2xx-adc.c | 57 +---
1 file changed, 38 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 0a6745e15a5d..728cc109a61c 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -697,13 +697,10 @@ static const struct iio_chan_spec ina219_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(4),
};
-static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+static int ina2xx_conversion_ready(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- /* data buffer needs space for channel data and timestap */
- unsigned short data[4 + sizeof(s64)/sizeof(short)];
- int bit, ret, i = 0;
- s64 time;
+ int ret;
unsigned int alert;
/*
@@ -717,22 +714,29 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
* For now, we do an extra read of the MASK_ENABLE register (INA226)
* resp. the BUS_VOLTAGE register (INA219).
*/
- if (!chip->allow_async_readout)
- do {
- if (chip->config->chip_id == ina226) {
- ret = regmap_read(chip->regmap,
- INA226_MASK_ENABLE, );
- alert &= INA226_CVRF;
- } else {
- ret = regmap_read(chip->regmap,
- INA2XX_BUS_VOLTAGE, );
- alert &= INA219_CNVR;
- }
+ if (chip->config->chip_id == ina226) {
+ ret = regmap_read(chip->regmap,
+ INA226_MASK_ENABLE, );
+ alert &= INA226_CVRF;
+ } else {
+ ret = regmap_read(chip->regmap,
+ INA2XX_BUS_VOLTAGE, );
+ alert &= INA219_CNVR;
+ }
- if (ret < 0)
- return ret;
+ if (ret < 0)
+ return ret;
- } while (!alert);
+ return !!alert;
+}
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+ struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
+ int bit, ret, i = 0;
+ s64 time;
time = iio_get_time_ns(indio_dev);
@@ -776,6 +780,21 @@ static int ina2xx_capture_thread(void *data)
ktime_get_ts64();
do {
+ while (!chip->allow_async_readout) {
+ ret = ina2xx_conversion_ready(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ /*
+* If the conversion was not yet finished,
+* reset the reference timestamp.
+*/
+ if (ret == 0)
+ ktime_get_ts64();
+ else
+ break;
+ }
+
ret = ina2xx_work_buffer(indio_dev);
if (ret < 0)
return ret;
--
2.15.1
[PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.
The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/iio/adc/ina2xx-adc.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
- int cnvr_need_clear = 0;
time_a = iio_get_time_ns(indio_dev);
@@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
ret = regmap_read(chip->regmap,
INA2XX_BUS_VOLTAGE, );
alert &= INA219_CNVR;
- cnvr_need_clear = alert;
}
if (ret < 0)
@@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
return ret;
data[i++] = val;
-
- if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
- cnvr_need_clear = 0;
- }
-
- /* Dummy read on INA219 power register to clear CNVR flag */
- if (cnvr_need_clear && chip->config->chip_id == ina219) {
- unsigned int val;
-
- ret = regmap_read(chip->regmap, INA2XX_POWER, );
- if (ret < 0)
- return ret;
}
time_b = iio_get_time_ns(indio_dev);
--
2.15.1
[PATCH v1 3/7] iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
Although the datasheet states the CNVR flag is cleared by reading the
BUS_VOLTAGE register, it is actually cleared by reading any of the
voltage/current/power registers.
The behaviour has been confirmed by TI support:
http://e2e.ti.com/support/amplifiers/current-shunt-monitors/f/931/p/647053/2378282
Signed-off-by: Stefan Brüns
---
drivers/iio/adc/ina2xx-adc.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8c8120406f52..b027d485398b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -705,7 +705,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
- int cnvr_need_clear = 0;
time_a = iio_get_time_ns(indio_dev);
@@ -730,7 +729,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
ret = regmap_read(chip->regmap,
INA2XX_BUS_VOLTAGE, );
alert &= INA219_CNVR;
- cnvr_need_clear = alert;
}
if (ret < 0)
@@ -752,18 +750,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
return ret;
data[i++] = val;
-
- if (INA2XX_SHUNT_VOLTAGE + bit == INA2XX_POWER)
- cnvr_need_clear = 0;
- }
-
- /* Dummy read on INA219 power register to clear CNVR flag */
- if (cnvr_need_clear && chip->config->chip_id == ina219) {
- unsigned int val;
-
- ret = regmap_read(chip->regmap, INA2XX_POWER, );
- if (ret < 0)
- return ret;
}
time_b = iio_get_time_ns(indio_dev);
--
2.15.1
[PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/iio/adc/ina2xx-adc.c | 35 +--
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..65bd9e69faf2 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = timespec64_to_ns() / 1000;
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also,
only part of the acquisition time is measured, thus the delay was longer
than intended.
Signed-off-by: Stefan Brüns
---
drivers/iio/adc/ina2xx-adc.c | 35 +--
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 2621a34ee5c6..65bd9e69faf2 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
/* data buffer needs space for channel data and timestap */
unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
- s64 time_a, time_b;
+ s64 time;
unsigned int alert;
- time_a = iio_get_time_ns(indio_dev);
+ time = iio_get_time_ns(indio_dev);
/*
* Because the timer thread and the chip conversion clock
@@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
data[i++] = val;
}
- time_b = iio_get_time_ns(indio_dev);
+ iio_push_to_buffers_with_timestamp(indio_dev, data, time);
- iio_push_to_buffers_with_timestamp(indio_dev, data, time_a);
-
- return (unsigned long)(time_b - time_a) / 1000;
+ return 0;
};
static int ina2xx_capture_thread(void *data)
@@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us, delay_us;
+ int ret;
+ struct timespec64 next, now, delta;
+ s64 delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data)
if (!chip->allow_async_readout)
sampling_us -= 200;
+ ktime_get_ts64();
+
do {
- buffer_us = ina2xx_work_buffer(indio_dev);
- if (buffer_us < 0)
- return buffer_us;
+ ret = ina2xx_work_buffer(indio_dev);
+ if (ret < 0)
+ return ret;
- if (sampling_us > buffer_us) {
- delay_us = sampling_us - buffer_us;
- usleep_range(delay_us, (delay_us * 3) >> 1);
- }
+ ktime_get_ts64();
+
+ do {
+ timespec64_add_ns(, 1000 * sampling_us);
+ delta = timespec64_sub(next, now);
+ delay_us = timespec64_to_ns() / 1000;
+ } while (delay_us <= 0);
+
+ usleep_range(delay_us, (delay_us * 3) >> 1);
} while (!kthread_should_stop());
--
2.15.1
[PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
Currently, the INA2xx driver may end up causing 100% load on a single core and fully loading the I2C bus, which is caused by two different issues: The code uses a udelay to bridge the gab between two subsequent samples. As the sampling interval may be up to 16 seconds, the CPU is busy waiting most of the time. The second issue manifests when using the (default) "synchronous" mode. The code polls for a set conversion ready flag, but fails to align the sampling interval to the raising flag. The polling interval is (rightfully) slighly shorter than the sampling interval, so after some samples the sampling thread is continously polling. The patch series fixes both issues: Patch 1 and 2 are just some small cosmetic changes. Patch 3 removes an unnecessary read. According to the datasheet, the CNVR flag is only cleared by reading the power register, but is cleared by reading any of the measurement registers, thus the dummy read can be skipped. This behaviour has been confirmed by TI technical support. Patch 4 replaces the udelay with usleep_range. Patch 5 reworks the delay logic. Previously the IIO timestamp clock was used to capture entry and exit times of the work function. The timestamp clock is user selectable and may be non-monotonic. Also, any time spent outside the work function is not accounted for. Patch 6 moves the timestap capture to the end of the conversion ready status poll. Patch 7 addresses the alignment issue. Every time an unset flag is seen on poll loop entry, the reference timestamp is readjusted. Both old and fixed behaviour has been verified using a logic analyzer. In synchrounous mode, every few samples a double read of the status register can be observed, showing the raising status flag, the other samples are evenly spaced at sampling intervals inbetween. Stefan Brüns (7): iio: adc: ina2xx: Remove bogus cast for data argument iio: adc: ina2xx: Clarify size requirement for data buffer iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag iio: adc: ina2xx: Do not udelay for several seconds iio: adc: ina2xx: Use a monotonic clock for delay calculation iio: adc: ina2xx: Align timestamp with conversion ready flag iio: adc: ina2xx: Actually align the loop with the conversion ready flag drivers/iio/adc/ina2xx-adc.c | 104 --- 1 file changed, 59 insertions(+), 45 deletions(-) -- 2.15.1
[PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
Currently, the INA2xx driver may end up causing 100% load on a single core and fully loading the I2C bus, which is caused by two different issues: The code uses a udelay to bridge the gab between two subsequent samples. As the sampling interval may be up to 16 seconds, the CPU is busy waiting most of the time. The second issue manifests when using the (default) "synchronous" mode. The code polls for a set conversion ready flag, but fails to align the sampling interval to the raising flag. The polling interval is (rightfully) slighly shorter than the sampling interval, so after some samples the sampling thread is continously polling. The patch series fixes both issues: Patch 1 and 2 are just some small cosmetic changes. Patch 3 removes an unnecessary read. According to the datasheet, the CNVR flag is only cleared by reading the power register, but is cleared by reading any of the measurement registers, thus the dummy read can be skipped. This behaviour has been confirmed by TI technical support. Patch 4 replaces the udelay with usleep_range. Patch 5 reworks the delay logic. Previously the IIO timestamp clock was used to capture entry and exit times of the work function. The timestamp clock is user selectable and may be non-monotonic. Also, any time spent outside the work function is not accounted for. Patch 6 moves the timestap capture to the end of the conversion ready status poll. Patch 7 addresses the alignment issue. Every time an unset flag is seen on poll loop entry, the reference timestamp is readjusted. Both old and fixed behaviour has been verified using a logic analyzer. In synchrounous mode, every few samples a double read of the status register can be observed, showing the raising status flag, the other samples are evenly spaced at sampling intervals inbetween. Stefan Brüns (7): iio: adc: ina2xx: Remove bogus cast for data argument iio: adc: ina2xx: Clarify size requirement for data buffer iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag iio: adc: ina2xx: Do not udelay for several seconds iio: adc: ina2xx: Use a monotonic clock for delay calculation iio: adc: ina2xx: Align timestamp with conversion ready flag iio: adc: ina2xx: Actually align the loop with the conversion ready flag drivers/iio/adc/ina2xx-adc.c | 104 --- 1 file changed, 59 insertions(+), 45 deletions(-) -- 2.15.1
[PATCH v1 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag
As the timestamp is no longer (ab-)used to measure the function run time, it can be taken at the correct time, i.e. when the conversion has finished. Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> --- drivers/iio/adc/ina2xx-adc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 65bd9e69faf2..0a6745e15a5d 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) s64 time; unsigned int alert; - time = iio_get_time_ns(indio_dev); - /* * Because the timer thread and the chip conversion clock * are asynchronous, the period difference will eventually @@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) } while (!alert); + time = iio_get_time_ns(indio_dev); + /* * Single register reads: bulk_read will not work with ina226/219 * as there is no auto-increment of the register pointer. -- 2.15.1
[PATCH v1 4/7] iio: adc: ina2xx: Do not udelay for several seconds
The conversion time can be up to 16 seconds (8 ms per channel, 2 channels,
1024 times averaging).
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/iio/adc/ina2xx-adc.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b027d485398b..2621a34ee5c6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -764,7 +764,7 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us;
+ int buffer_us, delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -778,8 +778,10 @@ static int ina2xx_capture_thread(void *data)
if (buffer_us < 0)
return buffer_us;
- if (sampling_us > buffer_us)
- udelay(sampling_us - buffer_us);
+ if (sampling_us > buffer_us) {
+ delay_us = sampling_us - buffer_us;
+ usleep_range(delay_us, (delay_us * 3) >> 1);
+ }
} while (!kthread_should_stop());
--
2.15.1
[PATCH v1 1/7] iio: adc: ina2xx: Remove bogus cast for data argument
iio_push_to_buffers_with_timestamp expects a void pointer, so the cast is both unnecessary and misleading. Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> --- drivers/iio/adc/ina2xx-adc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index ddf878163bf9..3195f8754c3b 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) time_b = iio_get_time_ns(indio_dev); - iio_push_to_buffers_with_timestamp(indio_dev, - (unsigned int *)data, time_a); + iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); return (unsigned long)(time_b - time_a) / 1000; }; -- 2.15.1
[PATCH v1 6/7] iio: adc: ina2xx: Align timestamp with conversion ready flag
As the timestamp is no longer (ab-)used to measure the function run time, it can be taken at the correct time, i.e. when the conversion has finished. Signed-off-by: Stefan Brüns --- drivers/iio/adc/ina2xx-adc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 65bd9e69faf2..0a6745e15a5d 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -706,8 +706,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) s64 time; unsigned int alert; - time = iio_get_time_ns(indio_dev); - /* * Because the timer thread and the chip conversion clock * are asynchronous, the period difference will eventually @@ -736,6 +734,8 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) } while (!alert); + time = iio_get_time_ns(indio_dev); + /* * Single register reads: bulk_read will not work with ina226/219 * as there is no auto-increment of the register pointer. -- 2.15.1
[PATCH v1 4/7] iio: adc: ina2xx: Do not udelay for several seconds
The conversion time can be up to 16 seconds (8 ms per channel, 2 channels,
1024 times averaging).
Signed-off-by: Stefan Brüns
---
drivers/iio/adc/ina2xx-adc.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index b027d485398b..2621a34ee5c6 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -764,7 +764,7 @@ static int ina2xx_capture_thread(void *data)
struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
int sampling_us = SAMPLING_PERIOD(chip);
- int buffer_us;
+ int buffer_us, delay_us;
/*
* Poll a bit faster than the chip internal Fs, in case
@@ -778,8 +778,10 @@ static int ina2xx_capture_thread(void *data)
if (buffer_us < 0)
return buffer_us;
- if (sampling_us > buffer_us)
- udelay(sampling_us - buffer_us);
+ if (sampling_us > buffer_us) {
+ delay_us = sampling_us - buffer_us;
+ usleep_range(delay_us, (delay_us * 3) >> 1);
+ }
} while (!kthread_should_stop());
--
2.15.1
[PATCH v1 1/7] iio: adc: ina2xx: Remove bogus cast for data argument
iio_push_to_buffers_with_timestamp expects a void pointer, so the cast is both unnecessary and misleading. Signed-off-by: Stefan Brüns --- drivers/iio/adc/ina2xx-adc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index ddf878163bf9..3195f8754c3b 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -767,8 +767,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) time_b = iio_get_time_ns(indio_dev); - iio_push_to_buffers_with_timestamp(indio_dev, - (unsigned int *)data, time_a); + iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); return (unsigned long)(time_b - time_a) / 1000; }; -- 2.15.1
[PATCH v1 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer
The timestamp is inserted into the buffer after the sample data by
iio_push_to_buffers_with_timestamp, document the space requirement for
the timestamp.
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/iio/adc/ina2xx-adc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3195f8754c3b..8c8120406f52 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -700,7 +700,8 @@ static const struct iio_chan_spec ina219_channels[] = {
static int ina2xx_work_buffer(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- unsigned short data[8];
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
--
2.15.1
[PATCH v1 2/7] iio: adc: ina2xx: Clarify size requirement for data buffer
The timestamp is inserted into the buffer after the sample data by
iio_push_to_buffers_with_timestamp, document the space requirement for
the timestamp.
Signed-off-by: Stefan Brüns
---
drivers/iio/adc/ina2xx-adc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 3195f8754c3b..8c8120406f52 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -700,7 +700,8 @@ static const struct iio_chan_spec ina219_channels[] = {
static int ina2xx_work_buffer(struct iio_dev *indio_dev)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- unsigned short data[8];
+ /* data buffer needs space for channel data and timestap */
+ unsigned short data[4 + sizeof(s64)/sizeof(short)];
int bit, ret, i = 0;
s64 time_a, time_b;
unsigned int alert;
--
2.15.1
[PATCH v2] sunxi-rsb: Include OF based modalias in device uevent
Include the OF-based modalias in the uevent sent when registering devices
on the sunxi RSB bus, so that user space has a chance to autoload the
kernel module for the device.
Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
the AXP803 PMIC is built as a module, it is not loaded and the system
ends up with an disfunctional MMC controller.
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/bus/sunxi-rsb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 328ca93781cf..1b76d9585902 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -178,6 +178,7 @@ static struct bus_type sunxi_rsb_bus = {
.match = sunxi_rsb_device_match,
.probe = sunxi_rsb_device_probe,
.remove = sunxi_rsb_device_remove,
+ .uevent = of_device_uevent_modalias,
};
static void sunxi_rsb_dev_release(struct device *dev)
--
2.15.0
[PATCH v2] sunxi-rsb: Include OF based modalias in device uevent
Include the OF-based modalias in the uevent sent when registering devices
on the sunxi RSB bus, so that user space has a chance to autoload the
kernel module for the device.
Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
the AXP803 PMIC is built as a module, it is not loaded and the system
ends up with an disfunctional MMC controller.
Cc: stable
Signed-off-by: Stefan Brüns
---
drivers/bus/sunxi-rsb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 328ca93781cf..1b76d9585902 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -178,6 +178,7 @@ static struct bus_type sunxi_rsb_bus = {
.match = sunxi_rsb_device_match,
.probe = sunxi_rsb_device_probe,
.remove = sunxi_rsb_device_remove,
+ .uevent = of_device_uevent_modalias,
};
static void sunxi_rsb_dev_release(struct device *dev)
--
2.15.0
Re: [PATCH] sunxi-rsb: Include OF based modalias in device uevent
On Monday, November 27, 2017 4:35:02 PM CET Maxime Ripard wrote:
> Hi,
>
> On Mon, Nov 27, 2017 at 01:17:25PM +0100, Stefan Brüns wrote:
> > Include the OF-based modalias in the uevent sent when registering devices
> > on the sunxi RSB bus, so that user space has a chance to autoload the
> > kernel module for the device.
> >
> > Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
> > pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
> > the AXP803 PMIC is built as a module, it is not loaded and the system
> > ends up with an disfunctional MMC controller.
> >
> > Cc: stable <sta...@vger.kernel.org>
> > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > ---
> >
> > drivers/bus/sunxi-rsb.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > index 328ca93781cf..37cb57244cbe 100644
> > --- a/drivers/bus/sunxi-rsb.c
> > +++ b/drivers/bus/sunxi-rsb.c
> > @@ -173,11 +173,24 @@ static int sunxi_rsb_device_remove(struct device
> > *dev)>
> > return drv->remove(to_sunxi_rsb_device(dev));
> >
> > }
> >
> > +static int sunxi_rsb_device_uevent(struct device *dev,
> > + struct kobj_uevent_env *env)
> > +{
> > + int ret;
> > +
> > + ret = of_device_uevent_modalias(dev, env);
> > + if (ret != -ENODEV)
> > + return ret;
>
> A comment explaining why we need to ignore the ENODEV error code would
> be great here.
Lazy answer - everyone else is doing the same, and nobody cared to add an
explanation.
For *some* drivers, this is likely because the same device may be enumerated
from e.g ACPI or OF, and for an ACPI device -ENODEV will be returned, as
dev->of_node is NULL.
For devices which are only usable in an OF context, this is bogus. Not sure
about sunxi-rsb.
> > + return 0;
> > +}
> > +
> >
> > static struct bus_type sunxi_rsb_bus = {
> >
> > .name = RSB_CTRL_NAME,
> > .match = sunxi_rsb_device_match,
> > .probe = sunxi_rsb_device_probe,
> > .remove = sunxi_rsb_device_remove,
> >
> > + .uevent = sunxi_rsb_device_uevent,
>
> Any reason to not use of_device_uevent_modalias directly here?
*If* sunxi-rsb can be used without OF, then yes, otherwise no.
Regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] sunxi-rsb: Include OF based modalias in device uevent
On Monday, November 27, 2017 4:35:02 PM CET Maxime Ripard wrote:
> Hi,
>
> On Mon, Nov 27, 2017 at 01:17:25PM +0100, Stefan Brüns wrote:
> > Include the OF-based modalias in the uevent sent when registering devices
> > on the sunxi RSB bus, so that user space has a chance to autoload the
> > kernel module for the device.
> >
> > Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
> > pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
> > the AXP803 PMIC is built as a module, it is not loaded and the system
> > ends up with an disfunctional MMC controller.
> >
> > Cc: stable
> > Signed-off-by: Stefan Brüns
> > ---
> >
> > drivers/bus/sunxi-rsb.c | 13 +
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> > index 328ca93781cf..37cb57244cbe 100644
> > --- a/drivers/bus/sunxi-rsb.c
> > +++ b/drivers/bus/sunxi-rsb.c
> > @@ -173,11 +173,24 @@ static int sunxi_rsb_device_remove(struct device
> > *dev)>
> > return drv->remove(to_sunxi_rsb_device(dev));
> >
> > }
> >
> > +static int sunxi_rsb_device_uevent(struct device *dev,
> > + struct kobj_uevent_env *env)
> > +{
> > + int ret;
> > +
> > + ret = of_device_uevent_modalias(dev, env);
> > + if (ret != -ENODEV)
> > + return ret;
>
> A comment explaining why we need to ignore the ENODEV error code would
> be great here.
Lazy answer - everyone else is doing the same, and nobody cared to add an
explanation.
For *some* drivers, this is likely because the same device may be enumerated
from e.g ACPI or OF, and for an ACPI device -ENODEV will be returned, as
dev->of_node is NULL.
For devices which are only usable in an OF context, this is bogus. Not sure
about sunxi-rsb.
> > + return 0;
> > +}
> > +
> >
> > static struct bus_type sunxi_rsb_bus = {
> >
> > .name = RSB_CTRL_NAME,
> > .match = sunxi_rsb_device_match,
> > .probe = sunxi_rsb_device_probe,
> > .remove = sunxi_rsb_device_remove,
> >
> > + .uevent = sunxi_rsb_device_uevent,
>
> Any reason to not use of_device_uevent_modalias directly here?
*If* sunxi-rsb can be used without OF, then yes, otherwise no.
Regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
[PATCH] sunxi-rsb: Include OF based modalias in device uevent
Include the OF-based modalias in the uevent sent when registering devices
on the sunxi RSB bus, so that user space has a chance to autoload the
kernel module for the device.
Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
the AXP803 PMIC is built as a module, it is not loaded and the system
ends up with an disfunctional MMC controller.
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
---
drivers/bus/sunxi-rsb.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 328ca93781cf..37cb57244cbe 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -173,11 +173,24 @@ static int sunxi_rsb_device_remove(struct device *dev)
return drv->remove(to_sunxi_rsb_device(dev));
}
+static int sunxi_rsb_device_uevent(struct device *dev,
+ struct kobj_uevent_env *env)
+{
+ int ret;
+
+ ret = of_device_uevent_modalias(dev, env);
+ if (ret != -ENODEV)
+ return ret;
+
+ return 0;
+}
+
static struct bus_type sunxi_rsb_bus = {
.name = RSB_CTRL_NAME,
.match = sunxi_rsb_device_match,
.probe = sunxi_rsb_device_probe,
.remove = sunxi_rsb_device_remove,
+ .uevent = sunxi_rsb_device_uevent,
};
static void sunxi_rsb_dev_release(struct device *dev)
--
2.15.0
[PATCH] sunxi-rsb: Include OF based modalias in device uevent
Include the OF-based modalias in the uevent sent when registering devices
on the sunxi RSB bus, so that user space has a chance to autoload the
kernel module for the device.
Fixes a regression caused by commit 3f241bfa60bd ("arm64: allwinner: a64:
pine64: Use dcdc1 regulator for mmc0"). When the axp20x-rsb module for
the AXP803 PMIC is built as a module, it is not loaded and the system
ends up with an disfunctional MMC controller.
Cc: stable
Signed-off-by: Stefan Brüns
---
drivers/bus/sunxi-rsb.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 328ca93781cf..37cb57244cbe 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -173,11 +173,24 @@ static int sunxi_rsb_device_remove(struct device *dev)
return drv->remove(to_sunxi_rsb_device(dev));
}
+static int sunxi_rsb_device_uevent(struct device *dev,
+ struct kobj_uevent_env *env)
+{
+ int ret;
+
+ ret = of_device_uevent_modalias(dev, env);
+ if (ret != -ENODEV)
+ return ret;
+
+ return 0;
+}
+
static struct bus_type sunxi_rsb_bus = {
.name = RSB_CTRL_NAME,
.match = sunxi_rsb_device_match,
.probe = sunxi_rsb_device_probe,
.remove = sunxi_rsb_device_remove,
+ .uevent = sunxi_rsb_device_uevent,
};
static void sunxi_rsb_dev_release(struct device *dev)
--
2.15.0
Re: [PATCH 1/2] iio: adc: ina2xx: Make calibration register value fixed
ion_value.
> */
> static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> {
> - u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -chip->shunt_resistor_uohm);
> -
> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> + return regmap_write(chip->regmap, INA2XX_CALIBRATION,
> + chip->config->calibration_value);
> }
>
> +/*
> + * In order to keep calibration register value fixed, the product
> + * of current_lsb and shunt_resistor should also be fixed and equal
> + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order
> + * to keep the scale.
> + */
> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int
> val) {
> - if (val <= 0 || val > chip->config->calibration_factor)
> + unsigned int dividend = DIV_ROUND_CLOSEST(10,
> + chip->config->shunt_div);
> +
> + if (val <= 0 || val > dividend)
> return -EINVAL;
>
> chip->shunt_resistor_uohm = val;
> + chip->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val);
> + chip->power_lsb_uW = chip->config->power_lsb_factor *
> + chip->current_lsb_uA;
As the calculation is trivial an only used in ina2xx_read_raw, I think you
should remove the extra struct member and do the calculation in
ina2xx_read_raw.
> return 0;
> }
> @@ -485,11 +495,6 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> device *dev, if (ret)
> return ret;
>
> - /* Update the Calibration register */
> - ret = ina2xx_set_calibration(chip);
> - if (ret)
> - return ret;
> -
> return len;
> }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH 1/2] iio: adc: ina2xx: Make calibration register value fixed
static int ina2xx_set_calibration(struct ina2xx_chip_info *chip)
> {
> - u16 regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> -chip->shunt_resistor_uohm);
> -
> - return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> + return regmap_write(chip->regmap, INA2XX_CALIBRATION,
> + chip->config->calibration_value);
> }
>
> +/*
> + * In order to keep calibration register value fixed, the product
> + * of current_lsb and shunt_resistor should also be fixed and equal
> + * to shunt_voltage_lsb = 1 / shunt_div multiplied by 10^9 in order
> + * to keep the scale.
> + */
> static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int
> val) {
> - if (val <= 0 || val > chip->config->calibration_factor)
> + unsigned int dividend = DIV_ROUND_CLOSEST(10,
> + chip->config->shunt_div);
> +
> + if (val <= 0 || val > dividend)
> return -EINVAL;
>
> chip->shunt_resistor_uohm = val;
> + chip->current_lsb_uA = DIV_ROUND_CLOSEST(dividend, val);
> + chip->power_lsb_uW = chip->config->power_lsb_factor *
> + chip->current_lsb_uA;
As the calculation is trivial an only used in ina2xx_read_raw, I think you
should remove the extra struct member and do the calculation in
ina2xx_read_raw.
> return 0;
> }
> @@ -485,11 +495,6 @@ static ssize_t ina2xx_shunt_resistor_store(struct
> device *dev, if (ret)
> return ret;
>
> - /* Update the Calibration register */
> - ret = ina2xx_set_calibration(chip);
> - if (ret)
> - return ret;
> -
> return len;
> }
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 2/3] iio: adc: ina2xx: Use LSB specifier instead of divider in config
On Sunday, November 19, 2017 5:15:03 PM CET Jonathan Cameron wrote: > On Sat, 28 Oct 2017 23:12:47 +0200 > > Stefan Brüns <stefan.bru...@rwth-aachen.de> wrote: > > While the config uses the physical value corresponding to the LSB > > for both the power and the bus voltage register, the shunt voltage is > > specified as parts of 1 mV. Use the LSB physical value for all registers. > > > > No functional change. > > > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> > > Applied Hi Maciej, I think you will have to rebase you calibration register patch on top of this series ... Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 2/3] iio: adc: ina2xx: Use LSB specifier instead of divider in config
On Sunday, November 19, 2017 5:15:03 PM CET Jonathan Cameron wrote: > On Sat, 28 Oct 2017 23:12:47 +0200 > > Stefan Brüns wrote: > > While the config uses the physical value corresponding to the LSB > > for both the power and the bus voltage register, the shunt voltage is > > specified as parts of 1 mV. Use the LSB physical value for all registers. > > > > No functional change. > > > > Signed-off-by: Stefan Brüns > > Applied Hi Maciej, I think you will have to rebase you calibration register patch on top of this series ... Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH 2/2] hwmon: (ina2xx) Make calibration register value fixed
On Saturday, November 25, 2017 7:50:16 PM CET Guenter Roeck wrote: > On 11/22/2017 07:32 AM, Maciej Purski wrote: > > Calibration register is used for calculating current register in > > hardware according to datasheet: > > current = shunt_volt * calib_register / 2048 (ina 226) > > current = shunt_volt * calib_register / 4096 (ina 219) > > > > Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in > > order to avoid truncation error and provide best precision allowed > > by shunt_voltage measurement. Make current scale value follow changes > > of shunt_resistor from sysfs as calib_register value is now fixed. > > > > Power_lsb value should also follow shunt_resistor changes as stated in > > datasheet: > > power_lsb = 25 * current_lsb (ina 226) > > power_lsb = 20 * current_lsb (ina 219) > > > > Signed-off-by: Maciej Purski <m.pur...@samsung.com> > > Setting the calibration register to a specific value may optimize precision, > but limits the supported value range, which is the whole point of providing > a calibration register. What am I missing here ? For the current register, any different calibration register value is completely useless - smaller values just truncate the register value, larger values adds noise in the lsbs. Both registers (current/shunt voltage) are 16bit, and nothing is going to change that. There is a *very* small allowed power operating area where scaling down would be of any use: 1. Bus voltage exceeds 25 (20) Volts 2. Shunt voltage is about 82 (320) mV In this case the power register overflows. More specifically, if: (shunt_voltage [mv] / 81.92) * (bus_voltage [V] / 25) > 1 Maximum allowed bus voltage is 36 V resp 26 V. (ina226/219). Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH 2/2] hwmon: (ina2xx) Make calibration register value fixed
On Saturday, November 25, 2017 7:50:16 PM CET Guenter Roeck wrote: > On 11/22/2017 07:32 AM, Maciej Purski wrote: > > Calibration register is used for calculating current register in > > hardware according to datasheet: > > current = shunt_volt * calib_register / 2048 (ina 226) > > current = shunt_volt * calib_register / 4096 (ina 219) > > > > Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in > > order to avoid truncation error and provide best precision allowed > > by shunt_voltage measurement. Make current scale value follow changes > > of shunt_resistor from sysfs as calib_register value is now fixed. > > > > Power_lsb value should also follow shunt_resistor changes as stated in > > datasheet: > > power_lsb = 25 * current_lsb (ina 226) > > power_lsb = 20 * current_lsb (ina 219) > > > > Signed-off-by: Maciej Purski > > Setting the calibration register to a specific value may optimize precision, > but limits the supported value range, which is the whole point of providing > a calibration register. What am I missing here ? For the current register, any different calibration register value is completely useless - smaller values just truncate the register value, larger values adds noise in the lsbs. Both registers (current/shunt voltage) are 16bit, and nothing is going to change that. There is a *very* small allowed power operating area where scaling down would be of any use: 1. Bus voltage exceeds 25 (20) Volts 2. Shunt voltage is about 82 (320) mV In this case the power register overflows. More specifically, if: (shunt_voltage [mv] / 81.92) * (bus_voltage [V] / 25) > 1 Maximum allowed bus voltage is 36 V resp 26 V. (ina226/219). Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 3:11:37 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 02:58:36AM +0100, Stefan Brüns wrote:
> > On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote:
> > > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote:
> > > > Currently all key events use autorelease, but this forbids use as a
> > > > modifier key.
> > > >
> > > > As all event codes come in even/odd pairs, we can lookup the key type
> > > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently
> > > > handled key down event. If the key up is ignored, we keep setting the
> > > > autorelease flag for the key down.
> > >
> > > What is the use-case for using these buttons as modifiers? I'm picturing
> > > one of these devices in tablet mode, with a physical Windows button.
> > > What other action does a user want to modify by holding the Windows
> > > button down? Or is there another scenario we're trying to support here?
> >
> > Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination
> > with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp
> > creates a screenshot.
> >
> > You can also use this in combination with an onscreen keyboard. Pressing
> > the hardware button with the hand holding the tablet and typing with the
> > other hand on the OSK is probably easier than hitting both keys on the
> > OSK.
> This all makes sense - good context for the commit message. If no other
> changes end up being needed, I'm happy to paste it in at merge. If changes
> are required, please add it in v3.
>
> > Additionally, the Volume Up/Down currently do not autorepeat, as the key
> > is
> > autoreleased on the press event. The XPS 12 does issue distinct
> > press/release events, so this could be done properly. The same apparently
> > holds for some other convertibles, see the links in Patch 1/5.
>
> It sounds like this is spotty across intel-vbtn implementations? Some
> devices emit release, others do not? How would you teach the driver about
> the differences? Assume autorelease and change the sparse keymap entry for
> DMI matched platforms with release? Doable... sounds unpleasant to maintain
> and update. Do we support this fully in userspace already?
- 0xcc/0xcd SW_TABLET mode seems to be consistent and widespread
- 0xc4-0xc7 KEY_VOLUME_{UP,DOWN} seems to be consistent, although not used
very often (Lenovo Helix 2, XPS 12, other Dell XPS), others probably use WMI
- 0xc2/0xc3 KEY_LEFTMETA is used on Helix 2 and XPS 12, but only the XPS
issues 0xc2 on press and 0xc3 on release, the Helix 2 issues both codes on
release and none on press.
The statements above are verified on the XPS 12, the other findings are from
the Internet, search for "unknown event index" + "intel-vbtn".
As far as I can see, there are no implementation which do not issue the events
in pairs, so currently the quirks table would be empty.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 3:11:37 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 02:58:36AM +0100, Stefan Brüns wrote:
> > On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote:
> > > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote:
> > > > Currently all key events use autorelease, but this forbids use as a
> > > > modifier key.
> > > >
> > > > As all event codes come in even/odd pairs, we can lookup the key type
> > > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently
> > > > handled key down event. If the key up is ignored, we keep setting the
> > > > autorelease flag for the key down.
> > >
> > > What is the use-case for using these buttons as modifiers? I'm picturing
> > > one of these devices in tablet mode, with a physical Windows button.
> > > What other action does a user want to modify by holding the Windows
> > > button down? Or is there another scenario we're trying to support here?
> >
> > Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination
> > with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp
> > creates a screenshot.
> >
> > You can also use this in combination with an onscreen keyboard. Pressing
> > the hardware button with the hand holding the tablet and typing with the
> > other hand on the OSK is probably easier than hitting both keys on the
> > OSK.
> This all makes sense - good context for the commit message. If no other
> changes end up being needed, I'm happy to paste it in at merge. If changes
> are required, please add it in v3.
>
> > Additionally, the Volume Up/Down currently do not autorepeat, as the key
> > is
> > autoreleased on the press event. The XPS 12 does issue distinct
> > press/release events, so this could be done properly. The same apparently
> > holds for some other convertibles, see the links in Patch 1/5.
>
> It sounds like this is spotty across intel-vbtn implementations? Some
> devices emit release, others do not? How would you teach the driver about
> the differences? Assume autorelease and change the sparse keymap entry for
> DMI matched platforms with release? Doable... sounds unpleasant to maintain
> and update. Do we support this fully in userspace already?
- 0xcc/0xcd SW_TABLET mode seems to be consistent and widespread
- 0xc4-0xc7 KEY_VOLUME_{UP,DOWN} seems to be consistent, although not used
very often (Lenovo Helix 2, XPS 12, other Dell XPS), others probably use WMI
- 0xc2/0xc3 KEY_LEFTMETA is used on Helix 2 and XPS 12, but only the XPS
issues 0xc2 on press and 0xc3 on release, the Helix 2 issues both codes on
release and none on press.
The statements above are verified on the XPS 12, the other findings are from
the Internet, search for "unknown event index" + "intel-vbtn".
As far as I can see, there are no implementation which do not issue the events
in pairs, so currently the quirks table would be empty.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
On Friday, November 10, 2017 2:54:22 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > > not
> > > > on BIOS A2).
> > > >
> > > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > > - Use separate up/down events
> > > >
> > > > drivers/platform/x86/intel-vbtn.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > > 100644
> > > > --- a/drivers/platform/x86/intel-vbtn.c
> > > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] =
> > > > {
> > > >
> > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up
> > > > key release
*/
> > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down
> > > > key press
*/
> > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down
> > > > key
release
> >
> > */
> >
> > > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/*
> > > > rotate-lock key
> > > > press */ + { KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },
> > > > /*
> > > > rotate-lock key release */
> > >
> > > How are those events sent? When pressing and releasing the key, do you
> > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > > releasing the first time, and 0xC9 when pressing and releasing a second
> > > time?
> > >
> > > If the former, then it's not going to work. The release is supposed to
> > > be ignored, as you send the event with sparse_keymap_report_event().
> > >
> > > If the latter, and there's an actual state, does it disable a device
> > > on-board, or activate an LED? If so, then it would need to be a switch,
> > > not a key.
> >
> > Do you think I don't test the patches before sending? Let me tell you, it
> > *does* work.
> >
> > You could also read the cover letter, where you find more details, putting
> > the patches in relation to each other.
>
> A point of process. If there is context that is needed to explain the
> patch, it belongs in the patch, not just in the cover letter. The cover
> letter is effectively lost once the patches are merged.
>
> > Just in case its not yet clear:
> > The codes are emitted when pressing a button. It is a button, not a
> > switch.
> > There is no state handled in hardware. On press (as noted by the code
> > comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> > emitted.
> This sounds like the "former" scenario Bastien described, for which I
> understand the use case to be:
>
> User presses and releases the rotate lock button to prevent the
> accelerometer for triggering screen rotation.
>
> User presses and releases the rotate lock button to allow the accelerometer
> to trigger screen rotation.
>
> Is that correct?
>
> If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time
> instead of just the press event like the volume buttons?
Volume buttons *should* send separate press/release events, to allow software
autorepeat.
For the rotate lock button, I see no reason *not* to report the actual state
instead of doing an autorelease.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
On Friday, November 10, 2017 2:54:22 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 01:15:09AM +0100, Stefan Brüns wrote:
> > On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> > > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > > not
> > > > on BIOS A2).
> > > >
> > > > Signed-off-by: Stefan Brüns
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > > - Use separate up/down events
> > > >
> > > > drivers/platform/x86/intel-vbtn.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > > 100644
> > > > --- a/drivers/platform/x86/intel-vbtn.c
> > > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] =
> > > > {
> > > >
> > > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up
> > > > key release
*/
> > > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down
> > > > key press
*/
> > > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down
> > > > key
release
> >
> > */
> >
> > > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/*
> > > > rotate-lock key
> > > > press */ + { KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },
> > > > /*
> > > > rotate-lock key release */
> > >
> > > How are those events sent? When pressing and releasing the key, do you
> > > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > > releasing the first time, and 0xC9 when pressing and releasing a second
> > > time?
> > >
> > > If the former, then it's not going to work. The release is supposed to
> > > be ignored, as you send the event with sparse_keymap_report_event().
> > >
> > > If the latter, and there's an actual state, does it disable a device
> > > on-board, or activate an LED? If so, then it would need to be a switch,
> > > not a key.
> >
> > Do you think I don't test the patches before sending? Let me tell you, it
> > *does* work.
> >
> > You could also read the cover letter, where you find more details, putting
> > the patches in relation to each other.
>
> A point of process. If there is context that is needed to explain the
> patch, it belongs in the patch, not just in the cover letter. The cover
> letter is effectively lost once the patches are merged.
>
> > Just in case its not yet clear:
> > The codes are emitted when pressing a button. It is a button, not a
> > switch.
> > There is no state handled in hardware. On press (as noted by the code
> > comment), event code 0xc8 is emitted. On release, event code 0xc9 is
> > emitted.
> This sounds like the "former" scenario Bastien described, for which I
> understand the use case to be:
>
> User presses and releases the rotate lock button to prevent the
> accelerometer for triggering screen rotation.
>
> User presses and releases the rotate lock button to allow the accelerometer
> to trigger screen rotation.
>
> Is that correct?
>
> If so, why do we need to emit two KEY_ROTATE_LOCK_TOGGLE events each time
> instead of just the press event like the volume buttons?
Volume buttons *should* send separate press/release events, to allow software
autorepeat.
For the rotate lock button, I see no reason *not* to report the actual state
instead of doing an autorelease.
Kind regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote: > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > Currently all key events use autorelease, but this forbids use as a > > modifier key. > > > > As all event codes come in even/odd pairs, we can lookup the key type > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > handled key down event. If the key up is ignored, we keep setting the > > autorelease flag for the key down. > > What is the use-case for using these buttons as modifiers? I'm picturing one > of these devices in tablet mode, with a physical Windows button. What other > action does a user want to modify by holding the Windows button down? Or is > there another scenario we're trying to support here? Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp creates a screenshot. You can also use this in combination with an onscreen keyboard. Pressing the hardware button with the hand holding the tablet and typing with the other hand on the OSK is probably easier than hitting both keys on the OSK. Additionally, the Volume Up/Down currently do not autorepeat, as the key is autoreleased on the press event. The XPS 12 does issue distinct press/release events, so this could be done properly. The same apparently holds for some other convertibles, see the links in Patch 1/5. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 2/5] platform/x86: intel-vbtn: Support separate press/release events
On Friday, November 10, 2017 2:34:17 AM CET Darren Hart wrote: > On Thu, Nov 09, 2017 at 11:44:33PM +0100, Stefan Brüns wrote: > > Currently all key events use autorelease, but this forbids use as a > > modifier key. > > > > As all event codes come in even/odd pairs, we can lookup the key type > > (KE_KEY/KE_IGNORE) for the key up event corresponding to the currently > > handled key down event. If the key up is ignored, we keep setting the > > autorelease flag for the key down. > > What is the use-case for using these buttons as modifiers? I'm picturing one > of these devices in tablet mode, with a physical Windows button. What other > action does a user want to modify by holding the Windows button down? Or is > there another scenario we're trying to support here? Windows/KEY_LEFTMETA can be used as a modifier key, e.g. in combination with the Volume Up/Down keys. On Windows, the default for Win + VolumeUp creates a screenshot. You can also use this in combination with an onscreen keyboard. Pressing the hardware button with the hand holding the tablet and typing with the other hand on the OSK is probably easier than hitting both keys on the OSK. Additionally, the Volume Up/Down currently do not autorepeat, as the key is autoreleased on the press event. The XPS 12 does issue distinct press/release events, so this could be done properly. The same apparently holds for some other convertibles, see the links in Patch 1/5. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
On Friday, November 10, 2017 12:46:30 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > >
> > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > > ---
> > >
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > >
> > > drivers/platform/x86/intel-vbtn.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > >
> > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key
> > > release
*/
> > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key
> > > press */
> > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down key
> > > release
*/
> > >
> > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock
> > > key
> > > press */ +{ KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },
> > > /*
> > > rotate-lock key release */>
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> >
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
>
> I expect the former, which is consistent with the volume keys preceding it
> (also ignoring the release).
Read the whole series and the cover letter, and stop making assumptions.
Regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
On Friday, November 10, 2017 12:46:30 AM CET Darren Hart wrote:
> On Fri, Nov 10, 2017 at 12:30:46AM +0100, Bastien Nocera wrote:
> > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > > not
> > > on BIOS A2).
> > >
> > > Signed-off-by: Stefan Brüns
> > > ---
> > >
> > > Changes in v2:
> > > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > > - Use separate up/down events
> > >
> > > drivers/platform/x86/intel-vbtn.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/platform/x86/intel-vbtn.c
> > > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > > 100644
> > > --- a/drivers/platform/x86/intel-vbtn.c
> > > +++ b/drivers/platform/x86/intel-vbtn.c
> > > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> > >
> > > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key
> > > release
*/
> > > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key
> > > press */
> > > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down key
> > > release
*/
> > >
> > > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock
> > > key
> > > press */ +{ KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },
> > > /*
> > > rotate-lock key release */>
> > How are those events sent? When pressing and releasing the key, do you
> > receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> > releasing the first time, and 0xC9 when pressing and releasing a second
> > time?
> >
> > If the former, then it's not going to work. The release is supposed to
> > be ignored, as you send the event with sparse_keymap_report_event().
>
> I expect the former, which is consistent with the volume keys preceding it
> (also ignoring the release).
Read the whole series and the cover letter, and stop making assumptions.
Regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button
On Friday, November 10, 2017 12:40:33 AM CET Bastien Nocera wrote: > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the > > front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3 > > on press/release. On the Dell, both press/release are distinct events > > while on the Helix 2 both events are generated on release. > > > > Tested on XPS 12, for info on the Helix 2 see: > > https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html > > > > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de> > > Same problem as mentioned in patch 3. Pretty sure you need to set the > Windows key release to KEY_IGNORE. > > Or better, teach the intel-vbtn driver which buttons should be > autoreleased, and which ones should send key presses and key releases > separately. This would allow handling long presses in the upper layers. First, I explicitly mentioned on the XPS 12, press/release are distinct events. Care to read? Second, if you read patch 2/5, you see support for press/release vs autorelease. Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 5/5] platform/x86: intel-vbtn: support panel front button
On Friday, November 10, 2017 12:40:33 AM CET Bastien Nocera wrote: > On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote: > > The Lenovo Helix 2 and Dell XPS 12 (9Q33) have an extra button on the > > front showing a 'Windows' logo, both reporting event codes 0xC2/0xC3 > > on press/release. On the Dell, both press/release are distinct events > > while on the Helix 2 both events are generated on release. > > > > Tested on XPS 12, for info on the Helix 2 see: > > https://www.spinics.net/lists/ibm-acpi-devel/msg03982.html > > > > Signed-off-by: Stefan Brüns > > Same problem as mentioned in patch 3. Pretty sure you need to set the > Windows key release to KEY_IGNORE. > > Or better, teach the intel-vbtn driver which buttons should be > autoreleased, and which ones should send key presses and key releases > separately. This would allow handling long presses in the upper layers. First, I explicitly mentioned on the XPS 12, press/release are distinct events. Care to read? Second, if you read patch 2/5, you see support for press/release vs autorelease. Regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 4/5] platform/x86: intel-vbtn: support KEY_ROTATE_LOCK_TOGGLE
On Friday, November 10, 2017 12:30:46 AM CET Bastien Nocera wrote:
> On Thu, 2017-11-09 at 23:44 +0100, Stefan Brüns wrote:
> > The Rotate Lock button event is emitted on the XPS 12 (BIOS A8, but
> > not
> > on BIOS A2).
> >
> > Signed-off-by: Stefan Brüns <stefan.bru...@rwth-aachen.de>
> > ---
> >
> > Changes in v2:
> > - Emit KEY_ROTATE_LOCK_TOGGLE instead of KEY_ROTATE_DISPLAY
> > - Use separate up/down events
> >
> > drivers/platform/x86/intel-vbtn.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel-vbtn.c
> > b/drivers/platform/x86/intel-vbtn.c index e3f6375af85c..a484bcc6393b
> > 100644
> > --- a/drivers/platform/x86/intel-vbtn.c
> > +++ b/drivers/platform/x86/intel-vbtn.c
> > @@ -42,6 +42,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
> >
> > { KE_IGNORE, 0xC5, { KEY_VOLUMEUP } }, /* volume-up key
> > release */
> > { KE_KEY, 0xC6, { KEY_VOLUMEDOWN } }, /* volume-down key
> > press */
> > { KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },/* volume-down key
> > release
*/
> >
> > + { KE_KEY,0xC8, { KEY_ROTATE_LOCK_TOGGLE } },/* rotate-lock
> > key
> > press */ + { KE_KEY,0xC9, { KEY_ROTATE_LOCK_TOGGLE } },/*
> > rotate-lock key release */
> How are those events sent? When pressing and releasing the key, do you
> receive 0xC8 followed by 0xC9? Or do you receive 0xC8 when pressing and
> releasing the first time, and 0xC9 when pressing and releasing a second
> time?
>
> If the former, then it's not going to work. The release is supposed to
> be ignored, as you send the event with sparse_keymap_report_event().
>
> If the latter, and there's an actual state, does it disable a device
> on-board, or activate an LED? If so, then it would need to be a switch,
> not a key.
Do you think I don't test the patches before sending? Let me tell you, it
*does* work.
You could also read the cover letter, where you find more details, putting the
patches in relation to each other.
Just in case its not yet clear:
The codes are emitted when pressing a button. It is a button, not a switch.
There is no state handled in hardware. On press (as noted by the code
comment), event code 0xc8 is emitted. On release, event code 0xc9 is emitted.
Regards,
Stefan
--
Stefan Brüns / Bergstraße 21 / 52062 Aachen
home: +49 241 53809034 mobile: +49 151 50412019
signature.asc
Description: This is a digitally signed message part.
