Re: [PATCH 1/8] dt-bindings: mfd: iqs62x: Add bindings
Hi Jonathan, On Tue, Oct 22, 2019 at 12:00:51PM +0100, Jonathan Cameron wrote: > On Sun, 20 Oct 2019 23:11:16 -0500 > Jeff LaBundy wrote: > > > This patch adds binding documentation for six-channel members of the > > Azoteq ProxFusion family of sensor devices. > > > > Signed-off-by: Jeff LaBundy > > I'm not sure if Lee has made the switch for mfd entirely yet, but > mostly new dt bindings need to be in yaml format as it allows > automated parsing of the examples + bindings using them for > correctness. > I'll wait for Lee or Rob's cue, but I'm happy to move to yaml if it's time to make the switch here. > One comment inline. I'm far from an expert on most of the stuff here > so will leave it for others! > > Jonathan > > > > --- > > Documentation/devicetree/bindings/mfd/iqs62x.txt | 242 > > +++ > > 1 file changed, 242 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mfd/iqs62x.txt > > > > diff --git a/Documentation/devicetree/bindings/mfd/iqs62x.txt > > b/Documentation/devicetree/bindings/mfd/iqs62x.txt > > new file mode 100644 > > index 000..089f567 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/iqs62x.txt > > @@ -0,0 +1,242 @@ > > +Azoteq IQS620A/621/622/624/625 ProxFusion Sensor Family > > + > > +Required properties: > > + > > +- compatible : Must be equal to one of the following: > > + "azoteq,iqs620a" > > + "azoteq,iqs621" > > + "azoteq,iqs622" > > + "azoteq,iqs624" > > + "azoteq,iqs625" > > + > > +- reg : I2C slave address for the device. > > + > > +- interrupts : GPIO to which the device's active-low > > RDY > > + output is connected (see [0]). > > + > > +Optional properties: > > + > > +- linux,fw-file: Specifies the name of the calibration > > and > > + configuration file selected by the driver. > > + If this property is omitted, the filename > > + is selected based on the device name with > > + ".bin" as the extension (e.g. iqs620a.bin > > + for IQS620A). > > + > > +All devices accommodate a child node (e.g. "keys") that represents touch > > key > > +support. Required properties for the "keys" child node include: > > + > > +- compatible : Must be equal to one of the following: > > + "azoteq,iqs620a-keys" > > + "azoteq,iqs621-keys" > > + "azoteq,iqs622-keys" > > + "azoteq,iqs624-keys" > > + "azoteq,iqs625-keys" > > + > > +- linux,keycodes : Specifies an array of up to 16 numeric key- > > + codes corresponding to each available touch > > + or proximity event. An 'x' in the following > > + table indicates an event is supported for a > > + given device; specify 0 for unused events. > > + > > + > > > > + | # | Event | IQS620A | IQS621 | IQS622 | IQS624 | > > IQS625 | > > + > > > > + | 0 | CH0 Touch |x|x |x |x |x > > | > > + || Antenna 1 Touch* |x|||| > > | > > + > > > > + | 1 | CH0 Proximity |x|x |x |x |x > > | > > + || Antenna 1 Proximity* |x|||| > > | > > + > > > > + | 2 | CH1 Touch |x|x |x |x |x > > | > > + || Antenna 1 Deep Touch* |x|||| > > | > > + > > > > + | 3 | CH1 Proximity |x|x |x |x |x > > | > > + > > > > + | 4 | CH2 Touch |x|||| > > | > > + > > > > + | 5 | CH2 Proximity |x|||| > > | > > + || Antenna 2 Proximity* |x|||| > > | > > + > >
Re: [PATCH 7/8] iio: proximity: Add support for Azoteq IQS622 proximity sensor
Hi Jonathan,
Thanks again for your prompt review here and the rest of the series.
On Tue, Oct 22, 2019 at 12:23:46PM +0100, Jonathan Cameron wrote:
> On Sun, 20 Oct 2019 23:11:22 -0500
> Jeff LaBundy wrote:
>
> > This patch adds support for the Azoteq IQS622 proximity sensor,
> > capable of reporting a unitless measurement of a target's prox-
> > imity to the sensor.
> >
> > Signed-off-by: Jeff LaBundy
> A few trivial bits inline + that question on the dt binding and
> whether it is something we ought to be deciding at device build
> time or whether there are devices where it should be configurable.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/proximity/Kconfig | 10 ++
> > drivers/iio/proximity/Makefile | 1 +
> > drivers/iio/proximity/iqs622-prox.c | 334
> >
> > 3 files changed, 345 insertions(+)
> > create mode 100644 drivers/iio/proximity/iqs622-prox.c
> >
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index d536014..2366fd7 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -21,6 +21,16 @@ endmenu
> >
> > menu "Proximity and distance sensors"
> >
> > +config IQS622_PROX
> > + tristate "Azoteq IQS622 proximity sensor"
> > + depends on MFD_IQS62X
> > + help
> > + Say Y here if you want to build support for the Azoteq IQS622
> > + proximity sensor.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called iqs622-prox.
> > +
> > config ISL29501
> > tristate "Intersil ISL29501 Time Of Flight sensor"
> > depends on I2C
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 0bb5f9d..802ba9d 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,6 +5,7 @@
> >
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AS3935) += as3935.o
> > +obj-$(CONFIG_IQS622_PROX) += iqs622-prox.o
> > obj-$(CONFIG_ISL29501) += isl29501.o
> > obj-$(CONFIG_LIDAR_LITE_V2)+= pulsedlight-lidar-lite-v2.o
> > obj-$(CONFIG_MB1232) += mb1232.o
> > diff --git a/drivers/iio/proximity/iqs622-prox.c
> > b/drivers/iio/proximity/iqs622-prox.c
> > new file mode 100644
> > index 000..a805fb21
> > --- /dev/null
> > +++ b/drivers/iio/proximity/iqs622-prox.c
> > @@ -0,0 +1,334 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Azoteq IQS622 Proximity Sensor
> > + *
> > + * Copyright (C) 2019
> > + * Author: Jeff LaBundy
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define IQS622_IR_FLAGS0x16
> > +#define IQS622_IR_FLAGS_TOUCH BIT(1)
> > +#define IQS622_IR_FLAGS_PROX BIT(0)
> > +
> > +#define IQS622_IR_UI_OUT 0x17
> > +
> > +#define IQS622_IR_THRESH_PROX 0x91
> > +#define IQS622_IR_THRESH_PROX_MAX 255
> > +#define IQS622_IR_THRESH_PROX_SHIFT0
> > +
> > +#define IQS622_IR_THRESH_TOUCH 0x92
> > +#define IQS622_IR_THRESH_TOUCH_MAX 1020
> > +#define IQS622_IR_THRESH_TOUCH_SHIFT 2
> > +
> > +struct iqs622_prox_private {
> > + struct iqs62x_core *iqs62x;
> > + struct notifier_block notifier;
> > + struct mutex lock;
> > + bool thresh_prox;
> > + bool event_en;
> > + u8 thresh;
> > + u8 flags;
> > +};
> > +
> > +static int iqs622_prox_init(struct iqs622_prox_private *iqs622_prox)
> > +{
> > + struct iqs62x_core *iqs62x = iqs622_prox->iqs62x;
> > + unsigned int val;
> > + int error;
> > +
> > + mutex_lock(_prox->lock);
> > +
> > + error = regmap_write(iqs62x->map,
> > +iqs622_prox->thresh_prox ? IQS622_IR_THRESH_PROX :
> > + IQS622_IR_THRESH_TOUCH,
> > +iqs622_prox->thresh);
> > + if (error)
> > + goto err_mutex;
> > +
> > + error = regmap_read(iqs62x->map, IQS622_IR_FLAGS, );
> > + if (error)
> > + goto err_mutex;
> > + iqs622_prox->flags = val;
> > +
> > + error = regmap_update_bits(iqs62x->map, IQS620_GLBL_EVENT_MASK,
> > + iqs62x->dev_desc->ir_mask,
> > + iqs622_prox->event_en ? 0 : 0xFF);
> > +
> > +err_mutex:
> > + mutex_unlock(_prox->lock);
> > +
> > + return error;
> > +}
> > +
> > +static int iqs622_prox_notifier(struct notifier_block *notifier,
> > + unsigned long event_flags, void *context)
> > +{
> > + struct iqs62x_event_data *event_data = context;
> > + struct iqs622_prox_private *iqs622_prox;
> > + struct iio_dev *indio_dev;
> > + enum iio_event_direction dir;
> > + int error;
> > +
Re: [PATCH 6/8] iio: light: Add support for Azoteq IQS621 ambient light sensor
Hi Jonathan,
Thank you for your prompt review.
On Tue, Oct 22, 2019 at 12:23:49PM +0100, Jonathan Cameron wrote:
> On Sun, 20 Oct 2019 23:11:21 -0500
> Jeff LaBundy wrote:
>
> > This patch adds support for the Azoteq IQS621 ambient light sensor,
> > capable of reporting intensity directly in units of lux.
>
> If they are in lux, should be using IIO_CHAN_INFO_PROCESSED to indicate
> that. I was wondering why we had no scale then noticed this comment.
>
Sure thing; will do.
> Other than that, this looks good to me. So with that tidied up in V2.
> Acked-by: Jonathan Cameron
>
> One trivial comment to perhaps drop an error print as overly verbose inline.
>
> >
> > Signed-off-by: Jeff LaBundy
> > ---
> > drivers/iio/light/Kconfig | 10 ++
> > drivers/iio/light/Makefile | 1 +
> > drivers/iio/light/iqs621-als.c | 361
> > +
> > 3 files changed, 372 insertions(+)
> > create mode 100644 drivers/iio/light/iqs621-als.c
> >
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 4a1a883..aad26dc 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -162,6 +162,16 @@ config GP2AP020A00F
> > To compile this driver as a module, choose M here: the
> > module will be called gp2ap020a00f.
> >
> > +config IQS621_ALS
> > + tristate "Azoteq IQS621 ambient light sensor"
> > + depends on MFD_IQS62X
> > + help
> > + Say Y here if you want to build support for the Azoteq IQS621
> > + ambient light sensor.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called iqs621-als.
> > +
> > config SENSORS_ISL29018
> > tristate "Intersil 29018 light and proximity sensor"
> > depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 00d1f9b..aa34358 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) +=
> > cros_ec_light_prox.o
> > obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> > obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> > obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o
> > +obj-$(CONFIG_IQS621_ALS) += iqs621-als.o
> > obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> > obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
> > obj-$(CONFIG_ISL29125) += isl29125.o
> > diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
> > new file mode 100644
> > index 000..92a6173
> > --- /dev/null
> > +++ b/drivers/iio/light/iqs621-als.c
> > @@ -0,0 +1,361 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Azoteq IQS621 Ambient Light Sensor
> > + *
> > + * Copyright (C) 2019
> > + * Author: Jeff LaBundy
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define IQS621_ALS_FLAGS 0x16
> > +#define IQS621_ALS_FLAGS_LIGHT BIT(7)
> > +
> > +#define IQS621_ALS_UI_OUT 0x17
> > +
> > +#define IQS621_ALS_THRESH_DARK 0x80
> > +#define IQS621_ALS_THRESH_DARK_MAX 1020
> > +#define IQS621_ALS_THRESH_DARK_SHIFT 2
> > +
> > +#define IQS621_ALS_THRESH_LIGHT0x81
> > +#define IQS621_ALS_THRESH_LIGHT_MAX4080
> > +#define IQS621_ALS_THRESH_LIGHT_SHIFT 4
> > +
> > +struct iqs621_als_private {
> > + struct iqs62x_core *iqs62x;
> > + struct notifier_block notifier;
> > + struct mutex lock;
> > + bool event_en;
> > + u8 thresh_light;
> > + u8 thresh_dark;
> > + u8 flags;
> > +};
> > +
> > +static int iqs621_als_init(struct iqs621_als_private *iqs621_als)
> > +{
> > + struct iqs62x_core *iqs62x = iqs621_als->iqs62x;
> > + unsigned int val;
> > + int error;
> > +
> > + mutex_lock(_als->lock);
> > +
> > + error = regmap_write(iqs62x->map, IQS621_ALS_THRESH_LIGHT,
> > +iqs621_als->thresh_light);
> > + if (error)
> > + goto err_mutex;
> > +
> > + error = regmap_write(iqs62x->map, IQS621_ALS_THRESH_DARK,
> > +iqs621_als->thresh_dark);
> > + if (error)
> > + goto err_mutex;
> > +
> > + error = regmap_read(iqs62x->map, IQS621_ALS_FLAGS, );
> > + if (error)
> > + goto err_mutex;
> > + iqs621_als->flags = val;
> > +
> > + error = regmap_update_bits(iqs62x->map, IQS620_GLBL_EVENT_MASK,
> > + iqs62x->dev_desc->als_mask,
> > + iqs621_als->event_en ? 0 : 0xFF);
> > +
> > +err_mutex:
> > + mutex_unlock(_als->lock);
> > +
> > + return error;
> > +}
> > +
> > +static int iqs621_als_notifier(struct notifier_block *notifier,
> > + unsigned long event_flags, void *context)
> > +{
> > + struct iqs62x_event_data
Re: [PATCH 5/8] pwm: Add support for Azoteq IQS620A PWM generator
Hi Uwe,
On Tue, Oct 22, 2019 at 08:54:15AM +0200, Uwe Kleine-König wrote:
> Hello Jeff,
>
> On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote:
> > On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote:
> > > > +struct iqs620_pwm_private {
> > > > + struct iqs62x_core *iqs62x;
> > > > + struct pwm_chip chip;
> > > > + struct notifier_block notifier;
> > > > + bool ready;
> > >
> > > This is always true, so you can drop it.
> > >
> >
> > This is here because iqs620_pwm_notifier references chip.pwms, which is
> > not allocated until after the notifier is registered and pwmchip_add is
> > called. So it protects against this (albeit unlikely) race condition:
> >
> > 1. iqs620_pwm_notifier is registered
> > 2. Device immediately suffers an asynchronous reset and notifier chain
> >is called (more on that in a bit)
> > 3. iqs620_pwm_notifier evaluates chips.pwms (NULL)
> >
> > I felt this was simpler than calling pwmchip_add before registering the
> > notifier and adding an error/tear-down path in iqs620_pwm_probe in case
> > of failure. I would be happy to add a comment or two to explain the not-
> > so-obvious purpose of this flag.
>
> Ah, understood. A comment is definitively necessary here.
>
Sure thing; will do.
> > > > +};
> > > > +
> > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > > > *pwm,
> > > > + struct pwm_state *state)
> > >
> > > Since
> > >
> > > 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state
> > > argument")
> > >
> > > this isn't the right prototype.
> > >
> >
> > Sure thing; I will add the 'const' qualifier and remove the two changes
> > to the state argument.
> >
> > > > +{
> > > > + struct iqs620_pwm_private *iqs620_pwm;
> > > > + struct iqs62x_core *iqs62x;
> > > > + int error;
> > > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS
> > > > - 1;
> > > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF);
>
> Another problem that we have here is that the period is fixed to 1 ms
> and if a consumer requests for example:
>
> .period = 500,
> .duty_cycle = 100,
>
> the hardware is actually configured for
>
> .period = 100,
> .duty_cycle = 100,
>
> . I don't have a good suggestion how to fix this. We'd need to
> draw a line somewhere and decline a request that is too far from the
> result. But where this line should be is not obvious, it should
> definitively not be implemented in the driver itself IMHO.
>
> (The only halfway sane approach would be to let lowlevel drivers
> implement a .round_state callback and then let the framework judge. But
> we're a long way from having that, so that's not a solution for today.)
>
Agreed on all counts. For now, I will mention in the 'Limitations' heading that
the period cannot be adjusted.
> > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private,
> > > > chip);
> > > > + iqs62x = iqs620_pwm->iqs62x;
> > > > +
> > > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE,
> > > > duty_clamp);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + state->period = IQS620_PWM_PERIOD_NS;
> > > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS /
> > > > 256;
> > >
> > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the
> > > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be
> > > constant inactive. If this is right please add a paragraph in the
> > > driver's comment at the top:
> > >
> > > * Limitations:
> > > * - The hardware cannot generate a 0% duty cycle
> > >
> > > (Please stick to this format, other drivers use it, too.)
> >
> > That's correct; the lowest duty cycle that can be achieved using only the
> > IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty
> > cycle by disabling the output altogether using a separate register. Would
> > that be better than flat-out saying it's impossible?
>
> There is (maybe) a small difference between disabled and 0% duty cycle,
> at least from the framework's POV: If you do:
>
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
> pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle =
> $DC, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
>
> and compare it to the expected result of
>
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 0, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
>
> the difference is that the duration of the inactive phase in the latter
> case is a multiple of 1 ms.
>
> There is no policy for lowlevel
Re: [PATCH 4/8] hwmon: Add support for Azoteq IQS620AT temperature sensor
Hi Jonathan and Guenter, On Tue, Oct 22, 2019 at 12:38:38PM +0100, Jonathan Cameron wrote: > On Mon, 21 Oct 2019 20:22:44 -0700 > Guenter Roeck wrote: > > > On 10/21/19 7:26 PM, Jeff LaBundy wrote: > > > Hi Guenter, > > > > > > Thank you for your prompt review. > > > > > > On Mon, Oct 21, 2019 at 08:38:25AM -0700, Guenter Roeck wrote: > > >> On Sun, Oct 20, 2019 at 11:11:19PM -0500, Jeff LaBundy wrote: > > >>> This patch adds support for the Azoteq IQS620AT temperature sensor, > > >>> capable of reporting its absolute die temperature. > > >>> > > >>> Signed-off-by: Jeff LaBundy > > >> > > >> Seems to me this might be more feasible as iio driver. > > >> Jonathan, what do you think ? > > >> > > > > > > Interestingly enough, this actually started as an iio driver; however the > > > "When to Use" slide of [0] made me suspect that conventional devices with > > > the temperature sensing element integrated on the die belong in hwmon. > > > > > > I then found the highly similar ad7314, which Jonathan himself appears to > > > have converted from iio to hwmon. Therefore, I placed this where existing > > > drivers seemed to match the most, especially since the temperature sensors > > > in iio generally use IR or a thermocouple. > > > > > > That being said, I would be happy to move this into iio so long as > > > Jonathan > > > does not mind, as it would limit the blast radius of this patch series. > > > > > > > I don't recall why the ad7314 driver was moved. With a conversion time of > > 40uS > > it is most definitely not a typical use case for a hwmon sensor. > > I'll be honest, I can't remember either ;) > > > > Anyway, not worth arguing about. Just don't complain later. There is an > > iio->hwmon bridge, but no hwmon->iio bridge, so the decision does have some > > impact. Specifically, userspace will have to implement both hwmon and iio > > access to handle the chip. > > So I had a very quick look at one of the data sheets. The temperature sensor > here is described as: > > "The IQS620(A) provides temperature monitoring capabilities which can be used > for temperature > change detection in order to ensure the integrity of other sensing > technology". > > Superficially this sounds like it's probably inappropriate for any sort > of system temperature monitoring. It's really just there to allow > for clever compensation algorithms for the other bits on this chip > (much like the temperature sensors we almost always get on a decent > IMU). > Correct on all counts. The "charge transfer" sensing mechanism employed by these devices is sensitive to temperature, and they employ a compensation algorithm to account for drift. Of the five devices in the series, the IQS620A and IQS621 expose the output of the temperature monitoring network to the outside world. However, the values are purely relative. The IQS620AT, however, is calibrated at the factory such that it can apply an internal scaling factor and offset in order to provide absolute measurements. The MFD driver checks these calibration values to determine if this driver can be loaded, or if the device is a plain IQS620A (no 'T') in which case only the input and PWM drivers are loaded. > Normally we'd just tack an extra channel for the temperature sensor on > to the the the sensor it is integrated with. This is a bit more > complex though as we have 3 different IIO sensors that are present > in particular part numbers and for some cases we have no IIO device > at all, but do have a temperature sensor. > > So if people are going to actually use this to compensate outputs > (not sure which ones are actually temperature sensitive btw ;) > then if those are IIO supported devices, then probably makes sense > for this to be an IIO device. It may make sense anyway if there > is any chance of adding temperature compensation to the drivers > in kernel. I suspect the only use that would actually be made > is as a trip point if something has gone horribly wrong, but > I might be wrong! > Correct again; in my opinion this device is unlikely to be chosen for any sort of system-level temperature monitoring. It's really meant for contactless key/ button/switch sensing and PWM control; a subset of the devices simply offer the internal temperature measurement to the outside world as a bonus in case it is useful. Hence, this patch. > Conclusion. I also don't feel strongly on this one as it kind of > sits between IIO and hwmon, but probably ever so slightly on the > IIO side as monitoring a sensor chip, not some other device. > Agreed on all counts; I'll move this to iio. Thank you both for the discussion. > Thanks, > > Jonathan > > > > > Guenter > > Kind regards, Jeff LaBundy
Re: [PATCH 3/8] input: keyboard: Add support for Azoteq IQS620A/621/622/624/625
Hi Dmitry,
Thank you for your prompt review.
On Tue, Oct 22, 2019 at 05:22:54PM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
>
> On Sun, Oct 20, 2019 at 11:11:18PM -0500, Jeff LaBundy wrote:
> > This patch adds touch key support for six-channel members of the
> > Azoteq ProxFusion family of sensor devices.
> >
> > Signed-off-by: Jeff LaBundy
> > ---
> > drivers/input/keyboard/Kconfig | 10 ++
> > drivers/input/keyboard/Makefile | 1 +
> > drivers/input/keyboard/iqs62x-keys.c | 340
> > +++
> > 3 files changed, 351 insertions(+)
> > create mode 100644 drivers/input/keyboard/iqs62x-keys.c
> >
> > diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> > index 8911bc2..ab10aff 100644
> > --- a/drivers/input/keyboard/Kconfig
> > +++ b/drivers/input/keyboard/Kconfig
> > @@ -657,6 +657,16 @@ config KEYBOARD_IPAQ_MICRO
> > To compile this driver as a module, choose M here: the
> > module will be called ipaq-micro-keys.
> >
> > +config KEYBOARD_IQS62X
> > + tristate "Azoteq IQS620A/621/622/624/625 touch keys"
> > + depends on MFD_IQS62X
> > + help
> > + Say Y here to enable touch-key support for six-channel members of
> > + the Azoteq ProxFusion family of sensor devices.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called iqs62x-keys.
> > +
> > config KEYBOARD_OMAP
> > tristate "TI OMAP keypad support"
> > depends on ARCH_OMAP1
> > diff --git a/drivers/input/keyboard/Makefile
> > b/drivers/input/keyboard/Makefile
> > index 9510325..ee85b7f 100644
> > --- a/drivers/input/keyboard/Makefile
> > +++ b/drivers/input/keyboard/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_KEYBOARD_TCA8418)+=
> > tca8418_keypad.o
> > obj-$(CONFIG_KEYBOARD_HIL) += hil_kbd.o
> > obj-$(CONFIG_KEYBOARD_HIL_OLD) += hilkbd.o
> > obj-$(CONFIG_KEYBOARD_IPAQ_MICRO) += ipaq-micro-keys.o
> > +obj-$(CONFIG_KEYBOARD_IQS62X) += iqs62x-keys.o
> > obj-$(CONFIG_KEYBOARD_IMX) += imx_keypad.o
> > obj-$(CONFIG_KEYBOARD_HP6XX) += jornada680_kbd.o
> > obj-$(CONFIG_KEYBOARD_HP7XX) += jornada720_kbd.o
> > diff --git a/drivers/input/keyboard/iqs62x-keys.c
> > b/drivers/input/keyboard/iqs62x-keys.c
> > new file mode 100644
> > index 000..9d929f1
> > --- /dev/null
> > +++ b/drivers/input/keyboard/iqs62x-keys.c
> > @@ -0,0 +1,340 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Azoteq IQS620A/621/622/624/625 Touch Keys
> > + *
> > + * Copyright (C) 2019
> > + * Author: Jeff LaBundy
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +enum {
> > + IQS62X_SW_HALL_N,
> > + IQS62X_SW_HALL_S,
> > +};
> > +
> > +static const char * const iqs62x_switch_names[] = {
> > + [IQS62X_SW_HALL_N] = "hall_switch_north",
> > + [IQS62X_SW_HALL_S] = "hall_switch_south",
> > +};
> > +
> > +struct iqs62x_switch_desc {
> > + enum iqs62x_event_flag flag;
> > + unsigned int code;
> > + bool enabled;
> > +};
> > +
> > +struct iqs62x_keys_private {
> > + struct iqs62x_core *iqs62x;
> > + struct input_dev *input;
> > + struct notifier_block notifier;
> > + struct iqs62x_switch_desc switches[ARRAY_SIZE(iqs62x_switch_names)];
> > + unsigned int keycode[IQS62X_NUM_KEYS];
> > + unsigned int keycodemax;
> > + u8 interval;
> > +};
> > +
> > +static int iqs62x_keys_parse_prop(struct platform_device *pdev,
> > + struct iqs62x_keys_private *iqs62x_keys)
> > +{
> > + struct device_node *keys_node = pdev->dev.of_node;
> > + struct device_node *hall_node;
> > + unsigned int val;
> > + int ret, i;
> > +
> > + if (!keys_node)
> > + return 0;
> > +
> > + ret = of_property_read_variable_u32_array(keys_node, "linux,keycodes",
> > + iqs62x_keys->keycode, 0,
> > + IQS62X_NUM_KEYS);
>
> I do not think this has to be OF-specific, so please use
> device_property_*() API.
>
Sure thing; will do.
> > + if (ret < 0) {
> > + dev_err(>dev, "Failed to read keycodes: %d\n", ret);
> > + return ret;
> > + }
> > + iqs62x_keys->keycodemax = ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(iqs62x_keys->switches); i++) {
> > + hall_node = of_get_child_by_name(keys_node,
> > +iqs62x_switch_names[i]);
> > + if (!hall_node)
> > + continue;
> > +
> > + ret = of_property_read_u32(hall_node, "linux,code", );
> > + if (ret < 0) {
> > + dev_err(>dev, "Failed to read switch code: %d\n",
> > + ret);
> > + of_node_put(hall_node);
> > + return ret;
> > + }
> > +
> > +
Re: [PATCH 3/8] input: keyboard: Add support for Azoteq IQS620A/621/622/624/625
Hi Jeff,
On Sun, Oct 20, 2019 at 11:11:18PM -0500, Jeff LaBundy wrote:
> This patch adds touch key support for six-channel members of the
> Azoteq ProxFusion family of sensor devices.
>
> Signed-off-by: Jeff LaBundy
> ---
> drivers/input/keyboard/Kconfig | 10 ++
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/iqs62x-keys.c | 340
> +++
> 3 files changed, 351 insertions(+)
> create mode 100644 drivers/input/keyboard/iqs62x-keys.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 8911bc2..ab10aff 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -657,6 +657,16 @@ config KEYBOARD_IPAQ_MICRO
> To compile this driver as a module, choose M here: the
> module will be called ipaq-micro-keys.
>
> +config KEYBOARD_IQS62X
> + tristate "Azoteq IQS620A/621/622/624/625 touch keys"
> + depends on MFD_IQS62X
> + help
> + Say Y here to enable touch-key support for six-channel members of
> + the Azoteq ProxFusion family of sensor devices.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called iqs62x-keys.
> +
> config KEYBOARD_OMAP
> tristate "TI OMAP keypad support"
> depends on ARCH_OMAP1
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 9510325..ee85b7f 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_KEYBOARD_TCA8418) +=
> tca8418_keypad.o
> obj-$(CONFIG_KEYBOARD_HIL) += hil_kbd.o
> obj-$(CONFIG_KEYBOARD_HIL_OLD) += hilkbd.o
> obj-$(CONFIG_KEYBOARD_IPAQ_MICRO)+= ipaq-micro-keys.o
> +obj-$(CONFIG_KEYBOARD_IQS62X)+= iqs62x-keys.o
> obj-$(CONFIG_KEYBOARD_IMX) += imx_keypad.o
> obj-$(CONFIG_KEYBOARD_HP6XX) += jornada680_kbd.o
> obj-$(CONFIG_KEYBOARD_HP7XX) += jornada720_kbd.o
> diff --git a/drivers/input/keyboard/iqs62x-keys.c
> b/drivers/input/keyboard/iqs62x-keys.c
> new file mode 100644
> index 000..9d929f1
> --- /dev/null
> +++ b/drivers/input/keyboard/iqs62x-keys.c
> @@ -0,0 +1,340 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Azoteq IQS620A/621/622/624/625 Touch Keys
> + *
> + * Copyright (C) 2019
> + * Author: Jeff LaBundy
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum {
> + IQS62X_SW_HALL_N,
> + IQS62X_SW_HALL_S,
> +};
> +
> +static const char * const iqs62x_switch_names[] = {
> + [IQS62X_SW_HALL_N] = "hall_switch_north",
> + [IQS62X_SW_HALL_S] = "hall_switch_south",
> +};
> +
> +struct iqs62x_switch_desc {
> + enum iqs62x_event_flag flag;
> + unsigned int code;
> + bool enabled;
> +};
> +
> +struct iqs62x_keys_private {
> + struct iqs62x_core *iqs62x;
> + struct input_dev *input;
> + struct notifier_block notifier;
> + struct iqs62x_switch_desc switches[ARRAY_SIZE(iqs62x_switch_names)];
> + unsigned int keycode[IQS62X_NUM_KEYS];
> + unsigned int keycodemax;
> + u8 interval;
> +};
> +
> +static int iqs62x_keys_parse_prop(struct platform_device *pdev,
> + struct iqs62x_keys_private *iqs62x_keys)
> +{
> + struct device_node *keys_node = pdev->dev.of_node;
> + struct device_node *hall_node;
> + unsigned int val;
> + int ret, i;
> +
> + if (!keys_node)
> + return 0;
> +
> + ret = of_property_read_variable_u32_array(keys_node, "linux,keycodes",
> + iqs62x_keys->keycode, 0,
> + IQS62X_NUM_KEYS);
I do not think this has to be OF-specific, so please use
device_property_*() API.
> + if (ret < 0) {
> + dev_err(>dev, "Failed to read keycodes: %d\n", ret);
> + return ret;
> + }
> + iqs62x_keys->keycodemax = ret;
> +
> + for (i = 0; i < ARRAY_SIZE(iqs62x_keys->switches); i++) {
> + hall_node = of_get_child_by_name(keys_node,
> + iqs62x_switch_names[i]);
> + if (!hall_node)
> + continue;
> +
> + ret = of_property_read_u32(hall_node, "linux,code", );
> + if (ret < 0) {
> + dev_err(>dev, "Failed to read switch code: %d\n",
> + ret);
> + of_node_put(hall_node);
> + return ret;
> + }
> +
> + if (of_property_read_bool(hall_node, "azoteq,use-prox"))
> + iqs62x_keys->switches[i].flag = (i == IQS62X_SW_HALL_N ?
> + IQS62X_EVENT_HALL_N_P :
> + IQS62X_EVENT_HALL_S_P);
> +
Re: [PATCH 6/8] iio: light: Add support for Azoteq IQS621 ambient light sensor
On Sun, 20 Oct 2019 23:11:21 -0500
Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS621 ambient light sensor,
> capable of reporting intensity directly in units of lux.
If they are in lux, should be using IIO_CHAN_INFO_PROCESSED to indicate
that. I was wondering why we had no scale then noticed this comment.
Other than that, this looks good to me. So with that tidied up in V2.
Acked-by: Jonathan Cameron
One trivial comment to perhaps drop an error print as overly verbose inline.
>
> Signed-off-by: Jeff LaBundy
> ---
> drivers/iio/light/Kconfig | 10 ++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/iqs621-als.c | 361
> +
> 3 files changed, 372 insertions(+)
> create mode 100644 drivers/iio/light/iqs621-als.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 4a1a883..aad26dc 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -162,6 +162,16 @@ config GP2AP020A00F
> To compile this driver as a module, choose M here: the
> module will be called gp2ap020a00f.
>
> +config IQS621_ALS
> + tristate "Azoteq IQS621 ambient light sensor"
> + depends on MFD_IQS62X
> + help
> + Say Y here if you want to build support for the Azoteq IQS621
> + ambient light sensor.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called iqs621-als.
> +
> config SENSORS_ISL29018
> tristate "Intersil 29018 light and proximity sensor"
> depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 00d1f9b..aa34358 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> obj-$(CONFIG_HID_SENSOR_PROX)+= hid-sensor-prox.o
> +obj-$(CONFIG_IQS621_ALS) += iqs621-als.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
> obj-$(CONFIG_ISL29125) += isl29125.o
> diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
> new file mode 100644
> index 000..92a6173
> --- /dev/null
> +++ b/drivers/iio/light/iqs621-als.c
> @@ -0,0 +1,361 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Azoteq IQS621 Ambient Light Sensor
> + *
> + * Copyright (C) 2019
> + * Author: Jeff LaBundy
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IQS621_ALS_FLAGS 0x16
> +#define IQS621_ALS_FLAGS_LIGHT BIT(7)
> +
> +#define IQS621_ALS_UI_OUT0x17
> +
> +#define IQS621_ALS_THRESH_DARK 0x80
> +#define IQS621_ALS_THRESH_DARK_MAX 1020
> +#define IQS621_ALS_THRESH_DARK_SHIFT 2
> +
> +#define IQS621_ALS_THRESH_LIGHT 0x81
> +#define IQS621_ALS_THRESH_LIGHT_MAX 4080
> +#define IQS621_ALS_THRESH_LIGHT_SHIFT4
> +
> +struct iqs621_als_private {
> + struct iqs62x_core *iqs62x;
> + struct notifier_block notifier;
> + struct mutex lock;
> + bool event_en;
> + u8 thresh_light;
> + u8 thresh_dark;
> + u8 flags;
> +};
> +
> +static int iqs621_als_init(struct iqs621_als_private *iqs621_als)
> +{
> + struct iqs62x_core *iqs62x = iqs621_als->iqs62x;
> + unsigned int val;
> + int error;
> +
> + mutex_lock(_als->lock);
> +
> + error = regmap_write(iqs62x->map, IQS621_ALS_THRESH_LIGHT,
> + iqs621_als->thresh_light);
> + if (error)
> + goto err_mutex;
> +
> + error = regmap_write(iqs62x->map, IQS621_ALS_THRESH_DARK,
> + iqs621_als->thresh_dark);
> + if (error)
> + goto err_mutex;
> +
> + error = regmap_read(iqs62x->map, IQS621_ALS_FLAGS, );
> + if (error)
> + goto err_mutex;
> + iqs621_als->flags = val;
> +
> + error = regmap_update_bits(iqs62x->map, IQS620_GLBL_EVENT_MASK,
> +iqs62x->dev_desc->als_mask,
> +iqs621_als->event_en ? 0 : 0xFF);
> +
> +err_mutex:
> + mutex_unlock(_als->lock);
> +
> + return error;
> +}
> +
> +static int iqs621_als_notifier(struct notifier_block *notifier,
> +unsigned long event_flags, void *context)
> +{
> + struct iqs62x_event_data *event_data = context;
> + struct iqs621_als_private *iqs621_als;
> + struct iio_dev *indio_dev;
> + enum iio_event_direction dir;
> + int error;
> +
> + iqs621_als = container_of(notifier, struct iqs621_als_private,
> + notifier);
> + indio_dev = iio_priv_to_dev(iqs621_als);
> +
> + if
Re: [PATCH 7/8] iio: proximity: Add support for Azoteq IQS622 proximity sensor
On Sun, 20 Oct 2019 23:11:22 -0500
Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS622 proximity sensor,
> capable of reporting a unitless measurement of a target's prox-
> imity to the sensor.
>
> Signed-off-by: Jeff LaBundy
A few trivial bits inline + that question on the dt binding and
whether it is something we ought to be deciding at device build
time or whether there are devices where it should be configurable.
Thanks,
Jonathan
> ---
> drivers/iio/proximity/Kconfig | 10 ++
> drivers/iio/proximity/Makefile | 1 +
> drivers/iio/proximity/iqs622-prox.c | 334
>
> 3 files changed, 345 insertions(+)
> create mode 100644 drivers/iio/proximity/iqs622-prox.c
>
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index d536014..2366fd7 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -21,6 +21,16 @@ endmenu
>
> menu "Proximity and distance sensors"
>
> +config IQS622_PROX
> + tristate "Azoteq IQS622 proximity sensor"
> + depends on MFD_IQS62X
> + help
> + Say Y here if you want to build support for the Azoteq IQS622
> + proximity sensor.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called iqs622-prox.
> +
> config ISL29501
> tristate "Intersil ISL29501 Time Of Flight sensor"
> depends on I2C
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 0bb5f9d..802ba9d 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -5,6 +5,7 @@
>
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AS3935) += as3935.o
> +obj-$(CONFIG_IQS622_PROX)+= iqs622-prox.o
> obj-$(CONFIG_ISL29501) += isl29501.o
> obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> obj-$(CONFIG_MB1232) += mb1232.o
> diff --git a/drivers/iio/proximity/iqs622-prox.c
> b/drivers/iio/proximity/iqs622-prox.c
> new file mode 100644
> index 000..a805fb21
> --- /dev/null
> +++ b/drivers/iio/proximity/iqs622-prox.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Azoteq IQS622 Proximity Sensor
> + *
> + * Copyright (C) 2019
> + * Author: Jeff LaBundy
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IQS622_IR_FLAGS 0x16
> +#define IQS622_IR_FLAGS_TOUCHBIT(1)
> +#define IQS622_IR_FLAGS_PROX BIT(0)
> +
> +#define IQS622_IR_UI_OUT 0x17
> +
> +#define IQS622_IR_THRESH_PROX0x91
> +#define IQS622_IR_THRESH_PROX_MAX255
> +#define IQS622_IR_THRESH_PROX_SHIFT 0
> +
> +#define IQS622_IR_THRESH_TOUCH 0x92
> +#define IQS622_IR_THRESH_TOUCH_MAX 1020
> +#define IQS622_IR_THRESH_TOUCH_SHIFT 2
> +
> +struct iqs622_prox_private {
> + struct iqs62x_core *iqs62x;
> + struct notifier_block notifier;
> + struct mutex lock;
> + bool thresh_prox;
> + bool event_en;
> + u8 thresh;
> + u8 flags;
> +};
> +
> +static int iqs622_prox_init(struct iqs622_prox_private *iqs622_prox)
> +{
> + struct iqs62x_core *iqs62x = iqs622_prox->iqs62x;
> + unsigned int val;
> + int error;
> +
> + mutex_lock(_prox->lock);
> +
> + error = regmap_write(iqs62x->map,
> + iqs622_prox->thresh_prox ? IQS622_IR_THRESH_PROX :
> + IQS622_IR_THRESH_TOUCH,
> + iqs622_prox->thresh);
> + if (error)
> + goto err_mutex;
> +
> + error = regmap_read(iqs62x->map, IQS622_IR_FLAGS, );
> + if (error)
> + goto err_mutex;
> + iqs622_prox->flags = val;
> +
> + error = regmap_update_bits(iqs62x->map, IQS620_GLBL_EVENT_MASK,
> +iqs62x->dev_desc->ir_mask,
> +iqs622_prox->event_en ? 0 : 0xFF);
> +
> +err_mutex:
> + mutex_unlock(_prox->lock);
> +
> + return error;
> +}
> +
> +static int iqs622_prox_notifier(struct notifier_block *notifier,
> + unsigned long event_flags, void *context)
> +{
> + struct iqs62x_event_data *event_data = context;
> + struct iqs622_prox_private *iqs622_prox;
> + struct iio_dev *indio_dev;
> + enum iio_event_direction dir;
> + int error;
> + u8 flags_mask;
> +
> + iqs622_prox = container_of(notifier, struct iqs622_prox_private,
> +notifier);
> + indio_dev = iio_priv_to_dev(iqs622_prox);
> +
> + if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
> + error = iqs622_prox_init(iqs622_prox);
> + if (error) {
> +
Re: [PATCH 4/8] hwmon: Add support for Azoteq IQS620AT temperature sensor
On Mon, 21 Oct 2019 20:22:44 -0700 Guenter Roeck wrote: > On 10/21/19 7:26 PM, Jeff LaBundy wrote: > > Hi Guenter, > > > > Thank you for your prompt review. > > > > On Mon, Oct 21, 2019 at 08:38:25AM -0700, Guenter Roeck wrote: > >> On Sun, Oct 20, 2019 at 11:11:19PM -0500, Jeff LaBundy wrote: > >>> This patch adds support for the Azoteq IQS620AT temperature sensor, > >>> capable of reporting its absolute die temperature. > >>> > >>> Signed-off-by: Jeff LaBundy > >> > >> Seems to me this might be more feasible as iio driver. > >> Jonathan, what do you think ? > >> > > > > Interestingly enough, this actually started as an iio driver; however the > > "When to Use" slide of [0] made me suspect that conventional devices with > > the temperature sensing element integrated on the die belong in hwmon. > > > > I then found the highly similar ad7314, which Jonathan himself appears to > > have converted from iio to hwmon. Therefore, I placed this where existing > > drivers seemed to match the most, especially since the temperature sensors > > in iio generally use IR or a thermocouple. > > > > That being said, I would be happy to move this into iio so long as Jonathan > > does not mind, as it would limit the blast radius of this patch series. > > > > I don't recall why the ad7314 driver was moved. With a conversion time of 40uS > it is most definitely not a typical use case for a hwmon sensor. I'll be honest, I can't remember either ;) > > Anyway, not worth arguing about. Just don't complain later. There is an > iio->hwmon bridge, but no hwmon->iio bridge, so the decision does have some > impact. Specifically, userspace will have to implement both hwmon and iio > access to handle the chip. So I had a very quick look at one of the data sheets. The temperature sensor here is described as: "The IQS620(A) provides temperature monitoring capabilities which can be used for temperature change detection in order to ensure the integrity of other sensing technology". Superficially this sounds like it's probably inappropriate for any sort of system temperature monitoring. It's really just there to allow for clever compensation algorithms for the other bits on this chip (much like the temperature sensors we almost always get on a decent IMU). Normally we'd just tack an extra channel for the temperature sensor on to the the the sensor it is integrated with. This is a bit more complex though as we have 3 different IIO sensors that are present in particular part numbers and for some cases we have no IIO device at all, but do have a temperature sensor. So if people are going to actually use this to compensate outputs (not sure which ones are actually temperature sensitive btw ;) then if those are IIO supported devices, then probably makes sense for this to be an IIO device. It may make sense anyway if there is any chance of adding temperature compensation to the drivers in kernel. I suspect the only use that would actually be made is as a trip point if something has gone horribly wrong, but I might be wrong! Conclusion. I also don't feel strongly on this one as it kind of sits between IIO and hwmon, but probably ever so slightly on the IIO side as monitoring a sensor chip, not some other device. Thanks, Jonathan > > Guenter
Re: [PATCH 8/8] iio: position: Add support for Azoteq IQS624/625 angle sensor
On Sun, 20 Oct 2019 23:11:23 -0500
Jeff LaBundy wrote:
> This patch adds support for the Azoteq IQS624 and IQS625 angular position
> sensors, capable of reporting the angle of a rotating shaft down to 1 and
> 10 degrees of accuracy, respectively.
>
> This patch also introduces a home for linear and angular position sensors.
> Unlike resolvers, they are typically contactless and use the Hall effect.
Hmm. I wonder if it makes sense to just move the resolvers into this directory
and not maintain the distinction. Guess we can do that in the future if we
feel like it! :)
This one looks good to me.
Acked-by: Jonathan Cameron
I'm assuming these will all go through mfd and an immutable branch once
everyone is happy.
Thanks,
Jonathan
>
> Signed-off-by: Jeff LaBundy
> ---
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/position/Kconfig | 19 +++
> drivers/iio/position/Makefile | 7 +
> drivers/iio/position/iqs624-pos.c | 302
> ++
> 5 files changed, 330 insertions(+)
> create mode 100644 drivers/iio/position/Kconfig
> create mode 100644 drivers/iio/position/Makefile
> create mode 100644 drivers/iio/position/iqs624-pos.c
>
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 5bd5185..d5c073a 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -88,6 +88,7 @@ source "drivers/iio/orientation/Kconfig"
> if IIO_TRIGGER
> source "drivers/iio/trigger/Kconfig"
> endif #IIO_TRIGGER
> +source "drivers/iio/position/Kconfig"
> source "drivers/iio/potentiometer/Kconfig"
> source "drivers/iio/potentiostat/Kconfig"
> source "drivers/iio/pressure/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bff682a..1712011 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -31,6 +31,7 @@ obj-y += light/
> obj-y += magnetometer/
> obj-y += multiplexer/
> obj-y += orientation/
> +obj-y += position/
> obj-y += potentiometer/
> obj-y += potentiostat/
> obj-y += pressure/
> diff --git a/drivers/iio/position/Kconfig b/drivers/iio/position/Kconfig
> new file mode 100644
> index 000..ed9f975
> --- /dev/null
> +++ b/drivers/iio/position/Kconfig
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Linear and angular position sensors
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Linear and angular position sensors"
> +
> +config IQS624_POS
> + tristate "Azoteq IQS624/625 angular position sensor"
> + depends on MFD_IQS62X
> + help
> + Say Y here if you want to build support for the Azoteq IQS624
> + and IQS625 angular position sensors.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called iqs624-pos.
> +
> +endmenu
> diff --git a/drivers/iio/position/Makefile b/drivers/iio/position/Makefile
> new file mode 100644
> index 000..3cbe7a7
> --- /dev/null
> +++ b/drivers/iio/position/Makefile
> @@ -0,0 +1,7 @@
> +#
> +# Makefile for IIO linear and angular position sensors
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +
> +obj-$(CONFIG_IQS624_POS) += iqs624-pos.o
> diff --git a/drivers/iio/position/iqs624-pos.c
> b/drivers/iio/position/iqs624-pos.c
> new file mode 100644
> index 000..d975065
> --- /dev/null
> +++ b/drivers/iio/position/iqs624-pos.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Azoteq IQS624/625 Angular Position Sensor
> + *
> + * Copyright (C) 2019
> + * Author: Jeff LaBundy
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define IQS624_POS_DEG_OUT 0x16
> +
> +#define IQS624_POS_SCALE1(314159 / 180)
> +#define IQS624_POS_SCALE210
> +
> +struct iqs624_pos_private {
> + struct iqs62x_core *iqs62x;
> + struct notifier_block notifier;
> + struct mutex lock;
> + bool event_en;
> + union {
> + u16 angle;
> + u8 interval;
> + };
> +};
> +
> +static int iqs624_pos_init(struct iqs624_pos_private *iqs624_pos)
> +{
> + struct iqs62x_core *iqs62x = iqs624_pos->iqs62x;
> + unsigned int val;
> + int error;
> + __le16 val_buf;
> +
> + if (iqs62x->dev_desc->prod_num == IQS624_PROD_NUM) {
> + error = regmap_raw_read(iqs62x->map, IQS624_POS_DEG_OUT,
> + _buf, sizeof(val_buf));
> + if (error)
> + return error;
> +
> + iqs624_pos->angle = le16_to_cpu(val_buf);
> + } else {
> + error = regmap_read(iqs62x->map, iqs62x->dev_desc->interval,
> + );
> + if (error)
> + return error;
> +
> + iqs624_pos->interval = val;
> + }
> +
> +
Re: [PATCH 1/8] dt-bindings: mfd: iqs62x: Add bindings
On Sun, 20 Oct 2019 23:11:16 -0500 Jeff LaBundy wrote: > This patch adds binding documentation for six-channel members of the > Azoteq ProxFusion family of sensor devices. > > Signed-off-by: Jeff LaBundy I'm not sure if Lee has made the switch for mfd entirely yet, but mostly new dt bindings need to be in yaml format as it allows automated parsing of the examples + bindings using them for correctness. One comment inline. I'm far from an expert on most of the stuff here so will leave it for others! Jonathan > --- > Documentation/devicetree/bindings/mfd/iqs62x.txt | 242 > +++ > 1 file changed, 242 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/iqs62x.txt > > diff --git a/Documentation/devicetree/bindings/mfd/iqs62x.txt > b/Documentation/devicetree/bindings/mfd/iqs62x.txt > new file mode 100644 > index 000..089f567 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/iqs62x.txt > @@ -0,0 +1,242 @@ > +Azoteq IQS620A/621/622/624/625 ProxFusion Sensor Family > + > +Required properties: > + > +- compatible : Must be equal to one of the following: > + "azoteq,iqs620a" > + "azoteq,iqs621" > + "azoteq,iqs622" > + "azoteq,iqs624" > + "azoteq,iqs625" > + > +- reg: I2C slave address for the device. > + > +- interrupts : GPIO to which the device's active-low RDY > + output is connected (see [0]). > + > +Optional properties: > + > +- linux,fw-file : Specifies the name of the calibration > and > + configuration file selected by the driver. > + If this property is omitted, the filename > + is selected based on the device name with > + ".bin" as the extension (e.g. iqs620a.bin > + for IQS620A). > + > +All devices accommodate a child node (e.g. "keys") that represents touch key > +support. Required properties for the "keys" child node include: > + > +- compatible : Must be equal to one of the following: > + "azoteq,iqs620a-keys" > + "azoteq,iqs621-keys" > + "azoteq,iqs622-keys" > + "azoteq,iqs624-keys" > + "azoteq,iqs625-keys" > + > +- linux,keycodes : Specifies an array of up to 16 numeric key- > + codes corresponding to each available touch > + or proximity event. An 'x' in the following > + table indicates an event is supported for a > + given device; specify 0 for unused events. > + > + > > + | # | Event | IQS620A | IQS621 | IQS622 | IQS624 | IQS625 > | > + > > + | 0 | CH0 Touch |x|x |x |x |x > | > + || Antenna 1 Touch* |x|||| > | > + > > + | 1 | CH0 Proximity |x|x |x |x |x > | > + || Antenna 1 Proximity* |x|||| > | > + > > + | 2 | CH1 Touch |x|x |x |x |x > | > + || Antenna 1 Deep Touch* |x|||| > | > + > > + | 3 | CH1 Proximity |x|x |x |x |x > | > + > > + | 4 | CH2 Touch |x|||| > | > + > > + | 5 | CH2 Proximity |x|||| > | > + || Antenna 2 Proximity* |x|||| > | > + > > + | 6 | Metal (+) Touch** |x|x ||| > | > + || Antenna 2 Deep Touch* |x|||| > | > + > > + | 7 | Metal (+) Proximity** |x|x ||| > | > + || Antenna 2 Touch* |x|
Re: [PATCH 5/8] pwm: Add support for Azoteq IQS620A PWM generator
Hello Jeff,
On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote:
> On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote:
> > > +struct iqs620_pwm_private {
> > > + struct iqs62x_core *iqs62x;
> > > + struct pwm_chip chip;
> > > + struct notifier_block notifier;
> > > + bool ready;
> >
> > This is always true, so you can drop it.
> >
>
> This is here because iqs620_pwm_notifier references chip.pwms, which is
> not allocated until after the notifier is registered and pwmchip_add is
> called. So it protects against this (albeit unlikely) race condition:
>
> 1. iqs620_pwm_notifier is registered
> 2. Device immediately suffers an asynchronous reset and notifier chain
>is called (more on that in a bit)
> 3. iqs620_pwm_notifier evaluates chips.pwms (NULL)
>
> I felt this was simpler than calling pwmchip_add before registering the
> notifier and adding an error/tear-down path in iqs620_pwm_probe in case
> of failure. I would be happy to add a comment or two to explain the not-
> so-obvious purpose of this flag.
Ah, understood. A comment is definitively necessary here.
> > > +};
> > > +
> > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device
> > > *pwm,
> > > + struct pwm_state *state)
> >
> > Since
> >
> > 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state
> > argument")
> >
> > this isn't the right prototype.
> >
>
> Sure thing; I will add the 'const' qualifier and remove the two changes
> to the state argument.
>
> > > +{
> > > + struct iqs620_pwm_private *iqs620_pwm;
> > > + struct iqs62x_core *iqs62x;
> > > + int error;
> > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1;
> > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF);
Another problem that we have here is that the period is fixed to 1 ms
and if a consumer requests for example:
.period = 500,
.duty_cycle = 100,
the hardware is actually configured for
.period = 100,
.duty_cycle = 100,
. I don't have a good suggestion how to fix this. We'd need to
draw a line somewhere and decline a request that is too far from the
result. But where this line should be is not obvious, it should
definitively not be implemented in the driver itself IMHO.
(The only halfway sane approach would be to let lowlevel drivers
implement a .round_state callback and then let the framework judge. But
we're a long way from having that, so that's not a solution for today.)
> > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> > > + iqs62x = iqs620_pwm->iqs62x;
> > > +
> > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp);
> > > + if (error)
> > > + return error;
> > > +
> > > + state->period = IQS620_PWM_PERIOD_NS;
> > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256;
> >
> > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the
> > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be
> > constant inactive. If this is right please add a paragraph in the
> > driver's comment at the top:
> >
> > * Limitations:
> > * - The hardware cannot generate a 0% duty cycle
> >
> > (Please stick to this format, other drivers use it, too.)
>
> That's correct; the lowest duty cycle that can be achieved using only the
> IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty
> cycle by disabling the output altogether using a separate register. Would
> that be better than flat-out saying it's impossible?
There is (maybe) a small difference between disabled and 0% duty cycle,
at least from the framework's POV: If you do:
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle =
$DC, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
and compare it to the expected result of
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 0, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
the difference is that the duration of the inactive phase in the latter
case is a multiple of 1 ms.
There is no policy for lowlevel drivers what to do, but disabling when
0% is requested is at least not unseen and probably more what consumers
expect.
> > How does the hardware behave on changes? For example you're first
> > committing the duty cycle and then on/off. Can it happen that between
> >
> > pwm_apply_state(pwm, { .duty_cycle = 3900, .period = 100, .enabled
> > = true)
> > ...
> > pwm_apply_state(pwm, { .duty_cycle = 100, .period = 100,
> > .enabled = false)
> >
> > the output is active for longer than 4 µs because the iqs620_pwm_apply
> > function is
