Re: [PATCH 6/8] iio: light: Add support for Azoteq IQS621 ambient light sensor

[Jeff LaBundy]

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(&iqs621_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, &val);
> > +   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(&iqs621_als->lock);
> > +
> > +   return error;
> > +}
> > +
> > +static int iqs621_als_notifier(struct notifier_block *notifier,
> > +  unsigned long event_flags, void *context)
> > +{
> > +   struct iqs62x_ev

Re: [PATCH 6/8] iio: light: Add support for Azoteq IQS621 ambient light sensor

[Jonathan Cameron]

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(&iqs621_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, &val);
> + 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(&iqs621_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

[PATCH 6/8] iio: light: Add support for Azoteq IQS621 ambient light sensor

[Jeff LaBundy]

This patch adds support for the Azoteq IQS621 ambient light sensor,
capable of reporting intensity directly in units of lux.

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(&iqs621_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, &val);
+   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(&iqs621_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 (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
+   error = iqs621_als_init(iqs621_als);
+   if (error) {
+   dev_err(indio_dev->dev.parent,
+   "Failed to re-initialize device: %d\n", error);
+   return NOTIFY_BAD;
+   }
+
+   return NOTIFY_OK;
+   }
+
+   if (!((event_data->als_flags ^ iqs621_als->flags) &
+   IQS621_ALS_FLAGS_LIGHT))
+   return NOTIFY_DONE;
+
+   iqs621_als->flags = event_data->als_flags;
+
+