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(_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,

Re: [PATCH 7/8] iio: proximity: Add support for Azoteq IQS622 proximity sensor

[Jonathan Cameron]

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

[Jonathan Cameron]

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

[Jonathan Cameron]

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_PO

Re: [PATCH 1/8] dt-bindings: mfd: iqs62x: Add bindings

[Jonathan Cameron]

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] hwmon: Remove ads1015 driver

[Jonathan Cameron]

On Mon,  1 Jul 2019 11:12:38 -0700
Guenter Roeck  wrote:

> A driver for ADS1015 with more functionality is available in the iio
> subsystem.
> 
> Remove the hwmon driver as duplicate. If the chip is used for hardware
> monitoring, the iio->hwmon bridge should be used.
> 
> Cc: Dirk Eibach 
> Signed-off-by: Guenter Roeck 

On the basis I agree with the concept of dropping this driver:

Acked-by: Jonathan Cameron 

Thanks for tidying this up.

Jonathan

> ---
> Current plan is to queue this removal for v5.4 (not v5.3) in the hwmon
> tree.
> 
>  .../devicetree/bindings/hwmon/ads1015.txt  |  73 -
>  .../devicetree/bindings/iio/adc/ads1015.txt|  73 +
>  Documentation/hwmon/ads1015.rst|  90 --
>  Documentation/hwmon/index.rst  |   1 -
>  MAINTAINERS|   8 -
>  drivers/hwmon/Kconfig  |  10 -
>  drivers/hwmon/Makefile |   1 -
>  drivers/hwmon/ads1015.c| 324 
> -
>  drivers/iio/adc/Kconfig|   2 +-
>  9 files changed, 74 insertions(+), 508 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/hwmon/ads1015.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ads1015.txt
>  delete mode 100644 Documentation/hwmon/ads1015.rst
>  delete mode 100644 drivers/hwmon/ads1015.c
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ads1015.txt 
> b/Documentation/devicetree/bindings/hwmon/ads1015.txt
> deleted file mode 100644
> index 918a507d1159..
> --- a/Documentation/devicetree/bindings/hwmon/ads1015.txt
> +++ /dev/null
> @@ -1,73 +0,0 @@
> -ADS1015 (I2C)
> -
> -This device is a 12-bit A-D converter with 4 inputs.
> -
> -The inputs can be used single ended or in certain differential combinations.
> -
> -For configuration all possible combinations are mapped to 8 channels:
> -  0: Voltage over AIN0 and AIN1.
> -  1: Voltage over AIN0 and AIN3.
> -  2: Voltage over AIN1 and AIN3.
> -  3: Voltage over AIN2 and AIN3.
> -  4: Voltage over AIN0 and GND.
> -  5: Voltage over AIN1 and GND.
> -  6: Voltage over AIN2 and GND.
> -  7: Voltage over AIN3 and GND.
> -
> -Each channel can be configured individually:
> - - pga is the programmable gain amplifier (values are full scale)
> -0: +/- 6.144 V
> -1: +/- 4.096 V
> -2: +/- 2.048 V (default)
> -3: +/- 1.024 V
> -4: +/- 0.512 V
> -5: +/- 0.256 V
> - - data_rate in samples per second
> -0: 128
> -1: 250
> -2: 490
> -3: 920
> -4: 1600 (default)
> -5: 2400
> -6: 3300
> -
> -1) The /ads1015 node
> -
> -  Required properties:
> -
> -   - compatible : must be "ti,ads1015"
> -   - reg : I2C bus address of the device
> -   - #address-cells : must be <1>
> -   - #size-cells : must be <0>
> -
> -  The node contains child nodes for each channel that the platform uses.
> -
> -  Example ADS1015 node:
> -
> -ads1015@49 {
> - compatible = "ti,ads1015";
> - reg = <0x49>;
> - #address-cells = <1>;
> - #size-cells = <0>;
> -
> - [ child node definitions... ]
> -}
> -
> -2) channel nodes
> -
> -  Required properties:
> -
> -   - reg : the channel number
> -
> -  Optional properties:
> -
> -   - ti,gain : the programmable gain amplifier setting
> -   - ti,datarate : the converter data rate
> -
> -  Example ADS1015 channel node:
> -
> -channel@4 {
> - reg = <4>;
> - ti,gain = <3>;
> - ti,datarate = <5>;
> -};
> diff --git a/Documentation/devicetree/bindings/iio/adc/ads1015.txt 
> b/Documentation/devicetree/bindings/iio/adc/ads1015.txt
> new file mode 100644
> index ..918a507d1159
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ads1015.txt
> @@ -0,0 +1,73 @@
> +ADS1015 (I2C)
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +For configuration all possible combinations are mapped to 8 channels:
> +  0: Voltage over AIN0 and AIN1.
> +  1: Voltage over AIN0 and AIN3.
> +  2: Voltage over AIN1 and AIN3.
> +  3: Voltage over AIN2 and AIN3.
> +  4: Voltage over AIN0 and GND.
> +  5: Voltage over AIN1 and GND.
> +  6: Voltage over AIN2 and GND.
> +  7: Voltage over AIN3 and GND.
> +
> +Each channel can be configured individually:
> + - pga is the programmable gain amplifier (values are full scale)
>

Re: Removing ADS1015 driver from hwmon subsystem

[Jonathan Cameron]

On Fri, 7 Jun 2019 18:13:27 -0700
Guenter Roeck  wrote:

> Hi all,
> 
> there are currently two drivers for ADS1015 in the linux kernel:
> 
> drivers/iio/adc/ti-ads1015.c
> drivers/hwmon/ads1015.c
> 
> The driver in the iio subsystem provides more flexible support for
> the chip's capabilities. Even if the hwmon driver is removed,
> the iio->hwmon bridge can still be used and would provide the same
> information as before to the user. Existing devicetree entries
> for ads1015 both point to the iio driver. The platform include file
> is only included by the drivers, but not from any platform code.
> 
> With that in mind, keeping the hwmon driver around does not really
> add value. I suggest to remove it. Are there any objections or
> concerns ?

It is probably not worth the dance of creating a stub driver
in hwmon to bring up the iio->hwmon bridge but that might be
an option if anyone minds just dropping the ads1015 hwmon driver.

Thanks,

Jonathan

> 
> Thanks,
> Guenter

Re: [PATCH 2/2] hwmon: Add ads1000/ads1100 voltage ADCs driver

[Jonathan Cameron]

On Thu, 30 May 2019 05:55:10 -0700
Guenter Roeck  wrote:

> Hi,
> 
> On Wed, May 15, 2019 at 01:58:09AM +0300, Serge Semin wrote:
> > These are simple Texas Instruments ADC working over i2c-interface with
> > just one differential input and with configurable 12-16 bits resolution.
> > Sample rate is fixed to 128 for ads1000 and can vary from 8 to 128 for
> > ads1100. Vdd value reference value must be supplied so to properly
> > translate the sampled code to the real voltage. All of these configs are
> > implemented in the device drivers for hwmon subsystem. The next dts
> > properties should be specified to comply the device platform setup:
> >  - vdd-supply - voltage regulator connected to the Vdd pin of the device
> >  - ti,gain - programmable gain amplifier
> >  - ti,datarate - converter data rate
> >  - ti,voltage-divider - possible resistors-base external divider
> > See bindings documentation file for details.
> > 
> > Even though these devices seem more like ads1015 series, they
> > in fact pretty much different. First of all ads1000/ads1100 got less
> > capabilities: just one port, no configurations of digital comparator, no
> > input multi-channel multiplexer, smaller PGA and data-rate ranges.
> > In addition they haven't got internal voltage reference, but instead
> > are created to use Vdd pin voltage. Finally the output code value is
> > provided in different format. As a result it was much easier for
> > development and for future support to create a separate driver.
> >   
> 
> This chicp doesn't have any real hardware monitoring characteristics
> (no limit registers). It seems to be better suited to be implemented
> as iio driver. If it is used as hardware monitor, the iio-hwmon bridge
> should work just fine.
> 
> Jonathan, what do you think ?
Sorry for slow response, was on vacation.

Agreed, this looks like a standard multipurpose ADC so probably more suited
to IIO. Whether you bother with a buffered /chardev interface or not given it
is a fairly slow device is a separate question (can always be added later
when someone wants it).

Note the voltage-divider in the DT properties is something that should
have a generic representation. In IIO we have drivers/iio/afe/iio-rescale.c
for that, in this case using the voltage divider binding.

gain and datarate are both characteristics that should be controlled from
userspace rather than via a binding.

Thanks,

Jonathan
> 
> Thanks,
> Guenter
> 
> > Signed-off-by: Serge Semin 
> > ---
> >  MAINTAINERS   |   8 +
> >  drivers/hwmon/Kconfig |  10 +
> >  drivers/hwmon/Makefile|   1 +
> >  drivers/hwmon/ads1000.c   | 320 ++
> >  include/linux/platform_data/ads1000.h |  20 ++
> >  5 files changed, 359 insertions(+)
> >  create mode 100644 drivers/hwmon/ads1000.c
> >  create mode 100644 include/linux/platform_data/ads1000.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ce573aaa04df..5c3a8107ef1a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -517,6 +517,14 @@ W: 
> > http://ez.analog.com/community/linux-device-drivers
> >  S: Supported
> >  F: drivers/video/backlight/adp8860_bl.c
> >  
> > +ADS1000 HARDWARE MONITOR DRIVER
> > +M: Serge Semin 
> > +L: linux-hwmon@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/hwmon/ads1000.rst
> > +F: drivers/hwmon/ads1000.c
> > +F: include/linux/platform_data/ads1000.h
> > +
> >  ADS1015 HARDWARE MONITOR DRIVER
> >  M: Dirk Eibach 
> >  L: linux-hwmon@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 1915a18b537b..a1220cc48f2f 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1569,6 +1569,16 @@ config SENSORS_ADC128D818
> >   This driver can also be built as a module. If so, the module
> >   will be called adc128d818.
> >  
> > +config SENSORS_ADS1000
> > +   tristate "Texas Instruments ADS1000"
> > +   depends on I2C
> > +   help
> > + If you say yes here you get support for Texas Instruments
> > + ADS1000/ADS1100 12-16-bit single channel ADC device.
> > +
> > + This driver can also be built as a module.  If so, the module
> > + will be called ads1000.
> > +
> >  config SENSORS_ADS1015
> > tristate "Texas Instruments ADS1015"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 8db472ea04f0..2cd82f6c651e 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_SENSORS_ADM1026) += adm1026.o
> >  obj-$(CONFIG_SENSORS_ADM1029)  += adm1029.o
> >  obj-$(CONFIG_SENSORS_ADM1031)  += adm1031.o
> >  obj-$(CONFIG_SENSORS_ADM9240)  += adm9240.o
> > +obj-$(CONFIG_SENSORS_ADS1000)  += ads1000.o
> >  obj-$(CONFIG_SENSORS_ADS1015)  += ads1015.o
> >  obj-$(CONFIG_SENSORS_ADS7828)  += ads7828.o
> >  obj-$(CONFIG_SENSORS_ADS7871)  += ads7871.o
> > diff --git a/drivers/hwmon/ads1000.c 

Re: [PATCH v3 2/3] iio: adc: Add Xilinx AMS driver

[Jonathan Cameron]

On Mon, 29 Oct 2018 17:40:25 +0530
Manish Narani  wrote:

> The AMS includes an ADC as well as on-chip sensors that can be used to
> sample external voltages and monitor on-die operating conditions, such
> as temperature and supply voltage levels. The AMS has two SYSMON blocks.
> PL-SYSMON block is capable of monitoring off chip voltage and
> temperature.
> PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring
> from external master. Out of these interface currently only DRP is
> supported.
> Other block PS-SYSMON is memory mapped to PS.
> The AMS can use internal channels to monitor voltage and temperature as
> well as one primary and up to 16 auxiliary channels for measuring
> external voltages.
> The voltage and temperature monitoring channels also have event
> capability which allows to generate an interrupt when their value falls
> below or raises above a set threshold.
> 
> Signed-off-by: Manish Narani 

Hi Manish,

Looking pretty good. Two things:

1) There are a lot of NULLs being passed around now we have mostly
dropped the extname stuff. I would drop them from the macros.

2) I hadn't really noticed before but you set chan->address to actually
be the address of the read register, which is fine on simple devices, but
here results in a number of reverse lookups (which can't easily be
put in a static lookup table) to get to things about the channel.
Perhaps consider if this would be better done by making chan->address
an enum value (the sequence enum might work though I haven't checked) and
then move to look up tables for the actual address and related bit
positions etc.  I think it might simplify the code a fair bit.  Feel 
free to say it doesn't if I'm wrong!  It's a complex mapping in somecases
so it may not be worth the effort.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig  |   10 +
>  drivers/iio/adc/Makefile |1 +
>  drivers/iio/adc/xilinx-ams.c | 1343 
> ++
>  3 files changed, 1354 insertions(+)
>  create mode 100644 drivers/iio/adc/xilinx-ams.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 4a75492..8f7dd62 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -941,4 +941,14 @@ config XILINX_XADC
> The driver can also be build as a module. If so, the module will be 
> called
> xilinx-xadc.
>  
> +config XILINX_AMS
> + tristate "Xilinx AMS driver"
> + depends on ARCH_ZYNQMP || COMPILE_TEST
> + depends on HAS_IOMEM
> + help
> +   Say yes here to have support for the Xilinx AMS.
> +
> +   The driver can also be built as a module. If so, the module will be 
> called
> +   xilinx-ams.
> +
>  endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 03db7b5..fbfcc45 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -85,4 +85,5 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>  xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
>  obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> +obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
>  obj-$(CONFIG_SD_ADC_MODULATOR) += sd_adc_modulator.o
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> new file mode 100644
> index 000..fee1d66
> --- /dev/null
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -0,0 +1,1343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx AMS driver
> + *
> + *  Copyright (C) 2017-2018 Xilinx, Inc.
> + *
> + *  Manish Narani 
> + *  Rajnikant Bhojani 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +static const unsigned int AMS_UNMASK_TIMEOUT_MS = 500;
> +
> +/* AMS registers definitions */
> +#define AMS_ISR_00x010
> +#define AMS_ISR_10x014
> +#define AMS_IER_00x020
> +#define AMS_IER_10x024
> +#define AMS_IDR_00x028
> +#define AMS_IDR_10x02c
> +#define AMS_PS_CSTS  0x040
> +#define AMS_PL_CSTS  0x044
> +
> +#define AMS_VCC_PSPLL0   0x060
> +#define AMS_VCC_PSPLL3   0x06C
> +#define AMS_VCCINT   0x078
> +#define AMS_VCCBRAM  0x07C
> +#define AMS_VCCAUX   0x080
> +#define AMS_PSDDRPLL 0x084
> +#define AMS_PSINTFPDDR   0x09C
> +
> +#define AMS_VCC_PSPLL0_CH48
> +#define AMS_VCC_PSPLL3_CH51
> +#define AMS_VCCINT_CH54
> +#define AMS_VCCBRAM_CH   55
> +#define AMS_VCCAUX_CH56
> +#define AMS_PSDDRPLL_CH  57
> +#define AMS_PSINTFPDDR_CH63
> +
> +#define AMS_REG_CONFIG0  0x100
> +#define 

Re: [PATCH] hwmon: (iio_hwmon) Use devm functions

[Jonathan Cameron]

On Fri, 20 Jul 2018 15:11:32 -0700
Guenter Roeck  wrote:

> On 07/20/2018 07:46 AM, Maxime Roussin-Bélanger wrote:
> > Use devm_iio_channel_get_all() to automatically release
> > channels.
> > 
> > Use devm_hwmon_device_register_with_groups() to
> > automatically unregister the device.
> > 
> > Signed-off-by: Maxime Roussin-Bélanger 

Other than Guenter's comment, looks good.

Acked-by: Jonathan Cameron 

Thanks,

Jonathan

> > ---
> >   drivers/hwmon/iio_hwmon.c | 43 ++-
> >   1 file changed, 11 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index 69031a0f7ed2..5cbd87b00ce0 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -73,17 +73,16 @@ static int iio_hwmon_probe(struct platform_device *pdev)
> > if (dev->of_node && dev->of_node->name)
> > name = dev->of_node->name;
> >   
> > -   channels = iio_channel_get_all(dev);
> > +   channels = devm_iio_channel_get_all(dev);
> > if (IS_ERR(channels)) {
> > if (PTR_ERR(channels) == -ENODEV)
> > return -EPROBE_DEFER;
> > return PTR_ERR(channels);
> > }
> >   
> > st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > if (st == NULL) {
> > -   ret = -ENOMEM;
> > -   goto error_release_channels;
> > +   return -ENOMEM;
> > }
> > 
> The { } is now no longer necessary. Please drop. Same almost everywhere below.
> 
> Thanks,
> Guenter
> 
> > st->channels = channels;
> > @@ -96,21 +95,19 @@ static int iio_hwmon_probe(struct platform_device *pdev)
> >  st->num_channels + 1, sizeof(*st->attrs),
> >  GFP_KERNEL);
> > if (st->attrs == NULL) {
> > -   ret = -ENOMEM;
> > -   goto error_release_channels;
> > +   return -ENOMEM;
> > }
> >   
> > for (i = 0; i < st->num_channels; i++) {
> > a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
> > if (a == NULL) {
> > -   ret = -ENOMEM;
> > -   goto error_release_channels;
> > +   return -ENOMEM;
> > }
> >   
> > sysfs_attr_init(>dev_attr.attr);
> > ret = iio_get_channel_type(>channels[i], );
> > if (ret < 0)
> > -   goto error_release_channels;
> > +   return ret;
> >   
> > switch (type) {
> > case IIO_VOLTAGE:
> > @@ -134,12 +131,10 @@ static int iio_hwmon_probe(struct platform_device 
> > *pdev)
> >humidity_i++);
> > break;
> > default:
> > -   ret = -EINVAL;
> > -   goto error_release_channels;
> > +   return -EINVAL;
> > }
> > if (a->dev_attr.attr.name == NULL) {
> > -   ret = -ENOMEM;
> > -   goto error_release_channels;
> > +   return -ENOMEM;
> > }
> > a->dev_attr.show = iio_hwmon_read_val;
> > a->dev_attr.attr.mode = S_IRUGO;
> > @@ -152,31 +147,16 @@ static int iio_hwmon_probe(struct platform_device 
> > *pdev)
> >   
> > sname = devm_kstrdup(dev, name, GFP_KERNEL);
> > if (!sname) {
> > -   ret = -ENOMEM;
> > -   goto error_release_channels;
> > +   return -ENOMEM;
> > }
> >   
> > strreplace(sname, '-', '_');
> > -   st->hwmon_dev = hwmon_device_register_with_groups(dev, sname, st,
> > - st->groups);
> > +   st->hwmon_dev = devm_hwmon_device_register_with_groups(dev, sname, st,
> > +  st->groups);
> > if (IS_ERR(st->hwmon_dev)) {
> > -   ret = PTR_ERR(st->hwmon_dev);
> > -   goto error_release_channels;
> > +   return PTR_ERR(st->hwmon_dev);
> > }
> > platform_set_drvdata(pdev, st);
> > -   return 0;
> > -
> > -error_release_channels:
> > -   iio_channel_release_all(channels);
> > -   return ret;
> > -}
> > -
> > -static int iio_hwmon_remove(struct platform_device *pdev)
> > -{
> > -   struct iio_hwmon_state *st = platform_get_drvdata(pdev);
> > -
> > -   hwmon_device_unregister(st->hwmon_dev);
> > -   iio_channel_release_all(st->channels);
> >   
> > return 0;
> >   }
> > @@ -193,7 +173,6 @@ static struct platform_driver __refdata 
> > iio_hwmon_driver = {
> > .of_match_table = iio_hwmon_of_match,
> > },
> > .probe = iio_hwmon_probe,
> > -   .remove = iio_hwmon_remove,
> >   };
> >   
> >   module_platform_driver(iio_hwmon_driver);
> >   
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] iio: adc: ina2xx: Make calibration register value fixed

[Jonathan Cameron]

On Fri, 15 Dec 2017 08:59:23 +0100
Maciej Purski  wrote:

> Calibration register is used for calculating current register in
> hardware according to datasheet:
> current = shunt_volt * calib_register / 2048 (ina 226)
> current = shunt_volt * calib_register / 4096 (ina 219)
> 
> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in
> order to avoid truncation error and provide best precision allowed
> by shunt_voltage measurement. Make current scale value follow changes
> of shunt_resistor from sysfs as calib_register value is now fixed.
> 
> Power_lsb value should also follow shunt_resistor changes as stated in
> datasheet:
> power_lsb = 25 * current_lsb (ina 226)
> power_lsb = 20 * current_lsb (ina 219)
> 
> This is a part of the patchset: https://lkml.org/lkml/2017/11/22/394
> 
> Signed-off-by: Maciej Purski 
Looks good to me. I'll let it sit for Stefan to have a change to look at it 
though.
If it looks like I've forgotten it by early Jan do give me a kick.

Thanks,

Jonathan
> 
> ---
> Changes in v3:
> - remove variable current_lsb and calculate it on each read of current
>   and power scale value
> - update comments
> 
> Changes in v2:
> - rebase on top of the latest next
> - remove a redundant variable - power_lsb_uW
> - fix comments
> ---
>  drivers/iio/adc/ina2xx-adc.c | 56 
> +---
>  1 file changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index ddf8781..3488100 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -124,11 +124,11 @@ enum ina2xx_ids { ina219, ina226 };
>  
>  struct ina2xx_config {
>   u16 config_default;
> - int calibration_factor;
> + int calibration_value;
>   int shunt_voltage_lsb;  /* nV */
>   int bus_voltage_shift;  /* position of lsb */
>   int bus_voltage_lsb;/* uV */
> - int power_lsb;  /* uW */
> + int power_lsb_factor;
>   enum ina2xx_ids chip_id;
>  };
>  
> @@ -149,20 +149,20 @@ struct ina2xx_chip_info {
>  static const struct ina2xx_config ina2xx_config[] = {
>   [ina219] = {
>   .config_default = INA219_CONFIG_DEFAULT,
> - .calibration_factor = 4096,
> + .calibration_value = 4096,
>   .shunt_voltage_lsb = 1,
>   .bus_voltage_shift = INA219_BUS_VOLTAGE_SHIFT,
>   .bus_voltage_lsb = 4000,
> - .power_lsb = 2,
> + .power_lsb_factor = 20,
>   .chip_id = ina219,
>   },
>   [ina226] = {
>   .config_default = INA226_CONFIG_DEFAULT,
> - .calibration_factor = 512,
> + .calibration_value = 2048,
>   .shunt_voltage_lsb = 2500,
>   .bus_voltage_shift = 0,
>   .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> + .power_lsb_factor = 25,
>   .chip_id = ina226,
>   },
>  };
> @@ -228,15 +228,17 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>   return IIO_VAL_FRACTIONAL;
>  
>   case INA2XX_POWER:
> - /* processed (mW) = raw*lsb (uW) / 1000 */
> - *val = chip->config->power_lsb;
> - *val2 = 1000;
> + /* processed (mW) = raw * factor * current_lsb (mW)*/
> + *val = chip->config->power_lsb_factor *
> +chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
>   return IIO_VAL_FRACTIONAL;
>  
>   case INA2XX_CURRENT:
> - /* processed (mA) = raw (mA) */
> - *val = 1;
> - return IIO_VAL_INT;
> + /* processed (mA) = raw * lsb ([nV] / [uOhm] = [mA]) */
> + *val = chip->config->shunt_voltage_lsb;
> + *val2 = chip->shunt_resistor_uohm;
> + return IIO_VAL_FRACTIONAL;
>   }
>  
>   case IIO_CHAN_INFO_HARDWAREGAIN:
> @@ -541,25 +543,26 @@ static ssize_t ina2xx_allow_async_readout_store(struct 
> device *dev,
>  }
>  
>  /*
> - * Set current LSB to 1mA, shunt is in uOhms
> - * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-3. The only remaining parameter is RShunt.
> - * There is no need to expose the CALIBRATION register
> - * to the user for now. But we need to reset this register
> - * if the user updates RShunt after driver init, e.g upon
> - * reading an EEPROM/Probe-type value.
> + * Calibration register is set to the best value, which eliminates
> + * truncation errors on calculating current register in hardware.
> + * According to datasheet (INA 226: eq. 3, INA219: eq. 4) the best values
> + * are 2048 for ina226 and 4096 for ina219. They are hardcoded as
> + * calibration_value.
>   */

Re: [PATCH 1/2] iio: adc: ina2xx: Make calibration register value fixed

[Jonathan Cameron]

On Wed, 22 Nov 2017 16:32:14 +0100
Maciej Purski  wrote:

> Calibration register is used for calculating current register in
> hardware according to datasheet:
> current = shunt_volt * calib_register / 2048 (ina 226)
> current = shunt_volt * calib_register / 4096 (ina 219)
> 
> Fix calib_register value to 2048 for ina226 and 4096 for ina 219 in
> order to avoid truncation error and provide best precision allowed
> by shunt_voltage measurement. Make current scale value follow changes
> of shunt_resistor from sysfs as calib_register value is now fixed.
> 
> Power_lsb value should also follow shunt_resistor changes as stated in
> datasheet:
> power_lsb = 25 * current_lsb (ina 226)
> power_lsb = 20 * current_lsb (ina 219)
> 
> Signed-off-by: Maciej Purski 

I like the patch but would like to let it sit for a little longer
to see if others wish to comment.

Looks good to me but there are users out there and I want some
feedback from others that this won't break them...

Whilst the change is fully within the ABI we might have
known user scripts that make assumptions about the scale for
example and need time to fix those before we push this out.

If no one shouts in a few weeks I'll probably take it to force
the issue!

Remind me if I seem to have forgotten it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ina2xx-adc.c | 59 
> 
>  1 file changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 84a4387..0f25087 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -110,11 +110,11 @@ enum ina2xx_ids { ina219, ina226 };
>  
>  struct ina2xx_config {
>   u16 config_default;
> - int calibration_factor;
> + int calibration_value;
>   int shunt_div;
>   int bus_voltage_shift;
>   int bus_voltage_lsb;/* uV */
> - int power_lsb;  /* uW */
> + int power_lsb_factor;
>   enum ina2xx_ids chip_id;
>  };
>  
> @@ -124,6 +124,8 @@ struct ina2xx_chip_info {
>   const struct ina2xx_config *config;
>   struct mutex state_lock;
>   unsigned int shunt_resistor_uohm;
> + unsigned int current_lsb_uA;
> + unsigned int power_lsb_uW;
>   int avg;
>   int int_time_vbus; /* Bus voltage integration time uS */
>   int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -133,20 +135,20 @@ struct ina2xx_chip_info {
>  static const struct ina2xx_config ina2xx_config[] = {
>   [ina219] = {
>   .config_default = INA219_CONFIG_DEFAULT,
> - .calibration_factor = 4096,
> + .calibration_value = 4096,
>   .shunt_div = 100,
>   .bus_voltage_shift = 3,
>   .bus_voltage_lsb = 4000,
> - .power_lsb = 2,
> + .power_lsb_factor = 20,
>   .chip_id = ina219,
>   },
>   [ina226] = {
>   .config_default = INA226_CONFIG_DEFAULT,
> - .calibration_factor = 512,
> + .calibration_value = 2048,
>   .shunt_div = 400,
>   .bus_voltage_shift = 0,
>   .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> + .power_lsb_factor = 25,
>   .chip_id = ina226,
>   },
>  };
> @@ -210,14 +212,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  
>   case INA2XX_POWER:
>   /* processed (mW) = raw*lsb (uW) / 1000 */
> - *val = chip->config->power_lsb;
> + *val = chip->power_lsb_uW;
>   *val2 = 1000;
>   return IIO_VAL_FRACTIONAL;
>  
>   case INA2XX_CURRENT:
> - /* processed (mA) = raw (mA) */
> - *val = 1;
> - return IIO_VAL_INT;
> + /* processed (mA) = raw*lsb (uA) / 1000 */
> + *val = chip->current_lsb_uA;
> + *val2 = 1000;
> + return IIO_VAL_FRACTIONAL;
>   }
>   }
>  
> @@ -434,28 +437,35 @@ static ssize_t ina2xx_allow_async_readout_store(struct 
> device *dev,
>  }
>  
>  /*
> - * Set current LSB to 1mA, shunt is in uOhms
> - * (equation 13 in datasheet). We hardcode a Current_LSB
> - * of 1.0 x10-3. The only remaining parameter is RShunt.
> - * There is no need to expose the CALIBRATION register
> - * to the user for now. But we need to reset this register
> - * if the user updates RShunt after driver init, e.g upon
> - * reading an EEPROM/Probe-type value.
> + * Calibration register is set to the best value, which eliminates
> + * truncation errors on calculating current register in hardware.
> + * According to datasheet (eq. 3) the best values are 2048 for
> + * ina226 and 4096 for ina219. They are hardcoded as calibration_value.
>   */
>  static int ina2xx_set_calibration(struct 

Re: [PATCH v2 3/4] dt-bindings: hwmon: Add ti-max-expected-current-microamp property to ina2xx

[Jonathan Cameron]

On Tue, 17 Oct 2017 13:58:21 -0700
Guenter Roeck  wrote:

> On Tue, Oct 17, 2017 at 03:36:31PM -0500, Rob Herring wrote:
> > On Thu, Oct 12, 2017 at 02:36:04PM +0200, Maciej Purski wrote:  
> > > Add optional max expected current property which allows calibrating
> > > the ina sensor in order to achieve requested measure scale. Document
> > > the changes in Documentation/hwmon/ina2xx.
> > > 
> > > Signed-off-by: Maciej Purski 
> > > ---
> > >  Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
> > >  Documentation/hwmon/ina2xx | 3 +++
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt 
> > > b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > index 02af0d9..49ef0be 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > +++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> > > @@ -14,11 +14,13 @@ Optional properties:
> > >  
> > >  - shunt-resistor
> > >   Shunt resistor value in micro-Ohm
> > > -
> > > +- ti-max-expected-current-microamp
> > > + Max expected current value in mA  
> > 
> > ti,max-...
> > 
> > The property name is a bit long. Does "expected" add anything? Is there 
> > a max unexpected current?
> >   
> I am not too happy with it either. To me it suggests that there _can_ be
> an unexpected current (why specify a max _expected_ current otherwise ?),
> and that unexpected current won't be measurable and thus not reported
> because it is ... well, unexpected.

After calibration I 'think' this corresponds to the maximum current
that the device can measure (it's applying scaling inside to make full use
of available range of the register - there are some arguments that there is
an optimum value after which we could do better in software).

I guess the issue here is that this that we might need to separate the maximum
current the device is capable of measuring from what it is currently configured
to measure.  No idea how to describe that :)

Jonathan

> 
> Guenter
> 
> > >  Example:
> > >  
> > >  ina220@44 {
> > >   compatible = "ti,ina220";
> > >   reg = <0x44>;
> > >   shunt-resistor = <1000>;
> > > + ti-max-expected-current-microamp = <3000>;
> > >  };
> > > diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
> > > index cfd31d9..30620e8 100644
> > > --- a/Documentation/hwmon/ina2xx
> > > +++ b/Documentation/hwmon/ina2xx
> > > @@ -55,6 +55,9 @@ The shunt value in micro-ohms can be set via platform 
> > > data or device tree at
> > >  compile-time or via the shunt_resistor attribute in sysfs at run-time. 
> > > Please
> > >  refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for 
> > > bindings
> > >  if the device tree is used.
> > > +The max expected current value in miliamp can be set via platform data
> > > +or device tree at compile-time or via currX_max attribute in sysfs
> > > +at run-time.
> > >  
> > >  Additionally ina226 supports update_interval attribute as described in
> > >  Documentation/hwmon/sysfs-interface. Internally the interval is the sum 
> > > of
> > > -- 
> > > 2.7.4
> > >   
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Wed, 11 Oct 2017 16:42:53 +0200
Maciej Purski <m.pur...@samsung.com> wrote:

> On 10/09/2017 03:35 PM, Jonathan Cameron wrote:
> > On Mon, 9 Oct 2017 10:08:42 +0200
> > Maciej Purski <m.pur...@samsung.com> wrote:
> >   
> >> On 10/08/2017 11:47 AM, Jonathan Cameron wrote:  
> >>> On Wed, 04 Oct 2017 09:11:31 +0200
> >>> Maciej Purski <m.pur...@samsung.com> wrote:
> >>>  
> >>>> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:  
> >>>>> On Thu, 28 Sep 2017 14:50:12 +0200
> >>>>> Maciej Purski <m.pur...@samsung.com> wrote:
> >>>>> 
> >>>>>> Max expected current is used for calculating calibration register 
> >>>>>> value,
> >>>>>> Current LSB and Power LSB according to equations found in ina 
> >>>>>> datasheet.
> >>>>>> Max expected current is now implicitly set to default value,
> >>>>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >>>>>> Power LSB is equal to 2 uW or 25000 uW depending on ina model.
> >>>>>>
> >>>>>> Make max expected current configurable, just like it's already done
> >>>>>> with shunt resistance: from device tree, platform_data or later
> >>>>>> from sysfs. On each max_expected_current change, calculate new values
> >>>>>> for Current LSB and Power LSB. According to datasheet Current LSB 
> >>>>>> should
> >>>>>> be calculated by dividing max expected current by 2^15, as values read
> >>>>>> from device registers are in this case 16-bit integers. Power LSB
> >>>>>> is calculated by multiplying Current LSB by a factor, which is defined
> >>>>>> in ina documentation.  
> >>>>>
> >>>>> One odd bit of casting inline.  Also this is new userspace ABI.
> >>>>> It needs documenting in
> >>>>>
> >>>>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> >>>>> I'm also unclear on one element about this - is it a value used only
> >>>>> for calibration or are we talking about the actual 'range' of the 
> >>>>> device?
> >>>>> 
> >>>>
> >>>> This is used for calibration. The device measures directly only voltage.
> >>>> However, it has also current and power registers. Their values are
> >>>> calculated by the device using the calibration register which is 
> >>>> calculated
> >>>> using max expected current. So I guess that it's not what you mean
> >>>> by the actual 'range' of the device.
> >>>> 
> >>>>> The interpretation of this value isn't clear against the more general
> >>>>> ABI.
> >>>>>
> >>>>> In particular it is it in raw units (adc counts) or mA?  Docs say
> >>>>> that but the naming of the attribute doesn't make this clear.
> >>>>> 
> >>>>
> >>>> It's in mA. I can make it clear in the attribute name.
> >>>> 
> >>>>> Also I'm unconvinced this isn't better represented using the
> >>>>> range specifications available for any IIO attribute on the raw
> >>>>> value in combination with adjusting the scale value.
> >>>>> Note not many drivers yet provide ranges on their raw outputs
> >>>>> but we do have core support for it.  I've been meaning to start
> >>>>> pushing this out into drivers, but been busy since we introduced
> >>>>> the core support.  The dpot-dac driver does use it for examplel
> >>>>>
> >>>>
> >>>>
> >>>> I'm not sure if what I'm about to add is similar to what is done
> >>>> in the mentioned dpot-dac driver. It seems that the callback read_avail
> >>>> returns information on raw values which can be obtained from the device.
> >>>> What I need is an adjustable value, which is then used by the device 
> >>>> internally
> >>>> in order to calculate current with the requested precision. Max expected 
> >>>> current
> >>>> is also used for calculating the scale value.
> >>>> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO 
> >>>&g

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Mon, 9 Oct 2017 10:08:42 +0200
Maciej Purski <m.pur...@samsung.com> wrote:

> On 10/08/2017 11:47 AM, Jonathan Cameron wrote:
> > On Wed, 04 Oct 2017 09:11:31 +0200
> > Maciej Purski <m.pur...@samsung.com> wrote:
> >   
> >> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:  
> >>> On Thu, 28 Sep 2017 14:50:12 +0200
> >>> Maciej Purski <m.pur...@samsung.com> wrote:
> >>>  
> >>>> Max expected current is used for calculating calibration register value,
> >>>> Current LSB and Power LSB according to equations found in ina datasheet.
> >>>> Max expected current is now implicitly set to default value,
> >>>> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >>>> Power LSB is equal to 2 uW or 25000 uW depending on ina model.
> >>>>
> >>>> Make max expected current configurable, just like it's already done
> >>>> with shunt resistance: from device tree, platform_data or later
> >>>> from sysfs. On each max_expected_current change, calculate new values
> >>>> for Current LSB and Power LSB. According to datasheet Current LSB should
> >>>> be calculated by dividing max expected current by 2^15, as values read
> >>>> from device registers are in this case 16-bit integers. Power LSB
> >>>> is calculated by multiplying Current LSB by a factor, which is defined
> >>>> in ina documentation.  
> >>>
> >>> One odd bit of casting inline.  Also this is new userspace ABI.
> >>> It needs documenting in
> >>>
> >>> Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> >>> I'm also unclear on one element about this - is it a value used only
> >>> for calibration or are we talking about the actual 'range' of the device?
> >>>  
> >>
> >> This is used for calibration. The device measures directly only voltage.
> >> However, it has also current and power registers. Their values are
> >> calculated by the device using the calibration register which is calculated
> >> using max expected current. So I guess that it's not what you mean
> >> by the actual 'range' of the device.
> >>  
> >>> The interpretation of this value isn't clear against the more general
> >>> ABI.
> >>>
> >>> In particular it is it in raw units (adc counts) or mA?  Docs say
> >>> that but the naming of the attribute doesn't make this clear.
> >>>  
> >>
> >> It's in mA. I can make it clear in the attribute name.
> >>  
> >>> Also I'm unconvinced this isn't better represented using the
> >>> range specifications available for any IIO attribute on the raw
> >>> value in combination with adjusting the scale value.
> >>> Note not many drivers yet provide ranges on their raw outputs
> >>> but we do have core support for it.  I've been meaning to start
> >>> pushing this out into drivers, but been busy since we introduced
> >>> the core support.  The dpot-dac driver does use it for examplel
> >>> 
> >>
> >>
> >> I'm not sure if what I'm about to add is similar to what is done
> >> in the mentioned dpot-dac driver. It seems that the callback read_avail
> >> returns information on raw values which can be obtained from the device.
> >> What I need is an adjustable value, which is then used by the device 
> >> internally
> >> in order to calculate current with the requested precision. Max expected 
> >> current
> >> is also used for calculating the scale value.
> >> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
> >> your solution fits in here.
> >>  
> > 
> > I think I answered this in the other branch of the thread.
> > _calibscale is what you want here as it's internal to the device.
> > 
> > It's not one often used for ADCs but quite a few other types of
> > device provide some front end analog adjustment (whilst it is digital
> > here, it is applied within the device - so we don't need to care).
> > 
> > Jonathan  
> 
> Thank you for your explanation. Calibscale seems suitable for me in this case,
> but what do you think I should do then with SCALE attribute? Should I get rid 
> of 
> it for current and use only calibscale? Or should I use both calibscale and 
> scale attributes and for current they will be the same value?

You'll have to leave it as it is existing ABI.  It

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Wed, 04 Oct 2017 09:11:31 +0200
Maciej Purski <m.pur...@samsung.com> wrote:

> On 10/01/2017 12:29 PM, Jonathan Cameron wrote:
> > On Thu, 28 Sep 2017 14:50:12 +0200
> > Maciej Purski <m.pur...@samsung.com> wrote:
> >   
> >> Max expected current is used for calculating calibration register value,
> >> Current LSB and Power LSB according to equations found in ina datasheet.
> >> Max expected current is now implicitly set to default value,
> >> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> >> Power LSB is equal to 2 uW or 25000 uW depending on ina model.
> >>
> >> Make max expected current configurable, just like it's already done
> >> with shunt resistance: from device tree, platform_data or later
> >> from sysfs. On each max_expected_current change, calculate new values
> >> for Current LSB and Power LSB. According to datasheet Current LSB should
> >> be calculated by dividing max expected current by 2^15, as values read
> >> from device registers are in this case 16-bit integers. Power LSB
> >> is calculated by multiplying Current LSB by a factor, which is defined
> >> in ina documentation.  
> > 
> > One odd bit of casting inline.  Also this is new userspace ABI.
> > It needs documenting in
> > 
> > Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
> > I'm also unclear on one element about this - is it a value used only
> > for calibration or are we talking about the actual 'range' of the device?
> >   
> 
> This is used for calibration. The device measures directly only voltage.
> However, it has also current and power registers. Their values are
> calculated by the device using the calibration register which is calculated
> using max expected current. So I guess that it's not what you mean
> by the actual 'range' of the device.
> 
> > The interpretation of this value isn't clear against the more general
> > ABI.
> > 
> > In particular it is it in raw units (adc counts) or mA?  Docs say
> > that but the naming of the attribute doesn't make this clear.
> >   
> 
> It's in mA. I can make it clear in the attribute name.
> 
> > Also I'm unconvinced this isn't better represented using the
> > range specifications available for any IIO attribute on the raw
> > value in combination with adjusting the scale value.
> > Note not many drivers yet provide ranges on their raw outputs
> > but we do have core support for it.  I've been meaning to start
> > pushing this out into drivers, but been busy since we introduced
> > the core support.  The dpot-dac driver does use it for examplel
> >  
> 
> 
> I'm not sure if what I'm about to add is similar to what is done
> in the mentioned dpot-dac driver. It seems that the callback read_avail
> returns information on raw values which can be obtained from the device.
> What I need is an adjustable value, which is then used by the device 
> internally
> in order to calculate current with the requested precision. Max expected 
> current
> is also used for calculating the scale value.
> Tell me if I'm wrong. Maybe I misunderstood the 'range' concept in IIO and
> your solution fits in here.
> 

I think I answered this in the other branch of the thread.
_calibscale is what you want here as it's internal to the device.

It's not one often used for ADCs but quite a few other types of
device provide some front end analog adjustment (whilst it is digital
here, it is applied within the device - so we don't need to care).

Jonathan

> Best regards,
> 
>   Maciej
> > This moves the burden of calculating the 1lsb value to userspace,
> > but importantly it would give us a consistent ABI where this fits
> > in with existing elements (largely buy not introducing any new
> > ones :).
> > 
> > Thanks,
> > 
> > Jonathan  
> >>
> >> Signed-off-by: Maciej Purski <m.pur...@samsung.com>
> >> ---
> >>   drivers/iio/adc/ina2xx-adc.c | 110 
> >> ++-
> >>   include/linux/platform_data/ina2xx.h |   2 +
> >>   2 files changed, 98 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> >> index f387b97..883fede 100644
> >> --- a/drivers/iio/adc/ina2xx-adc.c
> >> +++ b/drivers/iio/adc/ina2xx-adc.c
> >> @@ -56,6 +56,7 @@
> >>   #define INA226_DEFAULT_IT1110
> >>   
> >>   #define INA2XX_RSHUNT_DEFAULT   1
> >> +#define INA2XX_MAX_EXPECTED_A_DEFAULT (1 << 

Re: [PATCH 3/4] dt-bindings: hwmon: Add max-expected-current property to ina2xx

[Jonathan Cameron]

On Thu, 28 Sep 2017 14:50:14 +0200
Maciej Purski  wrote:

> Add optional max expected current property which allows calibrating
> the ina sensor in order to achieve requested precision. Document
> the changes in Documentation/hwmon/ina2xx.
> 

This is introducing new generic devicetree bindings..  
I'm not totally sure we want to do this rather than having a
manufacturer specific binding...  I don't have a good feeling for
how common this will be in other devices.

If it's generic, then this isn't necessarily the place to define it.

Jonathan

> Signed-off-by: Maciej Purski 
> ---
>  Documentation/devicetree/bindings/hwmon/ina2xx.txt | 4 +++-
>  Documentation/hwmon/ina2xx | 9 +
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ina2xx.txt 
> b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> index 02af0d9..25ba372 100644
> --- a/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> +++ b/Documentation/devicetree/bindings/hwmon/ina2xx.txt
> @@ -14,11 +14,13 @@ Optional properties:
>  
>  - shunt-resistor
>   Shunt resistor value in micro-Ohm
> -
> +- max-expected-current
> + Max expected current value in mA
>  Example:
>  
>  ina220@44 {
>   compatible = "ti,ina220";
>   reg = <0x44>;
>   shunt-resistor = <1000>;
> + max-expected-current = <3000>;
>  };
> diff --git a/Documentation/hwmon/ina2xx b/Documentation/hwmon/ina2xx
> index cfd31d9..e9ca838 100644
> --- a/Documentation/hwmon/ina2xx
> +++ b/Documentation/hwmon/ina2xx
> @@ -51,10 +51,11 @@ INA230 and INA231 are high or low side current shunt and 
> power monitors
>  with an I2C interface. The chips monitor both a shunt voltage drop and
>  bus supply voltage.
>  
> -The shunt value in micro-ohms can be set via platform data or device tree at
> -compile-time or via the shunt_resistor attribute in sysfs at run-time. Please
> -refer to the Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings
> -if the device tree is used.
> +The shunt value in micro-ohms and max expected current in mA can be set
> +via platform data or device tree at compile-time or via the shunt_resistor
> +and max_expected_current attributes in sysfs at run-time. Please refer to the
> +Documentation/devicetree/bindings/i2c/ina2xx.txt for bindings if the
> +device tree is used.
>  
>  Additionally ina226 supports update_interval attribute as described in
>  Documentation/hwmon/sysfs-interface. Internally the interval is the sum of

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] iio: adc: ina2xx: Make max expected current configurable

[Jonathan Cameron]

On Thu, 28 Sep 2017 14:50:12 +0200
Maciej Purski  wrote:

> Max expected current is used for calculating calibration register value,
> Current LSB and Power LSB according to equations found in ina datasheet.
> Max expected current is now implicitly set to default value,
> which is 2^15, thanks to which Current LSB is equal to 1 mA and
> Power LSB is equal to 2 uW or 25000 uW depending on ina model.
> 
> Make max expected current configurable, just like it's already done
> with shunt resistance: from device tree, platform_data or later
> from sysfs. On each max_expected_current change, calculate new values
> for Current LSB and Power LSB. According to datasheet Current LSB should
> be calculated by dividing max expected current by 2^15, as values read
> from device registers are in this case 16-bit integers. Power LSB
> is calculated by multiplying Current LSB by a factor, which is defined
> in ina documentation.

One odd bit of casting inline.  Also this is new userspace ABI.
It needs documenting in 

Documentation/ABI/testing/sysfs-bus-iio* as appropriate.
I'm also unclear on one element about this - is it a value used only
for calibration or are we talking about the actual 'range' of the device?

The interpretation of this value isn't clear against the more general
ABI.

In particular it is it in raw units (adc counts) or mA?  Docs say
that but the naming of the attribute doesn't make this clear.

Also I'm unconvinced this isn't better represented using the
range specifications available for any IIO attribute on the raw
value in combination with adjusting the scale value.
Note not many drivers yet provide ranges on their raw outputs
but we do have core support for it.  I've been meaning to start
pushing this out into drivers, but been busy since we introduced
the core support.  The dpot-dac driver does use it for examplel

This moves the burden of calculating the 1lsb value to userspace,
but importantly it would give us a consistent ABI where this fits
in with existing elements (largely buy not introducing any new
ones :).

Thanks,

Jonathan
> 
> Signed-off-by: Maciej Purski 
> ---
>  drivers/iio/adc/ina2xx-adc.c | 110 
> ++-
>  include/linux/platform_data/ina2xx.h |   2 +
>  2 files changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index f387b97..883fede 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -56,6 +56,7 @@
>  #define INA226_DEFAULT_IT1110
>  
>  #define INA2XX_RSHUNT_DEFAULT   1
> +#define INA2XX_MAX_EXPECTED_A_DEFAULT(1 << 15)   /* current_lsb 
> = 1 mA */
>  
>  /*
>   * bit masks for reading the settings in the configuration register
> @@ -114,7 +115,7 @@ struct ina2xx_config {
>   int shunt_div;
>   int bus_voltage_shift;
>   int bus_voltage_lsb;/* uV */
> - int power_lsb;  /* uW */
> + int power_lsb_factor;
>   enum ina2xx_ids chip_id;
>  };
>  
> @@ -123,7 +124,10 @@ struct ina2xx_chip_info {
>   struct task_struct *task;
>   const struct ina2xx_config *config;
>   struct mutex state_lock;
> - unsigned int shunt_resistor;
> + unsigned int shunt_resistor;/* uOhms */
> + unsigned int max_expected_current;  /* mA */
> + int current_lsb;/* uA */
> + int power_lsb;  /* uW */
>   int avg;
>   int int_time_vbus; /* Bus voltage integration time uS */
>   int int_time_vshunt; /* Shunt voltage integration time uS */
> @@ -137,7 +141,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   .shunt_div = 100,
>   .bus_voltage_shift = 3,
>   .bus_voltage_lsb = 4000,
> - .power_lsb = 2,
> + .power_lsb_factor = 20,
>   .chip_id = ina219,
>   },
>   [ina226] = {
> @@ -146,7 +150,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   .shunt_div = 400,
>   .bus_voltage_shift = 0,
>   .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> + .power_lsb_factor = 25,
>   .chip_id = ina226,
>   },
>  };
> @@ -210,14 +214,15 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>  
>   case INA2XX_POWER:
>   /* processed (mW) = raw*lsb (uW) / 1000 */
> - *val = chip->config->power_lsb;
> + *val = chip->power_lsb;
>   *val2 = 1000;
>   return IIO_VAL_FRACTIONAL;
>  
>   case INA2XX_CURRENT:
> - /* processed (mA) = raw (mA) */
> - *val = 1;
> - return IIO_VAL_INT;
> + /* processed (mA) = raw*lsb (uA) / 1000 */
> + *val = chip->current_lsb;
> +  

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Jonathan Cameron]

