Re: [PATCH v3 1/7] iio: adc: hi8435: Holt HI-8435 threshold detector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 16/08/15 19:54, Vladimir Barinov wrote:
> Hi Jonathan,
> 
> On 16.08.2015 12:00, Jonathan Cameron wrote:
>> On 11/08/15 15:37, Vladimir Barinov wrote:
>>> Hi Jonathan,
>>>
>>> Thank you for the review.
>> Sorry I was being so indecisive.  Always take a while when something
>> 'new' turns up to gather opinions and select the best option.  You
>> are just the unlucky guy who happened to hit the question first!
>>
>> Anyhow, I'm basically happy (now) with the events approach though
>> I would suggest some modifications to how exactly it is done
>> (see inline).
> Nice to hear!
> I've got some light in the tunnel :)
Yeah, sorry it took so long to pin down the approach!
> 
> Thank you for dealing with this stuff.
> Please find one minor questions below.
> 
> Regards,
> Vladimir
> 
>>
>> Jonathan
>>> On 08.08.2015 20:56, Jonathan Cameron wrote:
>>>> On 29/07/15 13:56, Vladimir Barinov wrote:
>>>>> Add Holt threshold detector driver for HI-8435 chip
>>>>>
>>>>> Signed-off-by: Vladimir Barinov <vladimir.barinov@xxxxxxxxxxxxxxxxxx>
>>>> Because it usually reads better that way, I review from the bottom.
>>>> Might make more sense if you read the comments that way around ;)
>>> I did try to match this way.
>>> At some places the comments below the code, but in other places the comments are above the code :)
>> Yeah, I've never been consistent ;)
>>>> I'm going to guess that the typical usecase for this device is in realtime
>>>> systems where polling gives a nice predictable timing (hence the lack of any
>>>> interrupt support).  These typically operate as 'scanning' devices
>>>> (e.g. you update all state at approximately the same time, by reading
>>>> one input after another in some predefined order).
>>> Probably you are right about 8435 h/w design or there were other
>>> reason why this functionality was dropped in comparison to 8429
>>> chip. The Holt chip hi-8429 has both hardware interrupt and hardware
>>> debounce enable lines, but it has less amount of sensors (just 8
>>> outputs).
>>>> Does the use of events actually map well to this use case?  You are taking
>>>> something that (other than the results being a bit different) is basically
>>>> a digital I/O interface and mapping it (sort of) to an interrupt chip
>>>> when it isn't nothing of the sort.
>>> a) Using hardware point of view.
>>>
>>> I was thinking that it matches the events a little bit more then
>>> buffer, because the chip senses for threshold changes (passing
>>> threshold high or threshold low margins) and then notifies the upper
>>> layer. The bad point here is that the chip does not have interrupt
>>> line for sensor state change (i.e. threshold passing).
>>>
>>> If it would have the interrupt line for sensor state changes then the
>>> event interface map this case best way.
>> Agreed.  I'm coming round to the argument Lars made that we should end
>> up with the interface being the same whether or not there is an interrupt
>> there.  Actually I suspect in the long run we will end up with both
>> a buffered interface and an event interface as they simply have different
>> usecases for the same hardware (which is why we are having these lengthy
>> discussions in the first place!)
>>
>>> b) Using s/w point of view
>>>
>>> I answer relying on current iio stack implementation. I do understand
>>> that the 8435 chip is not the common/usual iio client like
>>> adc/dac/light/others, so I'm trying to match the existent iio stack.
>> Yes.  It's certainly throwing up lots of questions!
>>>  From s/w point of view it does not matter much difference between
>>> event or buffer interface for this chip because in provides 1-bit
>>> change and does not have it's own interrupt source line.
>> Yes, just requires the userspace side to track the state if it wants
>> to know.  Actually the one nasty is initialization. When the events
>> are first enabled I guess we fire them all once (if any limits are currently
>> breached).  That way the state can always be known.
>>> The event interface has generic interface to fill in falling/rising
>>> thresholds but buffer interface does not.
>> We can always add more ABI.
>>
>>> The buffer interface has already has the trigger poll func support
>>> but event interface does not. So I've tried to implement the
>>> triggered event in iio stack. BTW: Doesn't the triggered event
>>> support match the usecase with iio_interrupt_trigger? Or it does not
>>> make sense to have triggered events (even with irqtrig) at all?
>> Initially I was unsure about this, but Lars and you have convinced me.
>> I'm happy having that in the subsystem.  Can also be used for devices
>> which have an interrupt but which didn't get wired for some reason rather
>> than having them have some internal poll loop (as we have done up to now).
>>
>>  
>>>> I'd like more opinions on this, but my gut reaction is that we would
>>>> be better making the normal buffered infrastructure handle this data
>>>> properly rather than pushing it through the events intrastructure.
>>>>
>>>> Your solution is neat and tidy in someways but feels like it is mashing
>>>> data into a particular format.
>>> Probably that's all because the chip lacks smth from buffer interface
>>> approach (the data should diferent the 1-bit) and smth from event
>>> interface approach (lack of h/w interrupt line)
>> Agreed.
>>>> To add to the complexity we could have debounce logic.  If we mapped to a
>>>> fill the buffer with a scan mode, how would we handle this?
>>>> My guess is that it would simply delay transistions.  Perhaps we even
>>>> support reading both the value right now and the debounced value if that
>>>> is possible?
>>> I did handle it in the driver the same way in both cases (buffer and
>>> event interfaces): event/buffer trigger issues interrupt and then the
>>> driver checks value right now and after delay (debounce_interval).
>> I am yet to be convinced we want software debounce in the kernel.  This
>> is something that the input subsystem has always had.  Partly the aim of
>> IIO was to provide a slim system where all this stuff was moved to userspace.
> Ok, I will remove the s/w debounce in the driver for now.
> 
>>
>> Ultimately the ideal option to my mind is to finally write an inkernel
>> interface for the events data flow (as we have for buffered data) then
>> we can have a generic attachable debounce unit independent of any particular
>> driver.  Hohum.  Now all I need is someone to write it or a few weeks off
>> work ;)
> This is a nice design :)
> But much more hard then implement this in the driver.
> 
>>> If we do not plan to put debounced values to buffer then we may cache
>>> during debouncing procedure and then return via
>>> IIO_CHAN_INFO_DEBOUNCE_VALUE (or other info name). Probably even with
>>> timestamp.
>>>> However, here the debounce is all software.  To my mind, move this into
>>>> userspace entirely. We aren't dealing with big data here so it's hardly
>>>> going to be particularly challenging.
>>> I will drop it on your demand in favour to encourage chipmakers to provide h/w debounce,
>>> but this was much easy/efficient to have it in kernel space, since
>>> during run generic_buffer sample (or other) we got the new data and then we have to start timer, then timer expires
>>> and we read raw values from sysfs manually to compare the one that came from buffer and one after debounce
>>> delay.
>> If you are clocking evenly and we were using buffering you'd simply do it as readback in time
>> rather than the other way around - thus no timer at all.  Just each time check the last N
>> samples to see if they are all high or all low or similar.
> This is right for high frequency clocking.
> If we are clocking evently at low frequency (~1Hz) and the recommended debounce interval is 10-100ms then
> we should use timer for issuing extra clock inside 10-100ms for check/validate the event.
>>
>>>> So my gut feeling is definitely we need to make the buffer infrastructure
>>>> handle 1 bit values (in groups) then push this data out that way.
>>> I will wait for your final decision.
>> Lets proceed with the event version.  If someone has a strong usecase
>> for adding buffering as well, there is nothing stopping the driver doing both
>> that I can see (once we have the core infrastructure in place).
> Ok, thx.
> 
>>>> Still I would like other opinions on this!
>>>>
>>>> Jonathan
>>>>
>>>>> ---
>>>>> Changes in version 2:
>>>>> - Added file sysfs-bus-iio-adc-hi8435
>>>>> - Changed naming from "discrete ADC" to "threshold detector"
>>>>> - Replaced swab16p/swab32p with be16_to_cpup/be32_to_cpup
>>>>> - Made *_show and *_store functions static
>>>>> - moved out from iio buffers to iio events
>>>>> - removed hi8436/hi8437 chips from the driver
>>>>> - moved from debounce_soft_delay/enable to debounce_interval via
>>>>>     IIO_CHAN_INFO_DEBOUNCE_TIME
>>>>> - added name extention "comparator"
>>>>> - moved threshold/hysteresis setup via generic iio event sysfs
>>>>> - added software mask/unmask channel events
>>>>> - added programming sensor outputs while in test mode via
>>>>>     IIO_CHAN_INFO_RAW
>>>>> - added channels .ext_info for programming sensing mode
>>>>> Changes in version 3:
>>>>> - fixed typos
>>>>> - moved <linux/interrupt.h> to match alphabetic order
>>>>> - fixed missed */event/* prefix in the ABI description
>>>>> - added missed colon in sysfs-bus-iio-adc-hi8435 file after "What"
>>>>> - moved in_voltageY_comparator_thresh_either_en and debounce_time
>>>>>     to sysfs-bus-iio file
>>>>> - added error checking for hi8435_write[b|w]
>>>>>
>>>>>    Documentation/ABI/testing/sysfs-bus-iio            |   1 +
>>>>>    Documentation/ABI/testing/sysfs-bus-iio-adc-hi8435 |  61 ++
>>>>>    drivers/iio/adc/Kconfig                            |  11 +
>>>>>    drivers/iio/adc/Makefile                           |   1 +
>>>>>    drivers/iio/adc/hi8435.c                           | 659 +++++++++++++++++++++
>>>>>    5 files changed, 742 insertions(+)
>>>>>    create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-hi8435
>>>>>    create mode 100644 drivers/iio/adc/hi8435.c
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>>>> index 70c9b1a..09eac44 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>>> @@ -572,6 +572,7 @@ What:        /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_r
>>>>>    What:        /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_en
>>>>>    What:        /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_rising_en
>>>>>    What:        /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_falling_en
>>>>> +What:        /sys/.../iio:deviceX/events/in_voltageY_comparator_thresh_either_en
>>>>>    What:        /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_rising_en
>>>>>    What:        /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_falling_en
>>>>>    What:        /sys/.../iio:deviceX/events/in_voltageY_thresh_rising_en
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-hi8435 b/Documentation/ABI/testing/sysfs-bus-iio-adc-hi8435
>>>>> new file mode 100644
>>>>> index 0000000..c59d81d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-hi8435
>>>>> @@ -0,0 +1,61 @@
>>>>> +What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_comparator_raw
>> We really try to use the extended names infrequently and normally only
>> for things like 'this voltage channel is internally connected to the power
>> supply rail'.  Don't think we need it here.   As suggested below, if we
>> are going events then we don't want to expose a _raw reading except via
>> in the 'events' directory where it would be a 'current event status'.
>> Anyhow, I talk about that futher down.
> I will remove *_raw.
> 
>>>>> +Date:        July 2015
>>>>> +KernelVersion:    4.2.0
>>>>> +Contact:    source@xxxxxxxxxxxxxxxxxx
>>>>> +Description:
>>>>> +        Read value is a voltage threshold measurement from channel Y.
>>>>> +        Could be either 0 if sensor voltage is lower then low voltage
>>>>> +        threshold or 1 if sensor voltage is higher then high voltage
>>>>> +        threshold.
>>>>> +        Write value is a programmed sensor output while in self test
>>>>> +        mode. Could be either 0 or 1. The programmed value will be read
>>>>> +        back if /sys/bus/iio/devices/iio:deviceX/test_enable is set to 1.
>>>>> +
>>>>> +What:        /sys/bus/iio/devices/iio:deviceX/test_enable
>>>>> +Date:        July 2015
>>>>> +KernelVersion:    4.2.0
>>>>> +Contact:    source@xxxxxxxxxxxxxxxxxx
>>>>> +Description:
>>>>> +        Enable/disable the HI-8435 self test mode.
>>>>> +        If enabled the in_voltageY_comparator_raw should be read back
>>>>> +        accordingly to written value to in_voltageY_comparator_raw.
>> Hmm. Normally we just do self tests at startup.  So set some patterns, read them
>> and then verify they are fine.  Drops the need for a userspace interface
>> that can get a little confusing (writing to what is normally a read
>> only attribute).  Would this work here?
>>
>> Alternatively, just add a debugfs interface to allow this to be set rather than
>> exposing it via sysfs all the time?
> I will add debugfs or drop it (since I did use write_raw() for setting sensor output in test mode)
> 
>>
>>>>> +
>>>>> +What:        /sys/bus/iio/devices/iio:deviceX/in_voltageY_comparator_sensing_mode
>>>>> +Date:        July 2015
>>>>> +KernelVersion:    4.2.0
>>>>> +Contact:    source@xxxxxxxxxxxxxxxxxx
>>>>> +Description:
>>>>> +        Program sensor type for threshold detector inputs.
>>>>> +        Could be either "GND-Open" or "Supply-Open" mode. Y is a
>>>>> +        threshold detector input channel. Channels 0..7, 8..15, 16..23
>>>>> +        and 24..31 has common sensor types.
>>>>> +
>>>>> +What:        /sys/bus/iio/devices/iio:deviceX/events/in_voltageY_comparator_thresh_falling_value
>> This is where the comparator extended name looks a bit silly as we are
>> clearly comparing it anyway due to the thresh_falling part ;)
> I will remove the extended name.
> 
>>>>> +Date:        July 2015
>>>>> +KernelVersion:    4.2.0
>>>>> +Contact:    source@xxxxxxxxxxxxxxxxxx
>>>>> +Description:
>>>>> +        Channel Y low voltage threshold. If sensor input voltage goes lower then
>>>>> +        this value then the threshold falling event is pushed.
>>>>> +        Depending on in_voltageY_comparator_sensing_mode the low voltage threshold
>>>>> +        is separately set for "GND-Open" and "Supply-Open" modes.
>>>>> +        Channels 0..31 have common low threshold values, but could have different
>>>>> +        sensing_modes.
>>>>> +        The low voltage threshold range is between 2..21V.
>>>>> +        Hysteresis between low and high thresholds can not be lower then 2 and
>>>>> +        can not be odd.
>>>>> +
>>>>> +What:        /sys/bus/iio/devices/iio:deviceX/events/in_voltageY_comparator_thresh_rising_value
>>>>> +Date:        July 2015
>>>>> +KernelVersion:    4.2.0
>>>>> +Contact:    source@xxxxxxxxxxxxxxxxxx
>>>>> +Description:
>>>>> +        Channel Y high voltage threshold. If sensor input voltage goes higher then
>>>>> +        this value then the threshold rising event is pushed.
>>>>> +        Depending on in_voltageY_comparator_sensing_mode the high voltage threshold
>>>>> +        is separately set for "GND-Open" and "Supply-Open" modes.
>>>>> +        Channels 0..31 have common high threshold values, but could have different
>>>>> +        sensing_modes.
>>>>> +        The high voltage threshold range is between 3..22V.
>>>>> +        Hysteresis between low and high thresholds can not be lower then 2 and
>>>>> +        can not be odd.
>>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>>> index eb0cd89..553c91e 100644
>>>>> --- a/drivers/iio/adc/Kconfig
>>>>> +++ b/drivers/iio/adc/Kconfig
>>>>> @@ -170,6 +170,17 @@ config EXYNOS_ADC
>>>>>          of SoCs for drivers such as the touchscreen and hwmon to use to share
>>>>>          this resource.
>>>>>    +config HI8435
>>>>> +    tristate "Holt Integrated Circuits HI-8435 threshold detector"
>>>>> +    select IIO_TRIGGERED_EVENT
>>>>> +    depends on SPI
>>>>> +    help
>>>>> +      If you say yes here you get support for Holt Integrated Circuits
>>>>> +      HI-8435 chip.
>>>>> +
>>>>> +      This driver can also be built as a module. If so, the module will be
>>>>> +      called hi8435.
>>>>> +
>>>>>    config LP8788_ADC
>>>>>        tristate "LP8788 ADC driver"
>>>>>        depends on MFD_LP8788
>>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>>> index a096210..00f367aa 100644
>>>>> --- a/drivers/iio/adc/Makefile
>>>>> +++ b/drivers/iio/adc/Makefile
>>>>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>>>>    obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>>>>    obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>>>>    obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>>>> +obj-$(CONFIG_HI8435) += hi8435.o
>>>>>    obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>>>    obj-$(CONFIG_MAX1027) += max1027.o
>>>>>    obj-$(CONFIG_MAX1363) += max1363.o
>>>>> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
>>>>> new file mode 100644
>>>>> index 0000000..f3dab5a
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/adc/hi8435.c
>>>>> @@ -0,0 +1,659 @@
>>>>> +/*
>>>>> + * Holt Integrated Circuits HI-8435 threshold detector driver
>>>>> + *
>>>>> + * Copyright (C) 2015 Zodiac Inflight Innovations
>>>>> + * Copyright (C) 2015 Cogent Embedded, Inc.
>>>>> + *
>>>>> + * 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 <linux/delay.h>
>>>>> +#include <linux/iio/events.h>
>>>>> +#include <linux/iio/iio.h>
>>>>> +#include <linux/iio/sysfs.h>
>>>>> +#include <linux/iio/trigger.h>
>>>>> +#include <linux/iio/trigger_consumer.h>
>>>>> +#include <linux/iio/triggered_event.h>
>>>>> +#include <linux/interrupt.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_device.h>
>>>>> +#include <linux/of_gpio.h>
>>>>> +#include <linux/spi/spi.h>
>>>>> +#include <linux/workqueue.h>
>>>>> +
>>>>> +#define DRV_NAME "hi8435"
>>>>> +
>>>>> +/* Register offsets for HI-8435 */
>>>>> +#define HI8435_CTRL_REG            0x02
>>>>> +#define HI8435_PSEN_REG            0x04
>>>>> +#define HI8435_TMDATA_REG        0x1E
>>>>> +#define HI8435_GOCENHYS_REG        0x3A
>>>>> +#define HI8435_SOCENHYS_REG        0x3C
>>>> Hmm. These aren't in the docs that I can immediately acquire...
>>>> Any chance of some more meaningful naming?
>>> I did follow the register naming in the chip datasheet:
>>> http://www.holtic.com/products/3081-hi-8435.aspx
>>>
>>> The SO7_0 means "Sensor status bank 0 register: SO<7:0>".
>>> Does HI8435_STATUS_BANK0_REG work?
>> Ah, I missed the meaning entirely by not noticing they were 8 bits
>> each ;)  This current naming is fine.
>>> Or should I just add comments instead of changing definition?
>>>>> +#define HI8435_SO7_0_REG        0x10
>>>>> +#define HI8435_SO15_8_REG        0x12
>>>>    +#define HI8435_SO23_16_REG        0x14
>>>>> +#define HI8435_SO31_24_REG        0x16
>>>>> +#define HI8435_SO31_0_REG        0x78
>>>>> +
>>>>> +#define HI8435_WRITE_OPCODE        0x00
>>>>> +#define HI8435_READ_OPCODE        0x80
>>>>> +
>>>>> +/* CTRL register bits */
>>>>> +#define HI8435_CTRL_TEST        0x01
>>>>> +#define HI8435_CTRL_SRST        0x02
>>>>> +
>>>>> +#define HI8435_DEBOUNCE_DELAY_MAX    1000    /* msec */
>>>>> +#define HI8435_DEBOUNCE_DELAY_DEF    100    /* msec */
>>>>> +
>>>>> +struct hi8435_priv {
>>>>> +    struct spi_device *spi;
>>>>> +    struct mutex lock;
>>>>> +    struct delayed_work work;
>>>>> +
>>>>> +    int reset_gpio;
>>>>> +    int debounce_interval; /* msec */
>>>>> +    u32 debounce_val; /* prev value to compare during software debounce */
>>>>> +
>>>>> +    unsigned long event_scan_mask; /* soft mask/unmask channels events */
>>>>> +    unsigned int event_prev_val;
>>>>> +
>>>>> +    unsigned threshold_lo[2]; /* GND-Open and Supply-Open thresholds */
>>>>> +    unsigned threshold_hi[2]; /* GND-Open and Supply-Open threshold */
>>>>> +    u8 reg_buffer[4] ____cacheline_aligned;
>>>>> +};
>>>>> +
>>>>> +static int hi8435_readb(struct hi8435_priv *priv, u8 reg, u8 *val)
>>>>> +{
>>>>> +    reg |= HI8435_READ_OPCODE;
>>>>> +    return spi_write_then_read(priv->spi, &reg, 1, val, 1);
>>>>> +}
>>>>> +
>>>>> +static int hi8435_readw(struct hi8435_priv *priv, u8 reg, u16 *val)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    reg |= HI8435_READ_OPCODE;
>>>>> +    ret = spi_write_then_read(priv->spi, &reg, 1, val, 2);
>>>>> +    *val = be16_to_cpup(val);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_readl(struct hi8435_priv *priv, u8 reg, u32 *val)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    reg |= HI8435_READ_OPCODE;
>>>>> +    ret = spi_write_then_read(priv->spi, &reg, 1, val, 4);
>>>>> +    *val = be32_to_cpup(val);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_writeb(struct hi8435_priv *priv, u8 reg, u8 val)
>>>>> +{
>>>>> +    priv->reg_buffer[0] = reg | HI8435_WRITE_OPCODE;
>>>>> +    priv->reg_buffer[1] = val;
>>>>> +
>>>>> +    return spi_write(priv->spi, priv->reg_buffer, 2);
>>>>> +}
>>>>> +
>>>>> +static int hi8435_writew(struct hi8435_priv *priv, u8 reg, u16 val)
>>>>> +{
>>>>> +    priv->reg_buffer[0] = reg | HI8435_WRITE_OPCODE;
>>>>> +    priv->reg_buffer[1] = (val >> 8) & 0xff;
>>>>> +    priv->reg_buffer[2] = val & 0xff;
>>>>> +
>>>>> +    return spi_write(priv->spi, priv->reg_buffer, 3);
>>>>> +}
>>>>> +
>>>>> +static ssize_t hi8435_test_enable_show(struct device *dev,
>>>>> +                       struct device_attribute *attr, char *buf)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(dev_to_iio_dev(dev));
>>>>> +    int ret;
>>>>> +    u8 reg;
>>>>> +
>>>>> +    ret = hi8435_readb(priv, HI8435_CTRL_REG, &reg);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return sprintf(buf, "%d\n", reg & HI8435_CTRL_TEST);
>>>>> +}
>>>>> +
>>>>> +static ssize_t hi8435_test_enable_store(struct device *dev,
>>>>> +                    struct device_attribute *attr,
>>>>> +                    const char *buf, size_t len)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(dev_to_iio_dev(dev));
>>>>> +    unsigned int val;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = kstrtouint(buf, 10, &val);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = hi8435_writeb(priv, HI8435_CTRL_REG, val ? HI8435_CTRL_TEST : 0);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return len;
>>>>> +}
>>>>> +
>>>>> +static IIO_DEVICE_ATTR(test_enable, S_IRUGO | S_IWUSR,
>>>>> +    hi8435_test_enable_show, hi8435_test_enable_store, 0);
>>>>> +
>>>>> +static struct attribute *hi8435_attributes[] = {
>>>>> +    &iio_dev_attr_test_enable.dev_attr.attr,
>>>>> +    NULL,
>>>>> +};
>>>>> +
>>>>> +static struct attribute_group hi8435_attribute_group = {
>>>>> +    .attrs = hi8435_attributes,
>>>>> +};
>>>>> +
>>>>> +static int hi8435_read_raw(struct iio_dev *idev,
>>>>> +               const struct iio_chan_spec *chan,
>>>>> +               int *val, int *val2, long mask)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    int ret;
>>>>> +
>>>>> +    switch (mask) {
>>>>> +    case IIO_CHAN_INFO_RAW:
>>>>> +        ret = hi8435_readl(priv, HI8435_SO31_0_REG, val);
>>>>> +        if (ret < 0)
>>>>> +            return ret;
>>>>> +        *val = !!(*val & BIT(chan->channel));
>>>>> +        return IIO_VAL_INT;
>> This is where we end up in an odd possition having elected to report these
>> as 'events'.
>> We are reporting a 'last event state' via a read raw rather than as part
>> of the events infrastructure. I wonder if instead we want to allow the
>> events stuff to give us a 'am I true now signal'.
>> Would be easy enough to add and would give a clear interface.
> I agree.
> 
> I was thinking to use it only for getting initial states.
> But it is ok if we assume that default state is all 0 and then
> make UI changes in accordance to coming events.
That would be the option that makes most sense to me.
> 
> Also It might be useful for UI debounce for clocking at low frequency when
> debounce interval is much smaller then clocking/polling period.
That would really mean that the polling wasn't fast enough. I can see your
point though that maybe you'd want to ensure a value had been up for a
particular period before accepting it.  Still I doubt this sort of chip is
ever really used for a UI interface!

> 
>>
>> IIO_EV_INFO_CURRENT_STATE and attibute naming something like
>> in_voltageY_thresh_rising_state
>> which reads either 1 or 0?
> Got it.
> I will remove *_raw in favour of new event interface entry.
> It should be read-only (read_event_value), right?
yes
> 
> May I use the pair callback write_event_value() for my test_mode for
> setting each sense output manually?
> Or should I put all test mode related stuff into debugfs?
For now I'd say debugfs.  It's a bit of an odd corner case
so I'm a little anxious not to set a precedence for it in the main
ABI. Debugfs stuff is always more fluid anyway so it will be fine there.
> 
>>
>> For other devices we might need to take into account the
>> we are not currently measuring but that can be done by an error
>> return.
>>
>>>>> +    case IIO_CHAN_INFO_DEBOUNCE_TIME:
>>>>> +        *val = priv->debounce_interval;
>>>>> +        return IIO_VAL_INT;
>>>>> +    default:
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static int hi8435_write_raw(struct iio_dev *idev,
>>>>> +                const struct iio_chan_spec *chan,
>>>>> +                int val, int val2, long mask)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +
>>>>> +    switch (mask) {
>>>>> +    case IIO_CHAN_INFO_RAW:
>>>>> +        /* program sensors outputs in test mode */
>>>>> +        return hi8435_writeb(priv, HI8435_TMDATA_REG, val ? 0x1 : 0x2);
>>>>> +    case IIO_CHAN_INFO_DEBOUNCE_TIME:
>>>>> +        if (val < 0)
>>>>> +            return -EINVAL;
>>>>> +        if (val > HI8435_DEBOUNCE_DELAY_MAX)
>>>>> +            val = HI8435_DEBOUNCE_DELAY_MAX;
>>>>> +        priv->debounce_interval = val;
>>>>> +        return 0;
>>>>> +    default:
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +static int hi8435_read_event_config(struct iio_dev *idev,
>>>>> +                    const struct iio_chan_spec *chan,
>>>>> +                    enum iio_event_type type,
>>>>> +                    enum iio_event_direction dir)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +
>>>>> +    return !!(priv->event_scan_mask & BIT(chan->channel));
>>>>> +}
>>>>> +
>>>>> +static int hi8435_write_event_config(struct iio_dev *idev,
>>>>> +                     const struct iio_chan_spec *chan,
>>>>> +                     enum iio_event_type type,
>>>>> +                     enum iio_event_direction dir, int state)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +
>>>>> +    priv->event_scan_mask &= ~BIT(chan->channel);
>>>>> +    if (state)
>>>>> +        priv->event_scan_mask |= BIT(chan->channel);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_read_event_value(struct iio_dev *idev,
>>>>> +                   const struct iio_chan_spec *chan,
>>>>> +                   enum iio_event_type type,
>>>>> +                   enum iio_event_direction dir,
>>>>> +                   enum iio_event_info info,
>>>>> +                   int *val, int *val2)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    int ret;
>>>>> +    u8 mode, psen;
>>>>> +    u16 reg;
>>>>> +
>>>>> +    ret = hi8435_readb(priv, HI8435_PSEN_REG, &psen);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    /* Supply-Open or GND-Open sensing mode */
>>>>> +    mode = !!(psen & BIT(chan->channel / 8));
>>>>> +
>>>>> +    ret = hi8435_readw(priv, mode ? HI8435_SOCENHYS_REG :
>>>>> +                 HI8435_GOCENHYS_REG, &reg);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    if (dir == IIO_EV_DIR_FALLING)
>>>>> +        *val = ((reg & 0xff) - (reg >> 8)) / 2;
>>>>> +
>>>>> +    if (dir == IIO_EV_DIR_RISING)
>>>>> +        *val = ((reg & 0xff) + (reg >> 8)) / 2;
>>>>> +
>>>>> +    return IIO_VAL_INT;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_write_event_value(struct iio_dev *idev,
>>>>> +                    const struct iio_chan_spec *chan,
>>>>> +                    enum iio_event_type type,
>>>>> +                    enum iio_event_direction dir,
>>>>> +                    enum iio_event_info info,
>>>>> +                    int val, int val2)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    int ret;
>>>>> +    u8 mode, psen;
>>>>> +    u16 reg;
>>>>> +
>>>>> +    ret = hi8435_readb(priv, HI8435_PSEN_REG, &psen);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    /* Supply-Open or GND-Open sensing mode */
>>>>> +    mode = !!(psen & BIT(chan->channel / 8));
>>>>> +
>>>>> +    ret = hi8435_readw(priv, mode ? HI8435_SOCENHYS_REG :
>>>>> +                 HI8435_GOCENHYS_REG, &reg);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    if (dir == IIO_EV_DIR_FALLING) {
>>>>> +        /* falling threshold range 2..21V, hysteresis minimum 2V */
>>>>> +        if (val < 2 || val > 21 || (val + 1) >= priv->threshold_hi[mode])
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        if (val == priv->threshold_lo[mode])
>>>>> +            return 0;
>>>>> +
>>>>> +        priv->threshold_lo[mode] = val;
>>>>> +
>>>>> +        /* hysteresis must not be odd */
>>>>> +        if ((priv->threshold_hi[mode] - priv->threshold_lo[mode]) % 2)
>>>>> +            priv->threshold_hi[mode]--;
>>>>> +    }
>>>>> +
>>>>> +    if (dir == IIO_EV_DIR_RISING) {
>>>>> +        /* rising threshold range 3..22V, hysteresis minimum 2V */
>>>>> +        if (val < 3 || val > 22 || val <= (priv->threshold_lo[mode] + 1))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +        if (val == priv->threshold_hi[mode])
>>>>> +            return 0;
>>>>> +
>>>>> +        priv->threshold_hi[mode] = val;
>>>>> +
>>>>> +        /* hysteresis must not be odd */
>>>>> +        if ((priv->threshold_hi[mode] - priv->threshold_lo[mode]) % 2)
>>>>> +            priv->threshold_lo[mode]++;
>>>>> +    }
>>>>> +
>>>>> +    /* program thresholds */
>>>>> +    mutex_lock(&priv->lock);
>>>>> +
>>>>> +    ret = hi8435_readw(priv, mode ? HI8435_SOCENHYS_REG :
>>>>> +                 HI8435_GOCENHYS_REG, &reg);
>>>>> +    if (ret < 0) {
>>>>> +        mutex_unlock(&priv->lock);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    /* hysteresis */
>>>>> +    reg = priv->threshold_hi[mode] - priv->threshold_lo[mode];
>>>>> +    reg <<= 8;
>>>>> +    /* threshold center */
>>>>> +    reg |= (priv->threshold_hi[mode] + priv->threshold_lo[mode]);
>>>>> +
>>>>> +    ret = hi8435_writew(priv, mode ? HI8435_SOCENHYS_REG :
>>>>> +                  HI8435_GOCENHYS_REG, reg);
>>>>> +
>>>>> +    mutex_unlock(&priv->lock);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_event_spec hi8435_events[] = {
>>>>> +    {
>>>>> +        .type = IIO_EV_TYPE_THRESH,
>>>>> +        .dir = IIO_EV_DIR_RISING,
>>>>> +        .mask_separate = BIT(IIO_EV_INFO_VALUE),
>>>>> +    }, {
>>>>> +        .type = IIO_EV_TYPE_THRESH,
>>>>> +        .dir = IIO_EV_DIR_FALLING,
>>>>> +        .mask_separate = BIT(IIO_EV_INFO_VALUE),
>>>>> +    }, {
>>>>> +        .type = IIO_EV_TYPE_THRESH,
>>>>> +        .dir = IIO_EV_DIR_EITHER,
>>>>> +        .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static int hi8435_get_sensing_mode(struct iio_dev *idev,
>>>>> +                   const struct iio_chan_spec *chan)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    int ret;
>>>>> +    u8 reg;
>>>>> +
>>>>> +    ret = hi8435_readb(priv, HI8435_PSEN_REG, &reg);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return !!(reg & BIT(chan->channel / 8));
>>>>> +}
>>>>> +
>>>>> +static int hi8435_set_sensing_mode(struct iio_dev *idev,
>>>>> +                   const struct iio_chan_spec *chan,
>>>>> +                   unsigned int mode)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    int ret;
>>>>> +    u8 reg;
>>>>> +
>>>>> +    mutex_lock(&priv->lock);
>>>>> +
>>>>> +    ret = hi8435_readb(priv, HI8435_PSEN_REG, &reg);
>>>>> +    if (ret < 0) {
>>>>> +        mutex_unlock(&priv->lock);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    reg &= ~BIT(chan->channel / 8);
>>>>> +    if (mode)
>>>>> +        reg |= BIT(chan->channel / 8);
>>>>> +
>>>>> +    ret = hi8435_writeb(priv, HI8435_PSEN_REG, reg);
>>>>> +
>>>>> +    mutex_unlock(&priv->lock);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static const char * const hi8435_sensing_modes[] = { "GND-Open",
>>>>> +                             "Supply-Open" };
>>>>> +
>>>>> +static const struct iio_enum hi8435_sensing_mode = {
>>>>> +    .items = hi8435_sensing_modes,
>>>>> +    .num_items = ARRAY_SIZE(hi8435_sensing_modes),
>>>>> +    .get = hi8435_get_sensing_mode,
>>>>> +    .set = hi8435_set_sensing_mode,
>>>>> +};
>>>>> +
>>>>> +static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
>>>>> +    IIO_ENUM("sensing_mode", IIO_SEPARATE, &hi8435_sensing_mode),
>>>>> +    {},
>>>>> +};
>>>>> +
>>>>> +#define HI8435_VOLTAGE_CHANNEL(num)                    \
>>>>> +{                                    \
>>>>> +    .type = IIO_VOLTAGE,                        \
>>>>> +    .indexed = 1,                            \
>>>>> +    .channel = num,                            \
>>>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),            \
>> If we are using events, why have the raw access at all?
> I will remove *_raw.
> 
> I will probably add  IIO_EV_INFO_CURRENT_STATE.
> 
>>>>> +    .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_DEBOUNCE_TIME),    \
>>>>> +    .event_spec = hi8435_events,                    \
>>>>> +    .num_event_specs = ARRAY_SIZE(hi8435_events),            \
>>>>> +    .ext_info = hi8435_ext_info,                    \
>>>>> +    .extend_name = "comparator"                    \
>>>> I'm really not sure about this approach.  Can see where you are coming
>>>> from. We already have an events interface supporting these things so
>>>> why not use it?   I'm just doubtful it is an efficient option or a clean
>>>> interface (would like more opinions on this!)
>>> Sorry, I did not understand here.
>>> Is it about .extend_name ?
>>> I did add "comparator" extention to have attributes look like "in_voltage0_comparator_raw"
>> I'd drop it, particularly if we also drop the raw access.
> Now I understand. Thank you for clarification.
> I will remove it.
> 
>>>>> +}
>>>>> +
>>>>> +static const struct iio_chan_spec hi8435_channels[] = {
>>>>> +    HI8435_VOLTAGE_CHANNEL(0),
>>>>> +    HI8435_VOLTAGE_CHANNEL(1),
>>>>> +    HI8435_VOLTAGE_CHANNEL(2),
>>>>> +    HI8435_VOLTAGE_CHANNEL(3),
>>>>> +    HI8435_VOLTAGE_CHANNEL(4),
>>>>> +    HI8435_VOLTAGE_CHANNEL(5),
>>>>> +    HI8435_VOLTAGE_CHANNEL(6),
>>>>> +    HI8435_VOLTAGE_CHANNEL(7),
>>>>> +    HI8435_VOLTAGE_CHANNEL(8),
>>>>> +    HI8435_VOLTAGE_CHANNEL(9),
>>>>> +    HI8435_VOLTAGE_CHANNEL(10),
>>>>> +    HI8435_VOLTAGE_CHANNEL(11),
>>>>> +    HI8435_VOLTAGE_CHANNEL(12),
>>>>> +    HI8435_VOLTAGE_CHANNEL(13),
>>>>> +    HI8435_VOLTAGE_CHANNEL(14),
>>>>> +    HI8435_VOLTAGE_CHANNEL(15),
>>>>> +    HI8435_VOLTAGE_CHANNEL(16),
>>>>> +    HI8435_VOLTAGE_CHANNEL(17),
>>>>> +    HI8435_VOLTAGE_CHANNEL(18),
>>>>> +    HI8435_VOLTAGE_CHANNEL(19),
>>>>> +    HI8435_VOLTAGE_CHANNEL(20),
>>>>> +    HI8435_VOLTAGE_CHANNEL(21),
>>>>> +    HI8435_VOLTAGE_CHANNEL(22),
>>>>> +    HI8435_VOLTAGE_CHANNEL(23),
>>>>> +    HI8435_VOLTAGE_CHANNEL(24),
>>>>> +    HI8435_VOLTAGE_CHANNEL(25),
>>>>> +    HI8435_VOLTAGE_CHANNEL(26),
>>>>> +    HI8435_VOLTAGE_CHANNEL(27),
>>>>> +    HI8435_VOLTAGE_CHANNEL(28),
>>>>> +    HI8435_VOLTAGE_CHANNEL(29),
>>>>> +    HI8435_VOLTAGE_CHANNEL(30),
>>>>> +    HI8435_VOLTAGE_CHANNEL(31),
>>>>> +    IIO_CHAN_SOFT_TIMESTAMP(32),
>>>>> +};
>>>>> +
>>>>> +static const struct iio_info hi8435_info = {
>>>>> +    .driver_module = THIS_MODULE,
>>>>> +    .attrs = &hi8435_attribute_group,
>>>>> +    .read_raw = hi8435_read_raw,
>>>>> +    .write_raw = hi8435_write_raw,
>>>>> +    .read_event_config = &hi8435_read_event_config,
>>>>> +    .write_event_config = hi8435_write_event_config,
>>>>> +    .read_event_value = &hi8435_read_event_value,
>>>>> +    .write_event_value = &hi8435_write_event_value,
>>>>> +};
>>>>> +
>>>>> +static void hi8435_iio_push_event(struct iio_dev *idev, unsigned int val)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    enum iio_event_direction dir;
>>>>> +    unsigned int i;
>>>>> +    unsigned int status = priv->event_prev_val ^ val;
>>>>> +
>>>>> +    if (!status)
>>>>> +        return;
>>>>> +
>>>>> +    for_each_set_bit(i, &priv->event_scan_mask, 32) {
>>>>> +        if (!(status & BIT(i)))
>>>>> +            continue;
>>>>> +
>>>>> +        dir = val & BIT(i) ? IIO_EV_DIR_RISING : IIO_EV_DIR_FALLING;
>>>>> +
>>>>> +        iio_push_event(idev,
>>>>> +                   IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
>>>>> +                            IIO_EV_TYPE_THRESH, dir),
>>>>> +                   iio_get_time_ns());
>>>>> +    }
>>>>> +
>>>>> +    priv->event_prev_val = val;
>>>>> +}
>>>>> +
>>>>> +static void hi8435_debounce_work(struct work_struct *work)
>>>>> +{
>>>>> +    struct hi8435_priv *priv = container_of(work, struct hi8435_priv,
>>>>> +                        work.work);
>>>>> +    struct iio_dev *idev = spi_get_drvdata(priv->spi);
>>>>> +    u32 val;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = hi8435_readl(priv, HI8435_SO31_0_REG, &val);
>>>>> +    if (ret < 0)
>>>>> +        return;
>>>>> +
>>>>> +    if (val == priv->debounce_val)
>>>>> +        hi8435_iio_push_event(idev, val);
>>>>> +    else
>>>>> +        dev_warn(&priv->spi->dev, "filtered by software debounce");
>>>> Definitely not.  Warning every time a standard event occurs?  Not
>>>> a good idea.  Use the debug stuff if you want a way of knowing this
>>>> happened, then it will silent under normal use.
>>> I'd say it is not standard event, but occasional/debounce event.
>>> Anyway I agree with you.
>>>
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t hi8435_trigger_handler(int irq, void *private)
>>>>> +{
>>>>> +    struct iio_poll_func *pf = private;
>>>>> +    struct iio_dev *idev = pf->indio_dev;
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +    u32 val;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = hi8435_readl(priv, HI8435_SO31_0_REG, &val);
>>>>> +    if (ret < 0)
>>>>> +        goto err_read;
>>>>> +
>>>>> +    if (priv->debounce_interval) {
>>>>> +        priv->debounce_val = val;
>>>>> +        schedule_delayed_work(&priv->work,
>>>>> +                msecs_to_jiffies(priv->debounce_interval));
>>>> This is another element that makes me doubt that the event interface
>>>> is the way to do this.  I'd much rather we pushed the debouncing out
>>>> to userspace where cleverer things can be done (and more adaptive things).
>>>> Here to keep the event flow low you have to do it in the kernel.
>>> Yes, it is helpful for reducing the data flow (buffer data or event data)
>>> Many kernel drivers support s/w event debouncing, f.e. usb cable plug-in, or other connectors.
>> True enough that it is common functionality.  However, we try to keep
>> the IIO datahandling as minimal as possible and leave the job to userspace.
>>
>> We aren't likely to be dealing with huge datarates here (as we are effectively
>> polling at maybe a few hundred Hz).   Also, with the hysterisis set sensibly
>> seems unlikely that much bounce due to noise will ever occur.  If you get
>> bounce that overcomes that then you probably have a wiring or logic problem!
> Ok, I see. I will remove it.
> 
>>>>> +    } else {
>>>>> +        hi8435_iio_push_event(idev, val);
>>>>> +    }
>>>>> +
>>>>> +err_read:
>>>>> +    iio_trigger_notify_done(idev->trig);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static void hi8435_parse_dt(struct hi8435_priv *priv)
>>>>> +{
>>>>> +    struct device_node *np = priv->spi->dev.of_node;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = of_get_named_gpio(np, "holt,reset-gpios", 0);
>>>>> +    priv->reset_gpio = ret < 0 ? 0 : ret;
>>>> Silly question, but can gpio0 exist on a board?  I suspect you
>>>> may need an additional variable to hold that this request failed
>>>> rather than using the magic value of 0.
>>> I will do handle this case, thank you.
>> Look at what Lars added on this question...
>>>>> +
>>>>> +    ret = of_property_read_u32(np, "holt,debounce-interval",
>>>>> +                   &priv->debounce_interval);
>>>>> +    if (ret)
>>>>> +        priv->debounce_interval = 0;
>>>>> +    if (priv->debounce_interval > HI8435_DEBOUNCE_DELAY_MAX)
>>>>> +        priv->debounce_interval = HI8435_DEBOUNCE_DELAY_MAX;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_probe(struct spi_device *spi)
>>>>> +{
>>>>> +    struct iio_dev *idev;
>>>>> +    struct hi8435_priv *priv;
>>>>> +    int ret;
>>>>> +
>>>>> +    idev = devm_iio_device_alloc(&spi->dev, sizeof(*priv));
>>>>> +    if (!idev)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    priv = iio_priv(idev);
>>>>> +    priv->spi = spi;
>>>>> +
>>>>> +    if (spi->dev.of_node)
>>>>> +        hi8435_parse_dt(priv);
>>>>> +
>>>>> +    spi_set_drvdata(spi, idev);
>>>>> +    mutex_init(&priv->lock);
>>>>> +    INIT_DELAYED_WORK(&priv->work, hi8435_debounce_work);
>>>>> +
>>>>> +    idev->dev.parent    = &spi->dev;
>>>>> +    idev->name        = spi_get_device_id(spi)->name;
>>>>> +    idev->modes        = INDIO_DIRECT_MODE;
>>>>> +    idev->info        = &hi8435_info;
>>>>> +    idev->channels        = hi8435_channels;
>>>>> +    idev->num_channels    = ARRAY_SIZE(hi8435_channels);
>>>>> +
>>>>> +    if (priv->reset_gpio) {
>>>>> +        ret = devm_gpio_request(&spi->dev, priv->reset_gpio, idev->name);
>>>>> +        if (!ret) {
>>>>> +            /* chip hardware reset */
>>>>> +            gpio_direction_output(priv->reset_gpio, 0);
>>>>> +            udelay(5);
>>>>> +            gpio_direction_output(priv->reset_gpio, 1);
>>>>> +        }
>>>>> +    } else {
>>>>> +        /* chip software reset */
>>>>> +        hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
>>>>> +        /* get out from reset state */
>>>>> +        hi8435_writeb(priv, HI8435_CTRL_REG, 0);
>>>>> +    }
>>>>> +
>>>>> +    /* unmask all events */
>>>>> +    priv->event_scan_mask = ~(0);
>>>>> +    /* initialize default thresholds */
>>>>> +    priv->threshold_lo[0] = priv->threshold_lo[1] = 2;
>>>>> +    priv->threshold_hi[0] = priv->threshold_hi[1] = 4;
>>>> These seem very random.  Why these values or
>>> The aim for thresholds initialization is that they are in undefined state after h/w reset and
>>> the driver allows userspace to change thresholds (high or low) separately.
>>>
>>> There is a restriction in the chip - the hysteresis can not be even.
>>> If the hysteresis is set to even value then it get's into lock state and not functional anymore. (I've figured this out empirically).
>> *nice*
>>> So we need to initialize thresholds (aka hysteresis + it's center) to some initial value to be able to change
>>> high and low thresholds separately and avoid even values for hysteresis.
>>>
>>> I will also add these comments.
>> Cool
>>>>> +    hi8435_writew(priv, HI8435_GOCENHYS_REG, 0x206);
>>>> What is 0x206?  Again, I guess a default. I'd like to see a comment
>>>> here saying what real world value it corresponds to.
>>> Yes it is default values.
>>>
>>> The 0x206 is a code for HI threshold voltage = 4V and low voltage threshold = 2V.
>>> The register HI8435_SOCENHYS_REG describes the voltage hysteresis of the sensor and the hysteresis center.
>>> The 0x206 is an encoded value for 2V-to-4V voltage threshold.
>>>
>>> I will add comments.
>> good.
>>>>> +    hi8435_writew(priv, HI8435_SOCENHYS_REG, 0x206);
>>>>> +
>>>> I am as yet unconvinced that using triggers to poll events is a terribly
>>>> good idea.  Feels rather like we are doing this due to a deficiency in
>>>> the buffer interface than because sending events makes sense here.
>>> Actually the first version of this driver was against the buffer interface.
>>> I did try to use your tip and switched to event interface :)
>> oops.
>>> Do you think I should switch to buffer interface back?
>> Nope. As discussed above, right now this approach is winning (just ;)
>>>>> +    ret = iio_triggered_event_setup(idev, NULL, hi8435_trigger_handler);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = iio_device_register(idev);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&spi->dev, "unable to register device\n");
>>>>> +        goto unregister_triggered_event;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> +unregister_triggered_event:
>>>>> +    iio_triggered_event_cleanup(idev);
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int hi8435_remove(struct spi_device *spi)
>>>>> +{
>>>>> +    struct iio_dev *idev = spi_get_drvdata(spi);
>>>>> +    struct hi8435_priv *priv = iio_priv(idev);
>>>>> +
>>>>> +    cancel_delayed_work_sync(&priv->work);
>>>>> +    iio_device_unregister(idev);
>>>>> +    iio_triggered_event_cleanup(idev);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id hi8435_dt_ids[] = {
>>>>> +    { .compatible = "holt,hi8435" },
>>>>> +    {},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, hi8435_dt_ids);
>>>>> +
>>>>> +static const struct spi_device_id hi8435_id[] = {
>>>>> +    { "hi8435", 0},
>>>>> +    { }
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(spi, hi8435_id);
>>>>> +
>>>>> +static struct spi_driver hi8435_driver = {
>>>>> +    .driver    = {
>>>>> +        .name        = DRV_NAME,
>>>>> +        .of_match_table    = of_match_ptr(hi8435_dt_ids),
>>>>> +    },
>>>>> +    .probe        = hi8435_probe,
>>>>> +    .remove        = hi8435_remove,
>>>>> +    .id_table    = hi8435_id,
>>>>> +};
>>>>> +module_spi_driver(hi8435_driver);
>>>>> +
>>>>> +MODULE_LICENSE("GPL");
>>>>> +MODULE_AUTHOR("Vladimir Barinov");
>>>>> +MODULE_DESCRIPTION("HI-8435 threshold detector");
>>>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux