[PATCH 1/4] iio: hi8435: add raw access

[PATCH 1/4] iio: hi8435: add raw access

[Nikita Yushchenko]

With current event-only driver, it is not possible for user space
application to know current senses if they don't change since
application starts.

Address that by adding raw access to channels.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/iio/adc/hi8435.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 678e8c7ea763..cb8e6342eddf 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -105,6 +105,26 @@ static int hi8435_writew(struct hi8435_priv *priv, u8 reg, u16 val)
 	return spi_write(priv->spi, priv->reg_buffer, 3);
 }
 
+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);
+	u32 tmp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
+		if (ret < 0)
+			return ret;
+		*val = !!(tmp & BIT(chan->channel));
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int hi8435_read_event_config(struct iio_dev *idev,
 				    const struct iio_chan_spec *chan,
 				    enum iio_event_type type,
@@ -333,6 +353,7 @@ static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
 	.type = IIO_VOLTAGE,				\
 	.indexed = 1,					\
 	.channel = num,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.event_spec = hi8435_events,			\
 	.num_event_specs = ARRAY_SIZE(hi8435_events),	\
 	.ext_info = hi8435_ext_info,			\
@@ -376,6 +397,7 @@ static const struct iio_chan_spec hi8435_channels[] = {
 
 static const struct iio_info hi8435_info = {
 	.driver_module = THIS_MODULE,
+	.read_raw = hi8435_read_raw,
 	.read_event_config = &hi8435_read_event_config,
 	.write_event_config = hi8435_write_event_config,
 	.read_event_value = &hi8435_read_event_value,
-- 
2.11.0

[PATCH 2/4] iio: hi8435: avoid garbage event at first enable

[Nikita Yushchenko]

Currently, driver generates events for channels if new reading differs
from previous one. This "previous value" is initialized to zero, which
results into event if value is constant-one.

Fix that by initializing "previous value" by reading at event enable
time.

This provides reliable sequence for userspace:
- enable event,
- AFTER THAT read current value,
- AFTER THAT each event will correspond to change.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/iio/adc/hi8435.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index cb8e6342eddf..45a92e3e8f2b 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -141,10 +141,21 @@ static int hi8435_write_event_config(struct iio_dev *idev,
 				     enum iio_event_direction dir, int state)
 {
 	struct hi8435_priv *priv = iio_priv(idev);
+	int ret;
+	u32 tmp;
+
+	if (state) {
+		ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
+		if (ret < 0)
+			return ret;
+		if (tmp & BIT(chan->channel))
+			priv->event_prev_val |= BIT(chan->channel);
+		else
+			priv->event_prev_val &= ~BIT(chan->channel);
 
-	priv->event_scan_mask &= ~BIT(chan->channel);
-	if (state)
 		priv->event_scan_mask |= BIT(chan->channel);
+	} else
+		priv->event_scan_mask &= ~BIT(chan->channel);
 
 	return 0;
 }
-- 
2.11.0

[PATCH 3/4] iio: hi8435: make in_voltage_sensing_mode_available visible

[Nikita Yushchenko]

Possible values of sensing_mode are encoded with strings and actual
atrings used are not obvious.

Provide a hint by enabling in_voltage_sensing_mode_available attribute.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/iio/adc/hi8435.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index 45a92e3e8f2b..d09cb6ff8044 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -356,6 +356,7 @@ static const struct iio_enum hi8435_sensing_mode = {
 
 static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
 	IIO_ENUM("sensing_mode", IIO_SEPARATE, &hi8435_sensing_mode),
+	IIO_ENUM_AVAILABLE("sensing_mode", &hi8435_sensing_mode),
 	{},
 };
 
-- 
2.11.0

[PATCH 4/4] iio: hi8435: cleanup reset gpio

[Nikita Yushchenko]

Reset GPIO is active low.

Currently driver uses gpiod_set_value(1) to clean reset, which depends
on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.

This fixes driver to use _raw version of gpiod_set_value() to enforce
active-low semantics despite of what's written in device tree. Allowing
device tree to override that only opens possibility for errors and does
not add any value.

Additionally, use _cansleep version to make things work with i2c-gpio
and other sleeping gpio drivers.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/iio/adc/hi8435.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
index d09cb6ff8044..ab59969b7c49 100644
--- a/drivers/iio/adc/hi8435.c
+++ b/drivers/iio/adc/hi8435.c
@@ -476,13 +476,15 @@ static int hi8435_probe(struct spi_device *spi)
 	priv->spi = spi;
 
 	reset_gpio = devm_gpiod_get(&spi->dev, NULL, GPIOD_OUT_LOW);
-	if (IS_ERR(reset_gpio)) {
-		/* chip s/w reset if h/w reset failed */
+	if (!IS_ERR(reset_gpio)) {
+		/* need >=100ns low pulse to reset chip */
+		gpiod_set_raw_value_cansleep(reset_gpio, 0);
+		udelay(1);
+		gpiod_set_raw_value_cansleep(reset_gpio, 1);
+	} else {
+		/* s/w reset chip if h/w reset is not available */
 		hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
 		hi8435_writeb(priv, HI8435_CTRL_REG, 0);
-	} else {
-		udelay(5);
-		gpiod_set_value(reset_gpio, 1);
 	}
 
 	spi_set_drvdata(spi, idev);
-- 
2.11.0

Re: [PATCH 1/4] iio: hi8435: add raw access

[Jonathan Cameron]

On 19/05/17 15:47, Nikita Yushchenko wrote:
> With current event-only driver, it is not possible for user space
> application to know current senses if they don't change since
> application starts.
> 
> Address that by adding raw access to channels.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Ideally I'd like Vladimir's ack on these, but I am going to guess
that he is fine with this and rely on him shouting if not ;)

Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
> ---
>   drivers/iio/adc/hi8435.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index 678e8c7ea763..cb8e6342eddf 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -105,6 +105,26 @@ static int hi8435_writew(struct hi8435_priv *priv, u8 reg, u16 val)
>   	return spi_write(priv->spi, priv->reg_buffer, 3);
>   }
>   
> +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);
> +	u32 tmp;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
> +		if (ret < 0)
> +			return ret;
> +		*val = !!(tmp & BIT(chan->channel));
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>   static int hi8435_read_event_config(struct iio_dev *idev,
>   				    const struct iio_chan_spec *chan,
>   				    enum iio_event_type type,
> @@ -333,6 +353,7 @@ static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
>   	.type = IIO_VOLTAGE,				\
>   	.indexed = 1,					\
>   	.channel = num,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>   	.event_spec = hi8435_events,			\
>   	.num_event_specs = ARRAY_SIZE(hi8435_events),	\
>   	.ext_info = hi8435_ext_info,			\
> @@ -376,6 +397,7 @@ static const struct iio_chan_spec hi8435_channels[] = {
>   
>   static const struct iio_info hi8435_info = {
>   	.driver_module = THIS_MODULE,
> +	.read_raw = hi8435_read_raw,
>   	.read_event_config = &hi8435_read_event_config,
>   	.write_event_config = hi8435_write_event_config,
>   	.read_event_value = &hi8435_read_event_value,
> 

Re: [PATCH 2/4] iio: hi8435: avoid garbage event at first enable

[Jonathan Cameron]

On 19/05/17 15:48, Nikita Yushchenko wrote:
> Currently, driver generates events for channels if new reading differs
> from previous one. This "previous value" is initialized to zero, which
> results into event if value is constant-one.
> 
> Fix that by initializing "previous value" by reading at event enable
> time.
> 
> This provides reliable sequence for userspace:
> - enable event,
> - AFTER THAT read current value,
> - AFTER THAT each event will correspond to change.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
I'm hoping there aren't any userspace apps out there relying on this
'unusual' behaviour.  *cross fingers*

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/hi8435.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index cb8e6342eddf..45a92e3e8f2b 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -141,10 +141,21 @@ static int hi8435_write_event_config(struct iio_dev *idev,
>   				     enum iio_event_direction dir, int state)
>   {
>   	struct hi8435_priv *priv = iio_priv(idev);
> +	int ret;
> +	u32 tmp;
> +
> +	if (state) {
> +		ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
> +		if (ret < 0)
> +			return ret;
> +		if (tmp & BIT(chan->channel))
> +			priv->event_prev_val |= BIT(chan->channel);
> +		else
> +			priv->event_prev_val &= ~BIT(chan->channel);
>   
> -	priv->event_scan_mask &= ~BIT(chan->channel);
> -	if (state)
>   		priv->event_scan_mask |= BIT(chan->channel);
> +	} else
> +		priv->event_scan_mask &= ~BIT(chan->channel);
>   
>   	return 0;
>   }
> 

Re: [PATCH 3/4] iio: hi8435: make in_voltage_sensing_mode_available visible

[Jonathan Cameron]

On 19/05/17 15:48, Nikita Yushchenko wrote:
> Possible values of sensing_mode are encoded with strings and actual
> atrings used are not obvious.
strings <fixed>
> 
> Provide a hint by enabling in_voltage_sensing_mode_available attribute.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
> ---
>   drivers/iio/adc/hi8435.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index 45a92e3e8f2b..d09cb6ff8044 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -356,6 +356,7 @@ static const struct iio_enum hi8435_sensing_mode = {
>   
>   static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
>   	IIO_ENUM("sensing_mode", IIO_SEPARATE, &hi8435_sensing_mode),
> +	IIO_ENUM_AVAILABLE("sensing_mode", &hi8435_sensing_mode),
>   	{},
>   };
>   
> 

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Jonathan Cameron]

On 19/05/17 15:48, Nikita Yushchenko wrote:
> Reset GPIO is active low.
> 
> Currently driver uses gpiod_set_value(1) to clean reset, which depends
> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
> 
> This fixes driver to use _raw version of gpiod_set_value() to enforce
> active-low semantics despite of what's written in device tree. Allowing
> device tree to override that only opens possibility for errors and does
> not add any value.
> 
> Additionally, use _cansleep version to make things work with i2c-gpio
> and other sleeping gpio drivers.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Hopefully we don't have any 'interesting' wiring schemes out there
that actually have a not gate on that wire.

Your argument seems sound to me so applied to the togreg branch
of iio.git and pushed out as testing for the autobuilders to play with
it.

Again, input from Vladimir would be particularly welcome!

Jonathan
> ---
>   drivers/iio/adc/hi8435.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index d09cb6ff8044..ab59969b7c49 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -476,13 +476,15 @@ static int hi8435_probe(struct spi_device *spi)
>   	priv->spi = spi;
>   
>   	reset_gpio = devm_gpiod_get(&spi->dev, NULL, GPIOD_OUT_LOW);
> -	if (IS_ERR(reset_gpio)) {
> -		/* chip s/w reset if h/w reset failed */
> +	if (!IS_ERR(reset_gpio)) {
> +		/* need >=100ns low pulse to reset chip */
> +		gpiod_set_raw_value_cansleep(reset_gpio, 0);
> +		udelay(1);
> +		gpiod_set_raw_value_cansleep(reset_gpio, 1);
> +	} else {
> +		/* s/w reset chip if h/w reset is not available */
>   		hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
>   		hi8435_writeb(priv, HI8435_CTRL_REG, 0);
> -	} else {
> -		udelay(5);
> -		gpiod_set_value(reset_gpio, 1);
>   	}
>   
>   	spi_set_drvdata(spi, idev);
> 

Re: [PATCH 1/4] iio: hi8435: add raw access

[Vladimir Barinov]

On 20.05.2017 19:33, Jonathan Cameron wrote:
> On 19/05/17 15:47, Nikita Yushchenko wrote:
>> With current event-only driver, it is not possible for user space
>> application to know current senses if they don't change since
>> application starts.
>>
>> Address that by adding raw access to channels.
>>
>> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Ideally I'd like Vladimir's ack on these, but I am going to guess
> that he is fine with this and rely on him shouting if not ;)
Acked-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>

Originally it was a part of the patch for HI8435 buffer interface but 
removed during migration to event interface:
http://marc.info/?l=linux-iio&m=143817462614547&w=4

This is a case when raw access is needed.

Re: [PATCH 2/4] iio: hi8435: avoid garbage event at first enable

[Vladimir Barinov]

Hi Nikita,

On 19.05.2017 17:48, Nikita Yushchenko wrote:
> Currently, driver generates events for channels if new reading differs
> from previous one. This "previous value" is initialized to zero, which
> results into event if value is constant-one.
>
> Fix that by initializing "previous value" by reading at event enable
> time.
>
> This provides reliable sequence for userspace:
> - enable event,
> - AFTER THAT read current value,
> - AFTER THAT each event will correspond to change.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>   drivers/iio/adc/hi8435.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index cb8e6342eddf..45a92e3e8f2b 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -141,10 +141,21 @@ static int hi8435_write_event_config(struct iio_dev *idev,
>   				     enum iio_event_direction dir, int state)
>   {
>   	struct hi8435_priv *priv = iio_priv(idev);
> +	int ret;
> +	u32 tmp;
> +
> +	if (state) {
> +		ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
> +		if (ret < 0)
> +			return ret;
> +		if (tmp & BIT(chan->channel))
> +			priv->event_prev_val |= BIT(chan->channel);
> +		else
> +			priv->event_prev_val &= ~BIT(chan->channel);
>   
> -	priv->event_scan_mask &= ~BIT(chan->channel);
> -	if (state)
>   		priv->event_scan_mask |= BIT(chan->channel);
> +	} else
> +		priv->event_scan_mask &= ~BIT(chan->channel);
>   

This will race with hi8435_iio_push_event for priv->event_scan_mask.

Since you adding raw access then it is reasonable to initialize 
priv->event_prev_val in hi8435_read_raw.
Then use the following sequence for your application:
1. read raw value
2. enable events

Regards,
Vladimir

Re: [PATCH 3/4] iio: hi8435: make in_voltage_sensing_mode_available visible

[Vladimir Barinov]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

On 19.05.2017 17:48, Nikita Yushchenko wrote:
> Possible values of sensing_mode are encoded with strings and actual
> atrings used are not obvious.
>
> Provide a hint by enabling in_voltage_sensing_mode_available attribute.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>   drivers/iio/adc/hi8435.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index 45a92e3e8f2b..d09cb6ff8044 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -356,6 +356,7 @@ static const struct iio_enum hi8435_sensing_mode = {
>   
>   static const struct iio_chan_spec_ext_info hi8435_ext_info[] = {
>   	IIO_ENUM("sensing_mode", IIO_SEPARATE, &hi8435_sensing_mode),
> +	IIO_ENUM_AVAILABLE("sensing_mode", &hi8435_sensing_mode),
>   	{},
>   };
>   

Acked-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>



[-- Attachment #2: Type: text/html, Size: 1485 bytes --]

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Vladimir Barinov]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

Hi Nikita,

On 19.05.2017 17:48, Nikita Yushchenko wrote:
> Reset GPIO is active low.
>
> Currently driver uses gpiod_set_value(1) to clean reset, which depends
> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
>
> This fixes driver to use _raw version of gpiod_set_value() to enforce
> active-low semantics despite of what's written in device tree. Allowing
> device tree to override that only opens possibility for errors and does
> not add any value.
>
> Additionally, use _cansleep version to make things work with i2c-gpio
> and other sleeping gpio drivers.
>
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>   drivers/iio/adc/hi8435.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/hi8435.c b/drivers/iio/adc/hi8435.c
> index d09cb6ff8044..ab59969b7c49 100644
> --- a/drivers/iio/adc/hi8435.c
> +++ b/drivers/iio/adc/hi8435.c
> @@ -476,13 +476,15 @@ static int hi8435_probe(struct spi_device *spi)
>   	priv->spi = spi;
>   
>   	reset_gpio = devm_gpiod_get(&spi->dev, NULL, GPIOD_OUT_LOW);
> -	if (IS_ERR(reset_gpio)) {
> -		/* chip s/w reset if h/w reset failed */
> +	if (!IS_ERR(reset_gpio)) {
> +		/* need >=100ns low pulse to reset chip */
> +		gpiod_set_raw_value_cansleep(reset_gpio, 0);
> +		udelay(1);
> +		gpiod_set_raw_value_cansleep(reset_gpio, 1);
> +	} else {
> +		/* s/w reset chip if h/w reset is not available */
>   		hi8435_writeb(priv, HI8435_CTRL_REG, HI8435_CTRL_SRST);
>   		hi8435_writeb(priv, HI8435_CTRL_REG, 0);
> -	} else {
> -		udelay(5);
> -		gpiod_set_value(reset_gpio, 1);
>   	}
>   
>   	spi_set_drvdata(spi, idev);

The reset gpio comes from platform hence it should be handled by DTS.

In driver the gpio should not be raw.

Even the hi8435 is active low but platform may invert signal (f.e. by 
adding trigger on the circuit path).


I'd suggest to update documentation 
Documentation/devicetree/bindings/iio/adc/hi8435.txt:

"

Optional properties:
  - gpios: GPIO used for controlling the reset pin (*active low*)

Example:
sensor@0 {
         compatible = "holt,hi8435";
         reg = <0>;
         gpios = <&gpio6 1 *GPIO_ACTIVE_LOW*>;
"

Regards,

Vladimir




[-- Attachment #2: Type: text/html, Size: 2905 bytes --]

Re: [PATCH 2/4] iio: hi8435: avoid garbage event at first enable

[Nikita Yushchenko]

>> +    int ret;
>> +    u32 tmp;
>> +
>> +    if (state) {
>> +        ret = hi8435_readl(priv, HI8435_SO31_0_REG, &tmp);
>> +        if (ret < 0)
>> +            return ret;
>> +        if (tmp & BIT(chan->channel))
>> +            priv->event_prev_val |= BIT(chan->channel);
>> +        else
>> +            priv->event_prev_val &= ~BIT(chan->channel);
>>   -    priv->event_scan_mask &= ~BIT(chan->channel);
>> -    if (state)
>>           priv->event_scan_mask |= BIT(chan->channel);
>> +    } else
>> +        priv->event_scan_mask &= ~BIT(chan->channel);
>>   
> 
> This will race with hi8435_iio_push_event for priv->event_scan_mask.

I was under impression that mutual-exclusion is provided by core. Now I
see it is not. Will submit a fix soon.

> Since you adding raw access then it is reasonable to initialize
> priv->event_prev_val in hi8435_read_raw.

This will cause complex interdependency between events and raw access.
E.g. should we generate event if change was discovered while processing
raw access from user space?  If we do, then we break semantics "events
generated only under trigger".  If we don't, we need complex state handling.

> Then use the following sequence for your application:
> 1. read raw value
> 2. enable events

So what if:
- one application calls read raw,
- then, far later, signal changes,
- then, far later, other application enables events?

Should second application get notification of change that happened long
before in enabled events?  I think no.

There is fundamental race between start-of-monitoring and
change-of-signal-near-start-of-monitoring. To avoid it, reading signal
must be either atomic with start of monitoring (which is not possible
with IIO API), or done after start of monitoring (which moves handling
of this race to user side).

Nikita

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Nikita Yushchenko]

>> Reset GPIO is active low.
>>
>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
>>
>> This fixes driver to use _raw version of gpiod_set_value() to enforce
>> active-low semantics despite of what's written in device tree. Allowing
>> device tree to override that only opens possibility for errors and does
>> not add any value.
>>
>> Additionally, use _cansleep version to make things work with i2c-gpio
>> and other sleeping gpio drivers.
> 
> The reset gpio comes from platform hence it should be handled by DTS.
> 
> In driver the gpio should not be raw.
> 
> Even the hi8435 is active low but platform may invert signal (f.e. by
> adding trigger on the circuit path).

I see.  However - isn't this pure theoretic?  Does such case exist?

In vast majority of cases, GPIO polarity is chip-specific, not
chip-use-specific.  Thus this knowlege belongs to driver and not to
device tree describing particular chip usage.  Having this always
defined at usage side is IMO major source of errors.

In this particular case, we already have device trees in the wild with
polarity set to active_high.  If we now change that to require
active_low, we break existing setups.  Not sure this is acceptable.

I'm unsure what is good way forward here. I believe it is at gpiolib
level, which perhaps should change how "logical value" is defined, such
that knowledge of chip-specific information is not forced to chip usage
scope.

Nikita

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Vladimir Barinov]

On 23.05.2017 11:18, Nikita Yushchenko wrote:
>>> Reset GPIO is active low.
>>>
>>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
>>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
>>>
>>> This fixes driver to use _raw version of gpiod_set_value() to enforce
>>> active-low semantics despite of what's written in device tree. Allowing
>>> device tree to override that only opens possibility for errors and does
>>> not add any value.
>>>
>>> Additionally, use _cansleep version to make things work with i2c-gpio
>>> and other sleeping gpio drivers.
>> The reset gpio comes from platform hence it should be handled by DTS.
>>
>> In driver the gpio should not be raw.
>>
>> Even the hi8435 is active low but platform may invert signal (f.e. by
>> adding trigger on the circuit path).
> I see.  However - isn't this pure theoretic?  Does such case exist?
I assure you that this is frequently used.

Simply search google for "simple voltage level shifter"
It might be on PNP or NPN transistor, hence logic might be inverted.

>
> In vast majority of cases, GPIO polarity is chip-specific, not
> chip-use-specific.  Thus this knowlege belongs to driver and not to
> device tree describing particular chip usage.  Having this always
> defined at usage side is IMO major source of errors.
GPIO comes from SoC then "circuit path" and finally chip reset input.

What do you propose if h/w circuit path has simple voltage level shifter 
on transistor. How to differentiate PNP and NPN cases?

Regards,
Vladimir

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Jonathan Cameron]

On Wed, 24 May 2017 14:27:50 +0300
Vladimir Barinov <vladimir.barinov@cogentembedded.com> wrote:

> On 23.05.2017 11:18, Nikita Yushchenko wrote:
> >>> Reset GPIO is active low.
> >>>
> >>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
> >>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
> >>>
> >>> This fixes driver to use _raw version of gpiod_set_value() to enforce
> >>> active-low semantics despite of what's written in device tree. Allowing
> >>> device tree to override that only opens possibility for errors and does
> >>> not add any value.
> >>>
> >>> Additionally, use _cansleep version to make things work with i2c-gpio
> >>> and other sleeping gpio drivers.  
> >> The reset gpio comes from platform hence it should be handled by DTS.
> >>
> >> In driver the gpio should not be raw.
> >>
> >> Even the hi8435 is active low but platform may invert signal (f.e. by
> >> adding trigger on the circuit path).  
> > I see.  However - isn't this pure theoretic?  Does such case exist?  
> I assure you that this is frequently used.
> 
> Simply search google for "simple voltage level shifter"
> It might be on PNP or NPN transistor, hence logic might be inverted.
> 
> >
> > In vast majority of cases, GPIO polarity is chip-specific, not
> > chip-use-specific.  Thus this knowlege belongs to driver and not to
> > device tree describing particular chip usage.  Having this always
> > defined at usage side is IMO major source of errors.  
> GPIO comes from SoC then "circuit path" and finally chip reset input.
> 
> What do you propose if h/w circuit path has simple voltage level shifter 
> on transistor. How to differentiate PNP and NPN cases?
> 
> Regards,
> Vladimir
> 

Hmm. Ah well, I clearly jumped too fast on this set and should have
left it for a while longer (I rushed a little as I'm away next weekend
and the cycle is moving towards rc3)

Sorry about that.

Anyhow, I am tempted to queue a revert of this patch.  The level
shifting case hadn't occurred to me (oops).

Thoughts?

I'm travelling at the end of this week, so may be the middle
of next before I can do anything about this one.

Jonathan

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Nikita Yushchenko]

>>>>> Reset GPIO is active low.
>>>>>
>>>>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
>>>>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
>>>>>
>>>>> This fixes driver to use _raw version of gpiod_set_value() to enforce
>>>>> active-low semantics despite of what's written in device tree. Allowing
>>>>> device tree to override that only opens possibility for errors and does
>>>>> not add any value.
>>>>>
>>>>> Additionally, use _cansleep version to make things work with i2c-gpio
>>>>> and other sleeping gpio drivers.  
>>>> The reset gpio comes from platform hence it should be handled by DTS.
>>>>
>>>> In driver the gpio should not be raw.
>>>>
>>>> Even the hi8435 is active low but platform may invert signal (f.e. by
>>>> adding trigger on the circuit path).  
>>> I see.  However - isn't this pure theoretic?  Does such case exist?  
>> I assure you that this is frequently used.
>>
>> Simply search google for "simple voltage level shifter"
>> It might be on PNP or NPN transistor, hence logic might be inverted.
>>
>>>
>>> In vast majority of cases, GPIO polarity is chip-specific, not
>>> chip-use-specific.  Thus this knowlege belongs to driver and not to
>>> device tree describing particular chip usage.  Having this always
>>> defined at usage side is IMO major source of errors.  
>> GPIO comes from SoC then "circuit path" and finally chip reset input.
>>
>> What do you propose if h/w circuit path has simple voltage level shifter 
>> on transistor. How to differentiate PNP and NPN cases?
> 
> Hmm. Ah well, I clearly jumped too fast on this set and should have
> left it for a while longer (I rushed a little as I'm away next weekend
> and the cycle is moving towards rc3)
> 
> Sorry about that.
> 
> Anyhow, I am tempted to queue a revert of this patch.  The level
> shifting case hadn't occurred to me (oops).
> 
> Thoughts?

Well here is the full story.

- I found that chip's reset line is active low per datasheet, but device
tree for board I work with states it is active high

- I checked driver code and found that driver depends on this incorrect
setting, it won't work if device tree will state that gpio is active low

- I could revert values in driver code AND in device tree, this way make
device tree be correct (against reality) but make dtb files flashed into
existing systems incompatible with future kernels -  which I disliked

- Thus I thought that I can remove explicit definition of polarity from
device tree (replacing it with neutrally-looking zero), and change
driver to use _raw.  I assumed that there is no real gain to let device
tree override gpio polarity for signal that is per-datasheet always
active low

- Thinking further on this, I realized that for common case signal
polarity is something defined by chip, and thus this knowledge belongs
to chip's driver and not to chip user's device tree. Moreover, device
tree writer could easily be not aware of signal polarity (too many
datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
other counterproductive approaches.

- Then Vladimir pointed real-life case with signal inversion by handmade
level shifter. Although scope of this is likely limited to hw labs,
support for this is wanted and thus some way to override polarity in
chip user's dts be available.  Still, this should be optional, without
requiring dts to always define polarity of each gpio.


It becomes obvious that this topic has global scope, it is not something
to solve within hi8345 driver or within iio.  For patch in question,
possibilities are:
- revert the patch, restore situation with driver depending on wrong
statement in dts, maybe document that in bindings,
- replace patch with code assuming that device tree has correct
definition of reset gpio polarity; break existing device trees (all are
out-of-tree as of today),
- keep the patch, thus not break anything and still stop requiring
device tree to contain wrong statement, but make entire situation
somewhat hacky and loose support for board reverting signal between gpio
provider and hi8435's pin (hopefully no such board exists).

I don't know. Maintainer should decide.

Nikita

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Jonathan Cameron]

On Thu, 25 May 2017 09:27:18 +0300
Nikita Yushchenko <nikita.yoush@cogentembedded.com> wrote:

> >>>>> Reset GPIO is active low.
> >>>>>
> >>>>> Currently driver uses gpiod_set_value(1) to clean reset, which depends
> >>>>> on device tree to contain GPIO_ACTIVE_HIGH - that does not match reality.
> >>>>>
> >>>>> This fixes driver to use _raw version of gpiod_set_value() to enforce
> >>>>> active-low semantics despite of what's written in device tree. Allowing
> >>>>> device tree to override that only opens possibility for errors and does
> >>>>> not add any value.
> >>>>>
> >>>>> Additionally, use _cansleep version to make things work with i2c-gpio
> >>>>> and other sleeping gpio drivers.    
> >>>> The reset gpio comes from platform hence it should be handled by DTS.
> >>>>
> >>>> In driver the gpio should not be raw.
> >>>>
> >>>> Even the hi8435 is active low but platform may invert signal (f.e. by
> >>>> adding trigger on the circuit path).    
> >>> I see.  However - isn't this pure theoretic?  Does such case exist?    
> >> I assure you that this is frequently used.
> >>
> >> Simply search google for "simple voltage level shifter"
> >> It might be on PNP or NPN transistor, hence logic might be inverted.
> >>  
> >>>
> >>> In vast majority of cases, GPIO polarity is chip-specific, not
> >>> chip-use-specific.  Thus this knowlege belongs to driver and not to
> >>> device tree describing particular chip usage.  Having this always
> >>> defined at usage side is IMO major source of errors.    
> >> GPIO comes from SoC then "circuit path" and finally chip reset input.
> >>
> >> What do you propose if h/w circuit path has simple voltage level shifter 
> >> on transistor. How to differentiate PNP and NPN cases?  
> > 
> > Hmm. Ah well, I clearly jumped too fast on this set and should have
> > left it for a while longer (I rushed a little as I'm away next weekend
> > and the cycle is moving towards rc3)
> > 
> > Sorry about that.
> > 
> > Anyhow, I am tempted to queue a revert of this patch.  The level
> > shifting case hadn't occurred to me (oops).
> > 
> > Thoughts?  
> 
> Well here is the full story.
> 
> - I found that chip's reset line is active low per datasheet, but device
> tree for board I work with states it is active high
> 
> - I checked driver code and found that driver depends on this incorrect
> setting, it won't work if device tree will state that gpio is active low
> 
> - I could revert values in driver code AND in device tree, this way make
> device tree be correct (against reality) but make dtb files flashed into
> existing systems incompatible with future kernels -  which I disliked
> 
> - Thus I thought that I can remove explicit definition of polarity from
> device tree (replacing it with neutrally-looking zero), and change
> driver to use _raw.  I assumed that there is no real gain to let device
> tree override gpio polarity for signal that is per-datasheet always
> active low
> 
> - Thinking further on this, I realized that for common case signal
> polarity is something defined by chip, and thus this knowledge belongs
> to chip's driver and not to chip user's device tree. Moreover, device
> tree writer could easily be not aware of signal polarity (too many
> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
> other counterproductive approaches.
> 
> - Then Vladimir pointed real-life case with signal inversion by handmade
> level shifter. Although scope of this is likely limited to hw labs,
> support for this is wanted and thus some way to override polarity in
> chip user's dts be available.  Still, this should be optional, without
> requiring dts to always define polarity of each gpio.
I have real hardware that effectively does the equivalent for interrupt
lines coming into the processor from some sensors.  Note quite the same,
but shows these things often do turn up as a fix on 'real' hardware.
> 
> 
> It becomes obvious that this topic has global scope, it is not something
> to solve within hi8345 driver or within iio.  For patch in question,
> possibilities are:
> - revert the patch, restore situation with driver depending on wrong
> statement in dts, maybe document that in bindings,
> - replace patch with code assuming that device tree has correct
> definition of reset gpio polarity; break existing device trees (all are
> out-of-tree as of today),
> - keep the patch, thus not break anything and still stop requiring
> device tree to contain wrong statement, but make entire situation
> somewhat hacky and loose support for board reverting signal between gpio
> provider and hi8435's pin (hopefully no such board exists).
> 
> I don't know. Maintainer should decide.
For now I went with a revert as the low risk option - we aren't making
the situation potentially worse as it's already broken.  It of
course would be great to rapidly come to a conclusion that has
the best of all possible worlds!

You are correct, this needs some wider guidance.  I've cc'd Linus
Walleij more to make sure he has a heads up than to suggest
we continue this conversation in this thread.

I don't know what the 'correct' way forward is.  Funnily enough
my immediate thought is to it it with a big hammer and define
an inverting gpiochip to represent the possible inverter.
So a bit like a gpiochip setting on i2c but in this case
it sits on a gpio.  So when you set the front gpio
(representing the inverter) it knows to set the opposite
on the actual gpio on the processor.

Probably overkill but would let you represent absolutely
many crazy topologies ;) Mind you might just as easily already
exist and I don't know about it (I did take a quick look
but didn't find anything like that)

Jonathan
> 
> Nikita

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Nikita Yushchenko]

>> - Thinking further on this, I realized that for common case signal
>> polarity is something defined by chip, and thus this knowledge belongs
>> to chip's driver and not to chip user's device tree. Moreover, device
>> tree writer could easily be not aware of signal polarity (too many
>> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
>> other counterproductive approaches.
>>
>> - Then Vladimir pointed real-life case with signal inversion by handmade
>> level shifter. Although scope of this is likely limited to hw labs,
>> support for this is wanted and thus some way to override polarity in
>> chip user's dts be available.  Still, this should be optional, without
>> requiring dts to always define polarity of each gpio.
>
> I have real hardware that effectively does the equivalent for interrupt
> lines coming into the processor from some sensors.  Note quite the same,
> but shows these things often do turn up as a fix on 'real' hardware.

Well no doubt that sometimes there is need to invert polarity for
particular use case.

But still forcing explicit definition of polarity of each and every gpio
usage is a bad way to solve this.

> You are correct, this needs some wider guidance.  I've cc'd Linus
> Walleij more to make sure he has a heads up than to suggest
> we continue this conversation in this thread.

As I've already written elsewhere, possible solution is a new pair of
gpio flags, GPIO_DEFAULT_POLARITY / GPIO_INVERTED_POLARITY, which means
that polarity of signal is, corresponding, chip's default or inverted
chip's default.  And matching API for drivers to set default when
requesting gpio.

> 
> I don't know what the 'correct' way forward is.  Funnily enough
> my immediate thought is to it it with a big hammer and define
> an inverting gpiochip to represent the possible inverter.
> So a bit like a gpiochip setting on i2c but in this case
> it sits on a gpio.  So when you set the front gpio
> (representing the inverter) it knows to set the opposite
> on the actual gpio on the processor.

This may be alternative to GPIO_DEFAULT_POLARITY /
GPIO_INVERTED_POLARITY, that perhaps allows for device tree to better
follow schematics.  Still this does not play well with existing
requirement to explicitly set ACTIVE_LOW / ACTIVE_HIGH at gpio usage side.


Nikita

P.S.

Another view is that for such a trivial entity as gpio, we already have
- physical gpio values,
- logical values that come from ACTIVE_LOW / ACTIVE_HIGH,
- for gpios used as interrupts, active low / active high irq's,
and discussing addition more to that... for simplification? Joking?

At least, if adding default-polarity / inverted-polarity, this should be
done mutually-exclusive with active-high / active-low.  And if adding
inverter gpiochip, then also this should be mutual exclusive with
setting active-high / active-low.

Re: [PATCH 4/4] iio: hi8435: cleanup reset gpio

[Linus Walleij]

On Thu, May 25, 2017 at 8:27 AM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:

> - I found that chip's reset line is active low per datasheet, but device
> tree for board I work with states it is active high

There you have it, there is a bug in the device tree, because it is
not properly describing the hardware.

> - I checked driver code and found that driver depends on this incorrect
> setting, it won't work if device tree will state that gpio is active low

Yeah...

> - I could revert values in driver code AND in device tree, this way make
> device tree be correct (against reality) but make dtb files flashed into
> existing systems incompatible with future kernels -  which I disliked

Is this one of those systems where people actually flash a DTB and
never change it?

If not, then just patch the DTS source and stop worrying already.
We don't do backwards compatibility just because it's fun.

If it is really deployed and really not seeing updates...

In that case I want a way to check the signature (such as a checksum)
of the DTB and apply a workaround for elder (incorrect) device trees
when detected, and let newer (corrected) device trees describe the
hardware properly.

> - Thus I thought that I can remove explicit definition of polarity from
> device tree (replacing it with neutrally-looking zero), and change
> driver to use _raw.  I assumed that there is no real gain to let device
> tree override gpio polarity for signal that is per-datasheet always
> active low
>
> - Thinking further on this, I realized that for common case signal
> polarity is something defined by chip, and thus this knowledge belongs
> to chip's driver and not to chip user's device tree.

That is a fallacy IMO.

Just like we define regulators for all voltage inputs on a chip and
set the voltage constraints accordingly in the device tree, so we
set the polarity of signals in the device tree.

GPIOs, like regulators, are defined in terms of the consumer
properties. Else having a signal iversion flag (*ACTIVE_LOW) would
not make sense.

> Moreover, device
> tree writer could easily be not aware of signal polarity (too many
> datasheets are NDA-closed), thus hello copy-pasting, try-and-check and
> other counterproductive approaches.

Mistakes will be made but seriously, device tree writers know as much as
driver writers do in my experience. If it is unknown they will look
at some hint from the rail, like it being named RESETN on the schematic
indicating negative (active low) polarity.

> - revert the patch, restore situation with driver depending on wrong
> statement in dts, maybe document that in bindings,
> - replace patch with code assuming that device tree has correct
> definition of reset gpio polarity; break existing device trees (all are
> out-of-tree as of today),
> - keep the patch, thus not break anything and still stop requiring
> device tree to contain wrong statement, but make entire situation
> somewhat hacky and loose support for board reverting signal between gpio
> provider and hi8435's pin (hopefully no such board exists).

I would fix the device tree and the driver to handle it correctly.

Signal active low in the device tree.

Assert with high level in the code.

Then, if backward compatibility for older device trees is needed,
find a way to detect elder device trees and then alter the behaviour
with an extra inversion on those.

Preferably by altering the device tree in memory actually, but that may be
hard. (Code for this exists.)

Yours,
Linus Walleij