On Sun, 10 Sep 2017 16:05:28 +0200
Davide Hug <d...@videhug.ch> wrote:

> On Sun, 10 Sep 2017 11:44:46 +0200
> Linus Walleij <linus.wall...@linaro.org> wrote:
> 
> > After finding out there are active users of this sensor I noticed:  
> 
> I would like to point out that I'm doing this for learning purposes and that I
> don't have any pressing needs for this driver to work. But of course I'm 
> really
> grateful for all the replays!
> 

I can't claim I ever had any real use for this part either.  It was
just looking at me from a dev board and asking for me to mainline
the driver :)

> > Cc: a...@kernel.org
> > Cc: Marco Franchi <marco.fran...@nxp.com>
> > Cc: Davide Hug <d...@videhug.ch>
> > Cc: Jonathan Cameron <ji...@cam.ac.uk>
> > Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
> > ---
> > ChangeLog v1->v2:
> > - Fix the return value from devm_gpiod_get() with PTR_ERR()
> >   so we defer etc correctly.
> > 
> > ARM SoC folks: please ACK this so the HWMON maintainer can merge
> > it when it is in reasonable shape.
> > 
> > Marco: can you test this patch with your setup?
> > 
> > Davide: can you test this patch with your setup?  
> 
> I tested it and I can make it work (as the current driver) by modifying 
> gpiolib
> to not test for the FLAG_USED_AS_IRQ in _gpiod_direction_output_raw.
> Otherwise again I get:
> 
>   gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO 
> tied to an IRQ as output
> 
> as with the current driver. But I guess this is as expected.
> (I did this on a beaglebone black with kernel 4.9.10. I'll compile a newer
> kernel and try it there to.)

Cool.

> 
> Thanks!

Thanks to you.  It's always nice when people fix issues with / update a
driver you've ignored for perhaps 8 years ;)

As you know this particular part has a truely horrendous custom bus.
I'm always amazed these old sensiron parts work at all.

I'm also really surprised to see they are still readily available.

Jonathan

> Davide

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hwmon/sht15: Root out platform data

[Jonathan Cameron]

On Sun, 10 Sep 2017 11:44:46 +0200
Linus Walleij <linus.wall...@linaro.org> wrote:

> After finding out there are active users of this sensor I noticed:
> 
> - It has a single PXA27x board file using the platform data
> - The platform data is only used to carry two GPIO pins, all other
>   fields are unused
> - The driver does not use GPIO descriptors but the legacy GPIO
>   API
> 
> I saw we can swiftly fix this by:
> 
> - Killing off the platform data entirely
> - Define a GPIO descriptor lookup table in the board file
> - Use the standard devm_gpiod_get() to grab the GPIO descriptors
>   from either the device tree or the board file table.
> 
> This compiles, but needs testing.
> 
> Cc: a...@kernel.org
> Cc: Marco Franchi <marco.fran...@nxp.com>
> Cc: Davide Hug <d...@videhug.ch>
> Cc: Jonathan Cameron <ji...@cam.ac.uk>
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>

Hmm. Firing up that board isn't something I'm going to manage for a few
weeks at least.   This looks safe though and as I suspect I'm the only
person who still has working boards running recent kernels I doubt
anyone else will notice :)

Perhaps give me a month and if I seem to have forgotten it ping
me again.  Or if someone else gives a tested by in the meantime
for another platform then I'm happy to assume the trivial bits
for the stargate2 and imote2 will work fine.

Jonathan

> ---
> ChangeLog v1->v2:
> - Fix the return value from devm_gpiod_get() with PTR_ERR()
>   so we defer etc correctly.
> 
> ARM SoC folks: please ACK this so the HWMON maintainer can merge
> it when it is in reasonable shape.
> 
> Marco: can you test this patch with your setup?
> 
> Davide: can you test this patch with your setup?
> 
> Jonathan: I gues you may feel this sensor needs migrating to IIO or
> so, like the Imote and Stargate2 needs migrating to device tree,
> but this helps a bit with that.

It was my fault it went into hwmon in the first place ;)

Humidity sensors have always occupied one of those odd corners.
They are actually present in some servers (or so Guenter once
once told me).  They are happy keeping them in hwmon and I'm not
that fussed about moving them until someone has a use case that
really needs it.  Humidity doesn't change quickly so sysfs alone
is fine.

Hmm. Device tree on these boards has always been on the todo list, but
I'm afraid it's a fair way down it.  It'll be 'interesting' as they
only uboot patches there ever were disappeared years ago and
we have a limited space to get the kernel in.  Not sure what other
fun I'll hit on that.  Also not sure I actually have cards to
test some of the ports any more so there will be a fair bit
of untested code in any port.

Jonathan

p.s. I'll send patches for my email address when I get a bored moment.
Could have sworn I did 5 years ago or more, but maybe it forgot.


