* [PATCH v2 0/2] add periodic mode, threshold options
@ 2021-06-21 14:30 Ivan Mikhaylov
2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Ivan Mikhaylov @ 2021-06-21 14:30 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler
Cc: Ivan Mikhaylov, linux-kernel, linux-iio
Add periodic mode enablement, high/low threshold options.
Changes from v1:
1. Remove changes for hwmon driver and changes affecting
vcnl3020 data structure.
2. Add enable/disable periodic mode functions.
Ivan Mikhaylov (2):
iio: proximity: vcnl3020: add periodic mode
iio: proximity: vcnl3020: add threshold options
drivers/iio/proximity/vcnl3020.c | 306 ++++++++++++++++++++++++++++++-
1 file changed, 304 insertions(+), 2 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode 2021-06-21 14:30 [PATCH v2 0/2] add periodic mode, threshold options Ivan Mikhaylov @ 2021-06-21 14:30 ` Ivan Mikhaylov 2021-06-23 8:47 ` kernel test robot 2021-07-04 17:25 ` Jonathan Cameron 2021-06-21 14:30 ` [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options Ivan Mikhaylov 2021-07-04 17:15 ` [PATCH v2 0/2] add periodic mode, " Jonathan Cameron 2 siblings, 2 replies; 7+ messages in thread From: Ivan Mikhaylov @ 2021-06-21 14:30 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: Ivan Mikhaylov, linux-kernel, linux-iio Add the possibility to run proximity sensor in periodic measurement mode. Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> --- drivers/iio/proximity/vcnl3020.c | 211 ++++++++++++++++++++++++++++++- 1 file changed, 209 insertions(+), 2 deletions(-) diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c index 43817f6b3086..2e65127d5359 100644 --- a/drivers/iio/proximity/vcnl3020.c +++ b/drivers/iio/proximity/vcnl3020.c @@ -3,7 +3,6 @@ * Support for Vishay VCNL3020 proximity sensor on i2c bus. * Based on Vishay VCNL4000 driver code. * - * TODO: interrupts. */ #include <linux/module.h> @@ -11,9 +10,11 @@ #include <linux/err.h> #include <linux/delay.h> #include <linux/regmap.h> +#include <linux/interrupt.h> #include <linux/iio/iio.h> -#include <linux/iio/sysfs.h> +#include <linux/iio/iio.h> +#include <linux/iio/events.h> #define VCNL3020_PROD_ID 0x21 @@ -37,6 +38,21 @@ * measurement */ +/* Enables periodic proximity measurement */ +#define VCNL_PS_EN BIT(1) + +/* Enables state machine and LP oscillator for self timed measurements */ +#define VCNL_PS_SELFTIMED_EN BIT(0) + +/* Bit masks for ICR */ + +/* Enable interrupts on low or high thresholds */ +#define VCNL_ICR_THRES_EN BIT(1) + +/* Bit masks for ISR */ +#define VCNL_INT_TH_HI BIT(0) /* High threshold hit */ +#define VCNL_INT_TH_LOW BIT(1) /* Low threshold hit */ + #define VCNL_ON_DEMAND_TIMEOUT_US 100000 #define VCNL_POLL_US 20000 @@ -215,12 +231,145 @@ static int vcnl3020_write_proxy_samp_freq(struct vcnl3020_data *data, int val, return regmap_write(data->regmap, VCNL_PROXIMITY_RATE, index); } +static bool vcnl3020_is_in_periodic_mode(struct vcnl3020_data *data) +{ + int rc; + unsigned int cmd; + + rc = regmap_read(data->regmap, VCNL_COMMAND, &cmd); + if (rc) + return false; + + return !!(cmd & VCNL_PS_SELFTIMED_EN); +} + +static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data) +{ + int rc; + unsigned int icr; + + rc = regmap_read(data->regmap, VCNL_PS_ICR, &icr); + if (rc) + return false; + + return !!(icr & VCNL_ICR_THRES_EN); +} + +static int vcnl3020_enable_periodic(struct iio_dev *indio_dev, + struct vcnl3020_data *data) +{ + int rc; + int icr; + int cmd; + + mutex_lock(&data->lock); + + /* Enable periodic measurement of proximity data. */ + cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN; + + /* + * Enable interrupts on threshold, for proximity data by + * default. + */ + icr = VCNL_ICR_THRES_EN; + + rc = regmap_write(data->regmap, VCNL_COMMAND, cmd); + if (rc) + goto out_mutex; + + rc = regmap_write(data->regmap, VCNL_PS_ICR, icr); + +out_mutex: + mutex_unlock(&data->lock); + + return rc; +} + +static int vcnl3020_disable_periodic(struct iio_dev *indio_dev, + struct vcnl3020_data *data) +{ + int rc; + + mutex_lock(&data->lock); + + rc = regmap_write(data->regmap, VCNL_COMMAND, 0); + if (rc) + goto out_mutex; + + rc = regmap_write(data->regmap, VCNL_PS_ICR, 0); + if (rc) + goto out_mutex; + + /* Clear interrupt flag bit */ + rc = regmap_write(data->regmap, VCNL_ISR, 0); + +out_mutex: + mutex_unlock(&data->lock); + + return rc; +} + +static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state) +{ + struct vcnl3020_data *data = iio_priv(indio_dev); + int rc; + + if (state) { + rc = vcnl3020_enable_periodic(indio_dev, data); + } else { + if (!vcnl3020_is_thr_enabled(data)) + return 0; + rc = vcnl3020_disable_periodic(indio_dev, data); + } + + return rc; +} + +static int vcnl3020_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + int state) +{ + switch (chan->type) { + case IIO_PROXIMITY: + return vcnl3020_config_threshold(indio_dev, state); + default: + return -EINVAL; + } +} + +static int vcnl3020_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct vcnl3020_data *data = iio_priv(indio_dev); + + switch (chan->type) { + case IIO_PROXIMITY: + return vcnl3020_is_thr_enabled(data); + default: + return -EINVAL; + } +} + +static const struct iio_event_spec vcnl3020_event_spec[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_separate = BIT(IIO_EV_INFO_ENABLE), + }, +}; + static const struct iio_chan_spec vcnl3020_channels[] = { { .type = IIO_PROXIMITY, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SAMP_FREQ), .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), + .event_spec = vcnl3020_event_spec, + .num_event_specs = ARRAY_SIZE(vcnl3020_event_spec), }, }; @@ -233,6 +382,11 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: + + /* Protect against event capture. */ + if (vcnl3020_is_in_periodic_mode(data)) + return -EBUSY; + rc = vcnl3020_measure_proximity(data, val); if (rc) return rc; @@ -254,6 +408,10 @@ static int vcnl3020_write_raw(struct iio_dev *indio_dev, int rc; struct vcnl3020_data *data = iio_priv(indio_dev); + /* Protect against event capture. */ + if (vcnl3020_is_in_periodic_mode(data)) + return -EBUSY; + switch (mask) { case IIO_CHAN_INFO_SAMP_FREQ: rc = iio_device_claim_direct_mode(indio_dev); @@ -287,6 +445,8 @@ static const struct iio_info vcnl3020_info = { .read_raw = vcnl3020_read_raw, .write_raw = vcnl3020_write_raw, .read_avail = vcnl3020_read_avail, + .read_event_config = vcnl3020_read_event_config, + .write_event_config = vcnl3020_write_event_config, }; static const struct regmap_config vcnl3020_regmap_config = { @@ -295,6 +455,38 @@ static const struct regmap_config vcnl3020_regmap_config = { .max_register = VCNL_PS_MOD_ADJ, }; +static irqreturn_t vcnl3020_handle_irq(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct vcnl3020_data *data = iio_priv(indio_dev); + unsigned int isr; + int rc; + + rc = regmap_read(data->regmap, VCNL_ISR, &isr); + if (rc) { + dev_err(data->dev, "Error (%d) reading reg (0x%x)\n", + rc, VCNL_ISR); + return IRQ_HANDLED; + } + + if (isr & VCNL_ICR_THRES_EN) { + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, + 1, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING), + iio_get_time_ns(indio_dev)); + + rc = regmap_write(data->regmap, VCNL_ISR, + isr & VCNL_ICR_THRES_EN); + if (rc) + dev_err(data->dev, "Error (%d) writing in reg (0x%x)\n", + rc, VCNL_ISR); + } + + return IRQ_HANDLED; +} + static int vcnl3020_probe(struct i2c_client *client) { struct vcnl3020_data *data; @@ -327,6 +519,21 @@ static int vcnl3020_probe(struct i2c_client *client) indio_dev->name = "vcnl3020"; indio_dev->modes = INDIO_DIRECT_MODE; + if (client->irq) { + rc = devm_request_irq(&client->dev, client->irq, + vcnl3020_handle_irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (rc) { + dev_err(&client->dev, + "Error (%d) irq request failed (%u)\n", rc, + client->irq); + return rc; + } + } else { + dev_info(&client->dev, "Starting without irq handle\n"); + } + return devm_iio_device_register(&client->dev, indio_dev); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode 2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov @ 2021-06-23 8:47 ` kernel test robot 2021-07-04 17:25 ` Jonathan Cameron 1 sibling, 0 replies; 7+ messages in thread From: kernel test robot @ 2021-06-23 8:47 UTC (permalink / raw) To: Ivan Mikhaylov, Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: kbuild-all, Ivan Mikhaylov, linux-kernel, linux-iio [-- Attachment #1: Type: text/plain, Size: 2350 bytes --] Hi Ivan, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on iio/togreg] [also build test WARNING on v5.13-rc7 next-20210621] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Ivan-Mikhaylov/add-periodic-mode-threshold-options/20210621-222331 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg config: x86_64-randconfig-b001-20210621 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project e1adf90826a57b674eee79b071fb46c1f5683cd0) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # apt-get install iwyu # include-what-you-use # https://github.com/0day-ci/linux/commit/06c36ea9525abdc7882a330375c877709fe7ee2d git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Ivan-Mikhaylov/add-periodic-mode-threshold-options/20210621-222331 git checkout 06c36ea9525abdc7882a330375c877709fe7ee2d # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross C=1 CHECK=iwyu ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> iwyu warnings: (new ones prefixed by >>) drivers/iio/proximity/vcnl3020.c:11:1: iwyu: warning: superfluous #include <linux/delay.h> >> drivers/iio/proximity/vcnl3020.c:16:1: iwyu: warning: superfluous #include <linux/iio/iio.h> vim +16 drivers/iio/proximity/vcnl3020.c ac101e6b315bfe Ivan Mikhaylov 2020-05-10 14 ac101e6b315bfe Ivan Mikhaylov 2020-05-10 15 #include <linux/iio/iio.h> 06c36ea9525abd Ivan Mikhaylov 2021-06-21 @16 #include <linux/iio/iio.h> 06c36ea9525abd Ivan Mikhaylov 2021-06-21 17 #include <linux/iio/events.h> ac101e6b315bfe Ivan Mikhaylov 2020-05-10 18 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 53846 bytes --] [-- Attachment #3: Type: text/plain, Size: 149 bytes --] _______________________________________________ kbuild mailing list -- kbuild@lists.01.org To unsubscribe send an email to kbuild-leave@lists.01.org ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode 2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov 2021-06-23 8:47 ` kernel test robot @ 2021-07-04 17:25 ` Jonathan Cameron 1 sibling, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2021-07-04 17:25 UTC (permalink / raw) To: Ivan Mikhaylov Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, linux-kernel, linux-iio On Mon, 21 Jun 2021 17:30:50 +0300 Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > Add the possibility to run proximity sensor in periodic measurement > mode. > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> Hi Ivan, I'd probably have combined the two patches here, on the basis that a threshold detector with no threshold value setting doesn't make much sense. Still doesn't matter much. A few minor comments inline. Thanks, Jonathan > --- > drivers/iio/proximity/vcnl3020.c | 211 ++++++++++++++++++++++++++++++- > 1 file changed, 209 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c > index 43817f6b3086..2e65127d5359 100644 > --- a/drivers/iio/proximity/vcnl3020.c > +++ b/drivers/iio/proximity/vcnl3020.c > @@ -3,7 +3,6 @@ > * Support for Vishay VCNL3020 proximity sensor on i2c bus. > * Based on Vishay VCNL4000 driver code. > * > - * TODO: interrupts. > */ > > #include <linux/module.h> > @@ -11,9 +10,11 @@ > #include <linux/err.h> > #include <linux/delay.h> > #include <linux/regmap.h> > +#include <linux/interrupt.h> > > #include <linux/iio/iio.h> > -#include <linux/iio/sysfs.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/events.h> > > #define VCNL3020_PROD_ID 0x21 > > @@ -37,6 +38,21 @@ > * measurement > */ > > +/* Enables periodic proximity measurement */ > +#define VCNL_PS_EN BIT(1) > + > +/* Enables state machine and LP oscillator for self timed measurements */ > +#define VCNL_PS_SELFTIMED_EN BIT(0) > + > +/* Bit masks for ICR */ > + > +/* Enable interrupts on low or high thresholds */ > +#define VCNL_ICR_THRES_EN BIT(1) > + > +/* Bit masks for ISR */ > +#define VCNL_INT_TH_HI BIT(0) /* High threshold hit */ > +#define VCNL_INT_TH_LOW BIT(1) /* Low threshold hit */ > + > #define VCNL_ON_DEMAND_TIMEOUT_US 100000 > #define VCNL_POLL_US 20000 > > @@ -215,12 +231,145 @@ static int vcnl3020_write_proxy_samp_freq(struct vcnl3020_data *data, int val, > return regmap_write(data->regmap, VCNL_PROXIMITY_RATE, index); > } > > +static bool vcnl3020_is_in_periodic_mode(struct vcnl3020_data *data) > +{ > + int rc; > + unsigned int cmd; > + > + rc = regmap_read(data->regmap, VCNL_COMMAND, &cmd); > + if (rc) > + return false; > + > + return !!(cmd & VCNL_PS_SELFTIMED_EN); > +} > + > +static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data) > +{ > + int rc; > + unsigned int icr; > + > + rc = regmap_read(data->regmap, VCNL_PS_ICR, &icr); > + if (rc) > + return false; > + > + return !!(icr & VCNL_ICR_THRES_EN); > +} > + > +static int vcnl3020_enable_periodic(struct iio_dev *indio_dev, > + struct vcnl3020_data *data) > +{ > + int rc; > + int icr; > + int cmd; > + > + mutex_lock(&data->lock); > + > + /* Enable periodic measurement of proximity data. */ > + cmd = VCNL_PS_EN | VCNL_PS_SELFTIMED_EN; > + > + /* > + * Enable interrupts on threshold, for proximity data by > + * default. > + */ > + icr = VCNL_ICR_THRES_EN; Why have this local variable. Just put it it directly in the regmap_write() call below. > + > + rc = regmap_write(data->regmap, VCNL_COMMAND, cmd); > + if (rc) > + goto out_mutex; > + > + rc = regmap_write(data->regmap, VCNL_PS_ICR, icr); > + > +out_mutex: > + mutex_unlock(&data->lock); > + > + return rc; > +} > + > +static int vcnl3020_disable_periodic(struct iio_dev *indio_dev, > + struct vcnl3020_data *data) > +{ > + int rc; > + > + mutex_lock(&data->lock); > + > + rc = regmap_write(data->regmap, VCNL_COMMAND, 0); > + if (rc) > + goto out_mutex; > + > + rc = regmap_write(data->regmap, VCNL_PS_ICR, 0); > + if (rc) > + goto out_mutex; > + > + /* Clear interrupt flag bit */ > + rc = regmap_write(data->regmap, VCNL_ISR, 0); > + > +out_mutex: > + mutex_unlock(&data->lock); > + > + return rc; > +} > + > +static int vcnl3020_config_threshold(struct iio_dev *indio_dev, bool state) > +{ > + struct vcnl3020_data *data = iio_priv(indio_dev); > + int rc; > + > + if (state) { > + rc = vcnl3020_enable_periodic(indio_dev, data); return vcnl... Save me reading down a few lines to see if anything else happens in this path. > + } else { > + if (!vcnl3020_is_thr_enabled(data)) > + return 0; > + rc = vcnl3020_disable_periodic(indio_dev, data); return vcnl... > + } > + > + return rc; > +} > + > +static int vcnl3020_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + int state) > +{ > + switch (chan->type) { > + case IIO_PROXIMITY: > + return vcnl3020_config_threshold(indio_dev, state); > + default: > + return -EINVAL; > + } > +} > + > +static int vcnl3020_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct vcnl3020_data *data = iio_priv(indio_dev); > + > + switch (chan->type) { > + case IIO_PROXIMITY: > + return vcnl3020_is_thr_enabled(data); > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_event_spec vcnl3020_event_spec[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_separate = BIT(IIO_EV_INFO_ENABLE), > + }, > +}; > + > static const struct iio_chan_spec vcnl3020_channels[] = { > { > .type = IIO_PROXIMITY, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > BIT(IIO_CHAN_INFO_SAMP_FREQ), > .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .event_spec = vcnl3020_event_spec, > + .num_event_specs = ARRAY_SIZE(vcnl3020_event_spec), > }, > }; > > @@ -233,6 +382,11 @@ static int vcnl3020_read_raw(struct iio_dev *indio_dev, > > switch (mask) { > case IIO_CHAN_INFO_RAW: > + > + /* Protect against event capture. */ > + if (vcnl3020_is_in_periodic_mode(data)) > + return -EBUSY; > + > rc = vcnl3020_measure_proximity(data, val); > if (rc) > return rc; > @@ -254,6 +408,10 @@ static int vcnl3020_write_raw(struct iio_dev *indio_dev, > int rc; > struct vcnl3020_data *data = iio_priv(indio_dev); > > + /* Protect against event capture. */ > + if (vcnl3020_is_in_periodic_mode(data)) What can happen if we race against enabling of periodic_mode? Is it just a bit unpredicatable or can we cause anything really nasty to happen? If just a bit unpredictable then the protection you have here is fine. > + return -EBUSY; > + > switch (mask) { > case IIO_CHAN_INFO_SAMP_FREQ: > rc = iio_device_claim_direct_mode(indio_dev); > @@ -287,6 +445,8 @@ static const struct iio_info vcnl3020_info = { > .read_raw = vcnl3020_read_raw, > .write_raw = vcnl3020_write_raw, > .read_avail = vcnl3020_read_avail, > + .read_event_config = vcnl3020_read_event_config, > + .write_event_config = vcnl3020_write_event_config, > }; > > static const struct regmap_config vcnl3020_regmap_config = { > @@ -295,6 +455,38 @@ static const struct regmap_config vcnl3020_regmap_config = { > .max_register = VCNL_PS_MOD_ADJ, > }; > > +static irqreturn_t vcnl3020_handle_irq(int irq, void *p) > +{ > + struct iio_dev *indio_dev = p; > + struct vcnl3020_data *data = iio_priv(indio_dev); > + unsigned int isr; > + int rc; > + > + rc = regmap_read(data->regmap, VCNL_ISR, &isr); > + if (rc) { > + dev_err(data->dev, "Error (%d) reading reg (0x%x)\n", > + rc, VCNL_ISR); > + return IRQ_HANDLED; > + } > + > + if (isr & VCNL_ICR_THRES_EN) { > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, > + 1, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + iio_get_time_ns(indio_dev)); > + > + rc = regmap_write(data->regmap, VCNL_ISR, > + isr & VCNL_ICR_THRES_EN); > + if (rc) > + dev_err(data->dev, "Error (%d) writing in reg (0x%x)\n", > + rc, VCNL_ISR); > + } If it's not an event, should we not return IRQ_NONE to let the kernel's spurious interrupt handing know about it? We can't safely do that in error cases, because it might be our interrupt, but in this case we know it isn't. > + > + return IRQ_HANDLED; > +} > + > static int vcnl3020_probe(struct i2c_client *client) > { > struct vcnl3020_data *data; > @@ -327,6 +519,21 @@ static int vcnl3020_probe(struct i2c_client *client) > indio_dev->name = "vcnl3020"; > indio_dev->modes = INDIO_DIRECT_MODE; > > + if (client->irq) { > + rc = devm_request_irq(&client->dev, client->irq, Use a threaded irq request. We don't want to have any chance of calling the handle_irq in interrupt context as many i2c controllers will sleep. > + vcnl3020_handle_irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, General convention these days is to not specify the direction in here as it prevents the driver working if there is an inverter on the interrupt line. The sense of the interrupt should be set by firmware / device tree etc. > + indio_dev->name, indio_dev); > + if (rc) { > + dev_err(&client->dev, > + "Error (%d) irq request failed (%u)\n", rc, > + client->irq); > + return rc; > + } > + } else { > + dev_info(&client->dev, "Starting without irq handle\n"); Not interesting. If anyone want to know the, can look in /proc/interrupts So drop this print. > + } > + > return devm_iio_device_register(&client->dev, indio_dev); > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options 2021-06-21 14:30 [PATCH v2 0/2] add periodic mode, threshold options Ivan Mikhaylov 2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov @ 2021-06-21 14:30 ` Ivan Mikhaylov 2021-07-04 17:30 ` Jonathan Cameron 2021-07-04 17:15 ` [PATCH v2 0/2] add periodic mode, " Jonathan Cameron 2 siblings, 1 reply; 7+ messages in thread From: Ivan Mikhaylov @ 2021-06-21 14:30 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler Cc: Ivan Mikhaylov, linux-kernel, linux-iio Add the low/high threshold options. Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> --- drivers/iio/proximity/vcnl3020.c | 95 ++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c index 2e65127d5359..f3320de014e4 100644 --- a/drivers/iio/proximity/vcnl3020.c +++ b/drivers/iio/proximity/vcnl3020.c @@ -255,6 +255,91 @@ static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data) return !!(icr & VCNL_ICR_THRES_EN); } +static int vcnl3020_read_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, int *val2) +{ + int rc; + struct vcnl3020_data *data = iio_priv(indio_dev); + __be16 res; + + switch (info) { + case IIO_EV_INFO_VALUE: + switch (dir) { + case IIO_EV_DIR_RISING: + rc = regmap_bulk_read(data->regmap, VCNL_PS_HI_THR_HI, + &res, sizeof(res)); + if (rc < 0) + return rc; + *val = be16_to_cpu(res); + return IIO_VAL_INT; + case IIO_EV_DIR_FALLING: + rc = regmap_bulk_read(data->regmap, VCNL_PS_LO_THR_HI, + &res, sizeof(res)); + if (rc < 0) + return rc; + *val = be16_to_cpu(res); + return IIO_VAL_INT; + default: + return -EINVAL; + } + default: + return -EINVAL; + } +} + +static int vcnl3020_write_event(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int val, int val2) +{ + int rc; + __be16 buf; + struct vcnl3020_data *data = iio_priv(indio_dev); + + rc = iio_device_claim_direct_mode(indio_dev); + if (rc) + return rc; + + switch (info) { + case IIO_EV_INFO_VALUE: + switch (dir) { + case IIO_EV_DIR_RISING: + /* 16 bit word/ low * high */ + buf = cpu_to_be16(val); + rc = regmap_bulk_write(data->regmap, VCNL_PS_HI_THR_HI, + &buf, sizeof(buf)); + if (rc < 0) + goto out_release_direct_mode; + rc = IIO_VAL_INT; + goto out_release_direct_mode; + case IIO_EV_DIR_FALLING: + buf = cpu_to_be16(val); + rc = regmap_bulk_write(data->regmap, VCNL_PS_LO_THR_HI, + &buf, sizeof(buf)); + if (rc < 0) + goto out_release_direct_mode; + rc = IIO_VAL_INT; + goto out_release_direct_mode; + default: + rc = -EINVAL; + goto out_release_direct_mode; + } + default: + rc = -EINVAL; + goto out_release_direct_mode; + } +out_release_direct_mode: + iio_device_release_direct_mode(indio_dev); + + return rc; +} + static int vcnl3020_enable_periodic(struct iio_dev *indio_dev, struct vcnl3020_data *data) { @@ -356,6 +441,14 @@ static int vcnl3020_read_event_config(struct iio_dev *indio_dev, static const struct iio_event_spec vcnl3020_event_spec[] = { { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE), + }, { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE), + }, { .type = IIO_EV_TYPE_THRESH, .dir = IIO_EV_DIR_EITHER, .mask_separate = BIT(IIO_EV_INFO_ENABLE), @@ -445,6 +538,8 @@ static const struct iio_info vcnl3020_info = { .read_raw = vcnl3020_read_raw, .write_raw = vcnl3020_write_raw, .read_avail = vcnl3020_read_avail, + .read_event_value = vcnl3020_read_event, + .write_event_value = vcnl3020_write_event, .read_event_config = vcnl3020_read_event_config, .write_event_config = vcnl3020_write_event_config, }; -- 2.31.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options 2021-06-21 14:30 ` [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options Ivan Mikhaylov @ 2021-07-04 17:30 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2021-07-04 17:30 UTC (permalink / raw) To: Ivan Mikhaylov Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, linux-kernel, linux-iio On Mon, 21 Jun 2021 17:30:51 +0300 Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > Add the low/high threshold options. > > Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com> Hi, A few comments inline. Thanks, Jonathan > --- > drivers/iio/proximity/vcnl3020.c | 95 ++++++++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/drivers/iio/proximity/vcnl3020.c b/drivers/iio/proximity/vcnl3020.c > index 2e65127d5359..f3320de014e4 100644 > --- a/drivers/iio/proximity/vcnl3020.c > +++ b/drivers/iio/proximity/vcnl3020.c > @@ -255,6 +255,91 @@ static bool vcnl3020_is_thr_enabled(struct vcnl3020_data *data) > return !!(icr & VCNL_ICR_THRES_EN); > } > > +static int vcnl3020_read_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, int *val2) > +{ > + int rc; > + struct vcnl3020_data *data = iio_priv(indio_dev); > + __be16 res; > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + rc = regmap_bulk_read(data->regmap, VCNL_PS_HI_THR_HI, > + &res, sizeof(res)); > + if (rc < 0) > + return rc; > + *val = be16_to_cpu(res); > + return IIO_VAL_INT; > + case IIO_EV_DIR_FALLING: > + rc = regmap_bulk_read(data->regmap, VCNL_PS_LO_THR_HI, > + &res, sizeof(res)); > + if (rc < 0) > + return rc; > + *val = be16_to_cpu(res); > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int vcnl3020_write_event(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int val, int val2) > +{ > + int rc; > + __be16 buf; > + struct vcnl3020_data *data = iio_priv(indio_dev); > + > + rc = iio_device_claim_direct_mode(indio_dev); Why? The intent of that function is to protect against mode transitions, so we can't move from sysfs type captures to interrupt driven ones whilst a sysfs read is in progress. That's not the case here (or in the existing driver where this is used). So I think what you really want is a locally defined lock that allows you to ensure device state is consistent if you have any read / modify / write cycles. For these particular registers I'm not even seeing that. Regmap has it's own locking to avoid concurrency issues inside it's functions, so I'm not sure you need a lock at all. > + if (rc) > + return rc; > + > + switch (info) { > + case IIO_EV_INFO_VALUE: > + switch (dir) { > + case IIO_EV_DIR_RISING: > + /* 16 bit word/ low * high */ > + buf = cpu_to_be16(val); > + rc = regmap_bulk_write(data->regmap, VCNL_PS_HI_THR_HI, > + &buf, sizeof(buf)); > + if (rc < 0) > + goto out_release_direct_mode; > + rc = IIO_VAL_INT; > + goto out_release_direct_mode; > + case IIO_EV_DIR_FALLING: > + buf = cpu_to_be16(val); > + rc = regmap_bulk_write(data->regmap, VCNL_PS_LO_THR_HI, > + &buf, sizeof(buf)); > + if (rc < 0) > + goto out_release_direct_mode; > + rc = IIO_VAL_INT; > + goto out_release_direct_mode; > + default: > + rc = -EINVAL; > + goto out_release_direct_mode; > + } > + default: > + rc = -EINVAL; > + goto out_release_direct_mode; > + } > +out_release_direct_mode: > + iio_device_release_direct_mode(indio_dev); > + > + return rc; > +} > + > static int vcnl3020_enable_periodic(struct iio_dev *indio_dev, > struct vcnl3020_data *data) > { > @@ -356,6 +441,14 @@ static int vcnl3020_read_event_config(struct iio_dev *indio_dev, > > static const struct iio_event_spec vcnl3020_event_spec[] = { > { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > + }, { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > + }, { > .type = IIO_EV_TYPE_THRESH, > .dir = IIO_EV_DIR_EITHER, > .mask_separate = BIT(IIO_EV_INFO_ENABLE), > @@ -445,6 +538,8 @@ static const struct iio_info vcnl3020_info = { > .read_raw = vcnl3020_read_raw, > .write_raw = vcnl3020_write_raw, > .read_avail = vcnl3020_read_avail, > + .read_event_value = vcnl3020_read_event, > + .write_event_value = vcnl3020_write_event, > .read_event_config = vcnl3020_read_event_config, > .write_event_config = vcnl3020_write_event_config, > }; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] add periodic mode, threshold options 2021-06-21 14:30 [PATCH v2 0/2] add periodic mode, threshold options Ivan Mikhaylov 2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov 2021-06-21 14:30 ` [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options Ivan Mikhaylov @ 2021-07-04 17:15 ` Jonathan Cameron 2 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2021-07-04 17:15 UTC (permalink / raw) To: Ivan Mikhaylov Cc: Lars-Peter Clausen, Peter Meerwald-Stadler, linux-kernel, linux-iio On Mon, 21 Jun 2021 17:30:49 +0300 Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote: > Add periodic mode enablement, high/low threshold options. title of cover letter should always include the part number. A lot of people use threaded email clients and may need to expand the thread to see what driver this affects. Jonathan > > Changes from v1: > 1. Remove changes for hwmon driver and changes affecting > vcnl3020 data structure. > 2. Add enable/disable periodic mode functions. > > Ivan Mikhaylov (2): > iio: proximity: vcnl3020: add periodic mode > iio: proximity: vcnl3020: add threshold options > > drivers/iio/proximity/vcnl3020.c | 306 ++++++++++++++++++++++++++++++- > 1 file changed, 304 insertions(+), 2 deletions(-) > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-04 17:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-21 14:30 [PATCH v2 0/2] add periodic mode, threshold options Ivan Mikhaylov 2021-06-21 14:30 ` [PATCH v2 1/2] iio: proximity: vcnl3020: add periodic mode Ivan Mikhaylov 2021-06-23 8:47 ` kernel test robot 2021-07-04 17:25 ` Jonathan Cameron 2021-06-21 14:30 ` [PATCH v2 2/2] iio: proximity: vcnl3020: add threshold options Ivan Mikhaylov 2021-07-04 17:30 ` Jonathan Cameron 2021-07-04 17:15 ` [PATCH v2 0/2] add periodic mode, " Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).