> ---
>  Documentation/hwmon/sht15   |   3 +-
>  arch/arm/mach-pxa/stargate2.c   |  17 ++--
>  drivers/hwmon/sht15.c   | 167 
> 
>  include/linux/platform_data/sht15.h |  38 
>  4 files changed, 65 insertions(+), 160 deletions(-)
>  delete mode 100644 include/linux/platform_data/sht15.h
> 
> diff --git a/Documentation/hwmon/sht15 b/Documentation/hwmon/sht15
> index 778987d1856f..5e3207c3b177 100644
> --- a/Documentation/hwmon/sht15
> +++ b/Documentation/hwmon/sht15
> @@ -42,8 +42,7 @@ chip. These coefficients are used to internally calibrate 
> the signals from the
>  sensors. Disabling the reload of those coefficients allows saving 10ms for 
> each
>  measurement and decrease power consumption, while losing on precision.
>  
> -Some options may be set directly in the sht15_platform_data structure
> -or via sysfs attributes.
> +Some options may be set via sysfs attributes.
>  
>  Notes:
>* The regulator supply name is set to "vcc".
> diff --git a/arch/arm/mach-pxa/stargate2.c b/arch/arm/mach-pxa/stargate2.c
> index 2d45d18b1a5e..6b7df6fd2448 100644
> --- a/arch/arm/mach-pxa/stargate2.c
> +++ b/arch/arm/mach-pxa/stargate2.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -52,7 +53,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  
>  #include "devices.h"
>  #include "generic.h"
> @@ -137,17 +137,18 @@ static unsigned long sg2_im2_unified_pin_config[] 
> __initdata = {
>   GPIO10_GPIO, /* large basic connector pin 23 */
>  };
>  
> -static struct sht15_platform_data platform_data_sht15 = {
> - .gpio_data =  100,
> - .gpio_sck  =  98,
> +static struct gpiod_lookup_table sht15_gpiod_table = {
> + .dev_id = "sht15",

Re: [PATCH] hwmon: Add LTC2471/LTC2473 driver

[Jonathan Cameron]

On Thu, 29 Jun 2017 06:34:22 -0700
Guenter Roeck  wrote:

> On 06/29/2017 03:45 AM, Mike Looijmans wrote:
> > The LTC2741 and LTC2473 are single voltage monitoring chips. The LTC2473
> > is similar to the LTC2471 but outputs a signed differential value.
> > 
> > Datasheet:
> >http://cds.linear.com/docs/en/datasheet/24713fb.pdf
> > 
> > Signed-off-by: Mike Looijmans   
> 
> Hi Mike,
> 
> that looks like a perfect candidate for an iio driver, since its use is
> not limited to hardware monitoring, it has a high sample rate, and it
> doesn't have limit registers or actions.
> 
> Jonathan, what do you think ?
Late response given I just applied the IIO driver ;)
Yes, fine for IIO.  Whilst they list hwmon as one of the possible
uses they list a whole load of others as well so IIO probably
makes more sense for this one.

Thanks,

Jonathan
> 
> Thanks,
> Guenter
> 
> > ---
> >   drivers/hwmon/Kconfig   |  10 
> >   drivers/hwmon/Makefile  |   1 +
> >   drivers/hwmon/ltc2471.c | 127 
> > 
> >   3 files changed, 138 insertions(+)
> >   create mode 100644 drivers/hwmon/ltc2471.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index fbde226..c9a2a87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -673,6 +673,16 @@ config SENSORS_LINEAGE
> >   This driver can also be built as a module.  If so, the module
> >   will be called lineage-pem.
> >   
> > +config SENSORS_LTC2471
> > +   tristate "Linear Technology LTC2471 and LTC2473"
> > +   depends on I2C
> > +   help
> > + If you say yes here you get support for Linear Technology LTC2471
> > + and LTC2473 voltage monitors.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called ltc2471.
> > +
> >   config SENSORS_LTC2945
> > tristate "Linear Technology LTC2945"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 58cc3ac..6f60fe7 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -99,6 +99,7 @@ obj-$(CONFIG_SENSORS_LM93)+= lm93.o
> >   obj-$(CONFIG_SENSORS_LM95234) += lm95234.o
> >   obj-$(CONFIG_SENSORS_LM95241) += lm95241.o
> >   obj-$(CONFIG_SENSORS_LM95245) += lm95245.o
> > +obj-$(CONFIG_SENSORS_LTC2471)  += ltc2471.o
> >   obj-$(CONFIG_SENSORS_LTC2945) += ltc2945.o
> >   obj-$(CONFIG_SENSORS_LTC2990) += ltc2990.o
> >   obj-$(CONFIG_SENSORS_LTC4151) += ltc4151.o
> > diff --git a/drivers/hwmon/ltc2471.c b/drivers/hwmon/ltc2471.c
> > new file mode 100644
> > index 000..17eaad8
> > --- /dev/null
> > +++ b/drivers/hwmon/ltc2471.c
> > @@ -0,0 +1,127 @@
> > +/*
> > + * Driver for Linear Technology LTC2471 and LTC2473 voltage monitors
> > + * The LTC2473 is identical to the 2471, but reports a differential signal.
> > + *
> > + * Copyright (C) 2017 Topic Embedded Products
> > + * Author: Mike Looijmans 
> > + *
> > + * License: GPLv2
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +enum chips {
> > +   ltc2471,
> > +   ltc2473
> > +};
> > +
> > +struct ltc2471_data {
> > +   struct i2c_client *i2c;
> > +   bool differential;
> > +};
> > +
> > +/* Reference voltage is 1.25V */
> > +#define LTC2471_VREF 1250
> > +
> > +/* Read two bytes from the I2C bus to obtain the ADC result */
> > +static int ltc2471_get_value(struct i2c_client *i2c)
> > +{
> > +   int ret;
> > +   __be16 buf;
> > +
> > +   ret = i2c_master_recv(i2c, (char *), 2);
> > +   if (ret < 0)
> > +   return ret;
> > +   if (ret != 2)
> > +   return -EIO;
> > +
> > +   /* MSB first */
> > +   return be16_to_cpu(buf);
> > +}
> > +
> > +static ssize_t ltc2471_show_value(struct device *dev,
> > + struct device_attribute *da, char *buf)
> > +{
> > +   struct ltc2471_data *data = dev_get_drvdata(dev);
> > +   int value;
> > +
> > +   value = ltc2471_get_value(data->i2c);
> > +   if (unlikely(value < 0))
> > +   return value;
> > +
> > +   if (data->differential)
> > +   /* Ranges from -VREF to +VREF with "0" at 0x8000 */
> > +   value = ((s32)LTC2471_VREF * (s32)(value - 0x8000)) >> 15;
> > +   else
> > +   /* Ranges from 0 to +VREF */
> > +   value = ((u32)LTC2471_VREF * (u32)value) >> 16;
> > +
> > +   return snprintf(buf, PAGE_SIZE, "%d\n", value);
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2471_show_value, NULL, 0);
> > +
> > +static struct attribute *ltc2471_attrs[] = {
> > +   _dev_attr_in0_input.dev_attr.attr,
> > +   NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(ltc2471);
> > +
> > +static int ltc2471_i2c_probe(struct i2c_client *i2c,
> > +const struct i2c_device_id *id)
> > +{
> > +   int ret;
> > +   struct device *hwmon_dev;
> > +   struct ltc2471_data *data;
> > +
> 

Re: [PATCH 4/7] iio: adc: twl4030: Unexport twl4030_madc_conversion()

[Jonathan Cameron]

On Sun, 30 Apr 2017 17:35:47 +0100
Jonathan Cameron <ji...@kernel.org> wrote:

> On 27/04/17 16:30, Sebastian Reichel wrote:
> > All madc users have been converted to IIO API, so drop the
> > legacy API. The function is still used inside of the driver.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>  
> Acked-by: Jonathan Cameron <ji...@kernel.org>
Applied to the togreg branch of iio.git and pushed out
as testing for the autobuilders to play with them.

thanks,

Jonathan
> > ---
> >  drivers/iio/adc/twl4030-madc.c   | 5 +++--
> >  include/linux/i2c/twl4030-madc.h | 2 --
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> > index 88e44066ef82..be60f76d1a50 100644
> > --- a/drivers/iio/adc/twl4030-madc.c
> > +++ b/drivers/iio/adc/twl4030-madc.c
> > @@ -72,6 +72,8 @@ struct twl4030_madc_data {
> > u8 isr;
> >  };
> >  
> > +static int twl4030_madc_conversion(struct twl4030_madc_request *req);
> > +
> >  static int twl4030_madc_read(struct iio_dev *iio_dev,
> >  const struct iio_chan_spec *chan,
> >  int *val, int *val2, long mask)
> > @@ -568,7 +570,7 @@ static int twl4030_madc_wait_conversion_ready(struct 
> > twl4030_madc_data *madc,
> >   * be a negative error value in the corresponding array element.
> >   * returns 0 if succeeds else error value
> >   */
> > -int twl4030_madc_conversion(struct twl4030_madc_request *req)
> > +static int twl4030_madc_conversion(struct twl4030_madc_request *req)
> >  {
> > const struct twl4030_madc_conversion_method *method;
> > int ret;
> > @@ -640,7 +642,6 @@ int twl4030_madc_conversion(struct twl4030_madc_request 
> > *req)
> >  
> > return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
> >  
> >  /**
> >   * twl4030_madc_set_current_generator() - setup bias current
> > diff --git a/include/linux/i2c/twl4030-madc.h 
> > b/include/linux/i2c/twl4030-madc.h
> > index 0c919ebb31e0..be9260e261ac 100644
> > --- a/include/linux/i2c/twl4030-madc.h
> > +++ b/include/linux/i2c/twl4030-madc.h
> > @@ -141,6 +141,4 @@ struct twl4030_madc_user_parms {
> > int status;
> > u16 result;
> >  };
> > -
> > -int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> >  #endif
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] iio: adc: twl4030: Drop struct twl4030_madc_user_parms

[Jonathan Cameron]

On Thu, 27 Apr 2017 17:30:10 +0200
Sebastian Reichel  wrote:

> This struct is no longer used by anything in the kernel.
> 
> Signed-off-by: Sebastian Reichel 
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to see if we have missed anything.

Jonathan
> ---
>  include/linux/i2c/twl4030-madc.h | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/include/linux/i2c/twl4030-madc.h 
> b/include/linux/i2c/twl4030-madc.h
> index be9260e261ac..f395700fb933 100644
> --- a/include/linux/i2c/twl4030-madc.h
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -135,10 +135,4 @@ enum sample_type {
>  #define TWL4030_REG_GPBR10x0c
>  #define TWL4030_GPBR1_MADC_HFCLK_EN  (1 << 7)
>  
> -struct twl4030_madc_user_parms {
> - int channel;
> - int average;
> - int status;
> - u16 result;
> -};
>  #endif

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] iio: adc: twl4030: Drop twl4030_get_madc_conversion()

[Jonathan Cameron]

On Sun, 30 Apr 2017 17:35:04 +0100
Jonathan Cameron <ji...@kernel.org> wrote:

> On 27/04/17 16:30, Sebastian Reichel wrote:
> > Drop legacy twl4030_get_madc_conversion() method. It has been
> > used by drivers to get madc data before it conversion to IIO
> > API. There are no users in the mainline kernel anymore.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>  
> Acked-by: Jonathan Cameron <ji...@kernel.org>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> > ---
> >  drivers/iio/adc/twl4030-madc.c   | 21 -
> >  include/linux/i2c/twl4030-madc.h |  1 -
> >  2 files changed, 22 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> > index 0c74869a540a..88e44066ef82 100644
> > --- a/drivers/iio/adc/twl4030-madc.c
> > +++ b/drivers/iio/adc/twl4030-madc.c
> > @@ -642,27 +642,6 @@ int twl4030_madc_conversion(struct 
> > twl4030_madc_request *req)
> >  }
> >  EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
> >  
> > -int twl4030_get_madc_conversion(int channel_no)
> > -{
> > -   struct twl4030_madc_request req;
> > -   int temp = 0;
> > -   int ret;
> > -
> > -   req.channels = (1 << channel_no);
> > -   req.method = TWL4030_MADC_SW2;
> > -   req.active = 0;
> > -   req.raw = 0;
> > -   req.func_cb = NULL;
> > -   ret = twl4030_madc_conversion();
> > -   if (ret < 0)
> > -   return ret;
> > -   if (req.rbuf[channel_no] > 0)
> > -   temp = req.rbuf[channel_no];
> > -
> > -   return temp;
> > -}
> > -EXPORT_SYMBOL_GPL(twl4030_get_madc_conversion);
> > -
> >  /**
> >   * twl4030_madc_set_current_generator() - setup bias current
> >   *
> > diff --git a/include/linux/i2c/twl4030-madc.h 
> > b/include/linux/i2c/twl4030-madc.h
> > index 1c0134dd3271..0c919ebb31e0 100644
> > --- a/include/linux/i2c/twl4030-madc.h
> > +++ b/include/linux/i2c/twl4030-madc.h
> > @@ -143,5 +143,4 @@ struct twl4030_madc_user_parms {
> >  };
> >  
> >  int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> > -int twl4030_get_madc_conversion(int channel_no);
> >  #endif
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] hwmon: twl4030-madc: drop driver

[Jonathan Cameron]

On 27/04/17 17:51, Guenter Roeck wrote:
> On Thu, Apr 27, 2017 at 05:30:06PM +0200, Sebastian Reichel wrote:
>> This driver is no longer needed:
>>
>>  * It has no mainline users
>>  * It has no DT support and OMAP is DT only
>>  * iio-hwmon can be used for madc, which also works with DT
>>
>> Signed-off-by: Sebastian Reichel 
> 
> Acked-by: Guenter Roeck 
> 
> ... assuming this will be pushed through the same tree as the rest
> of the series. Let me know if I should queue it up in hwmon for v4.12.
We are too late for the series to go through IIO for the 4.12 cycle, so I guess 
it
makes sense to take this one through hwmon if you still have time.

I can then pick up the rest once I have acks from Wolfram
(who should have been cc'd as we are hoovering up things from the
i2c directories in this series) + power supply side of things.

Jonathan
> 
> Thanks,
> Guenter
> 
>> ---
>>  drivers/hwmon/Kconfig  |  10 
>>  drivers/hwmon/Makefile |   1 -
>>  drivers/hwmon/twl4030-madc-hwmon.c | 118 
>> -
>>  3 files changed, 129 deletions(-)
>>  delete mode 100644 drivers/hwmon/twl4030-madc-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 0649d53f3d16..776d1ac2bfee 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1643,16 +1643,6 @@ config SENSORS_TMP421
>>This driver can also be built as a module.  If so, the module
>>will be called tmp421.
>>  
>> -config SENSORS_TWL4030_MADC
>> -tristate "Texas Instruments TWL4030 MADC Hwmon"
>> -depends on TWL4030_MADC
>> -help
>> -If you say yes here you get hwmon support for triton
>> -TWL4030-MADC.
>> -
>> -This driver can also be built as a module. If so it will be called
>> -twl4030-madc-hwmon.
>> -
>>  config SENSORS_VEXPRESS
>>  tristate "Versatile Express"
>>  depends on VEXPRESS_CONFIG
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 5509edf6186a..c05530d6cb6e 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -157,7 +157,6 @@ obj-$(CONFIG_SENSORS_TMP103) += tmp103.o
>>  obj-$(CONFIG_SENSORS_TMP108)+= tmp108.o
>>  obj-$(CONFIG_SENSORS_TMP401)+= tmp401.o
>>  obj-$(CONFIG_SENSORS_TMP421)+= tmp421.o
>> -obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
>>  obj-$(CONFIG_SENSORS_VEXPRESS)  += vexpress-hwmon.o
>>  obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>  obj-$(CONFIG_SENSORS_VIA686A)   += via686a.o
>> diff --git a/drivers/hwmon/twl4030-madc-hwmon.c 
>> b/drivers/hwmon/twl4030-madc-hwmon.c
>> deleted file mode 100644
>> index b5caf7fdb487..
>> --- a/drivers/hwmon/twl4030-madc-hwmon.c
>> +++ /dev/null
>> @@ -1,118 +0,0 @@
>> -/*
>> - *
>> - * TWL4030 MADC Hwmon driver-This driver monitors the real time
>> - * conversion of analog signals like battery temperature,
>> - * battery type, battery level etc. User can ask for the conversion on a
>> - * particular channel using the sysfs nodes.
>> - *
>> - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com/
>> - * J Keerthy 
>> - *
>> - * This program is free software; you can redistribute it and/or
>> - * modify it under the terms of the GNU General Public License
>> - * version 2 as published by the Free Software Foundation.
>> - *
>> - * This program is distributed in the hope that it will be useful, but
>> - * WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> - * General Public License for more details.
>> - *
>> - * You should have received a copy of the GNU General Public License
>> - * along with this program; if not, write to the Free Software
>> - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>> - * 02110-1301 USA
>> - *
>> - */
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -#include 
>> -
>> -/*
>> - * sysfs hook function
>> - */
>> -static ssize_t madc_read(struct device *dev,
>> - struct device_attribute *devattr, char *buf)
>> -{
>> -struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> -struct twl4030_madc_request req = {
>> -.channels = 1 << attr->index,
>> -.method = TWL4030_MADC_SW2,
>> -.type = TWL4030_MADC_WAIT,
>> -};
>> -long val;
>> -
>> -val = twl4030_madc_conversion();
>> -if (val < 0)
>> -return val;
>> -
>> -return sprintf(buf, "%d\n", req.rbuf[attr->index]);
>> -}
>> -
>> -/* sysfs nodes to read individual channels from user side */
>> -static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, madc_read, NULL, 0);
>> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, madc_read, NULL, 1);
>> -static 

Re: [PATCH 7/7] iio: adc: twl4030: Fold twl4030-madc.h into driver

[Jonathan Cameron]

On 27/04/17 16:30, Sebastian Reichel wrote:
> twl4030-madc.h is no longer used by anything outside of
> the iio driver, so it can be merged into the driver.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  drivers/iio/adc/twl4030-madc.c   | 113 +++-
>  include/linux/i2c/twl4030-madc.h | 137 
> ---
>  2 files changed, 112 insertions(+), 138 deletions(-)
>  delete mode 100644 include/linux/i2c/twl4030-madc.h
> 
> diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> index 21df5b932bd1..bd3d37fc2144 100644
> --- a/drivers/iio/adc/twl4030-madc.c
> +++ b/drivers/iio/adc/twl4030-madc.c
> @@ -36,7 +36,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -49,9 +48,121 @@
>  
>  #include 
>  
> +#define TWL4030_MADC_MAX_CHANNELS 16
> +
> +#define TWL4030_MADC_CTRL1   0x00
> +#define TWL4030_MADC_CTRL2   0x01
> +
> +#define TWL4030_MADC_RTSELECT_LSB0x02
> +#define TWL4030_MADC_SW1SELECT_LSB   0x06
> +#define TWL4030_MADC_SW2SELECT_LSB   0x0A
> +
> +#define TWL4030_MADC_RTAVERAGE_LSB   0x04
> +#define TWL4030_MADC_SW1AVERAGE_LSB  0x08
> +#define TWL4030_MADC_SW2AVERAGE_LSB  0x0C
> +
> +#define TWL4030_MADC_CTRL_SW10x12
> +#define TWL4030_MADC_CTRL_SW20x13
> +
> +#define TWL4030_MADC_RTCH0_LSB   0x17
> +#define TWL4030_MADC_GPCH0_LSB   0x37
> +
> +#define TWL4030_MADC_MADCON  (1 << 0)/* MADC power on */
> +#define TWL4030_MADC_BUSY(1 << 0)/* MADC busy */
> +/* MADC conversion completion */
> +#define TWL4030_MADC_EOC_SW  (1 << 1)
> +/* MADC SWx start conversion */
> +#define TWL4030_MADC_SW_START(1 << 5)
> +#define TWL4030_MADC_ADCIN0  (1 << 0)
> +#define TWL4030_MADC_ADCIN1  (1 << 1)
> +#define TWL4030_MADC_ADCIN2  (1 << 2)
> +#define TWL4030_MADC_ADCIN3  (1 << 3)
> +#define TWL4030_MADC_ADCIN4  (1 << 4)
> +#define TWL4030_MADC_ADCIN5  (1 << 5)
> +#define TWL4030_MADC_ADCIN6  (1 << 6)
> +#define TWL4030_MADC_ADCIN7  (1 << 7)
> +#define TWL4030_MADC_ADCIN8  (1 << 8)
> +#define TWL4030_MADC_ADCIN9  (1 << 9)
> +#define TWL4030_MADC_ADCIN10 (1 << 10)
> +#define TWL4030_MADC_ADCIN11 (1 << 11)
> +#define TWL4030_MADC_ADCIN12 (1 << 12)
> +#define TWL4030_MADC_ADCIN13 (1 << 13)
> +#define TWL4030_MADC_ADCIN14 (1 << 14)
> +#define TWL4030_MADC_ADCIN15 (1 << 15)
> +
> +/* Fixed channels */
> +#define TWL4030_MADC_BTEMP   TWL4030_MADC_ADCIN1
> +#define TWL4030_MADC_VBUSTWL4030_MADC_ADCIN8
> +#define TWL4030_MADC_VBKBTWL4030_MADC_ADCIN9
> +#define TWL4030_MADC_ICHGTWL4030_MADC_ADCIN10
> +#define TWL4030_MADC_VCHGTWL4030_MADC_ADCIN11
> +#define TWL4030_MADC_VBATTWL4030_MADC_ADCIN12
> +
> +/* Step size and prescaler ratio */
> +#define TEMP_STEP_SIZE  147
> +#define TEMP_PSR_R  100
> +#define CURR_STEP_SIZE   147
> +#define CURR_PSR_R1  44
> +#define CURR_PSR_R2  88
> +
> +#define TWL4030_BCI_BCICTL1  0x23
> +#define TWL4030_BCI_CGAIN0x020
> +#define TWL4030_BCI_MESBAT   (1 << 1)
> +#define TWL4030_BCI_TYPEN(1 << 4)
> +#define TWL4030_BCI_ITHEN(1 << 3)
> +
> +#define REG_BCICTL2 0x024
> +#define TWL4030_BCI_ITHSENS  0x007
> +
> +/* Register and bits for GPBR1 register */
> +#define TWL4030_REG_GPBR10x0c
> +#define TWL4030_GPBR1_MADC_HFCLK_EN  (1 << 7)
> +
>  #define TWL4030_USB_SEL_MADC_MCPC(1<<3)
>  #define TWL4030_USB_CARKIT_ANA_CTRL  0xBB
>  
> +struct twl4030_madc_conversion_method {
> + u8 sel;
> + u8 avg;
> + u8 rbase;
> + u8 ctrl;
> +};
> +
> +/**
> + * struct twl4030_madc_request - madc request packet for channel conversion
> + * @channels:16 bit bitmap for individual channels
> + * @do_avg:  sample the input channel for 4 consecutive cycles
> + * @method:  RT, SW1, SW2
> + * @type:Polling or interrupt based method
> + * @active:  Flag if request is active
> + * @result_pending: Flag from irq handler, that result is ready
> + * @raw: Return raw value, do not convert it
> + * @rbuf:Result buffer
> + */
> +struct twl4030_madc_request {
> + unsigned long channels;
> + bool do_avg;
> + u16 method;
> + u16 type;
> + bool active;
> + bool result_pending;
> + bool raw;
> + int rbuf[TWL4030_MADC_MA

Re: [PATCH 6/7] iio: adc: twl4030: Remove twl4030_madc_request.func_cb

[Jonathan Cameron]

On 27/04/17 16:30, Sebastian Reichel wrote:
> This functionality is not used by the IIO subsystem. Due
> to removal of legacy API it can also be removed.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  drivers/iio/adc/twl4030-madc.c   | 70 
> 
>  include/linux/i2c/twl4030-madc.h |  1 -
>  2 files changed, 71 deletions(-)
> 
> diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> index be60f76d1a50..21df5b932bd1 100644
> --- a/drivers/iio/adc/twl4030-madc.c
> +++ b/drivers/iio/adc/twl4030-madc.c
> @@ -86,7 +86,6 @@ static int twl4030_madc_read(struct iio_dev *iio_dev,
>  
>   req.channels = BIT(chan->channel);
>   req.active = false;
> - req.func_cb = NULL;
>   req.type = TWL4030_MADC_WAIT;
>   req.raw = !(mask == IIO_CHAN_INFO_PROCESSED);
>   req.do_avg = (mask == IIO_CHAN_INFO_AVERAGE_RAW);
> @@ -343,37 +342,6 @@ static int twl4030_madc_read_channels(struct 
> twl4030_madc_data *madc,
>  }
>  
>  /*
> - * Enables irq.
> - * @madc - pointer to twl4030_madc_data struct
> - * @id - irq number to be enabled
> - * can take one of TWL4030_MADC_RT, TWL4030_MADC_SW1, TWL4030_MADC_SW2
> - * corresponding to RT, SW1, SW2 conversion requests.
> - * If the i2c read fails it returns an error else returns 0.
> - */
> -static int twl4030_madc_enable_irq(struct twl4030_madc_data *madc, u8 id)
> -{
> - u8 val;
> - int ret;
> -
> - ret = twl_i2c_read_u8(TWL4030_MODULE_MADC, , madc->imr);
> - if (ret) {
> - dev_err(madc->dev, "unable to read imr register 0x%X\n",
> - madc->imr);
> - return ret;
> - }
> -
> - val &= ~(1 << id);
> - ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, madc->imr);
> - if (ret) {
> - dev_err(madc->dev,
> - "unable to write imr register 0x%X\n", madc->imr);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -/*
>   * Disables irq.
>   * @madc - pointer to twl4030_madc_data struct
>   * @id - irq number to be disabled
> @@ -442,11 +410,6 @@ static irqreturn_t twl4030_madc_threaded_irq_handler(int 
> irq, void *_madc)
>   /* Read results */
>   len = twl4030_madc_read_channels(madc, method->rbase,
>r->channels, r->rbuf, r->raw);
> - /* Return results to caller */
> - if (r->func_cb != NULL) {
> - r->func_cb(len, r->channels, r->rbuf);
> - r->func_cb = NULL;
> - }
>   /* Free request */
>   r->result_pending = 0;
>   r->active = 0;
> @@ -468,11 +431,6 @@ static irqreturn_t twl4030_madc_threaded_irq_handler(int 
> irq, void *_madc)
>   /* Read results */
>   len = twl4030_madc_read_channels(madc, method->rbase,
>r->channels, r->rbuf, r->raw);
> - /* Return results to caller */
> - if (r->func_cb != NULL) {
> - r->func_cb(len, r->channels, r->rbuf);
> - r->func_cb = NULL;
> - }
>   /* Free request */
>   r->result_pending = 0;
>   r->active = 0;
> @@ -482,23 +440,6 @@ static irqreturn_t twl4030_madc_threaded_irq_handler(int 
> irq, void *_madc)
>   return IRQ_HANDLED;
>  }
>  
> -static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> - struct twl4030_madc_request *req)
> -{
> - struct twl4030_madc_request *p;
> - int ret;
> -
> - p = >requests[req->method];
> - memcpy(p, req, sizeof(*req));
> - ret = twl4030_madc_enable_irq(madc, req->method);
> - if (ret < 0) {
> - dev_err(madc->dev, "enable irq failed!!\n");
> - return ret;
> - }
> -
> - return 0;
> -}
> -
>  /*
>   * Function which enables the madc conversion
>   * by writing to the control register.
> @@ -607,17 +548,6 @@ static int twl4030_madc_conversion(struct 
> twl4030_madc_request *req)
>   goto out;
>   }
>   }
> - if (req->type == TWL4030_MADC_IRQ_ONESHOT && req->func_cb != NULL) {
> - ret = twl4030_madc_set_irq(twl4030_madc, req);
> - if (ret < 0)
> - goto out;
> - ret 

Re: [PATCH 3/7] iio: adc: twl4030: Drop twl4030_get_madc_conversion()

[Jonathan Cameron]

On 27/04/17 16:30, Sebastian Reichel wrote:
> Drop legacy twl4030_get_madc_conversion() method. It has been
> used by drivers to get madc data before it conversion to IIO
> API. There are no users in the mainline kernel anymore.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  drivers/iio/adc/twl4030-madc.c   | 21 -
>  include/linux/i2c/twl4030-madc.h |  1 -
>  2 files changed, 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> index 0c74869a540a..88e44066ef82 100644
> --- a/drivers/iio/adc/twl4030-madc.c
> +++ b/drivers/iio/adc/twl4030-madc.c
> @@ -642,27 +642,6 @@ int twl4030_madc_conversion(struct twl4030_madc_request 
> *req)
>  }
>  EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
>  
> -int twl4030_get_madc_conversion(int channel_no)
> -{
> - struct twl4030_madc_request req;
> - int temp = 0;
> - int ret;
> -
> - req.channels = (1 << channel_no);
> - req.method = TWL4030_MADC_SW2;
> - req.active = 0;
> - req.raw = 0;
> - req.func_cb = NULL;
> - ret = twl4030_madc_conversion();
> - if (ret < 0)
> - return ret;
> - if (req.rbuf[channel_no] > 0)
> - temp = req.rbuf[channel_no];
> -
> - return temp;
> -}
> -EXPORT_SYMBOL_GPL(twl4030_get_madc_conversion);
> -
>  /**
>   * twl4030_madc_set_current_generator() - setup bias current
>   *
> diff --git a/include/linux/i2c/twl4030-madc.h 
> b/include/linux/i2c/twl4030-madc.h
> index 1c0134dd3271..0c919ebb31e0 100644
> --- a/include/linux/i2c/twl4030-madc.h
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -143,5 +143,4 @@ struct twl4030_madc_user_parms {
>  };
>  
>  int twl4030_madc_conversion(struct twl4030_madc_request *conv);
> -int twl4030_get_madc_conversion(int channel_no);
>  #endif
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] iio: adc: twl4030: Unexport twl4030_madc_conversion()

[Jonathan Cameron]

On 27/04/17 16:30, Sebastian Reichel wrote:
> All madc users have been converted to IIO API, so drop the
> legacy API. The function is still used inside of the driver.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  drivers/iio/adc/twl4030-madc.c   | 5 +++--
>  include/linux/i2c/twl4030-madc.h | 2 --
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/twl4030-madc.c b/drivers/iio/adc/twl4030-madc.c
> index 88e44066ef82..be60f76d1a50 100644
> --- a/drivers/iio/adc/twl4030-madc.c
> +++ b/drivers/iio/adc/twl4030-madc.c
> @@ -72,6 +72,8 @@ struct twl4030_madc_data {
>   u8 isr;
>  };
>  
> +static int twl4030_madc_conversion(struct twl4030_madc_request *req);
> +
>  static int twl4030_madc_read(struct iio_dev *iio_dev,
>const struct iio_chan_spec *chan,
>int *val, int *val2, long mask)
> @@ -568,7 +570,7 @@ static int twl4030_madc_wait_conversion_ready(struct 
> twl4030_madc_data *madc,
>   * be a negative error value in the corresponding array element.
>   * returns 0 if succeeds else error value
>   */
> -int twl4030_madc_conversion(struct twl4030_madc_request *req)
> +static int twl4030_madc_conversion(struct twl4030_madc_request *req)
>  {
>   const struct twl4030_madc_conversion_method *method;
>   int ret;
> @@ -640,7 +642,6 @@ int twl4030_madc_conversion(struct twl4030_madc_request 
> *req)
>  
>   return ret;
>  }
> -EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
>  
>  /**
>   * twl4030_madc_set_current_generator() - setup bias current
> diff --git a/include/linux/i2c/twl4030-madc.h 
> b/include/linux/i2c/twl4030-madc.h
> index 0c919ebb31e0..be9260e261ac 100644
> --- a/include/linux/i2c/twl4030-madc.h
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -141,6 +141,4 @@ struct twl4030_madc_user_parms {
>   int status;
>   u16 result;
>  };
> -
> -int twl4030_madc_conversion(struct twl4030_madc_request *conv);
>  #endif
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] hwmon: Add driver for Infineon DPS310

[Jonathan Cameron]

On 26/04/17 16:13, Guenter Roeck wrote:
> Hi Joel,
> 
> On Thu, Apr 27, 2017 at 12:03:21AM +0930, Joel Stanley wrote:
>> Hi Guenter,
>>
>> Below is a work in progress hwmon driver for the DPS310[1]. The device does
>> both pressure and temperature monitoring over either I2C or SPI. There's a
>> public datasheet available on Infineon's website.
>>
>> I require a driver that reads the temperature data over I2C and exposes it to
>> userspace as a hwmon device. I wanted your opinion weather this should be an
>> IIO driver (and we could then use the IIO HWMON bridge in our application), 
>> or
>> if it should be a native HWMON driver?
>>
> 
> IIO, I think. Pressure isn't really for hardware monitoring, and we would have
> a hard time supporting it. IIO already has the necessary infrastructure for 
> it.
> 
> Jonathan, any thoughts/comments ?
> 
Agree with you.  Pressure doesn't really fit in hwmon, so IIO is best location.
> Thanks,
> Guenter
> 
>> Once we've agreed on a subsystem I will clean the driver up and submit it
>> properly.
>>
>> Cheers,
>>
>> Joel
>>
>> [1] 
>> https://www.infineon.com/cms/en/product/sensor/capacitive-pressure-sensor-for-consumer-applications/channel.html
>>
>> ---
>>
>> The DPS310 is a temperature and pressure sensor. It can be accessed over
>> i2c and SPI.
>>
>> This driver supports continual measurement of temperature over i2c only.
>>
>> Signed-off-by: Joel Stanley 
>> ---
>>  drivers/hwmon/Kconfig  |  12 ++
>>  drivers/hwmon/Makefile |   1 +
>>  drivers/hwmon/dps310.c | 331 
>> +
>>  3 files changed, 344 insertions(+)
>>  create mode 100644 drivers/hwmon/dps310.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 190d270b20a2..8229b2f8c591 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1349,6 +1349,18 @@ config SENSORS_DME1737
>>This driver can also be built as a module.  If so, the module
>>will be called dme1737.
>>  
>> +config SENSORS_DPS310
>> +tristate "Infineon DPS310"
>> +depends on I2C
>> +select HWMON_VID
>> +select REGMAP_I2C
>> +help
>> +  If you say yes here you get support for the hardware monitoring
>> +  features of the Infineon DPS310 digital barometric pressure sensor.
>> +
>> +  This driver can also be built as a module.  If so, the module
>> +  will be called dps310.
>> +
>>  config SENSORS_EMC1403
>>  tristate "SMSC EMC1403/23 thermal sensor"
>>  depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index d2cb7e804a0f..3a5ad06d6d89 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -52,6 +52,7 @@ obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>>  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
>>  obj-$(CONFIG_SENSORS_DELL_SMM)  += dell-smm-hwmon.o
>>  obj-$(CONFIG_SENSORS_DME1737)   += dme1737.o
>> +obj-$(CONFIG_SENSORS_DPS310)+= dps310.o
>>  obj-$(CONFIG_SENSORS_DS620) += ds620.o
>>  obj-$(CONFIG_SENSORS_DS1621)+= ds1621.o
>>  obj-$(CONFIG_SENSORS_EMC1403)   += emc1403.o
>> diff --git a/drivers/hwmon/dps310.c b/drivers/hwmon/dps310.c
>> new file mode 100644
>> index ..b014f01d0d3f
>> --- /dev/null
>> +++ b/drivers/hwmon/dps310.c
>> @@ -0,0 +1,331 @@
>> +/*
>> + * Copyright 2017 IBM Corporation
>> + *
>> + * Joel Stanley 
>> + *
>> + * The DPS310 is a barometric pressure and temperature sensor.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define PRS_B2  0x00
>> +#define PRS_B1  0x01
>> +#define PRS_B0  0x02
>> +#define TMP_B2  0x03
>> +#define TMP_B1  0x04
>> +#define TMP_B0  0x05
>> +#define PRS_CFG 0x06
>> +#define TMP_CFG 0x07
>> +#define  TMP_RATE_BITS  GENMASK(6, 4)
>> +#define  TMP_EXTBIT(7)
>> +#define MEAS_CFG0x08
>> +#define  MEAS_CTRL_BITS GENMASK(2, 0)
>> +#define   PRESSURE_EN   BIT(0)
>> +#define   TEMP_EN   BIT(1)
>> +#define   BACKGROUNDBIT(2)
>> +#define  PRS_RDYBIT(4)
>> +#define  TMP_RDYBIT(5)
>> +#define  SENSOR_RDY BIT(6)
>> +#define  COEF_RDY   BIT(7)
>> +#define RESET   0x0c
>> +#define  RESET_MAGIC(BIT(0) | BIT(3))
>> +#define COEF_BASE   0x10
>> +
>> +#define TMP_BASETMP_B2
>> +#define PRS_BASEPRS_B2
>> +
>> +#define TMP_RATE(_n)ilog2(_n)
>> +#define TMP_PRC(_n) ilog2(_n)
>> +
>> +const int scale_factor[] = {
>> + 524288,
>> +1572864,
>> +3670016,
>> +7864320,
>> + 253952,
>> + 516096,
>> +1040384,
>> +2088960,
>> +};
>> +
>> +struct dps310_data {
>> + 

Re: [PATCH 1/3] iio: humidity: add sht21 relative humidity and temperature sensor driver

[Jonathan Cameron]

On 19/02/17 15:04, Jonathan Cameron wrote:
> On 19/02/17 14:58, Tomasz Duszynski wrote:
>> Add sht21 relative humidity and temperature sensor driver. There already
>> exists an in-kernel driver under hwmon but with limited functionality
>> like humidity and temperature always measured together at predefined
>> resolutions, etc.
>>
>> New iio driver comes without limitations of predecessor, uses non-blocking 
>> i2c
>> communication mode and adds triggered buffer support thus is suited for more
>> use cases.
>>
>> Device datasheet can be found under:
>>
>> https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/ \
>> Dokumente/2_Humidity_Sensors/Sensirion_Humidity_Sensors_SHT21_Datasheet_V4.pdf
>>
>> Signed-off-by: Tomasz Duszynski <tdusz...@gmail.com>
> Hi Tomasz,
> 
> I haven't looked at this yet, but one thing we have to do whenever suggesting
> replacement of an hwmon driver is to convince the hwmon guys that it is a
> good idea.  
> 
> As such I've cc'd Guenter, Jean and the list.
> 
> Jonathan
> 
> p.s Probably won't get to this today as running out of time.  Depending on
> how mad the week is it might be next weekend before I get a chance.
Very quick review follows so as not to leave you without feedback.

I may well have missed stuff though as did this in a few minutes...

J
>> ---
>>  drivers/iio/humidity/Kconfig  |  10 +
>>  drivers/iio/humidity/Makefile |   1 +
>>  drivers/iio/humidity/sht21.c  | 519 
>> ++
>>  3 files changed, 530 insertions(+)
>>  create mode 100644 drivers/iio/humidity/sht21.c
>>
>> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
>> index 866dda16..93247ecc9324 100644
>> --- a/drivers/iio/humidity/Kconfig
>> +++ b/drivers/iio/humidity/Kconfig
>> @@ -35,6 +35,16 @@ config HTU21
>>This driver can also be built as a module. If so, the module will
>>be called htu21.
>>
>> +config SHT21
>> +tristate "Sensirion SHT21 relative humidity and temperature sensor"
>> +depends on I2C
>> +help
>> +  Say yes here to build support for the  Sensirion SHT21 relative
>> +  humidity and temperature sensor.
>> +
>> +  To compile this driver as a module, choose M here: the module
>> +  will be called sht21.
>> +
>>  config SI7005
>>  tristate "SI7005 relative humidity and temperature sensor"
>>  depends on I2C
>> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
>> index c9f089a9a6b8..f2472fd2cc44 100644
>> --- a/drivers/iio/humidity/Makefile
>> +++ b/drivers/iio/humidity/Makefile
>> @@ -5,5 +5,6 @@
>>  obj-$(CONFIG_DHT11) += dht11.o
>>  obj-$(CONFIG_HDC100X) += hdc100x.o
>>  obj-$(CONFIG_HTU21) += htu21.o
>> +obj-$(CONFIG_SHT21) += sht21.o
>>  obj-$(CONFIG_SI7005) += si7005.o
>>  obj-$(CONFIG_SI7020) += si7020.o
>> diff --git a/drivers/iio/humidity/sht21.c b/drivers/iio/humidity/sht21.c
>> new file mode 100644
>> index ..199933b87297
>> --- /dev/null
>> +++ b/drivers/iio/humidity/sht21.c
>> @@ -0,0 +1,519 @@
>> +/*
>> + * Sensirion SHT21 relative humidity and temperature sensor driver
>> + *
>> + * Copyright (C) 2017 Tomasz Duszynski <tdusz...@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * 7-bit I2C slave address: 0x40
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define SHT21_DRV_NAME "sht21"
>> +
>> +#define SHT21_TRIGGER_T_MEASUREMENT 0xF3
>> +#define SHT21_TRIGGER_RH_MEASUREMENT 0xF5
>> +
>> +#define SHT21_WRITE_USER_REG 0xE6
>> +#define SHT21_READ_USER_REG 0xE7
>> +#define SHT21_SOFT_RESET 0xFE
>> +
>> +#define SHT21_USER_REG_RES_8_12 BIT(0)
>> +#define SHT21_USER_REG_RES_10_13 BIT(7)
>>

Re: [PATCH 1/3] iio: humidity: add sht21 relative humidity and temperature sensor driver

[Jonathan Cameron]

On 19/02/17 14:58, Tomasz Duszynski wrote:
> Add sht21 relative humidity and temperature sensor driver. There already
> exists an in-kernel driver under hwmon but with limited functionality
> like humidity and temperature always measured together at predefined
> resolutions, etc.
> 
> New iio driver comes without limitations of predecessor, uses non-blocking i2c
> communication mode and adds triggered buffer support thus is suited for more
> use cases.
> 
> Device datasheet can be found under:
> 
> https://www.sensirion.com/fileadmin/user_upload/customers/sensirion/ \
> Dokumente/2_Humidity_Sensors/Sensirion_Humidity_Sensors_SHT21_Datasheet_V4.pdf
> 
> Signed-off-by: Tomasz Duszynski 
Hi Tomasz,

I haven't looked at this yet, but one thing we have to do whenever suggesting
replacement of an hwmon driver is to convince the hwmon guys that it is a
good idea.  

As such I've cc'd Guenter, Jean and the list.

Jonathan

p.s Probably won't get to this today as running out of time.  Depending on
how mad the week is it might be next weekend before I get a chance.
> ---
>  drivers/iio/humidity/Kconfig  |  10 +
>  drivers/iio/humidity/Makefile |   1 +
>  drivers/iio/humidity/sht21.c  | 519 
> ++
>  3 files changed, 530 insertions(+)
>  create mode 100644 drivers/iio/humidity/sht21.c
> 
> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> index 866dda16..93247ecc9324 100644
> --- a/drivers/iio/humidity/Kconfig
> +++ b/drivers/iio/humidity/Kconfig
> @@ -35,6 +35,16 @@ config HTU21
> This driver can also be built as a module. If so, the module will
> be called htu21.
> 
> +config SHT21
> + tristate "Sensirion SHT21 relative humidity and temperature sensor"
> + depends on I2C
> + help
> +   Say yes here to build support for the  Sensirion SHT21 relative
> +   humidity and temperature sensor.
> +
> +   To compile this driver as a module, choose M here: the module
> +   will be called sht21.
> +
>  config SI7005
>   tristate "SI7005 relative humidity and temperature sensor"
>   depends on I2C
> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
> index c9f089a9a6b8..f2472fd2cc44 100644
> --- a/drivers/iio/humidity/Makefile
> +++ b/drivers/iio/humidity/Makefile
> @@ -5,5 +5,6 @@
>  obj-$(CONFIG_DHT11) += dht11.o
>  obj-$(CONFIG_HDC100X) += hdc100x.o
>  obj-$(CONFIG_HTU21) += htu21.o
> +obj-$(CONFIG_SHT21) += sht21.o
>  obj-$(CONFIG_SI7005) += si7005.o
>  obj-$(CONFIG_SI7020) += si7020.o
> diff --git a/drivers/iio/humidity/sht21.c b/drivers/iio/humidity/sht21.c
> new file mode 100644
> index ..199933b87297
> --- /dev/null
> +++ b/drivers/iio/humidity/sht21.c
> @@ -0,0 +1,519 @@
> +/*
> + * Sensirion SHT21 relative humidity and temperature sensor driver
> + *
> + * Copyright (C) 2017 Tomasz Duszynski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * 7-bit I2C slave address: 0x40
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SHT21_DRV_NAME "sht21"
> +
> +#define SHT21_TRIGGER_T_MEASUREMENT 0xF3
> +#define SHT21_TRIGGER_RH_MEASUREMENT 0xF5
> +
> +#define SHT21_WRITE_USER_REG 0xE6
> +#define SHT21_READ_USER_REG 0xE7
> +#define SHT21_SOFT_RESET 0xFE
> +
> +#define SHT21_USER_REG_RES_8_12 BIT(0)
> +#define SHT21_USER_REG_RES_10_13 BIT(7)
> +#define SHT21_USER_REG_RES_11_11 (BIT(7) | BIT(0))
> +
> +#define SHT21_USER_REG_ENABLE_HEATER BIT(2)
> +
> +enum {
> + RES_12_14,
> + RES_8_12,
> + RES_10_13,
> + RES_11_11,
> +};
> +
> +/*
> + * Combined sampling frequency i.e measuring RH and T together, which seems
> + * to be common case for pressure/humidity sensors, was chosen so that sensor
> + * is active for only 10% of time thus avoiding self-heating effect.
> + *
> + * Note that sampling frequencies are higher when measuring RH or T 
> separately.
> + *
> + * Following table shows how available resolutions, combined sampling 
> frequency
> + * and frequencies for RH and T when measured separately are related.
> + *
> + * +---+-+-++
> + * | RH / T resolution | RH + T [Hz] | RH [Hz] | T [Hz] |
> + * +---+-+-++
> + * | 12 / 14   | 0.88| 3.45| 1.18   |
> + * +---+-+-++
> 

Re: [PATCH 3/7] power: supply: ab8500_fg: convert to IIO ADC

[Jonathan Cameron]

On 12/01/17 03:03, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jan 11, 2017 at 12:47:41AM +0100, Linus Walleij wrote:
>> This switches the AB8500 fuel gauge driver to using
>> the standard IIO ADC channel lookup and conversion routines.
>>
>> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
> 
> Acked-By: Sebastian Reichel <s...@kernel.org>
Acked-by: Jonathan Cameron <ji...@kernel.org>
> 
> -- Sebastian
> 
>>  drivers/power/supply/ab8500_fg.c | 23 +++
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/power/supply/ab8500_fg.c 
>> b/drivers/power/supply/ab8500_fg.c
>> index c569f82a0071..b26122e1da51 100644
>> --- a/drivers/power/supply/ab8500_fg.c
>> +++ b/drivers/power/supply/ab8500_fg.c
>> @@ -32,7 +32,7 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>> +#include 
>>  #include 
>>  
>>  #define MILLI_TO_MICRO  1000
>> @@ -182,7 +182,7 @@ struct inst_curr_result_list {
>>   * @bat_cap:Structure for battery capacity specific 
>> parameters
>>   * @avg_cap:Average capacity filter
>>   * @parent: Pointer to the struct ab8500
>> - * @gpadc:  Pointer to the struct gpadc
>> + * @main_bat_v: ADC channel for the main battery voltage
>>   * @bm: Platform specific battery management information
>>   * @fg_psy: Structure that holds the FG specific battery properties
>>   * @fg_wq:  Work queue for running the FG algorithm
>> @@ -224,7 +224,7 @@ struct ab8500_fg {
>>  struct ab8500_fg_battery_capacity bat_cap;
>>  struct ab8500_fg_avg_cap avg_cap;
>>  struct ab8500 *parent;
>> -struct ab8500_gpadc *gpadc;
>> +struct iio_channel *main_bat_v;
>>  struct abx500_bm_data *bm;
>>  struct power_supply *fg_psy;
>>  struct workqueue_struct *fg_wq;
>> @@ -831,13 +831,13 @@ static void ab8500_fg_acc_cur_work(struct work_struct 
>> *work)
>>   */
>>  static int ab8500_fg_bat_voltage(struct ab8500_fg *di)
>>  {
>> -int vbat;
>> +int vbat, ret;
>>  static int prev;
>>  
>> -vbat = ab8500_gpadc_convert(di->gpadc, MAIN_BAT_V);
>> -if (vbat < 0) {
>> +ret = iio_read_channel_processed(di->main_bat_v, );
>> +if (ret < 0) {
>>  dev_err(di->dev,
>> -"%s gpadc conversion failed, using previous value\n",
>> +"%s ADC conversion failed, using previous value\n",
>>  __func__);
>>  return prev;
>>  }
>> @@ -3072,7 +3072,14 @@ static int ab8500_fg_probe(struct platform_device 
>> *pdev)
>>  /* get parent data */
>>  di->dev = >dev;
>>  di->parent = dev_get_drvdata(pdev->dev.parent);
>> -di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
>> +
>> +di->main_bat_v = devm_iio_channel_get(>dev, "main_bat_v");
>> +if (IS_ERR(di->main_bat_v)) {
>> +if (PTR_ERR(di->main_bat_v) == -ENODEV)
>> +return -EPROBE_DEFER;
>> +dev_err(>dev, "failed to get main battery ADC channel\n");
>> +return PTR_ERR(di->main_bat_v);
>> +}
>>  
>>  psy_cfg.supplied_to = supply_interface;
>>  psy_cfg.num_supplicants = ARRAY_SIZE(supply_interface);
>> -- 
>> 2.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] power: supply: ab8500_charger: convert to IIO ADC

[Jonathan Cameron]

On 10/01/17 23:47, Linus Walleij wrote:
> This switches the AB8500 battery charger driver to using
> the standard IIO ADC channel lookup and conversion routines.
> 
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
Other than the fact you have no dependencies on IIO being there
or similar (which the build bots pointed out anyway).

Acked-by: Jonathan Cameron <ji...@kernel.org>

> ---
>  drivers/power/supply/ab8500_charger.c | 78 
> ++-
>  1 file changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/power/supply/ab8500_charger.c 
> b/drivers/power/supply/ab8500_charger.c
> index 5cee9aa87aa3..c1c4873899e9 100644
> --- a/drivers/power/supply/ab8500_charger.c
> +++ b/drivers/power/supply/ab8500_charger.c
> @@ -29,10 +29,10 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /* Charger constants */
>  #define NO_PW_CONN   0
> @@ -235,7 +235,10 @@ struct ab8500_charger_max_usb_in_curr {
>   * @current_stepping_sessions:
>   *   Counter for current stepping sessions
>   * @parent:  Pointer to the struct ab8500
> - * @gpadc:   Pointer to the struct gpadc
> + * @adc_main_charger_v   ADC channel for main charger voltage
> + * @adc_main_charger_c   ADC channel for main charger current
> + * @adc_vbus_v   ADC channel for USB charger voltage
> + * @adc_usb_charger_cADC channel for USB charger current
>   * @bm:  Platform specific battery management information
>   * @flags:   Structure for information about events triggered
>   * @usb_state:   Structure for usb stack information
> @@ -285,7 +288,10 @@ struct ab8500_charger {
>   int is_aca_rid;
>   atomic_t current_stepping_sessions;
>   struct ab8500 *parent;
> - struct ab8500_gpadc *gpadc;
> + struct iio_channel *adc_main_charger_v;
> + struct iio_channel *adc_main_charger_c;
> + struct iio_channel *adc_vbus_v;
> + struct iio_channel *adc_usb_charger_c;
>   struct abx500_bm_data *bm;
>   struct ab8500_charger_event_flags flags;
>   struct ab8500_charger_usb_state usb_state;
> @@ -461,13 +467,13 @@ static void ab8500_charger_set_usb_connected(struct 
> ab8500_charger *di,
>   */
>  static int ab8500_charger_get_ac_voltage(struct ab8500_charger *di)
>  {
> - int vch;
> + int vch, ret;
>  
>   /* Only measure voltage if the charger is connected */
>   if (di->ac.charger_connected) {
> - vch = ab8500_gpadc_convert(di->gpadc, MAIN_CHARGER_V);
> - if (vch < 0)
> - dev_err(di->dev, "%s gpadc conv failed,\n", __func__);
> + ret = iio_read_channel_processed(di->adc_main_charger_v, );
> + if (ret < 0)
> + dev_err(di->dev, "%s ADC conv failed,\n", __func__);
>   } else {
>   vch = 0;
>   }
> @@ -512,13 +518,13 @@ static int ab8500_charger_ac_cv(struct ab8500_charger 
> *di)
>   */
>  static int ab8500_charger_get_vbus_voltage(struct ab8500_charger *di)
>  {
> - int vch;
> + int vch, ret;
>  
>   /* Only measure voltage if the charger is connected */
>   if (di->usb.charger_connected) {
> - vch = ab8500_gpadc_convert(di->gpadc, VBUS_V);
> - if (vch < 0)
> - dev_err(di->dev, "%s gpadc conv failed\n", __func__);
> + ret = iio_read_channel_processed(di->adc_vbus_v, );
> + if (ret < 0)
> + dev_err(di->dev, "%s ADC conv failed,\n", __func__);
>   } else {
>   vch = 0;
>   }
> @@ -534,13 +540,13 @@ static int ab8500_charger_get_vbus_voltage(struct 
> ab8500_charger *di)
>   */
>  static int ab8500_charger_get_usb_current(struct ab8500_charger *di)
>  {
> - int ich;
> + int ich, ret;
>  
>   /* Only measure current if the charger is online */
>   if (di->usb.charger_online) {
> - ich = ab8500_gpadc_convert(di->gpadc, USB_CHARGER_C);
> - if (ich < 0)
> - dev_err(di->dev, "%s gpadc conv failed\n", __func__);
> + ret = iio_read_channel_processed(di->adc_usb_charger_c, );
> + if (ret < 0)
> + dev_err(di->dev, "%s ADC conv failed,\n", __func__);
>   } else {
>   ich = 0;
>   }
> @@ -556,13 +562,13 @@ static int ab8500_charger_get_usb_current(struct 
> ab8500_charger *di)
>   */

Re: [PATCH 1/7] power: supply: ab8500_btemp: convert to IIO ADC

[Jonathan Cameron]

On 10/01/17 23:47, Linus Walleij wrote:
> This switches the AB8500 battery temperature driver to using
> the standard IIO ADC channel lookup and conversion routines.
> 
> Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
A comment inline on how I'm being useless and not getting round
to cleaning up the handling of consumers in IIO to avoid
the need for messing around in the deferred case...

Acked-by: Jonathan Cameron <ji...@kernel.org>
> ---
>  drivers/power/supply/ab8500_btemp.c | 41 
> ++---
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/ab8500_btemp.c 
> b/drivers/power/supply/ab8500_btemp.c
> index 6ffdc18f2599..61161db5cffa 100644
> --- a/drivers/power/supply/ab8500_btemp.c
> +++ b/drivers/power/supply/ab8500_btemp.c
> @@ -26,7 +26,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  
>  #define VTVOUT_V 1800
>  
> @@ -79,7 +79,8 @@ struct ab8500_btemp_ranges {
>   * @bat_temp:Dispatched battery temperature in degree Celcius
>   * @prev_bat_tempLast measured battery temperature in degree Celcius
>   * @parent:  Pointer to the struct ab8500
> - * @gpadc:   Pointer to the struct gpadc
> + * @adc_btemp_ball:  ADC channel for the battery ball temperature
> + * @adc_bat_ctrl:ADC channel for the battery control
>   * @fg:  Pointer to the struct fg
>   * @bm:  Platform specific battery management information
>   * @btemp_psy:   Structure for BTEMP specific battery properties
> @@ -96,7 +97,8 @@ struct ab8500_btemp {
>   int bat_temp;
>   int prev_bat_temp;
>   struct ab8500 *parent;
> - struct ab8500_gpadc *gpadc;
> + struct iio_channel *btemp_ball;
> + struct iio_channel *bat_ctrl;
>   struct ab8500_fg *fg;
>   struct abx500_bm_data *bm;
>   struct power_supply *btemp_psy;
> @@ -180,13 +182,13 @@ static int ab8500_btemp_batctrl_volt_to_res(struct 
> ab8500_btemp *di,
>   */
>  static int ab8500_btemp_read_batctrl_voltage(struct ab8500_btemp *di)
>  {
> - int vbtemp;
> + int vbtemp, ret;
>   static int prev;
>  
> - vbtemp = ab8500_gpadc_convert(di->gpadc, BAT_CTRL);
> - if (vbtemp < 0) {
> + ret = iio_read_channel_processed(di->bat_ctrl, );
> + if (ret < 0) {
>   dev_err(di->dev,
> - "%s gpadc conversion failed, using previous value",
> + "%s ADC conversion failed, using previous value",
>   __func__);
>   return prev;
>   }
> @@ -501,7 +503,7 @@ static int ab8500_btemp_res_to_temp(struct ab8500_btemp 
> *di,
>   */
>  static int ab8500_btemp_measure_temp(struct ab8500_btemp *di)
>  {
> - int temp;
> + int temp, ret;
>   static int prev;
>   int rbat, rntc, vntc;
>   u8 id;
> @@ -526,10 +528,10 @@ static int ab8500_btemp_measure_temp(struct 
> ab8500_btemp *di)
>   di->bm->bat_type[id].r_to_t_tbl,
>   di->bm->bat_type[id].n_temp_tbl_elements, rbat);
>   } else {
> - vntc = ab8500_gpadc_convert(di->gpadc, BTEMP_BALL);
> - if (vntc < 0) {
> + ret = iio_read_channel_processed(di->btemp_ball, );
> + if (ret < 0) {
>   dev_err(di->dev,
> - "%s gpadc conversion failed,"
> + "%s ADC conversion failed,"
>   " using previous value\n", __func__);
>   return prev;
>   }
> @@ -1085,7 +1087,22 @@ static int ab8500_btemp_probe(struct platform_device 
> *pdev)
>   /* get parent data */
>   di->dev = >dev;
>   di->parent = dev_get_drvdata(pdev->dev.parent);
> - di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
> +
> + /* Get ADC channels */
> + di->btemp_ball = devm_iio_channel_get(>dev, "btemp_ball");
> + if (IS_ERR(di->btemp_ball)) {
> + if (PTR_ERR(di->btemp_ball) == -ENODEV)
> +return -EPROBE_DEFER;
I (or someone else) still needs to look at reworking our in kernel stuff so
that this sort of trickery isn't needed.  Won't do any harm having this there
in the meantime.
> + dev_err(>dev, "failed to get BTEMP BALL ADC channel\n");
> + return PTR_ERR(di->btemp_ball);
> + }
> + di->bat_ctrl = devm_iio_channel_get(>dev, "bat_ctrl");
> + if (IS_ERR(di->bat_ctrl)) {
> + if (PTR_ERR(di->bat_ctrl) == -ENODEV)
> +return -EPROBE_DEFER;
> + dev_err(>dev, "failed to get BAT CTRL ADC channel\n");
> + return PTR_ERR(di->bat_ctrl);
> + }
>  
>   di->initialized = false;
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] mfd/iio: move the AB8500 GPADC driver to IIO

[Jonathan Cameron]

On 12/01/17 03:04, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Jan 11, 2017 at 12:47:38AM +0100, Linus Walleij wrote:
>> I don't know the best merge path for this but suspect that as
>> usual Lee can merge them all and provide an immutable branch to
>> the other subsystems, which he has lots of routine in handling.
>> The final DTS patch can probably be merged separately through
>> ARM SoC.
> 
> I'm fine with that solution.
> 
> -- Sebastian
> 
Agreed, that's the best way to do these.

Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 31/10/16 09:34, Quentin Schulz wrote:
> Hi Jonathan and Lee,
> 
> On 30/10/2016 17:59, Jonathan Cameron wrote:
>> On 15/09/16 13:44, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree. This registers
>>> the driver in the thermal framework.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences (registers bit and temperature computation)
>>> between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
>>> Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>
>>> Acked-by: Jonathan Cameron <ji...@kernel.org>
>> Hi Lee,
>>
>> As you applied the MFD part of this series, could you pick this up as well?
>>
> 
> Someone reported some bugs on the CHIP (sun5i-r8). I'm investigating that.
> 
> Also, it misses a scale for voltage (not that critical but still).
> 
> And also, Thomas Petazzoni just found a deadlock in sun4i_gpadc_read. If
> a regmap_write fails after getting the mutex, it is never released.
> 
> Is there some sort of deadline you want for a v7 since the mfd patch has
> been merged?
> 
No rush at all.  Doesn't matter as long as we know it's on it's way
sooner or later.  If it falls into the next cycle, that's fine -
I'll just pick it up directly as dependencies will in place by then.

Jonathan
> Thanks,
> Quentin
> 
>> Thanks,
>>
>> Jonathan
>>> ---
>>>
>>> v6:
>>>  - remove useless member (regs) from sun4i_gpadc_dev structure,
>>>  - rename sun4i_gpadc_dev structure to sun4i_gpadc_iio,
>>>  - remove regmap_update_bits used to disable hardware interrupts, it is 
>>> already
>>>  handled by devm functions,
>>>
>>> v5:
>>>  - correct mail address,
>>>  - correct several typos,
>>>  - move from const to static for sunxi_gpadc_chan_select functions,
>>>  - rename soc_specific struct to gpadc_data,
>>>  - rename soc_specific field to data in sun4i_gpadc_dev,
>>>  - return error code from regmap_write in case of failure in read_raws,
>>>  - share if condition in IIO_CHAN_INFO_RAW case,
>>>  - add comment on why we use parent device for registering in thermal,
>>>  - reordering remove function,
>>>
>>> v4:
>>>  - rename files and variables from sunxi* to sun4i*,
>>>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>>>  - factorize sysfs ADC and temp read_raws,
>>>  - use cached values when read_raw times out (except before a first value
>>>is gotten),
>>>  - remove mutex locks and unlocks from runtime_pm functions,
>>>  - factorize irq initializations,
>>>  - initialize temp_data and fifo_data values to -1 (error value),
>>>  - "impersonate" MFD to register in thermal framework,
>>>  - deactivate hardware interrupts one by one when probe fails or when
>>>removing driver instead of blindly deactivating all hardware interrupts,
>>>  - selects THERMAL_OF in Kconfig,
>>>
>>> v3:
>>>  - correct wrapping,
>>>  - add comment about thermal sensor inner working,
>>>  - move defines in mfd header,
>>>  - use structure to define SoC specific registers or behaviour,
>>>  - attach this structure to the device according to of_device_id of the
>>>platform device,
>>>  - use new mutex instead of iio_dev mutex,
>>>  - use atomic flags to avoid race between request_irq and disable_irq in
>>>probe,
>>>  - switch from processed value to raw, offset and scale values for
>>>temperature ADC channel,
>>>  - remove faulty sentinel in iio_chan_spec array,
>>>  - add pm_runtime support,
>>>  - register thermal sensor in thermal framework (forgotten since the
>>>beginning whereas it is present in current sun4i-ts driver),
>>>  - remove useless ret variables to store return value of regmap_reads,
>>>  - move comments on thermal sensor acquisition period in code instead of
>>>header,
>>>  - adding goto label to unregister iio_map_array when failing to register
>>>iio_dev,
>>>
>>> v2:
>>>  

Re: [PATCH v6 2/2] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 15/09/16 13:44, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
> Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>
> Acked-by: Jonathan Cameron <ji...@kernel.org>
Hi Lee,

As you applied the MFD part of this series, could you pick this up as well?

Thanks,

Jonathan
> ---
> 
> v6:
>  - remove useless member (regs) from sun4i_gpadc_dev structure,
>  - rename sun4i_gpadc_dev structure to sun4i_gpadc_iio,
>  - remove regmap_update_bits used to disable hardware interrupts, it is 
> already
>  handled by devm functions,
> 
> v5:
>  - correct mail address,
>  - correct several typos,
>  - move from const to static for sunxi_gpadc_chan_select functions,
>  - rename soc_specific struct to gpadc_data,
>  - rename soc_specific field to data in sun4i_gpadc_dev,
>  - return error code from regmap_write in case of failure in read_raws,
>  - share if condition in IIO_CHAN_INFO_RAW case,
>  - add comment on why we use parent device for registering in thermal,
>  - reordering remove function,
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 522 
> ++
>  3 files changed, 536 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + selec

Re: [PATCH v5 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 08/09/16 15:28, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
Acked-by: Jonathan Cameron <ji...@kernel.org>

(if Lee want's to take this through MFD - if I get an Ack from him
I'll pick it up through IIO).
> ---
> 
> v5:
>  - correct mail address,
>  - correct several typos,
>  - move from const to static for sunxi_gpadc_chan_select functions,
>  - rename soc_specific struct to gpadc_data,
>  - rename soc_specific field to data in sun4i_gpadc_dev,
>  - return error code from regmap_write in case of failure in read_raws,
>  - share if condition in IIO_CHAN_INFO_RAW case,
>  - add comment on why we use parent device for registering in thermal,
>  - reordering remove function,
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - shorten sunxi_gpadc_soc_specific structure to soc_specific,
>  - factorize sysfs ADC and temp read_raws,
>  - use cached values when read_raw times out (except before a first value
>is gotten),
>  - remove mutex locks and unlocks from runtime_pm functions,
>  - factorize irq initializations,
>  - initialize temp_data and fifo_data values to -1 (error value),
>  - "impersonate" MFD to register in thermal framework,
>  - deactivate hardware interrupts one by one when probe fails or when
>removing driver instead of blindly deactivating all hardware interrupts,
>  - selects THERMAL_OF in Kconfig,
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  13 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sun4i-gpadc-iio.c | 543 
> ++
>  3 files changed, 557 insertions(+)
>  create mode 100644 drivers/iio/adc/sun4i-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..ea36a4f 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,19 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + select THERMAL_OF
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will 

Re: [PATCH v5 1/3] hwmon: iio_hwmon: defer probe when no channel is found

[Jonathan Cameron]

On 09/09/16 05:26, Guenter Roeck wrote:
> On 09/08/2016 07:28 AM, Quentin Schulz wrote:
>> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
>> properties in the Device Tree or channels whose consumer_dev_name matches
>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>> which might be probed after iio_hwmon.
>>
>> It is better to defer the probe of iio_hwmon if such error is returned by
>> iio_channel_get_all in order to let a chance to iio drivers to expose
>> channels in iio_map_list.
>>
>> Signed-off-by: Quentin Schulz 
>> ---
>>
>> v5:
>>  - patch re-inserted,
>>
> Grumble ... applied to -next anyway.
> 
> I'll direct any complaints your way ;-)
> 
> Guenter
> 
Thanks.  I'll try and squeeze some time to look at a more general
solution (reworking how we set the maps up in the first place) if
no one else gets to it first!

It's on the list :)

Jonathan

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 05/09/16 07:29, Quentin Schulz wrote:
> On 04/09/2016 16:35, Jonathan Cameron wrote:
>> On 01/09/16 15:05, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree. This registers
>>> the driver in the thermal framework.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences (registers bit and temperature computation)
>>> between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
>> One utterly trivial point about unrolling code ordering inline.
>>
>> Other than the bit about patch 1 I'm basically happy with this..
> 
> ACK. Will revert this patch in v5. Thanks.
> 
>> However I would like some input (i.e. an Ack) from thermal given this
>> sets up a thermal zone.
>>
>> Zhang or Eduardo, could you take a quick look at this and confirm you
>> are happy with it?
>>
>> Thanks,
>>
>> Jonathan
> [...]
>>> +
>>> +err_map:
>>> +   iio_map_array_unregister(indio_dev);
>>> +
>>> +err_fifo_irq:
>>> +   /* Disable FIFO_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +err_temp_irq:
>>> +   /* Disable TEMP_DATA_PENDING interrupt on hardware side. */
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN,
>>> +  0);
>>> +
>>> +err:
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>>> +
>>> +   return ret;
>>> +}
>>> +
>>> +static int sun4i_gpadc_remove(struct platform_device *pdev)
>>> +{
>>> +   struct sun4i_gpadc_dev *info;
>>> +   struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +   info = iio_priv(indio_dev);
>>> +   iio_device_unregister(indio_dev);
>>> +   iio_map_array_unregister(indio_dev);
>>> +   pm_runtime_put(>dev);
>>> +   pm_runtime_disable(>dev);
>> Its really minor but in the interests of 'obviously correct' making
>> review easy I'd rather everything in the remove was in the reverse order
>> of probe (and hence the same as the error path in probe for most of it).
>>
>> That would put the pm_runtime stuff last I think..
>>
>> If you weren't rerolling anyway over patch 1 I'd probably have just let
>> this go, but might as well make this trivial change as well.
>>
> 
> I'm going with the following order:
> pm_runtime_put
> pm_runtime_disable
> regmap_update_bits
> iio_map_array_unregister
> iio_device_unregister
> 
> Is that okay? (I don't really know which one of iio_map_array_unregister
> or iio_device_unregister to put first, if it matters in any way).
Unless we have a really complex race condition involving a client driver
coming up just as the provider is unregistered I doubt it matters.

At some point we should probably chase down any paths through that
with some carefully crafted tests but it's in the seriously unlikely
category!

Jonathan
> 
> Thanks!
> Quentin
>>
>>> +   /*
>>> +* Disable TEMP_DATA_PENDING and FIFO_DATA_PENDING interrupts on
>>> +* hardware side.
>>> +*/
>>> +   regmap_update_bits(info->regmap, SUN4I_GPADC_INT_FIFOC,
>>> +  SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN |
>>> +   SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN,
>>> +  0);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id sun4i_gpadc_id[] = {
>>> +   { "sun4i-a10-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun5i-a13-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { "sun6i-a31-gpadc-iio", (kernel_ulong_t)_gpadc_soc_specific },
>>> +   { /* sentinel */ },
>>> +};
>>> +
>>> +static struct platform_driver sun4i_gpadc_driver = {
>>> +   .driver = {
>>> +   .name = "sun4i-gpadc-iio",
>>> +   .pm = _gpadc_pm_ops,
>>> +   },
>>> +   .id_table = sun4i_gpadc_id,
>>> +   .probe = sun4i_gpadc_probe,
>>> +   .remove = sun4i_gpadc_remove,
>>> +};
>>> +
>>> +module_platform_driver(sun4i_gpadc_driver);
>>> +
>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>> +MODULE_AUTHOR("Quentin Schulz <quentin.sch...@free-electrons.com>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/3] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 01/09/16 15:05, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
> 
> Signed-off-by: Quentin Schulz 
Other than the patch 1 revertion to the earlier approach, I'm happy with this.
I will need an Ack from Lee though given it's an mfd driver.

Makes sense to take this on through IIO I think.

Thanks,

Jonathan
> ---
> 
> v4:
>  - rename files and variables from sunxi* to sun4i*,
>  - rename defines from SUNXI_* to SUN4I_* or SUN6I_*,
>  - remove TP in defines name,
>  - rename SUNXI_IRQ_* to SUN4I_GPADC_IRQ_* for consistency,
>  - use devm functions for regmap_add_irq_chip and mfd_add_devices,
>  - remove remove functions (now empty thanks to devm functions),
> 
> v3:
>  - use defines in regmap_irq instead of hard coded BITs,
>  - use of_device_id data field to chose which MFD cells to add considering
>the compatible responsible of the MFD probe,
>  - remove useless initializations,
>  - disable all interrupts before adding them to regmap_irqchip,
>  - add goto error label in probe,
>  - correct wrapping in header license,
>  - move defines from IIO driver to header,
>  - use GENMASK to limit the size of the variable passed to a macro,
>  - prefix register BIT defines with the name of the register,
>  - reorder defines,
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig |  15 
>  drivers/mfd/Makefile|   2 +
>  drivers/mfd/sun4i-gpadc-mfd.c   | 174 
> 
>  include/linux/mfd/sun4i-gpadc-mfd.h |  94 +++
>  4 files changed, 285 insertions(+)
>  create mode 100644 drivers/mfd/sun4i-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sun4i-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..95b3c3e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -29,6 +29,21 @@ config MFD_ACT8945A
> linear regulators, along with a complete ActivePath battery
> charger.
>  
> +config MFD_SUN4I_GPADC
> + tristate "Allwinner sunxi platforms' GPADC MFD driver"
> + select MFD_CORE
> + select REGMAP_MMIO
> + depends on ARCH_SUNXI || COMPILE_TEST
> + help
> +   Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> +   This driver will only map the hardware interrupt and registers, you
> +   have to select individual drivers based on this MFD to be able to use
> +   the ADC or the thermal sensor. This will try to probe the ADC driver
> +   sun4i-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sun4i-gpadc-mfd.
> +
>  config MFD_AS3711
>   bool "AMS AS3711"
>   select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..3b964d7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -205,3 +205,5 @@ intel-soc-pmic-objs   := 
> intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)   += intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
>  obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
> +
> +obj-$(CONFIG_MFD_SUN4I_GPADC)+= sun4i-gpadc-mfd.o
> diff --git a/drivers/mfd/sun4i-gpadc-mfd.c b/drivers/mfd/sun4i-gpadc-mfd.c
> new file mode 100644
> index 000..75aa7a8
> --- /dev/null
> +++ b/drivers/mfd/sun4i-gpadc-mfd.c
> @@ -0,0 +1,174 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +static struct resource adc_resources[] = {
> + {
> + .name   = "FIFO_DATA_PENDING",
> + .start  = SUN4I_GPADC_IRQ_FIFO_DATA,
> + .end= SUN4I_GPADC_IRQ_FIFO_DATA,
> + .flags  = IORESOURCE_IRQ,
> + }, {
> + .name   = "TEMP_DATA_PENDING",
> + .start  = SUN4I_GPADC_IRQ_TEMP_DATA,
> + .end= SUN4I_GPADC_IRQ_TEMP_DATA,
> + .flags  = IORESOURCE_IRQ,
> + },
> +};
> +
> +static const struct regmap_irq sun4i_gpadc_mfd_regmap_irq[] = {
> + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_FIFO_DATA, 0,
> +SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN),
> + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_TEMP_DATA, 0,
> +SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN),
> +};
> +
> +static 

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 26/07/16 08:43, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree. This registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
> ---
> 
> I don't like how I get the temperature for the thermal framework
> (sunxi_gpadc_get_temp). I am doing the exact same process as the iio_hwmon
> but in this function. This is duplicated code. I could use
> iio_read_channel_processed but it needs to have the iio_channel structure
> of the thermal sensor which I can only get with iio_channel_get which
> matches the device name with the consumer_dev_name in the iio_map array.
> And frankly, I do not see myself declaring the driver both as the producer
> and the consumer of IIO channels.
I also can't see a clean way of reusing the value calculation code.  You
could construct and iio_channel by hand easily enough, but that would be
a pretty ugly bit of 'boundary crossing'.
> 
> Moreover, since the thermal sensor is configured to throw an interrupt
> every X seconds, only the first request within these X seconds will not
> time out. This means that because the thermal framework regularly poll the
> thermal sensor, it is really difficult to get a value from sysfs without
> getting first a tonne of times out. I don't know if we can do something
> about that?
Cache the value what ever read it? (thermal or sysfs) then return that
until a new one shows up...

Otherwise, I don't have anything to add to the other review comments.

Jonathan
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>probe,
>  - switch from processed value to raw, offset and scale values for
>temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>header,
>  - adding goto label to unregister iio_map_array when failing to register
>iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig   |  12 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 513 
> ++
>  3 files changed, 526 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..429ef16 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,18 @@ config ROCKCHIP_SARADC
> To compile this driver as a module, choose M here: the
> module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> + tristate "Support for the Allwinner SoCs GPADC"
> + depends on IIO
> + depends on MFD_SUN4I_GPADC
> + help
> +   Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +   GPADC. This ADC provides 4 channels which can be used as an ADC or as
> +   a touchscreen input and one channel for thermal sensor.
> +
> +   To compile this driver as a module, choose M here: the module will be
> +   called sunxi-gpadc-iio.
> +
>  config TI_ADC081C
>   tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>   depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..14d1739 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ 

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 15 August 2016 18:07:30 BST, Guenter Roeck <li...@roeck-us.net> wrote:
>On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>> On 26/07/16 17:04, Guenter Roeck wrote:
>> > On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> >> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>> >>> On 26/07/2016 11:05, Alexander Stein wrote:
>> >>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>> >>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>> >>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>> >>>>>>> iio_channel_get_all returns -ENODEV when it cannot find
>either phandles
>> >>>>>>> and
>> >>>>>>> properties in the Device Tree or channels whose
>consumer_dev_name
>> >>>>>>> matches
>> >>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by
>iio drivers
>> >>>>>>> which might be probed after iio_hwmon.
>> >>>>>>
>> >>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>for
>> >>>>>> returning
>> >>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>> >>>>>> driver/device
>> >>>>>> dependencies seems not right for me at this place.
>> >>>>>
>> >>>>> Then what if the iio_channel_get_all is called outside of the
>probe of a
>> >>>>> driver? We'll have to change the error code, things we are
>apparently
>> >>>>> trying to avoid (see v2 patches' discussions).
>> >>>>
>> >>>> Maybe I didn't express my idea enough. I don't want to change
>the behavior
>> >>>> of iio_channel_get_all at all. Just the result evaluation of
>> >>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>the patch
>> >>>> below in mind.
>> >>>>
>> >>>> Best regards,
>> >>>> Alexander
>> >>>> ---
>> >>>> diff --git a/drivers/hwmon/iio_hwmon.c
>b/drivers/hwmon/iio_hwmon.c
>> >>>> index b550ba5..e32d150 100644
>> >>>> --- a/drivers/hwmon/iio_hwmon.c
>> >>>> +++ b/drivers/hwmon/iio_hwmon.c
>> >>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>platform_device
>> >>>> *pdev)
>> >>>>
>> >>>> name = dev->of_node->name;
>> >>>> 
>> >>>> channels = iio_channel_get_all(dev);
>> >>>>
>> >>>> -   if (IS_ERR(channels))
>> >>>> -   return PTR_ERR(channels);
>> >>>> +   if (IS_ERR(channels)) {
>> >>>> +   if (PTR_ERR(channels) == -ENODEV)
>> >>>> +   return -EPROBE_DEFER;
>> >>>> +   else
>> >>>> +   return PTR_ERR(channels);
>> >>>> +   }
>> >>>>
>> >>>> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>> >>>> if (st == NULL) {
>> >>>
>> >>> Indeed, I misunderstood what you told me.
>> >>>
>> >>> Actually, the patch you proposed is part of my v1
>> >>> (https://lkml.org/lkml/2016/6/28/203) and v2
>> >>> (https://lkml.org/lkml/2016/7/15/215).
>> >>> Jonathan and Guenter didn't really like the idea of changing the
>-ENODEV
>> >>> in -EPROBE_DEFER.
>> >>
>> >> Thanks for the links.
>> >>
>> >>> What I thought you were proposing was to change the -ENODEV
>return code
>> >>> inside iio_channel_get_all. This cannot be an option since the
>function
>> >>> might be called outside of a probe (it is not yet, but might be
>in the
>> >>> future?).
>> >>
>> >> AFAICS this is a helper function not knowing about device probing
>itself. And 
>> >> it should stay at that.
>> >>
>> >>> Of what I understood, two possibilities are then possible
>(proposed
>> >>> either by Guenter or Jonathan): either rework the iio framework
>to
>> >>> register iio map array earlier or to use late_initcall instead of
>init
>> &g

Re: [PATCH V4 1/1] iio: as6200: add AS6200 temperature sensor driver from ams AG

[Jonathan Cameron]

On 04/08/16 09:35, Florian Lobmaier wrote:
> Hello Peter,
> 
> Thanks again for your valuable feedback. We use now IIO_EV_THRESH to
> set high and low limits for temperature. Also removed all the custom
> ABI as this are mainly settings which will be set one-time only. For
> the removed custom ABI init defines where introduced which will be
> written to the registers in the probe function. The remaining custom
> ABI is now documented as well as the device tree bindings.> 
> Br,
> Florian
> 
> Signed-off-by: Florian Lobmaier 
> Signed-off-by: Elitsa Polizoeva 
Please post as a fresh email thread with a clean title. Otherwise
people will assume it is simply a reply to a comment on an earlier
version.  Also don't include earlier versions as you have here!

i.e. drop the RE from the title as it's confusing!

Anyhow, right back at v1 Peter mentioned that this might be more
suitable as a hwmon driver than an IIO one.  If you have a good reason
for supporting this part via IIO you should put it in the patch
description. I'm afraid I've been more or less offline for the last
couple fo weeks or I'd have highlighted that this question was
important. A superficial look suggest to me that this is definitely
a part targeting hardware monitoring applications.

I'll only take a border line part with agreement form Guenter / Jean
who are the hwmon maintainers.

I'll do a quick review ignoring this question.
Mostly pretty good though I really don't like the remaining
custom ABI. Can't see a reason for its existence.

Jonathan
> ---
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-as6200.txt 
> b/Documentation/ABI/testing/sysfs-bus-iio-temperature-as6200.txt
> new file mode 100644
> index 000..0dfad7e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-as6200.txt
> @@ -0,0 +1,8 @@
> +What /sys/bus/iio/devices/iio:deviceX/al
'alert' at the very least.
> +Date:August 2016
> +KernelVersion:   4.3
> +Contact: Florian Lobmaier 
> +Description:
> + Get the current status of the alert bit in the configuration
> + register of the AS6200.
> + on/off
Why?  Just use the events accesses to wait for it.

> diff --git a/Documentation/devicetree/bindings/iio/temperature/as6200.txt 
> b/Documentation/devicetree/bindings/iio/temperature/as6200.txt
> new file mode 100644
> index 000..bfd2b5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/as6200.txt
> @@ -0,0 +1,20 @@
> +ams AG AS6200 temperature sensor driver
> +
> +Required properties:
> + - compatible: must be "ams,as6200"
> + - reg: I2C 7-bit address for the device
> + possible values: 0x48, 0x49, 0x4a, 0x4b
> + - interrupt-parent: should be the phandle for the interrupt controller
> + - interrupts: the sole interrupt generated by the device
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +Example:
> +
> +as6200@0x4b {
> + compatible = "ams,as6200";
> + reg = <0x4b>;
> + interrupt-parent = <>;
> + interrupts = <4 1>;
> +};
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 21feaa4..d82e80f 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,17 @@
>  #
>  menu "Temperature sensors"
>  
> +config AS6200
> + tristate "ams AG AS6200 temperature sensor"
> + depends on I2C
> + select REGMAP_I2C
> + help
> +   If you say yes here you get support for the AS6200 temperature
> +   sensor from ams AG.
> +
> +   This driver can also be built as a module. If so, the module will
> +   be called as6200.
> +
>  config MLX90614
>   tristate "MLX90614 contact-less infrared sensor"
>   depends on I2C
> diff --git a/drivers/iio/temperature/Makefile 
> b/drivers/iio/temperature/Makefile
> index 40710a8..c0c9a9a 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,5 +2,6 @@
>  # Makefile for industrial I/O temperature drivers
>  #
>  
> +obj-$(CONFIG_AS6200) += as6200.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
>  obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/as6200.c 
> b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 000..9bcab1d
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,492 @@
> +/*
> + * Driver for ams AS6200 temperature sensor.
> + *
> + * Sensor supports following 7-bit I2C addresses: 0x48, 0x49, 0x4A, 0x4B
> + *
> + * Copyright (c) 2016 ams AG. All rights reserved.
> + *
> + * Author: Florian Lobmaier 
> + * Author: Elitsa Polizoeva 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published 

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 26/07/16 17:04, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>> On 26/07/2016 11:05, Alexander Stein wrote:
 On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> On 26/07/2016 10:21, Alexander Stein wrote:
>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>> and
>>> properties in the Device Tree or channels whose consumer_dev_name
>>> matches
>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>> which might be probed after iio_hwmon.
>>
>> Would it work if iio_channel_get_all returning ENODEV is used for
>> returning
>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>> driver/device
>> dependencies seems not right for me at this place.
>
> Then what if the iio_channel_get_all is called outside of the probe of a
> driver? We'll have to change the error code, things we are apparently
> trying to avoid (see v2 patches' discussions).

 Maybe I didn't express my idea enough. I don't want to change the behavior
 of iio_channel_get_all at all. Just the result evaluation of
 iio_channel_get_all in iio_hwmon_probe. I have something link the patch
 below in mind.

 Best regards,
 Alexander
 ---
 diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
 index b550ba5..e32d150 100644
 --- a/drivers/hwmon/iio_hwmon.c
 +++ b/drivers/hwmon/iio_hwmon.c
 @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
 *pdev)

 name = dev->of_node->name;
 
 channels = iio_channel_get_all(dev);

 -   if (IS_ERR(channels))
 -   return PTR_ERR(channels);
 +   if (IS_ERR(channels)) {
 +   if (PTR_ERR(channels) == -ENODEV)
 +   return -EPROBE_DEFER;
 +   else
 +   return PTR_ERR(channels);
 +   }

 st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
 if (st == NULL) {
>>>
>>> Indeed, I misunderstood what you told me.
>>>
>>> Actually, the patch you proposed is part of my v1
>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>> (https://lkml.org/lkml/2016/7/15/215).
>>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>>> in -EPROBE_DEFER.
>>
>> Thanks for the links.
>>
>>> What I thought you were proposing was to change the -ENODEV return code
>>> inside iio_channel_get_all. This cannot be an option since the function
>>> might be called outside of a probe (it is not yet, but might be in the
>>> future?).
>>
>> AFAICS this is a helper function not knowing about device probing itself. 
>> And 
>> it should stay at that.
>>
>>> Of what I understood, two possibilities are then possible (proposed
>>> either by Guenter or Jonathan): either rework the iio framework to
>>> register iio map array earlier or to use late_initcall instead of init
>>> for the driver consuming the iio channels.
>>
>> Interestingly using this problem would not arise due to module dependencies. 
>> But using late_initcall would mean this needs to be done on any driver using 
>> iio channels? I would rather keep those consumers simple.
>>
> Me too, but that would imply a solution in iio. The change you propose above
> isn't exactly simple either, and would also be needed in each consumer driver.
> 
> Just for the record, I dislike the late_initcall solution as well, but I 
> prefer
> it over blindly converting ENODEV to EPROBE_DEFER.
I'm falling on the other side on this one right now. Though I'd be tempted
to renaming the function to something like iio_channel_get_all_or_defer
to make it explicit that it can result in deferred probing.

To my mind late_initcall tricks are rather hideous as well.  At least 
deferred probing should always work (with a fair cost associated with it of
course)

Lots of discussions ongoing on how to do dependency resolution that will
hopefully mean deferred probing only occurs in the pathological cases in
the future anyway. Interesting to see where that goes.

Jonathan
> 
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 26/07/16 10:33, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>> On 26/07/2016 10:21, Alexander Stein wrote:
 On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.

 Would it work if iio_channel_get_all returning ENODEV is used for
 returning
 EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
 driver/device
 dependencies seems not right for me at this place.
>>>
>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>> driver? We'll have to change the error code, things we are apparently
>>> trying to avoid (see v2 patches' discussions).
>>
>> Maybe I didn't express my idea enough. I don't want to change the behavior 
>> of  
>> iio_channel_get_all at all. Just the result evaluation of 
>> iio_channel_get_all 
>> in iio_hwmon_probe. I have something link the patch below in mind.
>>
>> Best regards,
>> Alexander
>> ---
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index b550ba5..e32d150 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>> name = dev->of_node->name;
>>  
>> channels = iio_channel_get_all(dev);
>> -   if (IS_ERR(channels))
>> -   return PTR_ERR(channels);
>> +   if (IS_ERR(channels)) {
>> +   if (PTR_ERR(channels) == -ENODEV)
>> +   return -EPROBE_DEFER;
>> +   else
>> +   return PTR_ERR(channels);
>> +   }
>>  
>> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>> if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.
> 
> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).
If that did happen I'd be tempted to introduce a new function to be
used outside of probe.  I definitely don't like rewriting in individual
drivers.

The defer question is still rather open in my mind. Will address it in the
other thread.

> 
> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.
Ultimately we probably need to rethink how the registration works
(it was written before deferred probing came along and that has rather
changed things for us!)

For now I'm not convinced that deferring is that painful even if we
do have to try reprobing every time a new module gets probed..

Jonathan
> 
> Thanks,
> Quentin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 19/07/16 09:33, Quentin Schulz wrote:
> On 18/07/2016 15:18, Jonathan Cameron wrote:
>> On 15/07/16 10:59, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
>>
>> Hi Quentin,
>>
>> Various bits inline.  In particular the irq handling looks flakey / racey
>> to me.  Definitely need some explanatory comments.
>>
>> Also another note on the craziness that using extended_name to provide
>> the hwmon labels will cause :)
>>
>> Jonathan
> [...]
>>> +struct sunxi_gpadc_dev {
>>> +   void __iomem*regs;
>>> +   struct completion   completion;
>>> +   int temp_data;
>>> +   u32 adc_data;
>>> +   struct regmap   *regmap;
>>> +   unsigned intfifo_data_irq;
>>> +   unsigned inttemp_data_irq;
>>> +   unsigned intflags;
>> I'd prefer something more explicit than this.  Right now only one
>> bit can ever be set - indicating a particular chip family.  Why not
>> just have an enum instead?
> 
> ACK.
> 
> [...]
>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>>> +   SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
>>> +   SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
>>> +   SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
>>> +   SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
>>> +   {
>>> +   .type = IIO_TEMP,
>>> +   .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> +   .datasheet_name = "temp_adc",
>>> +   .extend_name = "SoC temperature",
>> Just curious, did you look at the resultling sysfs entries?
>> Going to be something like
>> in_temp_SoC\ Temperature_input... Not good.
> 
> Just encountered this after further testing, not good as you said.
> 
>> If there is a strong enough reason (and there may be) to add a 'help string'
>> type label to struct iio_chan_spec then my only real worries would be that
>> we would be adding a whole pile of ABI that would be in the, near impossible
>> to change in future, category...
> 
> I don't understand your "adding a whole pile of ABI" thing. 
Adding a 'free form' description field to the sysfs ABI of IIO is
nasty in that such things tend not to be as reviewed as they should
be and the moment an unclear / wrong statement is there, it's in
the kernel ABI and chances are someone will go and write a script
using it.  Userspace breakage ensues so we get stuck with garbage. 
>Same as
> datasheet_name variable: const char* in struct iio_chan_spec and used
> whenever needed with some NULL checks. This is the easiest way to do it.
> Or maybe you're thinking of adding an item to iio_chan_info_enum and all
> its underlying modifications? This could allow us to expose a _label
> sysfs file in the iio_device per type/channel.
As you suggest - new field in iio_chan_spec.
> 
> I understand the "near impossible to change in future" concern though.
> 
> [...]
>>> +   enable_irq(info->temp_data_irq);
>> Is this hardware spitting out extra irqs?  If not, much better to just
>> leave it enabled all the time and control whether it can occur or not
>> by controlling the device state..
> 
> The temp_data_irq occurs every SUNXI_GPADC_TEMP_PERIOD(x) periods (in
> the current state of the driver: 2s). What do you mean by controlling
> the device state? Enabling or disabling the hardware part of the IP
> responsible of getting the temperature
> (SUNXI_GPADC_TP_TPR_TEMP_ENABLE(x) here)?
Yes, or something along those lines if it wakes up fast enough.
> Once the interrupt is activated, the IP periodically performs
> conversions. We don't really want interrupts to be activated when not
> needed.
> 
> [...]
>>> +
>>> +   if (info->flags & SUNXI_GPADC_ARCH_SUN4I)
>>> +   *val = info->temp_data * 133 - 257000;
>> Why report as processed?  I'd just report th

Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

[Jonathan Cameron]

On 15/07/16 10:59, Quentin Schulz wrote:
> Currently, iio_hwmon only exposes values of the IIO channels it can read
> but no label by channel is exposed.
> 
> This adds exposition of sysfs files containing label for IIO channels it
> can read based on extended_name field of the iio_chan_spec of the channel.
> If the extended_name field is empty, the sysfs file is not created by
> iio_hwmon.
Hmm. This is not the intent of extended name at all.  That exists to add
a small amount of information to an constructed IIO channel name.
Typically it's used to indicate physically wired stuff like:

in_voltage0_vdd_raw for cases where that channel of the ADC is hard wired
to the vdd.  In this particular case the use might actually work as the
vdd makes it clear it's a voltage - in general that's not the case.

Use of extended_name at all in IIO is only done after extensive review.
It adds nasty custom ABI to a device, so the gain has to be considerable
to use it.

When I read your original suggestion of adding labels, I was expecting the
use of datasheet_name.  That has the advantage of being well defined by
the datasheet (if not it should not be provided) + not being used in
the construction of the IIO channel related attributes.  However, that
may still not correspond well to the expected labelling in hwmon.
Thinking more on this, the label is going to often be a function of how
the board is wired up...  Perhaps it should be a characteristic of the
channel_map (hence from DT or similar) rather than part of the IIO driver
itself?

At first glance hwmon labels appear to be pretty freeform...  However
we need to be very careful here as this is effectively defining a large
chunk of new ABI.

This isn't a thing that I have a particularly clear view on (as you
might be able to tell ;).  Other opinions sought!

Jonathan
> 
> Signed-off-by: Quentin Schulz 
> ---
> 
> patch added in v2
> 
>  drivers/hwmon/iio_hwmon.c | 77 
> +--
>  1 file changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index c0da4d9..28d15b2 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  /**
> @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>   return sprintf(buf, "%d\n", result);
>  }
>  
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> + struct iio_hwmon_state *state = dev_get_drvdata(dev);
> + const char *label = state->channels[sattr->index].channel->extend_name;
> +
> + if (label)
> + return sprintf(buf, "%s\n", label);
> +
> + return 0;
> +}
> +
>  static int iio_hwmon_probe(struct platform_device *pdev)
>  {
>   struct device *dev = >dev;
>   struct iio_hwmon_state *st;
> - struct sensor_device_attribute *a;
> - int ret, i;
> + struct sensor_device_attribute *a, *b;
> + int ret, i, j = 0;
>   int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
>   enum iio_chan_type type;
>   struct iio_channel *channels;
> @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   st->num_channels++;
>  
>   st->attrs = devm_kzalloc(dev,
> -  sizeof(*st->attrs) * (st->num_channels + 1),
> +  sizeof(*st->attrs) *
> +  (2 * st->num_channels + 1),
>GFP_KERNEL);
>   if (st->attrs == NULL) {
>   ret = -ENOMEM;
> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   }
>  
>   sysfs_attr_init(>dev_attr.attr);
> +
> + b = NULL;
> + if (st->channels[i].channel->extend_name) {
> + b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
> + if (b == NULL) {
> + ret = -ENOMEM;
> + goto error_release_channels;
> + }
> +
> + sysfs_attr_init(>dev_attr.attr);
> + }
> +
>   ret = iio_get_channel_type(>channels[i], );
>   if (ret < 0)
>   goto error_release_channels;
> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   case IIO_VOLTAGE:
>   a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> "in%d_input",
> -   in_i++);
> +   in_i);
> + if (b)
> +  

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 03/07/16 12:54, Jonathan Cameron wrote:
> On 28/06/16 09:18, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
> Hi Quentin.
> 
> I'm a bit in two minds about some of this.  That temperature sensor is
> so obviously meant for hwmon purposes, I'm tempted to suggest it might
> actually make sense to put it directly in hwmon rather than using the
> bridge.  That obviously makes it less flexible in some ways (i.e. for
> use within the thermal subsystem at some point).
> 
> Guenter, what do you think?
> 
> I'm guessing detailed docs for this part aren't avaiable publicly? :(
> 
> So the rest of my comments are kind of predicated on me having roughtly
> understood how this device works from the structure of the driver.
> 
> The temperature sensor is really effectively as separate ADC?
> The main interest in this is for key detection?
Ah, I'm talking garbage, wrong ADC for the chip...  I'd crossed the two
threads.  Ignore that stuff.
> 
> Anyhow, if the data flow for the temperatures sensor is not synced with
> the other ADC channels, adding buffered (pushed) output from the driver in
> future will be fiddly and with a 250Hz device you'll probably want it.
> Basically IIO buffered supports assumes each iio device will sample data
> at a particular frequency. If channels are not synchronized in that fashion
> then you have to register multiple devices or only pick a subset of channels
> to export.
> 
> For the key detection you have already observed that IIO needs some
> additions to be able to have consumers of what we term 'events' e.g. threshold
> interrupts.
> 
> Looking at the lradc-keys driver in tree, it looks like we only really have
> really simple threshold interrups - configured to detect a very low voltage?
> + only one per channel.
> 
> So not too nasty a case, but you are right some work is needed in IIO as
> we simply don't have a means of passing these on as yet or configuring them
> from in kernel consumers.
> If we take the easy route and don't demux incoming events then it shouldn't
> be too hard to add (demux can follow later).  Hence any client device can try
> to enable events it wants, but may get events that other client devices wanted
> as well.
> 
> Config interface should be much the same as the write support for channels.
> Data flow marginally harder, but pretty much a list of callbacks within
> iio_push_event.
> 
> Not trivial, but not too tricky either.
> 
> The events subsystem has a few 'limitations' we need to address long term
> but as this is in kernel interface only, we can do this now and fix stuff
> up in future without any ABI breakage. (limitations are things like only
> one event of a given type and direction per channel - main challenge on
> that is finding a way of doing it without abi breakage).
> 
> Anyhow, sounds fun - wish I had the time to do it myself!
> 
> Otherwise, your remove is never going to work as indio_dev is always NULL.
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/Kconfig   |  12 ++
>>  drivers/iio/adc/Makefile  |   1 +
>>  drivers/iio/adc/sunxi-gpadc-iio.c | 371 
>> ++
>>  3 files changed, 384 insertions(+)
>>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 82c718c..b7b566a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -328,6 +328,18 @@ config NAU7802
>>To compile this driver as a module, choose M here: the
>>module will be called nau7802.
>>  
>> +config SUNXI_ADC
>> +tristate "ADC driver for sunxi platforms"
>> +depends on IIO
>> +depends on MFD_SUNXI_ADC
>> +help
>> +  Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
>> +  SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
>> +  as a touchscreen input and one channel for thermal sensor.
>> +
>> +  To compile this driver as a module, choose M here: the
>> +  module will be called sunxi-gpadc-iio.
>> +
>>  c

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 28/06/16 09:18, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz 
Hi Quentin.

I'm a bit in two minds about some of this.  That temperature sensor is
so obviously meant for hwmon purposes, I'm tempted to suggest it might
actually make sense to put it directly in hwmon rather than using the
bridge.  That obviously makes it less flexible in some ways (i.e. for
use within the thermal subsystem at some point).

Guenter, what do you think?

I'm guessing detailed docs for this part aren't avaiable publicly? :(

So the rest of my comments are kind of predicated on me having roughtly
understood how this device works from the structure of the driver.

The temperature sensor is really effectively as separate ADC?
The main interest in this is for key detection?

Anyhow, if the data flow for the temperatures sensor is not synced with
the other ADC channels, adding buffered (pushed) output from the driver in
future will be fiddly and with a 250Hz device you'll probably want it.
Basically IIO buffered supports assumes each iio device will sample data
at a particular frequency. If channels are not synchronized in that fashion
then you have to register multiple devices or only pick a subset of channels
to export.

For the key detection you have already observed that IIO needs some
additions to be able to have consumers of what we term 'events' e.g. threshold
interrupts.

Looking at the lradc-keys driver in tree, it looks like we only really have
really simple threshold interrups - configured to detect a very low voltage?
+ only one per channel.

So not too nasty a case, but you are right some work is needed in IIO as
we simply don't have a means of passing these on as yet or configuring them
from in kernel consumers.
If we take the easy route and don't demux incoming events then it shouldn't
be too hard to add (demux can follow later).  Hence any client device can try
to enable events it wants, but may get events that other client devices wanted
as well.

Config interface should be much the same as the write support for channels.
Data flow marginally harder, but pretty much a list of callbacks within
iio_push_event.

Not trivial, but not too tricky either.

The events subsystem has a few 'limitations' we need to address long term
but as this is in kernel interface only, we can do this now and fix stuff
up in future without any ABI breakage. (limitations are things like only
one event of a given type and direction per channel - main challenge on
that is finding a way of doing it without abi breakage).

Anyhow, sounds fun - wish I had the time to do it myself!

Otherwise, your remove is never going to work as indio_dev is always NULL.

Jonathan

> ---
>  drivers/iio/adc/Kconfig   |  12 ++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 371 
> ++
>  3 files changed, 384 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 82c718c..b7b566a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -328,6 +328,18 @@ config NAU7802
> To compile this driver as a module, choose M here: the
> module will be called nau7802.
>  
> +config SUNXI_ADC
> + tristate "ADC driver for sunxi platforms"
> + depends on IIO
> + depends on MFD_SUNXI_ADC
> + help
> +   Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
> +   SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
> +   as a touchscreen input and one channel for thermal sensor.
> +
> +  To compile this driver as a module, choose M here: the
> +  module will be called sunxi-gpadc-iio.
> +
>  config PALMAS_GPADC
>   tristate "TI Palmas General Purpose ADC"
>   depends on MFD_PALMAS
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 0cb7921..2996a5b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>  obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>  obj-$(CONFIG_NAU7802) += nau7802.o
> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c 
> b/drivers/iio/adc/sunxi-gpadc-iio.c
> 

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 28/06/16 09:18, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
> 
> Signed-off-by: Quentin Schulz 
The code looks fine to me. The 'controversial' bit of this is listing
iio-hwmon as an mfd child to get it to probe as a result of this being
present.  My immediately thought is that it should be separately
described in the devicetree and hence instantiated outside of this driver.

Perhaps you could describe your reasoning for doing it this way?

Thanks,

Jonathan
> ---
>  drivers/mfd/Kconfig |  14 +++
>  drivers/mfd/Makefile|   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c   | 188 
> 
>  include/linux/mfd/sunxi-gpadc-mfd.h |  14 +++
>  4 files changed, 218 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index eea61e3..1663db9 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
> by the probe function of this MFD driver according to a device tree
> property.
>  
> +config MFD_SUNXI_ADC
> + tristate "ADC MFD core driver for sunxi platforms"
> + select MFD_CORE
> + select REGMAP_MMIO
> + help
> +   Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> +   This driver will only map the hardware interrupt and registers, you
> +   have to select individual drivers based on this MFD to be able to use
> +   the ADC or the thermal sensor. This will try to probe the ADC driver
> +   sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>   tristate "Atmel HLCDC (High-end LCD Controller)"
>   select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5eaa6465d..b280d0a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,8 @@ obj-$(CONFIG_MFD_DLN2)+= dln2.o
>  obj-$(CONFIG_MFD_RT5033) += rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)  += sunxi-gpadc-mfd.o
> +
>  intel-soc-pmic-objs  := intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)   += intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 000..710e297
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,188 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SUNXI_IRQ_FIFO_DATA  0
> +#define SUNXI_IRQ_TEMP_DATA  1
> +
> +static struct resource adc_resources[] = {
> + {
> + .name   = "FIFO_DATA_PENDING",
> + .start  = SUNXI_IRQ_FIFO_DATA,
> + .end= SUNXI_IRQ_FIFO_DATA,
> + .flags  = IORESOURCE_IRQ,
> + }, {
> + .name   = "TEMP_DATA_PENDING",
> + .start  = SUNXI_IRQ_TEMP_DATA,
> + .end= SUNXI_IRQ_TEMP_DATA,
> + .flags  = IORESOURCE_IRQ,
> + },
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> + REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> + REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> + .name = "sunxi_gpadc_mfd_irq_chip",
> + .status_base = TP_INT_FIFOS,
> + .ack_base = TP_INT_FIFOS,
> + .mask_base = TP_INT_FIFOC,
> + .init_ack_masked = true,
> + .mask_invert = true,
> + .irqs = sunxi_gpadc_mfd_regmap_irq,
> + .num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> + .num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> + {
> + .name   = "sun4i-a10-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + .name = "iio_hwmon",
> + }
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> + {
> + .name   = "sun5i-a13-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + .name = "iio_hwmon",
> + },
> +};
> +
> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> + {
> + .name   = "sun6i-a31-gpadc-iio",
> + .resources = adc_resources,
> + .num_resources = ARRAY_SIZE(adc_resources),
> + }, {
> + 

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Jonathan Cameron]

On 30/06/16 15:51, Guenter Roeck wrote:
> On 06/30/2016 06:59 AM, Jonathan Cameron wrote:
>>
>>
>> On 30 June 2016 04:47:25 BST, Guenter Roeck <li...@roeck-us.net> wrote:
>>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>>>> iio_channel_get_all returns -ENODEV when it cannot find either
>>> phandles and
>>>> properties in the Device Tree or channels whose consumer_dev_name
>>> matches
>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>>> drivers
>>>> which might be probed after iio_hwmon.
>>>>
>>>> It is better to defer the probe of iio_hwmon if such error is
>>> returned by
>>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>>> channels in iio_map_list.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.sch...@free-electrons.com>
>>>> ---
>>>>   drivers/hwmon/iio_hwmon.c | 5 -
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>> index b550ba5..c0da4d9 100644
>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>>> *pdev)
>>>>   name = dev->of_node->name;
>>>>
>>>>   channels = iio_channel_get_all(dev);
>>>> -if (IS_ERR(channels))
>>>> +if (IS_ERR(channels)) {
>>>> +if (PTR_ERR(channels) == -ENODEV)
>>>> +return -EPROBE_DEFER;
>>>
>>> The problem, as I see it, is with iio, which should return
>>> -EPROBE_DEFER
>>> in this situation.
>> Agreed. New fangled stuff this deferred probing :)
>>>
>>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>>> channels are _really_ not there, which would result in endless
>>> "deferred"
>>> messages.
>> Hmm not entirely sure how we prevent that happening wherever it is done..
>>
> 
> Outch. Better at the source, though. I didn't look at the iio code recently,
> but can you detect the defer situation at least with devicetree ?
> 
> For non-devicetree situations, the only option I can think of would be
> to replace the module initcall with a later initcall. That should solve
> the problem if both iio_hwmon and and underlying drivers are built
> into the kernel. If iio_hwmon is modular, the only real option I can
> see is to make sure that all drivers it needs are loaded first.
> 
> Does this make sense ?
I think we need to look in a couple of directions.  Firstly, investigate doing
something similar to gpio and basically move the setup of maps much earlier.
This will replace drivers presenting their own maps

The other direction is to get userspace (i.e. configfs) setup of these maps
working so for cases where it's a bit less hardware defined (i.e. using an
accelerometer as an input device) we can do once we know all devices relevant
are present and instantiate new instances on the fly.

Anyhow, neither is trivial unfortunately.

J
> 
> Guenter
> 
>>>
>>> Guenter
>>>
>>>>   return PTR_ERR(channels);
>>>> +}
>>>>
>>>>   st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>   if (st == NULL) {
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Jonathan Cameron]

On 30 June 2016 04:47:25 BST, Guenter Roeck  wrote:
>On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>> iio_channel_get_all returns -ENODEV when it cannot find either
>phandles and
>> properties in the Device Tree or channels whose consumer_dev_name
>matches
>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>drivers
>> which might be probed after iio_hwmon.
>> 
>> It is better to defer the probe of iio_hwmon if such error is
>returned by
>> iio_channel_get_all in order to let a chance to iio drivers to expose
>> channels in iio_map_list.
>> 
>> Signed-off-by: Quentin Schulz 
>> ---
>>  drivers/hwmon/iio_hwmon.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index b550ba5..c0da4d9 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>*pdev)
>>  name = dev->of_node->name;
>>  
>>  channels = iio_channel_get_all(dev);
>> -if (IS_ERR(channels))
>> +if (IS_ERR(channels)) {
>> +if (PTR_ERR(channels) == -ENODEV)
>> +return -EPROBE_DEFER;
>
>The problem, as I see it, is with iio, which should return
>-EPROBE_DEFER
>in this situation.
Agreed. New fangled stuff this deferred probing :) 
>
>We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>channels are _really_ not there, which would result in endless
>"deferred"
>messages.
Hmm not entirely sure how we prevent that happening wherever it is done..

>
>Guenter
>
>>  return PTR_ERR(channels);
>> +}
>>  
>>  st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>  if (st == NULL) {
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

[Jonathan Cameron]

On 03/06/16 12:26, Laxman Dewangan wrote:
> 
> On Friday 03 June 2016 03:36 PM, Jonathan Cameron wrote:
>> On 01/06/16 13:34, Laxman Dewangan wrote:
>>> Add support for INA3221 SW driver via IIO ADC interface. The device is
>>> register as iio-device and provides interface for voltage/current and power
>>> monitor. Also provide interface for setting oneshot/continuous mode and
>>> critical/warning threshold for the shunt voltage drop.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
>> Hi Laxman,
>>
>> As ever with any driver lying on the border of IIO and hwmon, please include
>> a short justification of why you need an IIO driver and also cc the
>> hwmon list + maintainers. (cc'd on this reply).
>>
>> I simply won't take a driver where the hwmon maintainers aren't happy.
>> As it stands I'm not seeing obvious reasons in the code for why this
>> should be an IIO device.
> 
> I thought that all ADC or monitors are going to be part of IIO device
> framework. I saw the ina2xx which is same (single channel) which was
> my reference point.

That had a rather specific use case IIRC - they needed the buffered support
to get the data fast enough.
> 
>> Funily enough I know this datasheet a little as was evaluating
>> it for use on some boards at the day job a week or so ago.
>>
>> Various comments inline. Major points are:
>> * Don't use 'fake' channels to control events. If the events infrastructure
>> doesn't handle your events, then fix that rather than working around it.
>> * There is a lot of ABI in here concerned with oneshot vs continuous.
>> This seems to me to be more than it should be. We wouldn't expect to
>> see stuff changing as a result of switching between these modes other
>> than wrt to when the data shows up.  So I'd expect to not see this
>> directly exposed at all - but rather sit in oneshot unless either:
>> 1) Buffered mode is running (not currently supported)
>> 2) Alerts are on - which I think requires it to be in continuous mode.
>>
>> Other question to my mind is whether we should be reporting vshunt or
>> (using device tree to pass resistance) current.
> 

> This is bus and shunt voltage device for power monitoring. In our
> platforms, we use this device for bus current and so power monitor.
> 
> We have two usecases, one is one shot, read when it needs it. And
> other continuous when we have multiple core running then continuous
> mode to get the power consumption by rail.
That's fine, but continuous should be using the buffered interfaces
really as that's there explicitly to support groups of channels
captured using a sequencer.

Then the abi ends up much more standard which is nice. Also allows
for high speed ish continuous monitoring which is what the was
I think the point of the single channel driver.

> 
> Yaah, alert is used only on continuous mode and mainly used for
> throttling when rail power goes beyond some limit.
Of interesting in Linux, or routed directly to hardware?
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

[Jonathan Cameron]

On 03/06/16 11:06, Jonathan Cameron wrote:
> On 01/06/16 13:34, Laxman Dewangan wrote:
>> The INA3221 is a three-channel, high-side current and bus voltage monitor
>> with an I2C interface from Texas Instruments. The INA3221 monitors both
>> shunt voltage drops and bus supply voltages in addition to having
>> programmable conversion times and averaging modes for these signals.
>> The INA3221 offers both critical and warning alerts to detect multiple
>> programmable out-of-range conditions for each channel.
>>
>> Add support for INA3221 SW driver via IIO ADC interface. The device is
>> register as iio-device and provides interface for voltage/current and power
>> monitor. Also provide interface for setting oneshot/continuous mode and
>> critical/warning threshold for the shunt voltage drop.
>>
>> Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
> Hi Laxman,
> 
> As ever with any driver lying on the border of IIO and hwmon, please include
> a short justification of why you need an IIO driver and also cc the
> hwmon list + maintainers. (cc'd on this reply).
> 
> I simply won't take a driver where the hwmon maintainers aren't happy.
> As it stands I'm not seeing obvious reasons in the code for why this
> should be an IIO device.
> 
> Funily enough I know this datasheet a little as was evaluating
> it for use on some boards at the day job a week or so ago.
> 
> Various comments inline. Major points are:
> * Don't use 'fake' channels to control events. If the events infrastructure
> doesn't handle your events, then fix that rather than working around it.
> * There is a lot of ABI in here concerned with oneshot vs continuous.
> This seems to me to be more than it should be. We wouldn't expect to
> see stuff changing as a result of switching between these modes other
> than wrt to when the data shows up.  So I'd expect to not see this
> directly exposed at all - but rather sit in oneshot unless either:
> 1) Buffered mode is running (not currently supported)
> 2) Alerts are on - which I think requires it to be in continuous mode.
> 
> Other question to my mind is whether we should be reporting vshunt or
> (using device tree to pass resistance) current.
> 
> Code looks good, bu these more fundamental bits need sorting.
Another minor point - why do the power calculations in driver?
no hardware support for it, so why not just leave it to userspace?
> 
> Thanks,
> 
> Jonathan
>> ---
>>  drivers/iio/adc/Kconfig   |   12 +
>>  drivers/iio/adc/Makefile  |1 +
>>  drivers/iio/adc/ina3221.c | 1175 
>> +
>>  3 files changed, 1188 insertions(+)
>>  create mode 100644 drivers/iio/adc/ina3221.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 25378c5..65f3c27 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -223,6 +223,18 @@ config INA2XX_ADC
>>Say yes here to build support for TI INA2xx family of Power Monitors.
>>This driver is mutually exclusive with the HWMON version.
>>  
>> +config INA3221
>> +tristate "TI INA3221 3-Channel Shunt and Bus Voltage Monitor"
>> +depends on I2C
>> +select REGMAP_I2C
>> +help
>> +  INA3221 is Triple-Channel, High-Side Measurement, Shunt and Bus
>> +  Voltage Monitor device from TI. This driver support the reading
>> +  of all channel's voltage/current and power via IIO interface.
>> +  Say yes here to build support for TI INA3221. To compile this
>> +  driver as a module, choose M here: the module will be called
>> +  ina3221.
>> +
>>  config IMX7D_ADC
>>  tristate "IMX7D ADC driver"
>>  depends on ARCH_MXC || COMPILE_TEST
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 38638d4..c24f455 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>>  obj-$(CONFIG_HI8435) += hi8435.o
>>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>> +obj-$(CONFIG_INA3221) += ina3221.o
>>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>>  obj-$(CONFIG_MAX1027) += max1027.o
>> diff --git a/drivers/iio/adc/ina3221.c b/drivers/iio/adc/ina3221.c
>> new file mode 100644
>> index 000..a17f688
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina3221.c
>> @@ -0,0 +1,1175 @@
>> +/*
>> + * IIO driver for INA3221
>> + *
>> + * Copyright (C) 2016 NV

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

[Jonathan Cameron]

On 01/06/16 13:34, Laxman Dewangan wrote:
> The INA3221 is a three-channel, high-side current and bus voltage monitor
> with an I2C interface from Texas Instruments. The INA3221 monitors both
> shunt voltage drops and bus supply voltages in addition to having
> programmable conversion times and averaging modes for these signals.
> The INA3221 offers both critical and warning alerts to detect multiple
> programmable out-of-range conditions for each channel.
> 
> Add support for INA3221 SW driver via IIO ADC interface. The device is
> register as iio-device and provides interface for voltage/current and power
> monitor. Also provide interface for setting oneshot/continuous mode and
> critical/warning threshold for the shunt voltage drop.
> 
> Signed-off-by: Laxman Dewangan 
Hi Laxman,

As ever with any driver lying on the border of IIO and hwmon, please include
a short justification of why you need an IIO driver and also cc the
hwmon list + maintainers. (cc'd on this reply).

I simply won't take a driver where the hwmon maintainers aren't happy.
As it stands I'm not seeing obvious reasons in the code for why this
should be an IIO device.

Funily enough I know this datasheet a little as was evaluating
it for use on some boards at the day job a week or so ago.

Various comments inline. Major points are:
* Don't use 'fake' channels to control events. If the events infrastructure
doesn't handle your events, then fix that rather than working around it.
* There is a lot of ABI in here concerned with oneshot vs continuous.
This seems to me to be more than it should be. We wouldn't expect to
see stuff changing as a result of switching between these modes other
than wrt to when the data shows up.  So I'd expect to not see this
directly exposed at all - but rather sit in oneshot unless either:
1) Buffered mode is running (not currently supported)
2) Alerts are on - which I think requires it to be in continuous mode.

Other question to my mind is whether we should be reporting vshunt or
(using device tree to pass resistance) current.

Code looks good, bu these more fundamental bits need sorting.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/Kconfig   |   12 +
>  drivers/iio/adc/Makefile  |1 +
>  drivers/iio/adc/ina3221.c | 1175 
> +
>  3 files changed, 1188 insertions(+)
>  create mode 100644 drivers/iio/adc/ina3221.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..65f3c27 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -223,6 +223,18 @@ config INA2XX_ADC
> Say yes here to build support for TI INA2xx family of Power Monitors.
> This driver is mutually exclusive with the HWMON version.
>  
> +config INA3221
> + tristate "TI INA3221 3-Channel Shunt and Bus Voltage Monitor"
> + depends on I2C
> + select REGMAP_I2C
> + help
> +   INA3221 is Triple-Channel, High-Side Measurement, Shunt and Bus
> +   Voltage Monitor device from TI. This driver support the reading
> +   of all channel's voltage/current and power via IIO interface.
> +   Say yes here to build support for TI INA3221. To compile this
> +   driver as a module, choose M here: the module will be called
> +   ina3221.
> +
>  config IMX7D_ADC
>   tristate "IMX7D ADC driver"
>   depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..c24f455 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>  obj-$(CONFIG_HI8435) += hi8435.o
>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
> +obj-$(CONFIG_INA3221) += ina3221.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
> diff --git a/drivers/iio/adc/ina3221.c b/drivers/iio/adc/ina3221.c
> new file mode 100644
> index 000..a17f688
> --- /dev/null
> +++ b/drivers/iio/adc/ina3221.c
> @@ -0,0 +1,1175 @@
> +/*
> + * IIO driver for INA3221
> + *
> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* INA3221 registers definition */
> +#define INA3221_CONFIG   0x00
> +#define INA3221_SHUNT_VOL_CHAN1  0x01
> +#define INA3221_BUS_VOL_CHAN10x02
> +#define INA3221_SHUNT_VOL_CHAN2  0x03
> +#define INA3221_BUS_VOL_CHAN20x04
> +#define INA3221_SHUNT_VOL_CHAN3  0x05
> +#define INA3221_BUS_VOL_CHAN30x06
>