* [PATCH 1/5] iio: Use .realbits to extend a small signed integer
2021-10-30 11:18 [PATCH 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
@ 2021-10-30 11:18 ` Gwendal Grignou
2021-10-30 11:35 ` Lars-Peter Clausen
2021-10-30 11:18 ` [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Gwendal Grignou @ 2021-10-30 11:18 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
When calling sign_extend32() on a channel, use its .realbit information
to limit the number of bits to process, instead of a constant.
Changed only simple sign_extend32() calls, when .realbits was defined in
the same file. Use 'grep -r sign_extend32 $(grep -lr realbits drivers/iio/)'
to locate the files.
Some files were not processed:
gyro/fxas21002c_core.c : function parameter changes needed.
health/max30102.c: Incomplete channel definition.
health/max30100.c
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/iio/accel/bma220_spi.c | 3 ++-
drivers/iio/accel/kxcjk-1013.c | 3 ++-
drivers/iio/accel/mma7455_core.c | 3 ++-
drivers/iio/accel/stk8312.c | 2 +-
drivers/iio/accel/stk8ba50.c | 3 ++-
drivers/iio/adc/ad7266.c | 3 ++-
drivers/iio/adc/at91-sama5d2_adc.c | 3 ++-
drivers/iio/adc/ti-adc12138.c | 3 ++-
drivers/iio/adc/ti-ads1015.c | 8 +++-----
drivers/iio/adc/xilinx-xadc-core.c | 2 +-
drivers/iio/magnetometer/mag3110.c | 6 ++++--
drivers/iio/pressure/mpl3115.c | 7 ++++---
drivers/iio/proximity/sx9310.c | 3 +--
13 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index bc4c626e454d3..812d6749b24a7 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -125,7 +125,8 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
ret = bma220_read_reg(data->spi_device, chan->address);
if (ret < 0)
return -EINVAL;
- *val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
+ *val = sign_extend32(ret >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index a51fdd3c9b5b5..88cf0c276893a 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -927,7 +927,8 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->mutex);
return ret;
}
- *val = sign_extend32(ret >> 4, 11);
+ *val = sign_extend32(ret >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
ret = kxcjk1013_set_power_state(data, false);
}
mutex_unlock(&data->mutex);
diff --git a/drivers/iio/accel/mma7455_core.c b/drivers/iio/accel/mma7455_core.c
index 777c6c384b09e..e6739ba74edfa 100644
--- a/drivers/iio/accel/mma7455_core.c
+++ b/drivers/iio/accel/mma7455_core.c
@@ -134,7 +134,8 @@ static int mma7455_read_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
- *val = sign_extend32(le16_to_cpu(data), 9);
+ *val = sign_extend32(le16_to_cpu(data),
+ chan->scan_type.realbits - 1);
return IIO_VAL_INT;
diff --git a/drivers/iio/accel/stk8312.c b/drivers/iio/accel/stk8312.c
index 43c621d0f11e4..de0cdf8c1f94c 100644
--- a/drivers/iio/accel/stk8312.c
+++ b/drivers/iio/accel/stk8312.c
@@ -355,7 +355,7 @@ static int stk8312_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->lock);
return ret;
}
- *val = sign_extend32(ret, 7);
+ *val = sign_extend32(ret, chan->scan_type.realbits - 1);
ret = stk8312_set_mode(data,
data->mode & (~STK8312_MODE_ACTIVE));
mutex_unlock(&data->lock);
diff --git a/drivers/iio/accel/stk8ba50.c b/drivers/iio/accel/stk8ba50.c
index e137a34b5c9a9..517c57ed9e949 100644
--- a/drivers/iio/accel/stk8ba50.c
+++ b/drivers/iio/accel/stk8ba50.c
@@ -227,7 +227,8 @@ static int stk8ba50_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->lock);
return -EINVAL;
}
- *val = sign_extend32(ret >> STK8BA50_DATA_SHIFT, 9);
+ *val = sign_extend32(ret >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
stk8ba50_set_power(data, STK8BA50_MODE_SUSPEND);
mutex_unlock(&data->lock);
return IIO_VAL_INT;
diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index a8ec3efd659ed..1d345d66742d8 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -159,7 +159,8 @@ static int ad7266_read_raw(struct iio_dev *indio_dev,
*val = (*val >> 2) & 0xfff;
if (chan->scan_type.sign == 's')
- *val = sign_extend32(*val, 11);
+ *val = sign_extend32(*val,
+ chan->scan_type.realbits - 1);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4c922ef634f8e..92a57cf10fba4 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1586,7 +1586,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
*val = st->conversion_value;
ret = at91_adc_adjust_val_osr(st, val);
if (chan->scan_type.sign == 's')
- *val = sign_extend32(*val, 11);
+ *val = sign_extend32(*val,
+ chan->scan_type.realbits - 1);
st->conversion_done = false;
}
diff --git a/drivers/iio/adc/ti-adc12138.c b/drivers/iio/adc/ti-adc12138.c
index fcd5d39dd03ea..5b5d452105393 100644
--- a/drivers/iio/adc/ti-adc12138.c
+++ b/drivers/iio/adc/ti-adc12138.c
@@ -239,7 +239,8 @@ static int adc12138_read_raw(struct iio_dev *iio,
if (ret)
return ret;
- *value = sign_extend32(be16_to_cpu(data) >> 3, 12);
+ *value = sign_extend32(be16_to_cpu(data) >> channel->scan_type.shift,
+ channel->scan_type.realbits - 1);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index b0352e91ac165..b92d4cd1b8238 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -464,9 +464,7 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
mutex_lock(&data->lock);
switch (mask) {
- case IIO_CHAN_INFO_RAW: {
- int shift = chan->scan_type.shift;
-
+ case IIO_CHAN_INFO_RAW:
ret = iio_device_claim_direct_mode(indio_dev);
if (ret)
break;
@@ -487,7 +485,8 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
goto release_direct;
}
- *val = sign_extend32(*val >> shift, 15 - shift);
+ *val = sign_extend32(*val >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
ret = ads1015_set_power_state(data, false);
if (ret < 0)
@@ -497,7 +496,6 @@ static int ads1015_read_raw(struct iio_dev *indio_dev,
release_direct:
iio_device_release_direct_mode(indio_dev);
break;
- }
case IIO_CHAN_INFO_SCALE:
idx = data->channel_data[chan->address].pga;
*val = ads1015_fullscale_range[idx];
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 83bea5ef765da..05050709e4f8a 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -943,7 +943,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
*val = 1000;
break;
}
- *val2 = chan->scan_type.realbits;
+ *val2 = bits;
return IIO_VAL_FRACTIONAL_LOG2;
case IIO_TEMP:
/* Temp in C = (val * 503.975) / 2**bits - 273.15 */
diff --git a/drivers/iio/magnetometer/mag3110.c b/drivers/iio/magnetometer/mag3110.c
index c96415a1aeadd..17c62d806218d 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -291,7 +291,8 @@ static int mag3110_read_raw(struct iio_dev *indio_dev,
if (ret < 0)
goto release;
*val = sign_extend32(
- be16_to_cpu(buffer[chan->scan_index]), 15);
+ be16_to_cpu(buffer[chan->scan_index]),
+ chan->scan_type.realbits - 1);
ret = IIO_VAL_INT;
break;
case IIO_TEMP: /* in 1 C / LSB */
@@ -306,7 +307,8 @@ static int mag3110_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->lock);
if (ret < 0)
goto release;
- *val = sign_extend32(ret, 7);
+ *val = sign_extend32(ret,
+ chan->scan_type.realbits - 1);
ret = IIO_VAL_INT;
break;
default:
diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 1eb9e7b29e050..355854f0f59d2 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -74,7 +74,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct mpl3115_data *data = iio_priv(indio_dev);
- __be32 tmp = 0;
+ __be16 tmp = 0;
int ret;
switch (mask) {
@@ -96,7 +96,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->lock);
if (ret < 0)
break;
- *val = be32_to_cpu(tmp) >> 12;
+ *val = be32_to_cpu(tmp) >> chan->scan_type.shift;
ret = IIO_VAL_INT;
break;
case IIO_TEMP: /* in 0.0625 celsius / LSB */
@@ -111,7 +111,8 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
mutex_unlock(&data->lock);
if (ret < 0)
break;
- *val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
+ *val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
+ chan->scan_type.realbits - 1);
ret = IIO_VAL_INT;
break;
default:
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index a3fdb59b06d22..31d060c1b0103 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -443,8 +443,7 @@ static int sx9310_read_proximity(struct sx9310_data *data,
if (ret)
goto out_disable_irq;
- *val = sign_extend32(be16_to_cpu(rawval),
- chan->address == SX9310_REG_DIFF_MSB ? 11 : 15);
+ *val = sign_extend32(be16_to_cpu(rawval), chan->scan_type.realbits - 1);
ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
if (ret)
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio: Use .realbits to extend a small signed integer
2021-10-30 11:18 ` [PATCH 1/5] iio: Use .realbits to extend a small signed integer Gwendal Grignou
@ 2021-10-30 11:35 ` Lars-Peter Clausen
2021-10-30 16:09 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Lars-Peter Clausen @ 2021-10-30 11:35 UTC (permalink / raw)
To: Gwendal Grignou, jic23, swboyd; +Cc: andy.shevchenko, linux-iio
On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> When calling sign_extend32() on a channel, use its .realbit information
> to limit the number of bits to process, instead of a constant.
>
> Changed only simple sign_extend32() calls, when .realbits was defined in
> the same file. Use 'grep -r sign_extend32 $(grep -lr realbits drivers/iio/)'
> to locate the files.
>
> Some files were not processed:
> gyro/fxas21002c_core.c : function parameter changes needed.
> health/max30102.c: Incomplete channel definition.
> health/max30100.c
I think this is good work, but it seems a bit out of place in this
series. I think it will be easier to get this reviewed and merged if it
is submitted independently. It might make sense to only have the sx9310
changes as part of this series and send the other ones as a separate patch.
What's also missing in the commit description is the motivation for
this. The generated code will be a bit more complex, so there needs to
be a good justification. E.g. having a single source of truth for this
data and avoiding accidental mistakes.
The patch also uses `shift` were applicable, which is not mentioned in
the commit dscription.
> [...]
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 1eb9e7b29e050..355854f0f59d2 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -74,7 +74,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct mpl3115_data *data = iio_priv(indio_dev);
> - __be32 tmp = 0;
> + __be16 tmp = 0;
> int ret;
The be32 to be16 change might warrant its own patch. This is definitely
changing the behavior of the driver. And I don't think it is correct the
way its done. For the pressure data it is reading 3 bytes, which will
cause a stack overflow.
>
> switch (mask) {
> @@ -96,7 +96,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> mutex_unlock(&data->lock);
> if (ret < 0)
> break;
> - *val = be32_to_cpu(tmp) >> 12;
> + *val = be32_to_cpu(tmp) >> chan->scan_type.shift;
> ret = IIO_VAL_INT;
> break;
> case IIO_TEMP: /* in 0.0625 celsius / LSB */
> @@ -111,7 +111,8 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> mutex_unlock(&data->lock);
> if (ret < 0)
> break;
> - *val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
> + *val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
> + chan->scan_type.realbits - 1);
> ret = IIO_VAL_INT;
> break;
> default:
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio: Use .realbits to extend a small signed integer
2021-10-30 11:35 ` Lars-Peter Clausen
@ 2021-10-30 16:09 ` Jonathan Cameron
2021-11-01 7:30 ` Gwendal Grignou
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2021-10-30 16:09 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: Gwendal Grignou, swboyd, andy.shevchenko, linux-iio
On Sat, 30 Oct 2021 13:35:19 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> > When calling sign_extend32() on a channel, use its .realbit information
> > to limit the number of bits to process, instead of a constant.
> >
> > Changed only simple sign_extend32() calls, when .realbits was defined in
> > the same file. Use 'grep -r sign_extend32 $(grep -lr realbits drivers/iio/)'
> > to locate the files.
> >
> > Some files were not processed:
> > gyro/fxas21002c_core.c : function parameter changes needed.
> > health/max30102.c: Incomplete channel definition.
> > health/max30100.c
>
> I think this is good work, but it seems a bit out of place in this
> series. I think it will be easier to get this reviewed and merged if it
> is submitted independently. It might make sense to only have the sx9310
> changes as part of this series and send the other ones as a separate patch.
>
> What's also missing in the commit description is the motivation for
> this. The generated code will be a bit more complex, so there needs to
> be a good justification. E.g. having a single source of truth for this
> data and avoiding accidental mistakes.
>
> The patch also uses `shift` were applicable, which is not mentioned in
> the commit dscription.
Be careful. I have seen devices (with FIFOs) where the realbits doesn't
necessarily match with a separate read path used for polled reads.
It is an option for the sca3000 for example but that's carrying a hack where
ignore that and rely on some coincidental data alignment to pretend realbits
is 13 when it's actually 11.
Still in general it's a reasonable change but agree with Lars, separate series
please.
>
>
> > [...]
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 1eb9e7b29e050..355854f0f59d2 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -74,7 +74,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > int *val, int *val2, long mask)
> > {
> > struct mpl3115_data *data = iio_priv(indio_dev);
> > - __be32 tmp = 0;
> > + __be16 tmp = 0;
> > int ret;
> The be32 to be16 change might warrant its own patch. This is definitely
> changing the behavior of the driver. And I don't think it is correct the
> way its done. For the pressure data it is reading 3 bytes, which will
> cause a stack overflow.
> >
> > switch (mask) {
> > @@ -96,7 +96,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > mutex_unlock(&data->lock);
> > if (ret < 0)
> > break;
> > - *val = be32_to_cpu(tmp) >> 12;
> > + *val = be32_to_cpu(tmp) >> chan->scan_type.shift;
> > ret = IIO_VAL_INT;
> > break;
> > case IIO_TEMP: /* in 0.0625 celsius / LSB */
> > @@ -111,7 +111,8 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > mutex_unlock(&data->lock);
> > if (ret < 0)
> > break;
> > - *val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
> > + *val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
> > + chan->scan_type.realbits - 1);
> > ret = IIO_VAL_INT;
> > break;
> > default:
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/5] iio: Use .realbits to extend a small signed integer
2021-10-30 16:09 ` Jonathan Cameron
@ 2021-11-01 7:30 ` Gwendal Grignou
0 siblings, 0 replies; 18+ messages in thread
From: Gwendal Grignou @ 2021-11-01 7:30 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Lars-Peter Clausen, swboyd, andy.shevchenko, linux-iio
On Sat, Oct 30, 2021 at 9:04 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 Oct 2021 13:35:19 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> > On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> > > When calling sign_extend32() on a channel, use its .realbit information
> > > to limit the number of bits to process, instead of a constant.
> > >
> > > Changed only simple sign_extend32() calls, when .realbits was defined in
> > > the same file. Use 'grep -r sign_extend32 $(grep -lr realbits drivers/iio/)'
> > > to locate the files.
> > >
> > > Some files were not processed:
> > > gyro/fxas21002c_core.c : function parameter changes needed.
> > > health/max30102.c: Incomplete channel definition.
> > > health/max30100.c
> >
> > I think this is good work, but it seems a bit out of place in this
> > series. I think it will be easier to get this reviewed and merged if it
> > is submitted independently. It might make sense to only have the sx9310
> > changes as part of this series and send the other ones as a separate patch.
When moving code to sx9310, I notice we were using constants instead
of realbits, and I look at how other driver deal with bit information.
> >
> > What's also missing in the commit description is the motivation for
> > this. The generated code will be a bit more complex, so there needs to
> > be a good justification. E.g. having a single source of truth for this
> > data and avoiding accidental mistakes.
That's the idea, I will update the commit accordingly.
> >
> > The patch also uses `shift` were applicable, which is not mentioned in
> > the commit dscription.
>
> Be careful. I have seen devices (with FIFOs) where the realbits doesn't
> necessarily match with a separate read path used for polled reads.
>
> It is an option for the sca3000 for example but that's carrying a hack where
> ignore that and rely on some coincidental data alignment to pretend realbits
> is 13 when it's actually 11.
I see the hack in sca3000_ring_int_process(). The driver reconciles
the FIFO and raw data access by presenting elements as carrying 13
bits of information, assuming the 2 LSB are 0.
Given user space should get the same data when reading using the _raw
attributes or the buffer, given libiio use scan_type information to
process buffer data, (see libiio iio_channel_convert())
it makes sense for the kernel to always use scan_type as well when
presenting the _raw attributes.
>
> Still in general it's a reasonable change but agree with Lars, separate series
> please.
Will do.
>
> >
> >
> > > [...]
> > > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > > index 1eb9e7b29e050..355854f0f59d2 100644
> > > --- a/drivers/iio/pressure/mpl3115.c
> > > +++ b/drivers/iio/pressure/mpl3115.c
> > > @@ -74,7 +74,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > int *val, int *val2, long mask)
> > > {
> > > struct mpl3115_data *data = iio_priv(indio_dev);
> > > - __be32 tmp = 0;
> > > + __be16 tmp = 0;
> > > int ret;
> > The be32 to be16 change might warrant its own patch. This is definitely
> > changing the behavior of the driver. And I don't think it is correct the
> > way its done. For the pressure data it is reading 3 bytes, which will
> > cause a stack overflow.
You're right, it is wrong as is. Resent in a separate patch.
> > >
> > > switch (mask) {
> > > @@ -96,7 +96,7 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > mutex_unlock(&data->lock);
> > > if (ret < 0)
> > > break;
> > > - *val = be32_to_cpu(tmp) >> 12;
> > > + *val = be32_to_cpu(tmp) >> chan->scan_type.shift;
> > > ret = IIO_VAL_INT;
> > > break;
> > > case IIO_TEMP: /* in 0.0625 celsius / LSB */
> > > @@ -111,7 +111,8 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
> > > mutex_unlock(&data->lock);
> > > if (ret < 0)
> > > break;
> > > - *val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
> > > + *val = sign_extend32(be16_to_cpu(tmp) >> chan->scan_type.shift,
> > > + chan->scan_type.realbits - 1);
> > > ret = IIO_VAL_INT;
> > > break;
> > > default:
> >
>
Thanks for the prompt reviews and the feedback,
Gwendal.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-10-30 11:18 [PATCH 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
2021-10-30 11:18 ` [PATCH 1/5] iio: Use .realbits to extend a small signed integer Gwendal Grignou
@ 2021-10-30 11:18 ` Gwendal Grignou
2021-10-30 11:40 ` Lars-Peter Clausen
2021-10-30 16:42 ` Jonathan Cameron
2021-10-30 11:18 ` [PATCH 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
` (2 subsequent siblings)
4 siblings, 2 replies; 18+ messages in thread
From: Gwendal Grignou @ 2021-10-30 11:18 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Before adding new Semtech sensors, move common logic to all Semtech SAR
sensor in its own file:
- interface with IIO subsystem,
- interrupt management,
- channel access conrol,
- event processing.
The change adds a bidirectional interface between sx93xx and sx_common.
The change is quite mechanical, as the impacted functions are moved
and renamed.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/iio/proximity/sx9310.c | 629 +++---------------------
drivers/iio/proximity/sx_common.c | 618 +++++++++++++++++++++++
include/linux/iio/proximity/sx_common.h | 129 +++++
3 files changed, 803 insertions(+), 573 deletions(-)
create mode 100644 drivers/iio/proximity/sx_common.c
create mode 100644 include/linux/iio/proximity/sx_common.h
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 31d060c1b0103..577a16789f8b2 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -22,19 +22,16 @@
#include <linux/pm.h>
#include <linux/property.h>
#include <linux/regmap.h>
-#include <linux/regulator/consumer.h>
#include <linux/slab.h>
-#include <linux/iio/buffer.h>
#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
-#include <linux/iio/trigger.h>
-#include <linux/iio/triggered_buffer.h>
-#include <linux/iio/trigger_consumer.h>
+
+#include <linux/iio/proximity/sx_common.h>
/* Register definitions. */
-#define SX9310_REG_IRQ_SRC 0x00
+#define SX9310_REG_IRQ_SRC SX_COMMON_REG_IRQ_SRC
#define SX9310_REG_STAT0 0x01
#define SX9310_REG_STAT1 0x02
#define SX9310_REG_STAT1_COMPSTAT_MASK GENMASK(3, 0)
@@ -132,61 +129,12 @@
#define SX9310_REG_I2C_ADDR 0x40
#define SX9310_REG_PAUSE 0x41
#define SX9310_REG_WHOAMI 0x42
-#define SX9310_WHOAMI_VALUE 0x01
-#define SX9311_WHOAMI_VALUE 0x02
#define SX9310_REG_RESET 0x7f
-#define SX9310_SOFT_RESET 0xde
/* 4 hardware channels, as defined in STAT0: COMB, CS2, CS1 and CS0. */
#define SX9310_NUM_CHANNELS 4
-static_assert(SX9310_NUM_CHANNELS < BITS_PER_LONG);
-
-struct sx9310_data {
- /* Serialize access to registers and channel configuration */
- struct mutex mutex;
- struct i2c_client *client;
- struct iio_trigger *trig;
- struct regmap *regmap;
- struct regulator_bulk_data supplies[2];
- /*
- * Last reading of the proximity status for each channel.
- * We only send an event to user space when this changes.
- */
- unsigned long chan_prox_stat;
- bool trigger_enabled;
- /* Ensure correct alignment of timestamp when present. */
- struct {
- __be16 channels[SX9310_NUM_CHANNELS];
- s64 ts __aligned(8);
- } buffer;
- /* Remember enabled channels and sample rate during suspend. */
- unsigned int suspend_ctrl0;
- struct completion completion;
- unsigned long chan_read;
- unsigned long chan_event;
- unsigned int whoami;
-};
-
-static const struct iio_event_spec sx9310_events[] = {
- {
- .type = IIO_EV_TYPE_THRESH,
- .dir = IIO_EV_DIR_RISING,
- .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
- },
- {
- .type = IIO_EV_TYPE_THRESH,
- .dir = IIO_EV_DIR_FALLING,
- .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
- },
- {
- .type = IIO_EV_TYPE_THRESH,
- .dir = IIO_EV_DIR_EITHER,
- .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
- BIT(IIO_EV_INFO_HYSTERESIS) |
- BIT(IIO_EV_INFO_VALUE),
- },
-};
+static_assert(SX9310_NUM_CHANNELS <= SX_COMMON_MAX_NUM_CHANNELS);
#define SX9310_NAMED_CHANNEL(idx, name) \
{ \
@@ -200,8 +148,8 @@ static const struct iio_event_spec sx9310_events[] = {
.channel = idx, \
.extend_name = name, \
.address = SX9310_REG_DIFF_MSB, \
- .event_spec = sx9310_events, \
- .num_event_specs = ARRAY_SIZE(sx9310_events), \
+ .event_spec = sx_common_events, \
+ .num_event_specs = ARRAY_SIZE(sx_common_events), \
.scan_index = idx, \
.scan_type = { \
.sign = 's', \
@@ -320,64 +268,7 @@ static const struct regmap_config sx9310_regmap_config = {
.volatile_table = &sx9310_volatile_regs,
};
-static int sx9310_update_chan_en(struct sx9310_data *data,
- unsigned long chan_read,
- unsigned long chan_event)
-{
- int ret;
- unsigned long channels = chan_read | chan_event;
-
- if ((data->chan_read | data->chan_event) != channels) {
- ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
- SX9310_REG_PROX_CTRL0_SENSOREN_MASK,
- channels);
- if (ret)
- return ret;
- }
- data->chan_read = chan_read;
- data->chan_event = chan_event;
- return 0;
-}
-
-static int sx9310_get_read_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read | BIT(channel),
- data->chan_event);
-}
-
-static int sx9310_put_read_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read & ~BIT(channel),
- data->chan_event);
-}
-
-static int sx9310_get_event_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read,
- data->chan_event | BIT(channel));
-}
-
-static int sx9310_put_event_channel(struct sx9310_data *data, int channel)
-{
- return sx9310_update_chan_en(data, data->chan_read,
- data->chan_event & ~BIT(channel));
-}
-
-static int sx9310_enable_irq(struct sx9310_data *data, unsigned int irq)
-{
- if (!data->client->irq)
- return 0;
- return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, irq);
-}
-
-static int sx9310_disable_irq(struct sx9310_data *data, unsigned int irq)
-{
- if (!data->client->irq)
- return 0;
- return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, 0);
-}
-
-static int sx9310_read_prox_data(struct sx9310_data *data,
+static int sx9310_read_prox_data(struct sx_common_data *data,
const struct iio_chan_spec *chan, __be16 *val)
{
int ret;
@@ -393,7 +284,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
* If we have no interrupt support, we have to wait for a scan period
* after enabling a channel to get a result.
*/
-static int sx9310_wait_for_sample(struct sx9310_data *data)
+static int sx9310_wait_for_sample(struct sx_common_data *data)
{
int ret;
unsigned int val;
@@ -409,65 +300,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
return 0;
}
-static int sx9310_read_proximity(struct sx9310_data *data,
- const struct iio_chan_spec *chan, int *val)
-{
- int ret;
- __be16 rawval;
-
- mutex_lock(&data->mutex);
-
- ret = sx9310_get_read_channel(data, chan->channel);
- if (ret)
- goto out;
-
- ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out_put_channel;
-
- mutex_unlock(&data->mutex);
-
- if (data->client->irq) {
- ret = wait_for_completion_interruptible(&data->completion);
- reinit_completion(&data->completion);
- } else {
- ret = sx9310_wait_for_sample(data);
- }
-
- mutex_lock(&data->mutex);
-
- if (ret)
- goto out_disable_irq;
-
- ret = sx9310_read_prox_data(data, chan, &rawval);
- if (ret)
- goto out_disable_irq;
-
- *val = sign_extend32(be16_to_cpu(rawval), chan->scan_type.realbits - 1);
-
- ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out_put_channel;
-
- ret = sx9310_put_read_channel(data, chan->channel);
- if (ret)
- goto out;
-
- mutex_unlock(&data->mutex);
-
- return IIO_VAL_INT;
-
-out_disable_irq:
- sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
-out_put_channel:
- sx9310_put_read_channel(data, chan->channel);
-out:
- mutex_unlock(&data->mutex);
-
- return ret;
-}
-
-static int sx9310_read_gain(struct sx9310_data *data,
+static int sx9310_read_gain(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int regval, gain;
@@ -495,7 +328,7 @@ static int sx9310_read_gain(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
+static int sx9310_read_samp_freq(struct sx_common_data *data, int *val, int *val2)
{
unsigned int regval;
int ret;
@@ -515,7 +348,7 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, int *val,
int *val2, long mask)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
if (chan->type != IIO_PROXIMITY)
@@ -527,7 +360,7 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
- ret = sx9310_read_proximity(data, chan, val);
+ ret = sx_common_read_proximity(data, chan, val);
iio_device_release_direct_mode(indio_dev);
return ret;
case IIO_CHAN_INFO_HARDWAREGAIN:
@@ -545,27 +378,6 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
}
}
-static const int sx9310_gain_vals[] = { 1, 2, 4, 8 };
-
-static int sx9310_read_avail(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- const int **vals, int *type, int *length,
- long mask)
-{
- if (chan->type != IIO_PROXIMITY)
- return -EINVAL;
-
- switch (mask) {
- case IIO_CHAN_INFO_HARDWAREGAIN:
- *type = IIO_VAL_INT;
- *length = ARRAY_SIZE(sx9310_gain_vals);
- *vals = sx9310_gain_vals;
- return IIO_AVAIL_LIST;
- }
-
- return -EINVAL;
-}
-
static const unsigned int sx9310_pthresh_codes[] = {
2, 4, 6, 8, 12, 16, 20, 24, 28, 32, 40, 48, 56, 64, 72, 80, 88, 96, 112,
128, 144, 160, 192, 224, 256, 320, 384, 512, 640, 768, 1024, 1536
@@ -585,7 +397,7 @@ static int sx9310_get_thresh_reg(unsigned int channel)
return -EINVAL;
}
-static int sx9310_read_thresh(struct sx9310_data *data,
+static int sx9310_read_thresh(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int reg;
@@ -608,7 +420,7 @@ static int sx9310_read_thresh(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_hysteresis(struct sx9310_data *data,
+static int sx9310_read_hysteresis(struct sx_common_data *data,
const struct iio_chan_spec *chan, int *val)
{
unsigned int regval, pthresh;
@@ -632,7 +444,7 @@ static int sx9310_read_hysteresis(struct sx9310_data *data,
return IIO_VAL_INT;
}
-static int sx9310_read_far_debounce(struct sx9310_data *data, int *val)
+static int sx9310_read_far_debounce(struct sx_common_data *data, int *val)
{
unsigned int regval;
int ret;
@@ -650,7 +462,7 @@ static int sx9310_read_far_debounce(struct sx9310_data *data, int *val)
return IIO_VAL_INT;
}
-static int sx9310_read_close_debounce(struct sx9310_data *data, int *val)
+static int sx9310_read_close_debounce(struct sx_common_data *data, int *val)
{
unsigned int regval;
int ret;
@@ -674,7 +486,7 @@ static int sx9310_read_event_val(struct iio_dev *indio_dev,
enum iio_event_direction dir,
enum iio_event_info info, int *val, int *val2)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -698,7 +510,7 @@ static int sx9310_read_event_val(struct iio_dev *indio_dev,
}
}
-static int sx9310_write_thresh(struct sx9310_data *data,
+static int sx9310_write_thresh(struct sx_common_data *data,
const struct iio_chan_spec *chan, int val)
{
unsigned int reg;
@@ -728,7 +540,7 @@ static int sx9310_write_thresh(struct sx9310_data *data,
return ret;
}
-static int sx9310_write_hysteresis(struct sx9310_data *data,
+static int sx9310_write_hysteresis(struct sx_common_data *data,
const struct iio_chan_spec *chan, int _val)
{
unsigned int hyst, val = _val;
@@ -758,7 +570,7 @@ static int sx9310_write_hysteresis(struct sx9310_data *data,
return ret;
}
-static int sx9310_write_far_debounce(struct sx9310_data *data, int val)
+static int sx9310_write_far_debounce(struct sx_common_data *data, int val)
{
int ret;
unsigned int regval;
@@ -779,7 +591,7 @@ static int sx9310_write_far_debounce(struct sx9310_data *data, int val)
return ret;
}
-static int sx9310_write_close_debounce(struct sx9310_data *data, int val)
+static int sx9310_write_close_debounce(struct sx_common_data *data, int val)
{
int ret;
unsigned int regval;
@@ -806,7 +618,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
enum iio_event_direction dir,
enum iio_event_info info, int val, int val2)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -830,7 +642,7 @@ static int sx9310_write_event_val(struct iio_dev *indio_dev,
}
}
-static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
+static int sx9310_set_samp_freq(struct sx_common_data *data, int val, int val2)
{
int i, ret;
@@ -854,8 +666,8 @@ static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
return ret;
}
-static int sx9310_write_gain(struct sx9310_data *data,
- const struct iio_chan_spec *chan, int val)
+static int sx9310_write_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int val)
{
unsigned int gain, mask;
int ret;
@@ -889,7 +701,7 @@ static int sx9310_write_raw(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, int val, int val2,
long mask)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
@@ -904,132 +716,6 @@ static int sx9310_write_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
-static irqreturn_t sx9310_irq_handler(int irq, void *private)
-{
- struct iio_dev *indio_dev = private;
- struct sx9310_data *data = iio_priv(indio_dev);
-
- if (data->trigger_enabled)
- iio_trigger_poll(data->trig);
-
- /*
- * Even if no event is enabled, we need to wake the thread to clear the
- * interrupt state by reading SX9310_REG_IRQ_SRC.
- * It is not possible to do that here because regmap_read takes a mutex.
- */
- return IRQ_WAKE_THREAD;
-}
-
-static void sx9310_push_events(struct iio_dev *indio_dev)
-{
- int ret;
- unsigned int val, chan;
- struct sx9310_data *data = iio_priv(indio_dev);
- s64 timestamp = iio_get_time_ns(indio_dev);
- unsigned long prox_changed;
-
- /* Read proximity state on all channels */
- ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val);
- if (ret) {
- dev_err(&data->client->dev, "i2c transfer error in irq\n");
- return;
- }
-
- /*
- * Only iterate over channels with changes on proximity status that have
- * events enabled.
- */
- prox_changed = (data->chan_prox_stat ^ val) & data->chan_event;
-
- for_each_set_bit(chan, &prox_changed, SX9310_NUM_CHANNELS) {
- int dir;
- u64 ev;
-
- dir = (val & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
- ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
- IIO_EV_TYPE_THRESH, dir);
-
- iio_push_event(indio_dev, ev, timestamp);
- }
- data->chan_prox_stat = val;
-}
-
-static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
-{
- struct iio_dev *indio_dev = private;
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret;
- unsigned int val;
-
- mutex_lock(&data->mutex);
-
- ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
- if (ret) {
- dev_err(&data->client->dev, "i2c transfer error in irq\n");
- goto out;
- }
-
- if (val & (SX9310_FAR_IRQ | SX9310_CLOSE_IRQ))
- sx9310_push_events(indio_dev);
-
- if (val & SX9310_CONVDONE_IRQ)
- complete(&data->completion);
-
-out:
- mutex_unlock(&data->mutex);
-
- return IRQ_HANDLED;
-}
-
-static int sx9310_read_event_config(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
-
- return !!(data->chan_event & BIT(chan->channel));
-}
-
-static int sx9310_write_event_config(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir, int state)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- unsigned int eventirq = SX9310_FAR_IRQ | SX9310_CLOSE_IRQ;
- int ret;
-
- /* If the state hasn't changed, there's nothing to do. */
- if (!!(data->chan_event & BIT(chan->channel)) == state)
- return 0;
-
- mutex_lock(&data->mutex);
- if (state) {
- ret = sx9310_get_event_channel(data, chan->channel);
- if (ret)
- goto out_unlock;
- if (!(data->chan_event & ~BIT(chan->channel))) {
- ret = sx9310_enable_irq(data, eventirq);
- if (ret)
- sx9310_put_event_channel(data, chan->channel);
- }
- } else {
- ret = sx9310_put_event_channel(data, chan->channel);
- if (ret)
- goto out_unlock;
- if (!data->chan_event) {
- ret = sx9310_disable_irq(data, eventirq);
- if (ret)
- sx9310_get_event_channel(data, chan->channel);
- }
- }
-
-out_unlock:
- mutex_unlock(&data->mutex);
- return ret;
-}
-
static struct attribute *sx9310_attributes[] = {
&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
NULL
@@ -1042,110 +728,15 @@ static const struct attribute_group sx9310_attribute_group = {
static const struct iio_info sx9310_info = {
.attrs = &sx9310_attribute_group,
.read_raw = sx9310_read_raw,
- .read_avail = sx9310_read_avail,
+ .read_avail = sx_common_read_avail,
.read_event_value = sx9310_read_event_val,
.write_event_value = sx9310_write_event_val,
.write_raw = sx9310_write_raw,
- .read_event_config = sx9310_read_event_config,
- .write_event_config = sx9310_write_event_config,
-};
-
-static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state)
-{
- struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret = 0;
-
- mutex_lock(&data->mutex);
-
- if (state)
- ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
- else if (!data->chan_read)
- ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
- if (ret)
- goto out;
-
- data->trigger_enabled = state;
-
-out:
- mutex_unlock(&data->mutex);
-
- return ret;
-}
-
-static const struct iio_trigger_ops sx9310_trigger_ops = {
- .set_trigger_state = sx9310_set_trigger_state,
+ .read_event_config = sx_common_read_event_config,
+ .write_event_config = sx_common_write_event_config,
};
-static irqreturn_t sx9310_trigger_handler(int irq, void *private)
-{
- struct iio_poll_func *pf = private;
- struct iio_dev *indio_dev = pf->indio_dev;
- struct sx9310_data *data = iio_priv(indio_dev);
- __be16 val;
- int bit, ret, i = 0;
-
- mutex_lock(&data->mutex);
-
- for_each_set_bit(bit, indio_dev->active_scan_mask,
- indio_dev->masklength) {
- ret = sx9310_read_prox_data(data, &indio_dev->channels[bit],
- &val);
- if (ret)
- goto out;
-
- data->buffer.channels[i++] = val;
- }
-
- iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
- pf->timestamp);
-
-out:
- mutex_unlock(&data->mutex);
-
- iio_trigger_notify_done(indio_dev->trig);
-
- return IRQ_HANDLED;
-}
-
-static int sx9310_buffer_preenable(struct iio_dev *indio_dev)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- unsigned long channels = 0;
- int bit, ret;
-
- mutex_lock(&data->mutex);
- for_each_set_bit(bit, indio_dev->active_scan_mask,
- indio_dev->masklength)
- __set_bit(indio_dev->channels[bit].channel, &channels);
-
- ret = sx9310_update_chan_en(data, channels, data->chan_event);
- mutex_unlock(&data->mutex);
- return ret;
-}
-
-static int sx9310_buffer_postdisable(struct iio_dev *indio_dev)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- int ret;
-
- mutex_lock(&data->mutex);
- ret = sx9310_update_chan_en(data, 0, data->chan_event);
- mutex_unlock(&data->mutex);
- return ret;
-}
-
-static const struct iio_buffer_setup_ops sx9310_buffer_setup_ops = {
- .preenable = sx9310_buffer_preenable,
- .postdisable = sx9310_buffer_postdisable,
-};
-
-struct sx9310_reg_default {
- u8 reg;
- u8 def;
-};
-
-static const struct sx9310_reg_default sx9310_default_regs[] = {
+static const struct sx_common_reg_default sx9310_default_regs[] = {
{ SX9310_REG_IRQ_MSK, 0x00 },
{ SX9310_REG_IRQ_FUNC, 0x00 },
/*
@@ -1190,7 +781,7 @@ static const struct sx9310_reg_default sx9310_default_regs[] = {
/* Activate all channels and perform an initial compensation. */
static int sx9310_init_compensation(struct iio_dev *indio_dev)
{
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
unsigned int val;
unsigned int ctrl0;
@@ -1220,9 +811,9 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
return ret;
}
-static const struct sx9310_reg_default *
+static const struct sx_common_reg_default *
sx9310_get_default_reg(struct device *dev, int idx,
- struct sx9310_reg_default *reg_def)
+ struct sx_common_reg_default *reg_def)
{
u32 combined[SX9310_NUM_CHANNELS];
u32 start = 0, raw = 0, pos = 0;
@@ -1323,104 +914,31 @@ sx9310_get_default_reg(struct device *dev, int idx,
return reg_def;
}
-static int sx9310_init_device(struct iio_dev *indio_dev)
-{
- struct sx9310_data *data = iio_priv(indio_dev);
- struct sx9310_reg_default tmp;
- const struct sx9310_reg_default *initval;
- int ret;
- unsigned int i, val;
-
- ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET);
- if (ret)
- return ret;
-
- usleep_range(1000, 2000); /* power-up time is ~1ms. */
-
- /* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */
- ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
- if (ret)
- return ret;
-
- /* Program some sane defaults. */
- for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) {
- initval = sx9310_get_default_reg(&indio_dev->dev, i, &tmp);
- ret = regmap_write(data->regmap, initval->reg, initval->def);
- if (ret)
- return ret;
- }
-
- return sx9310_init_compensation(indio_dev);
-}
-
-static int sx9310_set_indio_dev_name(struct device *dev,
- struct iio_dev *indio_dev,
- unsigned int whoami)
-{
- unsigned int long ddata;
-
- ddata = (uintptr_t)device_get_match_data(dev);
- if (ddata != whoami) {
- dev_err(dev, "WHOAMI does not match device data: %u\n", whoami);
- return -ENODEV;
- }
-
- switch (whoami) {
- case SX9310_WHOAMI_VALUE:
- indio_dev->name = "sx9310";
- break;
- case SX9311_WHOAMI_VALUE:
- indio_dev->name = "sx9311";
- break;
- default:
- dev_err(dev, "unexpected WHOAMI response: %u\n", whoami);
- return -ENODEV;
- }
-
- return 0;
-}
-
-static void sx9310_regulator_disable(void *_data)
-{
- struct sx9310_data *data = _data;
-
- regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
-}
-
static int sx9310_probe(struct i2c_client *client)
{
int ret;
struct device *dev = &client->dev;
struct iio_dev *indio_dev;
- struct sx9310_data *data;
+ struct sx_common_data *data;
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
data = iio_priv(indio_dev);
- data->client = client;
- data->supplies[0].supply = "vdd";
- data->supplies[1].supply = "svdd";
- mutex_init(&data->mutex);
- init_completion(&data->completion);
-
- data->regmap = devm_regmap_init_i2c(client, &sx9310_regmap_config);
- if (IS_ERR(data->regmap))
- return PTR_ERR(data->regmap);
-
- ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
- data->supplies);
- if (ret)
- return ret;
+ data->reg_stat = SX9310_REG_STAT0;
+ data->reg_irq_msk = SX9310_REG_IRQ_MSK;
+ data->reg_enable_chan = SX9310_REG_PROX_CTRL0;
+ data->reg_reset = SX9310_REG_RESET;
- ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
- if (ret)
- return ret;
- /* Must wait for Tpor time after initial power up */
- usleep_range(1000, 1100);
+ data->mask_enable_chan = SX9310_REG_STAT1_COMPSTAT_MASK;
+ data->irq_msk_offset = 3;
+ data->num_channels = SX9310_NUM_CHANNELS;
- ret = devm_add_action_or_reset(dev, sx9310_regulator_disable, data);
+ /* Number of default registers to set at init */
+ data->num_default_regs = ARRAY_SIZE(sx9310_default_regs);
+
+ ret = sx_common_probe_setup(indio_dev, client, &sx9310_regmap_config);
if (ret)
return ret;
@@ -1430,58 +948,23 @@ static int sx9310_probe(struct i2c_client *client)
return ret;
}
- ret = sx9310_set_indio_dev_name(dev, indio_dev, data->whoami);
- if (ret)
- return ret;
+ /* Low level entry points */
+ data->read_prox_data = sx9310_read_prox_data;
+ data->init_compensation = sx9310_init_compensation;
+ data->wait_for_sample = sx9310_wait_for_sample;
+ data->get_default_reg = sx9310_get_default_reg;
- ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
indio_dev->channels = sx9310_channels;
indio_dev->num_channels = ARRAY_SIZE(sx9310_channels);
indio_dev->info = &sx9310_info;
- indio_dev->modes = INDIO_DIRECT_MODE;
- i2c_set_clientdata(client, indio_dev);
-
- ret = sx9310_init_device(indio_dev);
- if (ret)
- return ret;
-
- if (client->irq) {
- ret = devm_request_threaded_irq(dev, client->irq,
- sx9310_irq_handler,
- sx9310_irq_thread_handler,
- IRQF_ONESHOT,
- "sx9310_event", indio_dev);
- if (ret)
- return ret;
-
- data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
- indio_dev->name,
- iio_device_id(indio_dev));
- if (!data->trig)
- return -ENOMEM;
-
- data->trig->ops = &sx9310_trigger_ops;
- iio_trigger_set_drvdata(data->trig, indio_dev);
-
- ret = devm_iio_trigger_register(dev, data->trig);
- if (ret)
- return ret;
- }
-
- ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
- iio_pollfunc_store_time,
- sx9310_trigger_handler,
- &sx9310_buffer_setup_ops);
- if (ret)
- return ret;
- return devm_iio_device_register(dev, indio_dev);
+ return sx_common_probe_register(indio_dev);
}
static int __maybe_unused sx9310_suspend(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
u8 ctrl0;
int ret;
@@ -1489,11 +972,11 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
mutex_lock(&data->mutex);
ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
- &data->suspend_ctrl0);
+ &data->suspend_ctrl);
if (ret)
goto out;
- ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
+ ctrl0 = data->suspend_ctrl & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
if (ret)
goto out;
@@ -1508,7 +991,7 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
static int __maybe_unused sx9310_resume(struct device *dev)
{
struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
- struct sx9310_data *data = iio_priv(indio_dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
int ret;
mutex_lock(&data->mutex);
@@ -1517,7 +1000,7 @@ static int __maybe_unused sx9310_resume(struct device *dev)
goto out;
ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
- data->suspend_ctrl0);
+ data->suspend_ctrl);
out:
mutex_unlock(&data->mutex);
diff --git a/drivers/iio/proximity/sx_common.c b/drivers/iio/proximity/sx_common.c
new file mode 100644
index 0000000000000..1dd5dcb75de71
--- /dev/null
+++ b/drivers/iio/proximity/sx_common.c
@@ -0,0 +1,618 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 Google LLC.
+ *
+ * Driver for Semtech's SX9310/SX9311 capacitive proximity/button solution.
+ * Based on SX9500 driver and Semtech driver using the input framework
+ * <https://my.syncplicity.com/share/teouwsim8niiaud/
+ * linux-driver-sx_common_NoSmartHSensing>.
+ * Reworked in April 2019 by Evan Green <evgreen@chromium.org>
+ * and in January 2020 by Daniel Campello <campello@chromium.org>.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#include <linux/iio/proximity/sx_common.h>
+
+/* All Semtech SAR sensor have IRQ bit in same order. */
+#define SX_COMMON_CONVDONE_IRQ BIT(0)
+#define SX_COMMON_FAR_IRQ BIT(2)
+#define SX_COMMON_CLOSE_IRQ BIT(3)
+
+const struct iio_event_spec sx_common_events[3] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_EITHER,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+ BIT(IIO_EV_INFO_HYSTERESIS) |
+ BIT(IIO_EV_INFO_VALUE),
+ },
+};
+EXPORT_SYMBOL_GPL(sx_common_events);
+
+static irqreturn_t sx_common_irq_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ if (data->trigger_enabled)
+ iio_trigger_poll(data->trig);
+
+ /*
+ * Even if no event is enabled, we need to wake the thread to clear the
+ * interrupt state by reading SX_COMMON_REG_IRQ_SRC.
+ * It is not possible to do that here because regmap_read takes a mutex.
+ */
+ return IRQ_WAKE_THREAD;
+}
+
+static void sx_common_push_events(struct iio_dev *indio_dev)
+{
+ int ret;
+ unsigned int val, chan;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ s64 timestamp = iio_get_time_ns(indio_dev);
+ unsigned long prox_changed;
+
+ /* Read proximity state on all channels */
+ ret = regmap_read(data->regmap, data->reg_stat, &val);
+ if (ret) {
+ dev_err(&data->client->dev, "i2c transfer error in irq\n");
+ return;
+ }
+
+ /*
+ * Only iterate over channels with changes on proximity status that have
+ * events enabled.
+ */
+ prox_changed = (data->chan_prox_stat ^ val) & data->chan_event;
+
+ for_each_set_bit(chan, &prox_changed, data->num_channels) {
+ int dir;
+ u64 ev;
+
+ dir = (val & BIT(chan)) ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+ ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
+ IIO_EV_TYPE_THRESH, dir);
+
+ iio_push_event(indio_dev, ev, timestamp);
+ }
+ data->chan_prox_stat = val;
+}
+
+static int sx_common_enable_irq(struct sx_common_data *data, unsigned int irq)
+{
+ if (!data->client->irq)
+ return 0;
+ return regmap_update_bits(data->regmap, data->reg_irq_msk,
+ irq << data->irq_msk_offset,
+ irq << data->irq_msk_offset);
+}
+
+static int sx_common_disable_irq(struct sx_common_data *data, unsigned int irq)
+{
+ if (!data->client->irq)
+ return 0;
+ return regmap_update_bits(data->regmap, data->reg_irq_msk,
+ irq << data->irq_msk_offset, 0);
+}
+
+static int sx_common_update_chan_en(struct sx_common_data *data,
+ unsigned long chan_read,
+ unsigned long chan_event)
+{
+ int ret;
+ unsigned long channels = chan_read | chan_event;
+
+ if ((data->chan_read | data->chan_event) != channels) {
+ ret = regmap_update_bits(data->regmap, data->reg_enable_chan,
+ data->mask_enable_chan,
+ channels);
+ if (ret)
+ return ret;
+ }
+ data->chan_read = chan_read;
+ data->chan_event = chan_event;
+ return 0;
+}
+
+static int sx_common_get_read_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read | BIT(channel),
+ data->chan_event);
+}
+
+static int sx_common_put_read_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read & ~BIT(channel),
+ data->chan_event);
+}
+
+static int sx_common_get_event_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read,
+ data->chan_event | BIT(channel));
+}
+
+static int sx_common_put_event_channel(struct sx_common_data *data, int channel)
+{
+ return sx_common_update_chan_en(data, data->chan_read,
+ data->chan_event & ~BIT(channel));
+}
+
+static const int sx_common_gain_vals[] = { 1, 2, 4, 8 };
+
+/**
+ * sx_common_read_avail() - Return list of gain available.
+ */
+int sx_common_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(sx_common_gain_vals);
+ *vals = sx_common_gain_vals;
+ return IIO_AVAIL_LIST;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(sx_common_read_avail);
+
+/**
+ * sx_common_read_proximity() - Read raw proximity value.
+ *
+ * Request a conversion, wait for the sensor to be ready and
+ * return the raw proximity value.
+ */
+int sx_common_read_proximity(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ int ret;
+ __be16 rawval;
+
+ mutex_lock(&data->mutex);
+
+ ret = sx_common_get_read_channel(data, chan->channel);
+ if (ret)
+ goto out;
+
+ ret = sx_common_enable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out_put_channel;
+
+ mutex_unlock(&data->mutex);
+
+ if (data->client->irq) {
+ ret = wait_for_completion_interruptible(&data->completion);
+ reinit_completion(&data->completion);
+ } else {
+ ret = data->wait_for_sample(data);
+ }
+
+ mutex_lock(&data->mutex);
+
+ if (ret)
+ goto out_disable_irq;
+
+ ret = data->read_prox_data(data, chan, &rawval);
+ if (ret)
+ goto out_disable_irq;
+
+ *val = sign_extend32(be16_to_cpu(rawval), chan->scan_type.realbits - 1);
+
+ ret = sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out_put_channel;
+
+ ret = sx_common_put_read_channel(data, chan->channel);
+ if (ret)
+ goto out;
+
+ mutex_unlock(&data->mutex);
+
+ return IIO_VAL_INT;
+
+out_disable_irq:
+ sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+out_put_channel:
+ sx_common_put_read_channel(data, chan->channel);
+out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sx_common_read_proximity);
+
+/**
+ * sx_common_read_event_config() - Configure event setting.
+ *
+ * IIO vector to return the current event configuration.
+ */
+int sx_common_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ return !!(data->chan_event & BIT(chan->channel));
+}
+EXPORT_SYMBOL_GPL(sx_common_read_event_config);
+
+/**
+ * sx_common_write_event_config() - Configure event setting.
+ *
+ * IIO vector to enable/disable events.
+ */
+int sx_common_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int eventirq = SX_COMMON_FAR_IRQ | SX_COMMON_CLOSE_IRQ;
+ int ret;
+
+ /* If the state hasn't changed, there's nothing to do. */
+ if (!!(data->chan_event & BIT(chan->channel)) == state)
+ return 0;
+
+ mutex_lock(&data->mutex);
+ if (state) {
+ ret = sx_common_get_event_channel(data, chan->channel);
+ if (ret)
+ goto out_unlock;
+ if (!(data->chan_event & ~BIT(chan->channel))) {
+ ret = sx_common_enable_irq(data, eventirq);
+ if (ret)
+ sx_common_put_event_channel(data, chan->channel);
+ }
+ } else {
+ ret = sx_common_put_event_channel(data, chan->channel);
+ if (ret)
+ goto out_unlock;
+ if (!data->chan_event) {
+ ret = sx_common_disable_irq(data, eventirq);
+ if (ret)
+ sx_common_get_event_channel(data, chan->channel);
+ }
+ }
+
+out_unlock:
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(sx_common_write_event_config);
+
+static int sx_common_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret = 0;
+
+ mutex_lock(&data->mutex);
+
+ if (state)
+ ret = sx_common_enable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ else if (!data->chan_read)
+ ret = sx_common_disable_irq(data, SX_COMMON_CONVDONE_IRQ);
+ if (ret)
+ goto out;
+
+ data->trigger_enabled = state;
+
+out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct iio_trigger_ops sx_common_trigger_ops = {
+ .set_trigger_state = sx_common_set_trigger_state,
+};
+
+static irqreturn_t sx_common_irq_thread_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+ unsigned int val;
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_read(data->regmap, SX_COMMON_REG_IRQ_SRC, &val);
+ if (ret) {
+ dev_err(&data->client->dev, "i2c transfer error in irq\n");
+ goto out;
+ }
+
+ if (val & ((SX_COMMON_FAR_IRQ | SX_COMMON_CLOSE_IRQ) << data->irq_msk_offset))
+ sx_common_push_events(indio_dev);
+
+ if (val & (SX_COMMON_CONVDONE_IRQ << data->irq_msk_offset))
+ complete(&data->completion);
+
+out:
+ mutex_unlock(&data->mutex);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t sx_common_trigger_handler(int irq, void *private)
+{
+ struct iio_poll_func *pf = private;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct sx_common_data *data = iio_priv(indio_dev);
+ __be16 val;
+ int bit, ret, i = 0;
+
+ mutex_lock(&data->mutex);
+
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ ret = data->read_prox_data(data, &indio_dev->channels[bit],
+ &val);
+ if (ret)
+ goto out;
+
+ data->buffer.channels[i++] = val;
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+ pf->timestamp);
+
+out:
+ mutex_unlock(&data->mutex);
+
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int sx_common_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned long channels = 0;
+ int bit, ret;
+
+ mutex_lock(&data->mutex);
+ for_each_set_bit(bit, indio_dev->active_scan_mask,
+ indio_dev->masklength)
+ __set_bit(indio_dev->channels[bit].channel, &channels);
+
+ ret = sx_common_update_chan_en(data, channels, data->chan_event);
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static int sx_common_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = sx_common_update_chan_en(data, 0, data->chan_event);
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+const struct iio_buffer_setup_ops sx_common_buffer_setup_ops = {
+ .preenable = sx_common_buffer_preenable,
+ .postdisable = sx_common_buffer_postdisable,
+};
+EXPORT_SYMBOL_GPL(sx_common_buffer_setup_ops);
+
+static int sx_common_set_indio_dev_name(struct device *dev,
+ struct iio_dev *indio_dev,
+ unsigned int whoami)
+{
+ unsigned int long ddata;
+
+ ddata = (uintptr_t)device_get_match_data(dev);
+ if (ddata != whoami) {
+ dev_err(dev, "WHOAMI does not match device data: %u\n", whoami);
+ return -ENODEV;
+ }
+
+ switch (whoami) {
+ case SX9310_WHOAMI_VALUE:
+ indio_dev->name = "sx9310";
+ break;
+ case SX9311_WHOAMI_VALUE:
+ indio_dev->name = "sx9311";
+ break;
+ case SX9324_WHOAMI_VALUE:
+ indio_dev->name = "sx9324";
+ break;
+ case SX9360_WHOAMI_VALUE:
+ indio_dev->name = "sx9360";
+ break;
+ default:
+ dev_err(dev, "unexpected WHOAMI response: %u\n", whoami);
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
+static void sx_common_regulator_disable(void *_data)
+{
+ struct sx_common_data *data = _data;
+
+ regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+}
+
+#define SX_COMMON_SOFT_RESET 0xde
+
+static int sx_common_init_device(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ struct sx_common_reg_default tmp;
+ const struct sx_common_reg_default *initval;
+ int ret;
+ unsigned int i, val;
+
+ ret = regmap_write(data->regmap, data->reg_reset, SX_COMMON_SOFT_RESET);
+ if (ret)
+ return ret;
+
+ usleep_range(1000, 2000); /* power-up time is ~1ms. */
+
+ /* Clear reset interrupt state by reading SX_COMMON_REG_IRQ_SRC. */
+ ret = regmap_read(data->regmap, SX_COMMON_REG_IRQ_SRC, &val);
+ if (ret)
+ return ret;
+
+ /* Program some sane defaults. */
+ for (i = 0; i < data->num_default_regs; i++) {
+ initval = data->get_default_reg(&indio_dev->dev, i, &tmp);
+ ret = regmap_write(data->regmap, initval->reg, initval->def);
+ if (ret)
+ return ret;
+ }
+
+ return data->init_compensation(indio_dev);
+}
+
+/**
+ * sx_common_probe_setup() - Common setup for Semtech SAR sensor
+ * @indio_dev: iio device structure of the device
+ * @client: I2C client object
+ * @regmap_config: Sensor registers map configuration.
+ */
+int sx_common_probe_setup(struct iio_dev *indio_dev,
+ struct i2c_client *client,
+ const struct regmap_config *regmap_config)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ struct device *dev = &client->dev;
+ int ret;
+
+ data->client = client;
+ data->supplies[0].supply = "vdd";
+ data->supplies[1].supply = "svdd";
+ mutex_init(&data->mutex);
+ init_completion(&data->completion);
+
+ data->regmap = devm_regmap_init_i2c(client, regmap_config);
+ if (IS_ERR(data->regmap))
+ return PTR_ERR(data->regmap);
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+ data->supplies);
+ if (ret)
+ return ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+ if (ret)
+ return ret;
+ /* Must wait for Tpor time after initial power up */
+ usleep_range(1000, 1100);
+
+ ret = devm_add_action_or_reset(dev, sx_common_regulator_disable, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(sx_common_probe_setup);
+
+/**
+ * sx_common_probe_register() - Common register function for Semtech SAR sensor
+ * @indio_dev: iio device structure of the device
+ *
+ * Register the IRQ handlers and trigger object.
+ * Register to the IIO subsystem.
+ */
+int sx_common_probe_register(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ struct i2c_client *client = data->client;
+ struct device *dev = &client->dev;
+ int ret;
+
+ ret = sx_common_set_indio_dev_name(dev, indio_dev, data->whoami);
+ if (ret)
+ return ret;
+
+ ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = sx_common_init_device(indio_dev);
+ if (ret)
+ return ret;
+
+ if (client->irq) {
+ ret = devm_request_threaded_irq(dev, client->irq,
+ sx_common_irq_handler,
+ sx_common_irq_thread_handler,
+ IRQF_ONESHOT,
+ "sx_event", indio_dev);
+ if (ret)
+ return ret;
+
+ data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->trig)
+ return -ENOMEM;
+
+ data->trig->ops = &sx_common_trigger_ops;
+ iio_trigger_set_drvdata(data->trig, indio_dev);
+
+ ret = devm_iio_trigger_register(dev, data->trig);
+ if (ret)
+ return ret;
+ }
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time,
+ sx_common_trigger_handler,
+ &sx_common_buffer_setup_ops);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+EXPORT_SYMBOL_GPL(sx_common_probe_register);
+
+MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
+MODULE_DESCRIPTION("Common functions and structures for Semtech sensor");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/iio/proximity/sx_common.h b/include/linux/iio/proximity/sx_common.h
new file mode 100644
index 0000000000000..60c3291e7925c
--- /dev/null
+++ b/include/linux/iio/proximity/sx_common.h
@@ -0,0 +1,129 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Code shared between the different Qualcomm PMIC voltage ADCs
+ */
+
+#ifndef IIO_SX_COMMON_H
+#define IIO_SX_COMMON_H
+
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#define SX_COMMON_REG_IRQ_SRC 0x00
+
+#define SX9310_WHOAMI_VALUE 0x01
+#define SX9311_WHOAMI_VALUE 0x02
+#define SX9324_WHOAMI_VALUE 0x23
+#define SX9360_WHOAMI_VALUE 0x60
+
+#define SX_COMMON_MAX_NUM_CHANNELS 4
+static_assert(SX_COMMON_MAX_NUM_CHANNELS < BITS_PER_LONG);
+
+struct sx_common_reg_default {
+ u8 reg;
+ u8 def;
+};
+
+/**
+ * struct sx_common_data: Semtech Sensor private data structure.
+ @reg_stat: Main status register address.
+ @reg_irq_msk: IRQ mask register address.
+ @reg_enable_chan: Address to enable/disable channels/phases.
+ @reg_reset: Reset register address.
+
+ @mask_enable_chan: Mask over the channels bits in the enable channel
+ register.
+ @irq_msk_offset: Offset to enable interrupt in the IRQ mask
+ register.
+
+ @mutex: Serialize access to registers and channel configuration.
+ @num_channels: Number of channel/phase.
+ @completion: completion object to wait for data acquisition.
+ @client: I2C client structure.
+ @trig: IIO trigger object.
+ @regmap: Register map.
+
+ @num_default_regs: Number of default registers to set at init.
+ @supplies: Power supplies object.
+ @chan_prox_stat: Last reading of the proximity status for each channel.
+ We only send an event to user space when this changes.
+ @trigger_enabled: True when the device trigger is enabled.
+
+ @buffer: Bufffer to store raw samples.
+ @suspend_ctrl: Remember enabled channels and sample rate during suspend.
+ @chan_read: Bit field for each raw channel enabled.
+ @chan_event: Bit field for each event enabled.
+ @whoami: Content of WHOAMI register.
+
+ @read_prox_data: Function to read proximity data.
+ @init_compensation: Function to set initial compensation.
+ @wait_for_sample: When there are no physical IRQ, function to wait for a
+ sample to be ready.
+ */
+struct sx_common_data {
+ unsigned int reg_stat;
+ unsigned int reg_irq_msk;
+ unsigned int reg_enable_chan;
+ unsigned int reg_reset;
+
+ unsigned int mask_enable_chan;
+ unsigned int irq_msk_offset;
+
+ struct mutex mutex;
+ unsigned int num_channels;
+ struct completion completion;
+ struct i2c_client *client;
+ struct iio_trigger *trig;
+ struct regmap *regmap;
+
+ int num_default_regs;
+ struct regulator_bulk_data supplies[2];
+ unsigned long chan_prox_stat;
+ bool trigger_enabled;
+
+ /* Ensure correct alignment of timestamp when present. */
+ struct {
+ __be16 channels[SX_COMMON_MAX_NUM_CHANNELS];
+ s64 ts __aligned(8);
+ } buffer;
+
+ unsigned int suspend_ctrl;
+ unsigned long chan_read;
+ unsigned long chan_event;
+ unsigned int whoami;
+
+ int (*read_prox_data)(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, __be16 *val);
+ int (*init_compensation)(struct iio_dev *indio_dev);
+ int (*wait_for_sample)(struct sx_common_data *data);
+ const struct sx_common_reg_default *
+ (*get_default_reg)(struct device *dev, int idx,
+ struct sx_common_reg_default *reg_def);
+};
+
+int sx_common_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask);
+
+int sx_common_read_proximity(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val);
+
+int sx_common_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir);
+int sx_common_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state);
+
+int sx_common_probe_setup(struct iio_dev *indio_dev,
+ struct i2c_client *client,
+ const struct regmap_config *regmap_config);
+int sx_common_probe_register(struct iio_dev *indio_dev);
+
+extern const struct iio_buffer_setup_ops sx_common_buffer_setup_ops;
+extern const struct iio_event_spec sx_common_events[3];
+
+#endif /* IIO_SX_COMMON_H */
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-10-30 11:18 ` [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
@ 2021-10-30 11:40 ` Lars-Peter Clausen
2021-10-30 16:42 ` Jonathan Cameron
1 sibling, 0 replies; 18+ messages in thread
From: Lars-Peter Clausen @ 2021-10-30 11:40 UTC (permalink / raw)
To: Gwendal Grignou, jic23, swboyd; +Cc: andy.shevchenko, linux-iio
On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> [...]
> diff --git a/include/linux/iio/proximity/sx_common.h b/include/linux/iio/proximity/sx_common.h
> new file mode 100644
> index 0000000000000..60c3291e7925c
> --- /dev/null
> +++ b/include/linux/iio/proximity/sx_common.h
Can this header live in drivers/iio/proximity? No need to put it in the
global space if it is only used locally.
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Code shared between the different Qualcomm PMIC voltage ADCs
PMIC? I though this was a proximity sensor :)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic
2021-10-30 11:18 ` [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
2021-10-30 11:40 ` Lars-Peter Clausen
@ 2021-10-30 16:42 ` Jonathan Cameron
1 sibling, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-10-30 16:42 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, 30 Oct 2021 04:18:24 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> Before adding new Semtech sensors, move common logic to all Semtech SAR
> sensor in its own file:
> - interface with IIO subsystem,
> - interrupt management,
> - channel access conrol,
> - event processing.
>
> The change adds a bidirectional interface between sx93xx and sx_common.
>
> The change is quite mechanical, as the impacted functions are moved
> and renamed.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Hi Gwendal,
The 2 step approach used for a common probe is unusual and I'm not sure
it's a good idea. I'd be more inclined to have a single probe function
and pass some static const data into there to fill in most of the stuff
you have left in the individual driver.
See below.
Jonathan
> ---
> drivers/iio/proximity/sx9310.c | 629 +++---------------------
> drivers/iio/proximity/sx_common.c | 618 +++++++++++++++++++++++
> include/linux/iio/proximity/sx_common.h | 129 +++++
What Lars said on location. drivers/iio/proximity/sx_common.h
> 3 files changed, 803 insertions(+), 573 deletions(-)
> create mode 100644 drivers/iio/proximity/sx_common.c
> create mode 100644 include/linux/iio/proximity/sx_common.h
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 31d060c1b0103..577a16789f8b2 100644
> --- a/drivers/iio/proximity/sx9310.c
...
> static int sx9310_probe(struct i2c_client *client)
> {
> int ret;
> struct device *dev = &client->dev;
> struct iio_dev *indio_dev;
> - struct sx9310_data *data;
> + struct sx_common_data *data;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> data = iio_priv(indio_dev);
> - data->client = client;
> - data->supplies[0].supply = "vdd";
> - data->supplies[1].supply = "svdd";
> - mutex_init(&data->mutex);
> - init_completion(&data->completion);
> -
> - data->regmap = devm_regmap_init_i2c(client, &sx9310_regmap_config);
> - if (IS_ERR(data->regmap))
> - return PTR_ERR(data->regmap);
> -
> - ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> - data->supplies);
> - if (ret)
> - return ret;
> + data->reg_stat = SX9310_REG_STAT0;
> + data->reg_irq_msk = SX9310_REG_IRQ_MSK;
> + data->reg_enable_chan = SX9310_REG_PROX_CTRL0;
> + data->reg_reset = SX9310_REG_RESET;
>
> - ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> - if (ret)
> - return ret;
> - /* Must wait for Tpor time after initial power up */
> - usleep_range(1000, 1100);
> + data->mask_enable_chan = SX9310_REG_STAT1_COMPSTAT_MASK;
> + data->irq_msk_offset = 3;
> + data->num_channels = SX9310_NUM_CHANNELS;
>
> - ret = devm_add_action_or_reset(dev, sx9310_regulator_disable, data);
> + /* Number of default registers to set at init */
> + data->num_default_regs = ARRAY_SIZE(sx9310_default_regs);
> +
> + ret = sx_common_probe_setup(indio_dev, client, &sx9310_regmap_config);
> if (ret)
> return ret;
>
> @@ -1430,58 +948,23 @@ static int sx9310_probe(struct i2c_client *client)
> return ret;
> }
>
> - ret = sx9310_set_indio_dev_name(dev, indio_dev, data->whoami);
> - if (ret)
> - return ret;
> + /* Low level entry points */
> + data->read_prox_data = sx9310_read_prox_data;
> + data->init_compensation = sx9310_init_compensation;
> + data->wait_for_sample = sx9310_wait_for_sample;
> + data->get_default_reg = sx9310_get_default_reg;
>
> - ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
> indio_dev->channels = sx9310_channels;
> indio_dev->num_channels = ARRAY_SIZE(sx9310_channels);
> indio_dev->info = &sx9310_info;
> - indio_dev->modes = INDIO_DIRECT_MODE;
> - i2c_set_clientdata(client, indio_dev);
> -
> - ret = sx9310_init_device(indio_dev);
> - if (ret)
> - return ret;
> -
> - if (client->irq) {
> - ret = devm_request_threaded_irq(dev, client->irq,
> - sx9310_irq_handler,
> - sx9310_irq_thread_handler,
> - IRQF_ONESHOT,
> - "sx9310_event", indio_dev);
> - if (ret)
> - return ret;
> -
> - data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> - indio_dev->name,
> - iio_device_id(indio_dev));
> - if (!data->trig)
> - return -ENOMEM;
> -
> - data->trig->ops = &sx9310_trigger_ops;
> - iio_trigger_set_drvdata(data->trig, indio_dev);
> -
> - ret = devm_iio_trigger_register(dev, data->trig);
> - if (ret)
> - return ret;
> - }
> -
> - ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> - iio_pollfunc_store_time,
> - sx9310_trigger_handler,
> - &sx9310_buffer_setup_ops);
> - if (ret)
> - return ret;
>
> - return devm_iio_device_register(dev, indio_dev);
> + return sx_common_probe_register(indio_dev);
> }
>
...
> new file mode 100644
> index 0000000000000..1dd5dcb75de71
> --- /dev/null
> +++ b/drivers/iio/proximity/sx_common.c
> @@ -0,0 +1,618 @@
...
> +
> +/**
> + * sx_common_read_event_config() - Configure event setting.
> + *
> + * IIO vector to return the current event configuration.
> + */
> +int sx_common_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> +
> + return !!(data->chan_event & BIT(chan->channel));
> +}
> +EXPORT_SYMBOL_GPL(sx_common_read_event_config);
> +
> +/**
> + * sx_common_write_event_config() - Configure event setting.
kernel-doc should be complete. Please check the files by pointing
scripts/kernel-doc at them as it will warn about this.
> + *
> + * IIO vector to enable/disable events.
> + */
> +int sx_common_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> + unsigned int eventirq = SX_COMMON_FAR_IRQ | SX_COMMON_CLOSE_IRQ;
> + int ret;
> +
> + /* If the state hasn't changed, there's nothing to do. */
> + if (!!(data->chan_event & BIT(chan->channel)) == state)
> + return 0;
> +
> + mutex_lock(&data->mutex);
> + if (state) {
> + ret = sx_common_get_event_channel(data, chan->channel);
> + if (ret)
> + goto out_unlock;
> + if (!(data->chan_event & ~BIT(chan->channel))) {
> + ret = sx_common_enable_irq(data, eventirq);
> + if (ret)
> + sx_common_put_event_channel(data, chan->channel);
> + }
> + } else {
> + ret = sx_common_put_event_channel(data, chan->channel);
> + if (ret)
> + goto out_unlock;
> + if (!data->chan_event) {
> + ret = sx_common_disable_irq(data, eventirq);
> + if (ret)
> + sx_common_get_event_channel(data, chan->channel);
> + }
> + }
> +
> +out_unlock:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(sx_common_write_event_config);
...
> +
> +const struct iio_buffer_setup_ops sx_common_buffer_setup_ops = {
> + .preenable = sx_common_buffer_preenable,
> + .postdisable = sx_common_buffer_postdisable,
> +};
> +EXPORT_SYMBOL_GPL(sx_common_buffer_setup_ops);
Why? Currently only used in here. If you need to do this for a later
patch please do it in that patch.
> +
> +static int sx_common_set_indio_dev_name(struct device *dev,
> + struct iio_dev *indio_dev,
> + unsigned int whoami)
> +{
> + unsigned int long ddata;
> +
> + ddata = (uintptr_t)device_get_match_data(dev);
> + if (ddata != whoami) {
> + dev_err(dev, "WHOAMI does not match device data: %u\n", whoami);
> + return -ENODEV;
> + }
> +
> + switch (whoami) {
> + case SX9310_WHOAMI_VALUE:
> + indio_dev->name = "sx9310";
> + break;
> + case SX9311_WHOAMI_VALUE:
> + indio_dev->name = "sx9311";
> + break;
> + case SX9324_WHOAMI_VALUE:
Too early. You don't support these yet... Also this is a job for
the drivers implementing the per device type support. Move it out there
and pass the name into the common probe.
> + indio_dev->name = "sx9324";
> + break;
> + case SX9360_WHOAMI_VALUE:
> + indio_dev->name = "sx9360";
> + break;
> + default:
> + dev_err(dev, "unexpected WHOAMI response: %u\n", whoami);
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +static void sx_common_regulator_disable(void *_data)
> +{
> + struct sx_common_data *data = _data;
> +
> + regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
> +}
> +
> +#define SX_COMMON_SOFT_RESET 0xde
> +
> +static int sx_common_init_device(struct iio_dev *indio_dev)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> + struct sx_common_reg_default tmp;
> + const struct sx_common_reg_default *initval;
> + int ret;
> + unsigned int i, val;
> +
> + ret = regmap_write(data->regmap, data->reg_reset, SX_COMMON_SOFT_RESET);
> + if (ret)
> + return ret;
> +
> + usleep_range(1000, 2000); /* power-up time is ~1ms. */
> +
> + /* Clear reset interrupt state by reading SX_COMMON_REG_IRQ_SRC. */
> + ret = regmap_read(data->regmap, SX_COMMON_REG_IRQ_SRC, &val);
> + if (ret)
> + return ret;
> +
> + /* Program some sane defaults. */
> + for (i = 0; i < data->num_default_regs; i++) {
> + initval = data->get_default_reg(&indio_dev->dev, i, &tmp);
> + ret = regmap_write(data->regmap, initval->reg, initval->def);
> + if (ret)
> + return ret;
> + }
> +
> + return data->init_compensation(indio_dev);
> +}
> +
> +/**
> + * sx_common_probe_setup() - Common setup for Semtech SAR sensor
> + * @indio_dev: iio device structure of the device
> + * @client: I2C client object
> + * @regmap_config: Sensor registers map configuration.
> + */
> +int sx_common_probe_setup(struct iio_dev *indio_dev,
> + struct i2c_client *client,
> + const struct regmap_config *regmap_config)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> + struct device *dev = &client->dev;
> + int ret;
> +
> + data->client = client;
> + data->supplies[0].supply = "vdd";
> + data->supplies[1].supply = "svdd";
> + mutex_init(&data->mutex);
> + init_completion(&data->completion);
> +
> + data->regmap = devm_regmap_init_i2c(client, regmap_config);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
> +
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> + data->supplies);
> + if (ret)
> + return ret;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> + if (ret)
> + return ret;
> + /* Must wait for Tpor time after initial power up */
> + usleep_range(1000, 1100);
> +
> + ret = devm_add_action_or_reset(dev, sx_common_regulator_disable, data);
> + if (ret)
> + return ret;
> +
> + return 0;
It will go anyway if you follow suggestion below, but...
return devm_add_action...
btw one of the static checkers loves this so if you don't 'fix' it on submission
we'll just end up with an annoying tidy up patch a few weeks later :)
> +}
> +EXPORT_SYMBOL_GPL(sx_common_probe_setup);
> +
> +/**
> + * sx_common_probe_register() - Common register function for Semtech SAR sensor
> + * @indio_dev: iio device structure of the device
> + *
> + * Register the IRQ handlers and trigger object.
> + * Register to the IIO subsystem.
> + */
> +int sx_common_probe_register(struct iio_dev *indio_dev)
Interesting to do a split register like this. You might be better
off passing a const structure into an sx_common_probe() with all
the relevant data to copy to it's final destination. Particularly as
much of it is constant - or ops.
I'd have something like.
struct sx_ops {
int (*read_prox_data)(struct sx_common_data *data,
const struct iio_chan_spec *chan, __be16 *val);
int (*init_compensation)(struct iio_dev *indio_dev);
int (*wait_for_sample)(struct sx_common_data *data);
const struct sx_common_reg_default *
(*get_default_reg)(struct device *dev, int idx,
struct sx_common_reg_default *reg_def);
};
struct sx_chip_info {
unsigned int reg_stat;
unsigned int reg_irq_msk;
unsigned int reg_enable_chan;
unsigned int reg_reset;
unsigned int mask_enable_chan;
unsigned int irq_msk_offset;
int num_default_regs;
struct iio_chan_spec *channels; /* copy this into place in probe_common() */
int num_channels; /* copy into place in probe_common()
struct sx_ops ops; /* As it's all const you just have the ops inline */
struct iio_info *info;
// some other stuff like attributes, though might be better to convert that over
// to read_avail() before doing the rest of this if possible.
};
Have a static const instance of this an pass it into a single probe function that does
all your setup.
Everything else plus a pointer to the sx_chip_info structure in your struct sx_common_data
That small amount of indirection means a lot of stuff is now constant that we had to
set in code before.
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + ret = sx_common_set_indio_dev_name(dev, indio_dev, data->whoami);
> + if (ret)
> + return ret;
> +
> + ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = sx_common_init_device(indio_dev);
> + if (ret)
> + return ret;
> +
> + if (client->irq) {
> + ret = devm_request_threaded_irq(dev, client->irq,
> + sx_common_irq_handler,
> + sx_common_irq_thread_handler,
> + IRQF_ONESHOT,
> + "sx_event", indio_dev);
> + if (ret)
> + return ret;
> +
> + data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->ops = &sx_common_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> + ret = devm_iio_trigger_register(dev, data->trig);
> + if (ret)
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time,
> + sx_common_trigger_handler,
> + &sx_common_buffer_setup_ops);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(sx_common_probe_register);
> +
> +MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
> +MODULE_DESCRIPTION("Common functions and structures for Semtech sensor");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/iio/proximity/sx_common.h b/include/linux/iio/proximity/sx_common.h
> new file mode 100644
> index 0000000000000..60c3291e7925c
> --- /dev/null
> +++ b/include/linux/iio/proximity/sx_common.h
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Code shared between the different Qualcomm PMIC voltage ADCs
> + */
> +
> +#ifndef IIO_SX_COMMON_H
> +#define IIO_SX_COMMON_H
> +
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
drop regmap.h and provide the forwards defintions for the few structures need
for pointers. But some missing that should be here.
#include <linux/bitops.h>
#include <linux/completion.h>
#include <linux/mutex.h>
struct regmap;
struct regmap_config;
struct iio_chan_spec;
struct iio_device;
struct iio_trigger;
struct i2c_client;
etc
I'm trying to scrub the IIO includes to sort this stuff out, so not keen to
see more unnecessary or missing includes to clean up.
More or less always include the headers you use in a file and ensure you don't
include ones you don't have to.
> +
> +#define SX_COMMON_REG_IRQ_SRC 0x00
> +
> +#define SX9310_WHOAMI_VALUE 0x01
> +#define SX9311_WHOAMI_VALUE 0x02
> +#define SX9324_WHOAMI_VALUE 0x23
> +#define SX9360_WHOAMI_VALUE 0x60
> +
> +#define SX_COMMON_MAX_NUM_CHANNELS 4
> +static_assert(SX_COMMON_MAX_NUM_CHANNELS < BITS_PER_LONG);
> +
> +struct sx_common_reg_default {
> + u8 reg;
> + u8 def;
> +};
> +
> +/**
> + * struct sx_common_data: Semtech Sensor private data structure.
> + @reg_stat: Main status register address.
> + @reg_irq_msk: IRQ mask register address.
> + @reg_enable_chan: Address to enable/disable channels/phases.
> + @reg_reset: Reset register address.
> +
> + @mask_enable_chan: Mask over the channels bits in the enable channel
> + register.
> + @irq_msk_offset: Offset to enable interrupt in the IRQ mask
> + register.
> +
> + @mutex: Serialize access to registers and channel configuration.
> + @num_channels: Number of channel/phase.
> + @completion: completion object to wait for data acquisition.
> + @client: I2C client structure.
> + @trig: IIO trigger object.
> + @regmap: Register map.
> +
> + @num_default_regs: Number of default registers to set at init.
> + @supplies: Power supplies object.
> + @chan_prox_stat: Last reading of the proximity status for each channel.
> + We only send an event to user space when this changes.
> + @trigger_enabled: True when the device trigger is enabled.
> +
> + @buffer: Bufffer to store raw samples.
> + @suspend_ctrl: Remember enabled channels and sample rate during suspend.
> + @chan_read: Bit field for each raw channel enabled.
> + @chan_event: Bit field for each event enabled.
> + @whoami: Content of WHOAMI register.
> +
> + @read_prox_data: Function to read proximity data.
> + @init_compensation: Function to set initial compensation.
> + @wait_for_sample: When there are no physical IRQ, function to wait for a
> + sample to be ready.
get_default_reg is missing.
> + */
> +struct sx_common_data {
> + unsigned int reg_stat;
> + unsigned int reg_irq_msk;
> + unsigned int reg_enable_chan;
> + unsigned int reg_reset;
> +
> + unsigned int mask_enable_chan;
> + unsigned int irq_msk_offset;
> +
> + struct mutex mutex;
> + unsigned int num_channels;
> + struct completion completion;
> + struct i2c_client *client;
> + struct iio_trigger *trig;
> + struct regmap *regmap;
> +
> + int num_default_regs;
> + struct regulator_bulk_data supplies[2];
> + unsigned long chan_prox_stat;
> + bool trigger_enabled;
> +
> + /* Ensure correct alignment of timestamp when present. */
> + struct {
> + __be16 channels[SX_COMMON_MAX_NUM_CHANNELS];
> + s64 ts __aligned(8);
> + } buffer;
> +
> + unsigned int suspend_ctrl;
> + unsigned long chan_read;
> + unsigned long chan_event;
> + unsigned int whoami;
> +
> + int (*read_prox_data)(struct sx_common_data *data,
> + const struct iio_chan_spec *chan, __be16 *val);
> + int (*init_compensation)(struct iio_dev *indio_dev);
> + int (*wait_for_sample)(struct sx_common_data *data);
> + const struct sx_common_reg_default *
> + (*get_default_reg)(struct device *dev, int idx,
> + struct sx_common_reg_default *reg_def);
These look to be constant ops pointers. Break them out to a separate structure
and that can then be static const in the individual drivers.
Good thing to do form a security point of view and normally ends
up simpler.
> +};
> +
> +int sx_common_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask);
> +
> +int sx_common_read_proximity(struct sx_common_data *data,
> + const struct iio_chan_spec *chan, int *val);
> +
> +int sx_common_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir);
> +int sx_common_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state);
> +
> +int sx_common_probe_setup(struct iio_dev *indio_dev,
> + struct i2c_client *client,
> + const struct regmap_config *regmap_config);
> +int sx_common_probe_register(struct iio_dev *indio_dev);
> +
> +extern const struct iio_buffer_setup_ops sx_common_buffer_setup_ops;
> +extern const struct iio_event_spec sx_common_events[3];
> +
> +#endif /* IIO_SX_COMMON_H */
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] iio: proximity: Add SX9324 support
2021-10-30 11:18 [PATCH 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
2021-10-30 11:18 ` [PATCH 1/5] iio: Use .realbits to extend a small signed integer Gwendal Grignou
2021-10-30 11:18 ` [PATCH 2/5] iio: sx9310: Extract common Semtech sensor logic Gwendal Grignou
@ 2021-10-30 11:18 ` Gwendal Grignou
2021-10-30 11:56 ` Lars-Peter Clausen
2021-10-30 17:04 ` Jonathan Cameron
2021-10-30 11:18 ` [PATCH 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
2021-10-30 11:18 ` [PATCH 5/5] iio: sx9324: Add dt_bidding support Gwendal Grignou
4 siblings, 2 replies; 18+ messages in thread
From: Gwendal Grignou @ 2021-10-30 11:18 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Semtech SAR sensor SX9324 is an evolution of the SX9310:
It has 4 phases that can be configure to capture and process data
from any of 3 CS pins and provide independent detection:
proximity, table proximity or body proximity.
Gather antenna data:
echo sx9324-dev3 > trigger/current_trigger
echo 1 > scan_elements/in_proximity0_en
echo 1 > buffer/enable
od -v -An --endian=big -t d2 -w2 /dev/iio\:device3
(at 10Hz, the default).
Trigger events:
Setting:
thresh_falling_period: 2 (events)
thresh_rising_period: 2 (events)
in_proximity0_thresh_either_value: 300
in_proximity0_thresh_either_hysteresis: 72
using iio_event_monitor /dev/iio\:deviceX, approaching my hand to the
antenna pad, I see:
...
Event: time: 1634763907532035297, type: proximity, channel: 0, evtype:
thresh, direction: falling
Event: time: 1634763910138104640, type: proximity, channel: 0, evtype:
thresh, direction: rising
...
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/iio/proximity/Kconfig | 18 +
drivers/iio/proximity/Makefile | 3 +-
drivers/iio/proximity/sx9324.c | 931 +++++++++++++++++++++++++++++++++
3 files changed, 951 insertions(+), 1 deletion(-)
create mode 100644 drivers/iio/proximity/sx9324.c
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index 7c7203ca3ac63..aaddf97f9b219 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -112,11 +112,15 @@ config SRF04
To compile this driver as a module, choose M here: the
module will be called srf04.
+config SX_COMMON
+ tristate
+
config SX9310
tristate "SX9310/SX9311 Semtech proximity sensor"
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
select REGMAP_I2C
+ select SX_COMMON
depends on I2C
help
Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
@@ -125,6 +129,20 @@ config SX9310
To compile this driver as a module, choose M here: the
module will be called sx9310.
+config SX9324
+ tristate "SX9324 Semtech proximity sensor"
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ select REGMAP_I2C
+ select SX_COMMON
+ depends on I2C
+ help
+ Say Y here to build a driver for Semtech's SX9324
+ proximity/button sensor.
+
+ To compile this driver as a module, choose M here: the
+ module will be called sx9324.
+
config SX9500
tristate "SX9500 Semtech proximity sensor"
select IIO_BUFFER
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index cbdac09433eb5..1b026fedc396c 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -14,7 +14,8 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
obj-$(CONFIG_SRF04) += srf04.o
obj-$(CONFIG_SRF08) += srf08.o
obj-$(CONFIG_SX9310) += sx9310.o
+obj-$(CONFIG_SX9324) += sx9324.o
+obj-$(CONFIG_SX_COMMON) += sx_common.o
obj-$(CONFIG_SX9500) += sx9500.o
obj-$(CONFIG_VCNL3020) += vcnl3020.o
obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
-
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
new file mode 100644
index 0000000000000..41d9c950c5abd
--- /dev/null
+++ b/drivers/iio/proximity/sx9324.c
@@ -0,0 +1,931 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2021 Google LLC.
+ *
+ * Driver for Semtech's SX9324 capacitive proximity/button solution.
+ * Based on SX9324 driver and copy of datasheet at:
+ * https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf
+ *
+ * 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 <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#include <linux/iio/proximity/sx_common.h>
+
+#define SX9324_DRIVER_NAME "sx9324"
+
+/* Register definitions. */
+#define SX9324_REG_IRQ_SRC SX_COMMON_REG_IRQ_SRC
+#define SX9324_REG_STAT0 0x01
+#define SX9324_REG_STAT1 0x02
+#define SX9324_REG_STAT2 0x03
+#define SX9324_REG_STAT2_COMPSTAT_MASK GENMASK(3, 0)
+#define SX9324_REG_STAT3 0x04
+#define SX9324_REG_IRQ_MSK 0x05
+#define SX9324_CONVDONE_IRQ BIT(3)
+#define SX9324_FAR_IRQ BIT(5)
+#define SX9324_CLOSE_IRQ BIT(6)
+#define SX9324_REG_IRQ_CFG0 0x06
+#define SX9324_REG_IRQ_CFG1 0x07
+#define SX9324_REG_IRQ_CFG1_FAILCOND 0x80
+#define SX9324_REG_IRQ_CFG2 0x08
+
+#define SX9324_REG_GNRL_CTRL0 0x10
+#define SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK GENMASK(4, 0)
+#define SX9324_REG_GNRL_CTRL0_SCANPERIOD_100MS 0x16
+#define SX9324_REG_GNRL_CTRL1 0x11
+#define SX9324_REG_GNRL_CTRL1_PHEN_MASK GENMASK(3, 0)
+#define SX9324_REG_GNRL_CTRL1_PAUSECTRL 0x20
+
+#define SX9324_REG_I2C_ADDR 0x14
+#define SX9324_REG_CLK_SPRD 0x15
+
+#define SX9324_REG_AFE_CTRL0 0x20
+#define SX9324_REG_AFE_CTRL1 0x21
+#define SX9324_REG_AFE_CTRL2 0x22
+#define SX9324_REG_AFE_CTRL3 0x23
+#define SX9324_REG_AFE_CTRL4 0x24
+#define SX9324_REG_AFE_CTRL4_FREQ_83_33HZ 0x40
+#define SX9324_REG_AFE_CTRL4_RESOLUTION_MASK GENMASK(2, 0)
+#define SX9324_REG_AFE_CTRL4_RES_100 0x04
+#define SX9324_REG_AFE_CTRL5 0x25
+#define SX9324_REG_AFE_CTRL6 0x26
+#define SX9324_REG_AFE_CTRL7 0x27
+#define SX9324_REG_AFE_PH0 0x28
+#define SX9324_REG_AFE_PH0_PIN_MASK(_pin) \
+ GENMASK(2 * (_pin) + 1, 2 * (_pin))
+
+#define SX9324_REG_AFE_PH1 0x29
+#define SX9324_REG_AFE_PH2 0x2a
+#define SX9324_REG_AFE_PH3 0x2b
+#define SX9324_REG_AFE_CTRL8 0x2c
+#define SX9324_REG_AFE_CTRL8_RESFILTN_4KOHM 0x02
+#define SX9324_REG_AFE_CTRL9 0x2d
+#define SX9324_REG_AFE_CTRL9_AGAIN_1 0x08
+
+#define SX9324_REG_PROX_CTRL0 0x30
+#define SX9324_REG_PROX_CTRL0_GAIN_MASK GENMASK(5, 3)
+#define SX9324_REG_PROX_CTRL0_GAIN_1 0x80
+#define SX9324_REG_PROX_CTRL0_RAWFILT_1P50 0x01
+#define SX9324_REG_PROX_CTRL1 0x31
+#define SX9324_REG_PROX_CTRL2 0x32
+#define SX9324_REG_PROX_CTRL2_AVGNEG_THRESH_16K 0x20
+#define SX9324_REG_PROX_CTRL3 0x33
+#define SX9324_REG_PROX_CTRL3_AVGDEB_2SAMPLES 0x40
+#define SX9324_REG_PROX_CTRL3_AVGPOS_THRESH_16K 0x20
+#define SX9324_REG_PROX_CTRL4 0x34
+#define SX9324_REG_PROX_CTRL4_AVGNEGFILT_MASK GENMASK(5, 3)
+#define SX9324_REG_PROX_CTRL4_AVGNEG_FILT_1P50 0x08
+#define SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK GENMASK(2, 0)
+#define SX9324_REG_PROX_CTRL3_AVGPOS_FILT_1P256 0x04
+#define SX9324_REG_PROX_CTRL5 0x35
+#define SX9324_REG_PROX_CTRL5_HYST_MASK GENMASK(5, 4)
+#define SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK GENMASK(3, 2)
+#define SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK GENMASK(1, 0)
+#define SX9324_REG_PROX_CTRL6 0x36
+#define SX9324_REG_PROX_CTRL6_PROXTHRESH_32 0x08
+#define SX9324_REG_PROX_CTRL7 0x37
+
+#define SX9324_REG_ADV_CTRL0 0x40
+#define SX9324_REG_ADV_CTRL1 0x41
+#define SX9324_REG_ADV_CTRL2 0x42
+#define SX9324_REG_ADV_CTRL3 0x43
+#define SX9324_REG_ADV_CTRL4 0x44
+#define SX9324_REG_ADV_CTRL5 0x45
+#define SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK GENMASK(3, 2)
+#define SX9324_REG_ADV_CTRL5_STARTUP_SENSOR_1 0x04
+#define SX9324_REG_ADV_CTRL5_STARTUP_METHOD_1 0x01
+#define SX9324_REG_ADV_CTRL6 0x46
+#define SX9324_REG_ADV_CTRL7 0x47
+#define SX9324_REG_ADV_CTRL8 0x48
+#define SX9324_REG_ADV_CTRL9 0x49
+#define SX9324_REG_ADV_CTRL10 0x4a
+#define SX9324_REG_ADV_CTRL11 0x4b
+#define SX9324_REG_ADV_CTRL12 0x4c
+#define SX9324_REG_ADV_CTRL13 0x4d
+#define SX9324_REG_ADV_CTRL14 0x4e
+#define SX9324_REG_ADV_CTRL15 0x4f
+#define SX9324_REG_ADV_CTRL16 0x50
+#define SX9324_REG_ADV_CTRL17 0x51
+#define SX9324_REG_ADV_CTRL18 0x52
+#define SX9324_REG_ADV_CTRL19 0x53
+#define SX9324_REG_ADV_CTRL20 0x54
+#define SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION 0xf0
+
+#define SX9324_REG_PHASE_SEL 0x60
+
+#define SX9324_REG_USEFUL_MSB 0x61
+#define SX9324_REG_USEFUL_LSB 0x62
+
+#define SX9324_REG_AVG_MSB 0x63
+#define SX9324_REG_AVG_LSB 0x64
+
+#define SX9324_REG_DIFF_MSB 0x65
+#define SX9324_REG_DIFF_LSB 0x66
+
+#define SX9324_REG_OFFSET_MSB 0x67
+#define SX9324_REG_OFFSET_LSB 0x68
+
+#define SX9324_REG_SAR_MSB 0x69
+#define SX9324_REG_SAR_LSB 0x6a
+
+#define SX9324_REG_RESET 0x9f
+/* Write this to REG_RESET to do a soft reset. */
+#define SX9324_SOFT_RESET 0xde
+
+#define SX9324_REG_WHOAMI 0xfa
+#define SX9324_REG_REVISION 0xfe
+
+/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
+#define SX9324_NUM_CHANNELS 4
+/* 3 CS pins: CS0, CS1, CS2. */
+#define SX9324_NUM_PINS 3
+
+#define SX9324_CHANNEL(idx) \
+ { \
+ .type = IIO_PROXIMITY, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
+ .info_mask_separate_available = \
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
+ .indexed = 1, \
+ .channel = idx, \
+ .address = SX9324_REG_DIFF_MSB, \
+ .event_spec = sx_common_events, \
+ .num_event_specs = ARRAY_SIZE(sx_common_events), \
+ .scan_index = idx, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_BE, \
+ }, \
+ }
+
+static const struct iio_chan_spec sx9324_channels[] = {
+ SX9324_CHANNEL(0), /* Phase 0 */
+ SX9324_CHANNEL(1), /* Phase 1 */
+ SX9324_CHANNEL(2), /* Phase 2 */
+ SX9324_CHANNEL(3), /* Phase 3 */
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
+
+static ssize_t sx9324_phase_configuration_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int i, ret, pin_idx;
+ size_t len = 0;
+
+ ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
+
+ for (i = 0; i < SX9324_NUM_PINS; i++) {
+ pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
+ sx9324_cs_pin_usage[pin_idx]);
+ }
+ buf[len - 1] = '\n';
+ return len;
+}
+
+static ssize_t sx9324_phase_configuration_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t len)
+{
+ return -EINVAL;
+}
+
+#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
+IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
+ sx9324_phase_configuration_show, \
+ sx9324_phase_configuration_store, _idx)
+
+static IIO_DEV_ATTR_PHASE_CONFIG(0);
+static IIO_DEV_ATTR_PHASE_CONFIG(1);
+static IIO_DEV_ATTR_PHASE_CONFIG(2);
+static IIO_DEV_ATTR_PHASE_CONFIG(3);
+
+/*
+ * Each entry contains the integer part (val) and the fractional part, in micro
+ * seconds. It conforms to the IIO output IIO_VAL_INT_PLUS_MICRO.
+ */
+static const struct {
+ int val;
+ int val2;
+} sx9324_samp_freq_table[] = {
+ {1000, 0}, /* 00000: Min (no idle time) */
+ {500, 0}, /* 00001: 2 ms */
+ {250, 0}, /* 00010: 4 ms */
+ {166, 666666}, /* 00011: 6 ms */
+ {125, 0}, /* 00100: 8 ms */
+ {100, 0}, /* 00101: 10 ms */
+ {71, 428571}, /* 00110: 14 ms */
+ {55, 555556}, /* 00111: 18 ms */
+ {45, 454545}, /* 01000: 22 ms */
+ {38, 461538}, /* 01001: 26 ms */
+ {33, 333333}, /* 01010: 30 ms */
+ {29, 411765}, /* 01011: 34 ms */
+ {26, 315789}, /* 01100: 38 ms */
+ {23, 809524}, /* 01101: 42 ms */
+ {21, 739130}, /* 01110: 46 ms */
+ {20, 0}, /* 01111: 50 ms */
+ {17, 857143}, /* 10000: 56 ms */
+ {16, 129032}, /* 10001: 62 ms */
+ {14, 705882}, /* 10010: 68 ms */
+ {13, 513514}, /* 10011: 74 ms */
+ {12, 500000}, /* 10100: 80 ms */
+ {11, 111111}, /* 10101: 90 ms */
+ {10, 0}, /* 10110: 100 ms (Typ.) */
+ {5, 0}, /* 10111: 200 ms */
+ {3, 333333}, /* 11000: 300 ms */
+ {2, 500000}, /* 11001: 400 ms */
+ {1, 666667}, /* 11010: 600 ms */
+ {1, 250000}, /* 11011: 800 ms */
+ {1, 0}, /* 11100: 1 s */
+ {0, 500000}, /* 11101: 2 s */
+ {0, 333333}, /* 11110: 3 s */
+ {0, 250000}, /* 11111: 4 s */
+};
+
+static const unsigned int sx9324_scan_period_table[] = {
+ 2, 15, 30, 45, 60, 90, 120, 200,
+ 400, 600, 800, 1000, 2000, 3000, 4000, 5000,
+};
+
+static ssize_t sx9324_show_samp_freq_avail(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ size_t len = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%d ",
+ sx9324_samp_freq_table[i].val,
+ sx9324_samp_freq_table[i].val2);
+ buf[len - 1] = '\n';
+ return len;
+}
+static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(sx9324_show_samp_freq_avail);
+
+static const struct regmap_range sx9324_writable_reg_ranges[] = {
+ /*
+ * To set COMPSTAT for compensation, even if datasheet says register is
+ * RO.
+ */
+ regmap_reg_range(SX9324_REG_STAT2, SX9324_REG_STAT2),
+ regmap_reg_range(SX9324_REG_IRQ_MSK, SX9324_REG_IRQ_CFG2),
+ regmap_reg_range(SX9324_REG_GNRL_CTRL0, SX9324_REG_GNRL_CTRL1),
+ /* Leave i2c and clock spreading as unavailable */
+ regmap_reg_range(SX9324_REG_AFE_CTRL0, SX9324_REG_AFE_CTRL9),
+ regmap_reg_range(SX9324_REG_PROX_CTRL0, SX9324_REG_PROX_CTRL7),
+ regmap_reg_range(SX9324_REG_ADV_CTRL0, SX9324_REG_ADV_CTRL20),
+ regmap_reg_range(SX9324_REG_PHASE_SEL, SX9324_REG_PHASE_SEL),
+ regmap_reg_range(SX9324_REG_OFFSET_MSB, SX9324_REG_OFFSET_LSB),
+ regmap_reg_range(SX9324_REG_RESET, SX9324_REG_RESET),
+};
+
+static const struct regmap_access_table sx9324_writeable_regs = {
+ .yes_ranges = sx9324_writable_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(sx9324_writable_reg_ranges),
+};
+
+/*
+ * All allocated registers are readable, so we just list unallocated
+ * ones.
+ */
+static const struct regmap_range sx9324_non_readable_reg_ranges[] = {
+ regmap_reg_range(SX9324_REG_IRQ_CFG2 + 1, SX9324_REG_GNRL_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_GNRL_CTRL1 + 1, SX9324_REG_AFE_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_AFE_CTRL9 + 1, SX9324_REG_PROX_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_PROX_CTRL7 + 1, SX9324_REG_ADV_CTRL0 - 1),
+ regmap_reg_range(SX9324_REG_ADV_CTRL20 + 1, SX9324_REG_PHASE_SEL - 1),
+ regmap_reg_range(SX9324_REG_SAR_LSB + 1, SX9324_REG_RESET - 1),
+ regmap_reg_range(SX9324_REG_RESET + 1, SX9324_REG_WHOAMI - 1),
+ regmap_reg_range(SX9324_REG_WHOAMI + 1, SX9324_REG_REVISION - 1),
+};
+
+static const struct regmap_access_table sx9324_readable_regs = {
+ .no_ranges = sx9324_non_readable_reg_ranges,
+ .n_no_ranges = ARRAY_SIZE(sx9324_non_readable_reg_ranges),
+};
+
+static const struct regmap_range sx9324_volatile_reg_ranges[] = {
+ regmap_reg_range(SX9324_REG_IRQ_SRC, SX9324_REG_STAT3),
+ regmap_reg_range(SX9324_REG_USEFUL_MSB, SX9324_REG_DIFF_LSB),
+ regmap_reg_range(SX9324_REG_SAR_MSB, SX9324_REG_SAR_LSB),
+ regmap_reg_range(SX9324_REG_WHOAMI, SX9324_REG_WHOAMI),
+ regmap_reg_range(SX9324_REG_REVISION, SX9324_REG_REVISION),
+};
+
+static const struct regmap_access_table sx9324_volatile_regs = {
+ .yes_ranges = sx9324_volatile_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(sx9324_volatile_reg_ranges),
+};
+
+static const struct regmap_config sx9324_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = SX9324_REG_REVISION,
+ .cache_type = REGCACHE_RBTREE,
+
+ .wr_table = &sx9324_writeable_regs,
+ .rd_table = &sx9324_readable_regs,
+ .volatile_table = &sx9324_volatile_regs,
+};
+
+static int sx9324_read_prox_data(struct sx_common_data *data,
+ const struct iio_chan_spec *chan,
+ __be16 *val)
+{
+ int ret;
+
+ ret = regmap_write(data->regmap, SX9324_REG_PHASE_SEL, chan->channel);
+ if (ret < 0)
+ return ret;
+
+ return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
+}
+
+/*
+ * If we have no interrupt support, we have to wait for a scan period
+ * after enabling a channel to get a result.
+ */
+static int sx9324_wait_for_sample(struct sx_common_data *data)
+{
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL0, &val);
+ if (ret < 0)
+ return ret;
+ val = FIELD_GET(SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, val);
+
+ msleep(sx9324_scan_period_table[val]);
+
+ return 0;
+}
+
+static int sx9324_read_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int reg, regval;
+ int ret;
+
+ reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret)
+ return ret;
+
+ *val = 1 << FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_samp_freq(struct sx_common_data *data,
+ int *val, int *val2)
+{
+ int ret;
+ unsigned int regval;
+
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL0, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, regval);
+ *val = sx9324_samp_freq_table[regval].val;
+ *val2 = sx9324_samp_freq_table[regval].val2;
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int sx9324_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = sx_common_read_proximity(data, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = sx9324_read_gain(data, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return sx9324_read_samp_freq(data, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sx9324_set_samp_freq(struct sx_common_data *data,
+ int val, int val2)
+{
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
+ if (val == sx9324_samp_freq_table[i].val &&
+ val2 == sx9324_samp_freq_table[i].val2)
+ break;
+
+ if (i == ARRAY_SIZE(sx9324_samp_freq_table))
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+
+ ret = regmap_update_bits(data->regmap,
+ SX9324_REG_GNRL_CTRL0,
+ SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
+
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_read_thresh(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int reg;
+ unsigned int regval;
+ int ret;
+
+ /*
+ * TODO(gwendal): Depending on the phase function
+ * (proximity/table/body), retrieve the right threshold.
+ */
+ reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
+ ret = regmap_read(data->regmap, reg, ®val);
+ if (ret)
+ return ret;
+
+ if (regval <= 1)
+ *val = regval;
+ else
+ *val = (regval * regval) / 2;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_hysteresis(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int *val)
+{
+ unsigned int regval, pthresh;
+ int ret;
+
+ ret = sx9324_read_thresh(data, chan, &pthresh);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_HYST_MASK, regval);
+ if (!regval)
+ *val = 0;
+ else
+ *val = pthresh >> (5 - regval);
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_far_debounce(struct sx_common_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, regval);
+ if (regval)
+ *val = 1 << regval;
+ else
+ *val = 0;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_close_debounce(struct sx_common_data *data, int *val)
+{
+ unsigned int regval;
+ int ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_PROX_CTRL5, ®val);
+ if (ret)
+ return ret;
+
+ regval = FIELD_GET(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, regval);
+ if (regval)
+ *val = 1 << regval;
+ else
+ *val = 0;
+
+ return IIO_VAL_INT;
+}
+
+static int sx9324_read_event_val(struct iio_dev *indio_dev,
+ 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 sx_common_data *data = iio_priv(indio_dev);
+
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return sx9324_read_thresh(data, chan, val);
+ case IIO_EV_INFO_PERIOD:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return sx9324_read_far_debounce(data, val);
+ case IIO_EV_DIR_FALLING:
+ return sx9324_read_close_debounce(data, val);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_HYSTERESIS:
+ return sx9324_read_hysteresis(data, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sx9324_write_thresh(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int val)
+{
+ unsigned int reg;
+ int ret;
+
+ reg = SX9324_REG_PROX_CTRL6 + chan->channel / 2;
+
+ if (val >= 1)
+ val = int_sqrt(2 * val);
+
+ if (val > 0xff)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, reg, val);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_hysteresis(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int _val)
+{
+ unsigned int hyst, val = _val;
+ int ret, pthresh;
+
+ ret = sx9324_read_thresh(data, chan, &pthresh);
+ if (ret < 0)
+ return ret;
+
+ if (val == 0)
+ hyst = 0;
+ else if (val >= pthresh >> 2)
+ hyst = 3;
+ else if (val >= pthresh >> 3)
+ hyst = 2;
+ else if (val >= pthresh >> 4)
+ hyst = 1;
+ else
+ return -EINVAL;
+
+ hyst = FIELD_PREP(SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_HYST_MASK, hyst);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_far_debounce(struct sx_common_data *data, int val)
+{
+ int ret;
+ unsigned int regval;
+
+ if (val > 0)
+ val = ilog2(val);
+ if (!FIELD_FIT(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val))
+ return -EINVAL;
+
+ regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, val);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK,
+ regval);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_close_debounce(struct sx_common_data *data, int val)
+{
+ int ret;
+ unsigned int regval;
+
+ if (val > 0)
+ val = ilog2(val);
+ if (!FIELD_FIT(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val))
+ return -EINVAL;
+
+ regval = FIELD_PREP(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, val);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, SX9324_REG_PROX_CTRL5,
+ SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK,
+ regval);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_event_val(struct iio_dev *indio_dev,
+ 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 sx_common_data *data = iio_priv(indio_dev);
+
+ if (chan->type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return sx9324_write_thresh(data, chan, val);
+ case IIO_EV_INFO_PERIOD:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return sx9324_write_far_debounce(data, val);
+ case IIO_EV_DIR_FALLING:
+ return sx9324_write_close_debounce(data, val);
+ default:
+ return -EINVAL;
+ }
+ case IIO_EV_INFO_HYSTERESIS:
+ return sx9324_write_hysteresis(data, chan, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int sx9324_write_gain(struct sx_common_data *data,
+ const struct iio_chan_spec *chan, int val)
+{
+ unsigned int gain, reg;
+ int ret;
+
+ gain = ilog2(val);
+ reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
+ gain = FIELD_PREP(SX9324_REG_PROX_CTRL0_GAIN_MASK, gain);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, reg,
+ SX9324_REG_PROX_CTRL0_GAIN_MASK,
+ gain);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int sx9324_write_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int val, int val2,
+ long mask)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return sx9324_set_samp_freq(data, val, val2);
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ return sx9324_write_gain(data, chan, val);
+ }
+
+ return -EINVAL;
+}
+
+static struct attribute *sx9324_attributes[] = {
+ &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+ &iio_dev_attr_in_proximity_configuration0.dev_attr.attr,
+ &iio_dev_attr_in_proximity_configuration1.dev_attr.attr,
+ &iio_dev_attr_in_proximity_configuration2.dev_attr.attr,
+ &iio_dev_attr_in_proximity_configuration3.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group sx9324_attribute_group = {
+ .attrs = sx9324_attributes,
+};
+
+static const struct iio_info sx9324_info = {
+ .attrs = &sx9324_attribute_group,
+ .read_raw = sx9324_read_raw,
+ .read_avail = sx_common_read_avail,
+ .read_event_value = sx9324_read_event_val,
+ .write_raw = sx9324_write_raw,
+ .write_event_value = sx9324_write_event_val,
+ .read_event_config = sx_common_read_event_config,
+ .write_event_config = sx_common_write_event_config,
+};
+
+/* Activate all channels and perform an initial compensation. */
+static int sx9324_init_compensation(struct iio_dev *indio_dev)
+{
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ /* run the compensation phase on all channels */
+ ret = regmap_update_bits(data->regmap, SX9324_REG_STAT2,
+ SX9324_REG_STAT2_COMPSTAT_MASK,
+ SX9324_REG_STAT2_COMPSTAT_MASK);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(data->regmap, SX9324_REG_STAT2, val,
+ !(val & SX9324_REG_STAT2_COMPSTAT_MASK),
+ 20000, 2000000);
+ if (ret == -ETIMEDOUT)
+ dev_err(&data->client->dev,
+ "initial compensation timed out: 0x%02x\n",
+ val);
+ return ret;
+}
+
+static int sx9324_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct iio_dev *indio_dev;
+ struct sx_common_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->reg_stat = SX9324_REG_STAT0;
+ data->reg_irq_msk = SX9324_REG_IRQ_MSK;
+ data->reg_enable_chan = SX9324_REG_GNRL_CTRL1;
+ data->reg_reset = SX9324_REG_RESET;
+
+ data->mask_enable_chan = SX9324_REG_GNRL_CTRL1_PHEN_MASK;
+ data->irq_msk_offset = 3;
+ data->num_channels = SX9324_NUM_CHANNELS;
+
+ ret = sx_common_probe_setup(indio_dev, client, &sx9324_regmap_config);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(data->regmap, SX9324_REG_WHOAMI, &data->whoami);
+ if (ret) {
+ dev_err(dev, "error in reading WHOAMI register: %d", ret);
+ return ret;
+ }
+
+ /* Low level entry points */
+ data->read_prox_data = sx9324_read_prox_data;
+ data->init_compensation = sx9324_init_compensation;
+ data->wait_for_sample = sx9324_wait_for_sample;
+
+ indio_dev->channels = sx9324_channels;
+ indio_dev->num_channels = ARRAY_SIZE(sx9324_channels);
+ indio_dev->info = &sx9324_info;
+
+ return sx_common_probe_register(indio_dev);
+}
+
+static int __maybe_unused sx9324_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct sx_common_data *data = iio_priv(indio_dev);
+ unsigned int regval;
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, SX9324_REG_GNRL_CTRL1, ®val);
+
+ data->suspend_ctrl =
+ FIELD_GET(SX9324_REG_GNRL_CTRL1_PHEN_MASK, regval);
+
+ if (ret < 0)
+ goto out;
+
+ // disable all phases, send the device to sleep
+ ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1, 0);
+
+out:
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static int __maybe_unused sx9324_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, SX9324_REG_GNRL_CTRL1,
+ data->suspend_ctrl | SX9324_REG_GNRL_CTRL1_PAUSECTRL);
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static const struct dev_pm_ops sx9324_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sx9324_suspend, sx9324_resume)
+};
+
+static const struct acpi_device_id sx9324_acpi_match[] = {
+ { "STH9324", SX9324_WHOAMI_VALUE},
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
+
+static const struct of_device_id sx9324_of_match[] = {
+ { .compatible = "semtech,sx9324", (void *)SX9324_WHOAMI_VALUE},
+ { }
+};
+MODULE_DEVICE_TABLE(of, sx9324_of_match);
+
+static const struct i2c_device_id sx9324_id[] = {
+ {"sx9324", SX9324_WHOAMI_VALUE},
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, sx9324_id);
+
+static struct i2c_driver sx9324_driver = {
+ .driver = {
+ .name = SX9324_DRIVER_NAME,
+ .acpi_match_table = ACPI_PTR(sx9324_acpi_match),
+ .of_match_table = of_match_ptr(sx9324_of_match),
+ .pm = &sx9324_pm_ops,
+
+ /*
+ * Lots of i2c transfers in probe + over 200 ms waiting in
+ * sx9324_init_compensation() mean a slow probe; prefer async
+ * so we don't delay boot if we're builtin to the kernel.
+ */
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe_new = sx9324_probe,
+ .id_table = sx9324_id,
+};
+module_i2c_driver(sx9324_driver);
+
+MODULE_AUTHOR("Gwendal Grignou <gwendal@chromium.org>");
+MODULE_DESCRIPTION("Driver for Semtech SX9324 proximity sensor");
+MODULE_LICENSE("GPL v2");
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] iio: proximity: Add SX9324 support
2021-10-30 11:18 ` [PATCH 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
@ 2021-10-30 11:56 ` Lars-Peter Clausen
2021-10-30 17:04 ` Jonathan Cameron
1 sibling, 0 replies; 18+ messages in thread
From: Lars-Peter Clausen @ 2021-10-30 11:56 UTC (permalink / raw)
To: Gwendal Grignou, jic23, swboyd; +Cc: andy.shevchenko, linux-iio
On 10/30/21 1:18 PM, Gwendal Grignou wrote:
> [...]
Driver looks really great! Just a few small comments.
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 7c7203ca3ac63..aaddf97f9b219 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -112,11 +112,15 @@ config SRF04
> To compile this driver as a module, choose M here: the
> module will be called srf04.
>
> +config SX_COMMON
> + tristate
> +
> config SX9310
> tristate "SX9310/SX9311 Semtech proximity sensor"
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> select REGMAP_I2C
> + select SX_COMMON
This part belongs int the previous patch. Same with the Makefile update
for the common code.
> depends on I2C
> help
> Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
> [...]
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> new file mode 100644
> index 0000000000000..41d9c950c5abd
> --- /dev/null
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -0,0 +1,931 @@
> [...]
> +
> +#include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/gpio/consumer.h>
No GPIOs in this driver.
> +#include <linux/kernel.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> [..]
> +
> +static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
> +
> +static ssize_t sx9324_phase_configuration_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct sx_common_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int i, ret, pin_idx;
> + size_t len = 0;
> +
> + ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
> +
> + for (i = 0; i < SX9324_NUM_PINS; i++) {
> + pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
> + sx9324_cs_pin_usage[pin_idx]);
> + }
> + buf[len - 1] = '\n';
> + return len;
> +}
We have IIO_ENUM, which seems like a good candidate for this.
> +
> +static ssize_t sx9324_phase_configuration_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + return -EINVAL;
> +}
> +
> +#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
> +IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
> + sx9324_phase_configuration_show, \
> + sx9324_phase_configuration_store, _idx)
> +
> +static IIO_DEV_ATTR_PHASE_CONFIG(0);
> +static IIO_DEV_ATTR_PHASE_CONFIG(1);
> +static IIO_DEV_ATTR_PHASE_CONFIG(2);
> +static IIO_DEV_ATTR_PHASE_CONFIG(3);
> +
> +/*
> + * Each entry contains the integer part (val) and the fractional part, in micro
> + * seconds. It conforms to the IIO output IIO_VAL_INT_PLUS_MICRO.
> + */
> +static const struct {
> + int val;
> + int val2;
> +} sx9324_samp_freq_table[] = {
> + {1000, 0}, /* 00000: Min (no idle time) */
> + {500, 0}, /* 00001: 2 ms */
> + {250, 0}, /* 00010: 4 ms */
> + {166, 666666}, /* 00011: 6 ms */
> + {125, 0}, /* 00100: 8 ms */
> + {100, 0}, /* 00101: 10 ms */
> + {71, 428571}, /* 00110: 14 ms */
> + {55, 555556}, /* 00111: 18 ms */
> + {45, 454545}, /* 01000: 22 ms */
> + {38, 461538}, /* 01001: 26 ms */
> + {33, 333333}, /* 01010: 30 ms */
> + {29, 411765}, /* 01011: 34 ms */
> + {26, 315789}, /* 01100: 38 ms */
> + {23, 809524}, /* 01101: 42 ms */
> + {21, 739130}, /* 01110: 46 ms */
> + {20, 0}, /* 01111: 50 ms */
> + {17, 857143}, /* 10000: 56 ms */
> + {16, 129032}, /* 10001: 62 ms */
> + {14, 705882}, /* 10010: 68 ms */
> + {13, 513514}, /* 10011: 74 ms */
> + {12, 500000}, /* 10100: 80 ms */
> + {11, 111111}, /* 10101: 90 ms */
> + {10, 0}, /* 10110: 100 ms (Typ.) */
> + {5, 0}, /* 10111: 200 ms */
> + {3, 333333}, /* 11000: 300 ms */
> + {2, 500000}, /* 11001: 400 ms */
> + {1, 666667}, /* 11010: 600 ms */
> + {1, 250000}, /* 11011: 800 ms */
> + {1, 0}, /* 11100: 1 s */
> + {0, 500000}, /* 11101: 2 s */
> + {0, 333333}, /* 11110: 3 s */
> + {0, 250000}, /* 11111: 4 s */
> +};
> +
> +static const unsigned int sx9324_scan_period_table[] = {
> + 2, 15, 30, 45, 60, 90, 120, 200,
> + 400, 600, 800, 1000, 2000, 3000, 4000, 5000,
> +};
> +
> +static ssize_t sx9324_show_samp_freq_avail(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + size_t len = 0;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%d ",
> + sx9324_samp_freq_table[i].val,
> + sx9324_samp_freq_table[i].val2);
> + buf[len - 1] = '\n';
> + return len;
> +}
> +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(sx9324_show_samp_freq_avail);
Consider implementing the `read_avail`. This means you just have to
return your table and the IIO core will handle the formatting.
> +
> [...]
> +
> +static int sx9324_set_samp_freq(struct sx_common_data *data,
> + int val, int val2)
> +{
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(sx9324_samp_freq_table); i++)
> + if (val == sx9324_samp_freq_table[i].val &&
> + val2 == sx9324_samp_freq_table[i].val2)
> + break;
> +
> + if (i == ARRAY_SIZE(sx9324_samp_freq_table))
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
Does this lock here actually protect against anything?
regmap_update_bits() has its own internal lock. Same with other locks
around regmap calls.
> +
> + ret = regmap_update_bits(data->regmap,
> + SX9324_REG_GNRL_CTRL0,
> + SX9324_REG_GNRL_CTRL0_SCANPERIOD_MASK, i);
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] iio: proximity: Add SX9324 support
2021-10-30 11:18 ` [PATCH 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
2021-10-30 11:56 ` Lars-Peter Clausen
@ 2021-10-30 17:04 ` Jonathan Cameron
2021-11-04 7:15 ` Gwendal Grignou
1 sibling, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2021-10-30 17:04 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, 30 Oct 2021 04:18:25 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> Semtech SAR sensor SX9324 is an evolution of the SX9310:
> It has 4 phases that can be configure to capture and process data
> from any of 3 CS pins and provide independent detection:
> proximity, table proximity or body proximity.
>
> Gather antenna data:
> echo sx9324-dev3 > trigger/current_trigger
> echo 1 > scan_elements/in_proximity0_en
> echo 1 > buffer/enable
> od -v -An --endian=big -t d2 -w2 /dev/iio\:device3
> (at 10Hz, the default).
>
> Trigger events:
> Setting:
> thresh_falling_period: 2 (events)
> thresh_rising_period: 2 (events)
> in_proximity0_thresh_either_value: 300
> in_proximity0_thresh_either_hysteresis: 72
>
> using iio_event_monitor /dev/iio\:deviceX, approaching my hand to the
> antenna pad, I see:
> ...
> Event: time: 1634763907532035297, type: proximity, channel: 0, evtype:
> thresh, direction: falling
> Event: time: 1634763910138104640, type: proximity, channel: 0, evtype:
> thresh, direction: rising
> ...
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Various comment inline, but biggest one is lack of docs of non standard ABI
which means discussion is very difficult.
Jonathan
> ---
> drivers/iio/proximity/Kconfig | 18 +
> drivers/iio/proximity/Makefile | 3 +-
> drivers/iio/proximity/sx9324.c | 931 +++++++++++++++++++++++++++++++++
> 3 files changed, 951 insertions(+), 1 deletion(-)
> create mode 100644 drivers/iio/proximity/sx9324.c
>
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index 7c7203ca3ac63..aaddf97f9b219 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -112,11 +112,15 @@ config SRF04
> To compile this driver as a module, choose M here: the
> module will be called srf04.
>
> +config SX_COMMON
> + tristate
> +
> config SX9310
> tristate "SX9310/SX9311 Semtech proximity sensor"
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> select REGMAP_I2C
> + select SX_COMMON
> depends on I2C
> help
> Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
> @@ -125,6 +129,20 @@ config SX9310
> To compile this driver as a module, choose M here: the
> module will be called sx9310.
>
> +config SX9324
> + tristate "SX9324 Semtech proximity sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select REGMAP_I2C
> + select SX_COMMON
> + depends on I2C
> + help
> + Say Y here to build a driver for Semtech's SX9324
> + proximity/button sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called sx9324.
> +
> config SX9500
> tristate "SX9500 Semtech proximity sensor"
> select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index cbdac09433eb5..1b026fedc396c 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -14,7 +14,8 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
> obj-$(CONFIG_SRF04) += srf04.o
> obj-$(CONFIG_SRF08) += srf08.o
> obj-$(CONFIG_SX9310) += sx9310.o
> +obj-$(CONFIG_SX9324) += sx9324.o
> +obj-$(CONFIG_SX_COMMON) += sx_common.o
Previous patch...
> obj-$(CONFIG_SX9500) += sx9500.o
> obj-$(CONFIG_VCNL3020) += vcnl3020.o
> obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
> -
stray
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> new file mode 100644
> index 0000000000000..41d9c950c5abd
> --- /dev/null
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -0,0 +1,931 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2021 Google LLC.
> + *
> + * Driver for Semtech's SX9324 capacitive proximity/button solution.
> + * Based on SX9324 driver and copy of datasheet at:
> + * https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf
> + *
> + * 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.
> + */
SPDX tag usually means we can drop the GPL license blurb.
> +
> +
> +/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
> +#define SX9324_NUM_CHANNELS 4
> +/* 3 CS pins: CS0, CS1, CS2. */
> +#define SX9324_NUM_PINS 3
> +
> +#define SX9324_CHANNEL(idx) \
> + { \
> + .type = IIO_PROXIMITY, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> + .info_mask_separate_available = \
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> + .indexed = 1, \
> + .channel = idx, \
> + .address = SX9324_REG_DIFF_MSB, \
> + .event_spec = sx_common_events, \
> + .num_event_specs = ARRAY_SIZE(sx_common_events), \
> + .scan_index = idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + }, \
> + }
> +
> +static const struct iio_chan_spec sx9324_channels[] = {
> + SX9324_CHANNEL(0), /* Phase 0 */
> + SX9324_CHANNEL(1), /* Phase 1 */
> + SX9324_CHANNEL(2), /* Phase 2 */
> + SX9324_CHANNEL(3), /* Phase 3 */
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
> +
> +static ssize_t sx9324_phase_configuration_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct sx_common_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int i, ret, pin_idx;
> + size_t len = 0;
> +
> + ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
> +
> + for (i = 0; i < SX9324_NUM_PINS; i++) {
> + pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
> + sx9324_cs_pin_usage[pin_idx]);
> + }
> + buf[len - 1] = '\n';
> + return len;
> +}
> +
> +static ssize_t sx9324_phase_configuration_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + return -EINVAL;
Don't do this. If they are read only treat them correctly as such
with appropriate permissions etc.
> +}
> +
> +#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
> +IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
> + sx9324_phase_configuration_show, \
> + sx9324_phase_configuration_store, _idx)
> +
> +static IIO_DEV_ATTR_PHASE_CONFIG(0);
> +static IIO_DEV_ATTR_PHASE_CONFIG(1);
> +static IIO_DEV_ATTR_PHASE_CONFIG(2);
> +static IIO_DEV_ATTR_PHASE_CONFIG(3);
Documentation of these? I'm not going to comment on them without appropriate
docs in
Documentation/ABI/testing/
Just wastes time figuring out what they are.
...
> +static int sx9324_write_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int val, int val2,
> + long mask)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return sx9324_set_samp_freq(data, val, val2);
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + return sx9324_write_gain(data, chan, val);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static struct attribute *sx9324_attributes[] = {
> + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
As mentioned in previous review, ideally move this to using the
read_avail callback.
> + &iio_dev_attr_in_proximity_configuration0.dev_attr.attr,
> + &iio_dev_attr_in_proximity_configuration1.dev_attr.attr,
> + &iio_dev_attr_in_proximity_configuration2.dev_attr.attr,
> + &iio_dev_attr_in_proximity_configuration3.dev_attr.attr,
> + NULL
> +};
...
> +
> +static const struct acpi_device_id sx9324_acpi_match[] = {
> + { "STH9324", SX9324_WHOAMI_VALUE},
Any reference to a board, preferably with a dump of the DSDT/SSDT
that uses this or an official doc etc to justify that it's valid?
STH isn't in the pnp database so if it's in the wild we need a comment
to say where.
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
> +
...
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] iio: proximity: Add SX9324 support
2021-10-30 17:04 ` Jonathan Cameron
@ 2021-11-04 7:15 ` Gwendal Grignou
2021-11-04 18:15 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Gwendal Grignou @ 2021-11-04 7:15 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, Oct 30, 2021 at 9:59 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 Oct 2021 04:18:25 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > Semtech SAR sensor SX9324 is an evolution of the SX9310:
> > It has 4 phases that can be configure to capture and process data
> > from any of 3 CS pins and provide independent detection:
> > proximity, table proximity or body proximity.
> >
> > Gather antenna data:
> > echo sx9324-dev3 > trigger/current_trigger
> > echo 1 > scan_elements/in_proximity0_en
> > echo 1 > buffer/enable
> > od -v -An --endian=big -t d2 -w2 /dev/iio\:device3
> > (at 10Hz, the default).
> >
> > Trigger events:
> > Setting:
> > thresh_falling_period: 2 (events)
> > thresh_rising_period: 2 (events)
> > in_proximity0_thresh_either_value: 300
> > in_proximity0_thresh_either_hysteresis: 72
> >
> > using iio_event_monitor /dev/iio\:deviceX, approaching my hand to the
> > antenna pad, I see:
> > ...
> > Event: time: 1634763907532035297, type: proximity, channel: 0, evtype:
> > thresh, direction: falling
> > Event: time: 1634763910138104640, type: proximity, channel: 0, evtype:
> > thresh, direction: rising
> > ...
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>
> Various comment inline, but biggest one is lack of docs of non standard ABI
> which means discussion is very difficult.
>
> Jonathan
>
> > ---
> > drivers/iio/proximity/Kconfig | 18 +
> > drivers/iio/proximity/Makefile | 3 +-
> > drivers/iio/proximity/sx9324.c | 931 +++++++++++++++++++++++++++++++++
> > 3 files changed, 951 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/iio/proximity/sx9324.c
> >
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index 7c7203ca3ac63..aaddf97f9b219 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -112,11 +112,15 @@ config SRF04
> > To compile this driver as a module, choose M here: the
> > module will be called srf04.
> >
> > +config SX_COMMON
> > + tristate
> > +
> > config SX9310
> > tristate "SX9310/SX9311 Semtech proximity sensor"
> > select IIO_BUFFER
> > select IIO_TRIGGERED_BUFFER
> > select REGMAP_I2C
> > + select SX_COMMON
> > depends on I2C
> > help
> > Say Y here to build a driver for Semtech's SX9310/SX9311 capacitive
> > @@ -125,6 +129,20 @@ config SX9310
> > To compile this driver as a module, choose M here: the
> > module will be called sx9310.
> >
> > +config SX9324
> > + tristate "SX9324 Semtech proximity sensor"
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + select REGMAP_I2C
> > + select SX_COMMON
> > + depends on I2C
> > + help
> > + Say Y here to build a driver for Semtech's SX9324
> > + proximity/button sensor.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called sx9324.
> > +
> > config SX9500
> > tristate "SX9500 Semtech proximity sensor"
> > select IIO_BUFFER
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index cbdac09433eb5..1b026fedc396c 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -14,7 +14,8 @@ obj-$(CONFIG_RFD77402) += rfd77402.o
> > obj-$(CONFIG_SRF04) += srf04.o
> > obj-$(CONFIG_SRF08) += srf08.o
> > obj-$(CONFIG_SX9310) += sx9310.o
> > +obj-$(CONFIG_SX9324) += sx9324.o
> > +obj-$(CONFIG_SX_COMMON) += sx_common.o
> Previous patch...
Done
>
> > obj-$(CONFIG_SX9500) += sx9500.o
> > obj-$(CONFIG_VCNL3020) += vcnl3020.o
> > obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
> > -
> stray
I just realized I missed this one.
>
> > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> > new file mode 100644
> > index 0000000000000..41d9c950c5abd
> > --- /dev/null
> > +++ b/drivers/iio/proximity/sx9324.c
> > @@ -0,0 +1,931 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2021 Google LLC.
> > + *
> > + * Driver for Semtech's SX9324 capacitive proximity/button solution.
> > + * Based on SX9324 driver and copy of datasheet at:
> > + * https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf
> > + *
> > + * 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.
> > + */
>
> SPDX tag usually means we can drop the GPL license blurb.
Done
>
> > +
>
> > +
> > +/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
> > +#define SX9324_NUM_CHANNELS 4
> > +/* 3 CS pins: CS0, CS1, CS2. */
> > +#define SX9324_NUM_PINS 3
> > +
> > +#define SX9324_CHANNEL(idx) \
> > + { \
> > + .type = IIO_PROXIMITY, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> > + .info_mask_separate_available = \
> > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> > + .indexed = 1, \
> > + .channel = idx, \
> > + .address = SX9324_REG_DIFF_MSB, \
> > + .event_spec = sx_common_events, \
> > + .num_event_specs = ARRAY_SIZE(sx_common_events), \
> > + .scan_index = idx, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = 12, \
> > + .storagebits = 16, \
> > + .endianness = IIO_BE, \
> > + }, \
> > + }
> > +
> > +static const struct iio_chan_spec sx9324_channels[] = {
> > + SX9324_CHANNEL(0), /* Phase 0 */
> > + SX9324_CHANNEL(1), /* Phase 1 */
> > + SX9324_CHANNEL(2), /* Phase 2 */
> > + SX9324_CHANNEL(3), /* Phase 3 */
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const char * const sx9324_cs_pin_usage[] = { "HZ", "MI", "DS", "GD" };
> > +
> > +static ssize_t sx9324_phase_configuration_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct sx_common_data *data = iio_priv(indio_dev);
> > + unsigned int val;
> > + int i, ret, pin_idx;
> > + size_t len = 0;
> > +
> > + ret = regmap_read(data->regmap, SX9324_REG_AFE_PH0 + this_attr->address, &val);
> > +
> > + for (i = 0; i < SX9324_NUM_PINS; i++) {
> > + pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i);
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s,",
> > + sx9324_cs_pin_usage[pin_idx]);
> > + }
> > + buf[len - 1] = '\n';
> > + return len;
> > +}
> > +
> > +static ssize_t sx9324_phase_configuration_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf,
> > + size_t len)
> > +{
> > + return -EINVAL;
>
> Don't do this. If they are read only treat them correctly as such
> with appropriate permissions etc.
used channel .ext_info instead.
>
> > +}
> > +
> > +#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
> > +IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
> > + sx9324_phase_configuration_show, \
> > + sx9324_phase_configuration_store, _idx)
> > +
> > +static IIO_DEV_ATTR_PHASE_CONFIG(0);
> > +static IIO_DEV_ATTR_PHASE_CONFIG(1);
> > +static IIO_DEV_ATTR_PHASE_CONFIG(2);
> > +static IIO_DEV_ATTR_PHASE_CONFIG(3);
>
> Documentation of these? I'm not going to comment on them without appropriate
> docs in
>
> Documentation/ABI/testing/
>
> Just wastes time figuring out what they are.
It is not easy to explain without the doc (not released by semtech but
available at https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf),
but let me try:
Each sensor has 3 inputs, CS0, CS1 and CS2. Hardware engineers decide
if the input is not connected (HZ), grounded (GD), connected to an
antenna where it can act as a base (DS - data shield), or measured
input (MI).
The sensor rotates measurement across 4 phases (PH0, PH1, PH2, PH3),
where the inputs are configured and then measured.
By default, during the first phase, [PH0], CS0 is measured, while CS1
and CS2 are used as shields. `cat in_proximity0_mode` returns
"MI,DS,DS".
I did not make this change configurable: if the hardware designer
decided to ground or leave an input dangling, it does not make sense
to set it as a shield. [it can be changed if the need arises].
Having 4 phases allows different treatment of the input. For instance,
one phase could be used for object detection,the other for body
detection. Configuring each phase is not implemented yet.
Add missing documentation in ABI/testing/sysfs-bus-iio-sx9324.
>
>
> ...
>us
>
>
> > +static int sx9324_write_raw(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, int val, int val2,
> > + long mask)
> > +{
> > + struct sx_common_data *data = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + return sx9324_set_samp_freq(data, val, val2);
> > + case IIO_CHAN_INFO_HARDWAREGAIN:
> > + return sx9324_write_gain(data, chan, val);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static struct attribute *sx9324_attributes[] = {
> > + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>
> As mentioned in previous review, ideally move this to using the
> read_avail callback.
Done.
>
> > + &iio_dev_attr_in_proximity_configuration0.dev_attr.attr,
> > + &iio_dev_attr_in_proximity_configuration1.dev_attr.attr,
> > + &iio_dev_attr_in_proximity_configuration2.dev_attr.attr,
> > + &iio_dev_attr_in_proximity_configuration3.dev_attr.attr,
> > + NULL
> > +};
>
> ...
>
> > +
> > +static const struct acpi_device_id sx9324_acpi_match[] = {
> > + { "STH9324", SX9324_WHOAMI_VALUE},
>
> Any reference to a board, preferably with a dump of the DSDT/SSDT
> that uses this or an official doc etc to justify that it's valid?
>
> STH isn't in the pnp database so if it's in the wild we need a comment
> to say where.
STH93xx follow the convention adopted for older sensors:
Semtech SX9310: STH9310: https://drivers.eu/DeviceId/ACPI%5CSTH9310
Semtech SX9500: STH9500: https://drivers.eu/DeviceId/ACPI%5CSTH9500
>
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, sx9324_acpi_match);
> > +
> ...
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/5] iio: proximity: Add SX9324 support
2021-11-04 7:15 ` Gwendal Grignou
@ 2021-11-04 18:15 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-11-04 18:15 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
> > > +static ssize_t sx9324_phase_configuration_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf,
> > > + size_t len)
> > > +{
> > > + return -EINVAL;
> >
> > Don't do this. If they are read only treat them correctly as such
> > with appropriate permissions etc.
> used channel .ext_info instead.
Even better.
> >
> > > +}
> > > +
> > > +#define IIO_DEV_ATTR_PHASE_CONFIG(_idx) \
> > > +IIO_DEVICE_ATTR(in_proximity_configuration##_idx, 0644, \
> > > + sx9324_phase_configuration_show, \
> > > + sx9324_phase_configuration_store, _idx)
> > > +
> > > +static IIO_DEV_ATTR_PHASE_CONFIG(0);
> > > +static IIO_DEV_ATTR_PHASE_CONFIG(1);
> > > +static IIO_DEV_ATTR_PHASE_CONFIG(2);
> > > +static IIO_DEV_ATTR_PHASE_CONFIG(3);
> >
> > Documentation of these? I'm not going to comment on them without appropriate
> > docs in
> >
> > Documentation/ABI/testing/
> >
> > Just wastes time figuring out what they are.
> It is not easy to explain without the doc (not released by semtech but
> available at https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf),
> but let me try:
>
> Each sensor has 3 inputs, CS0, CS1 and CS2. Hardware engineers decide
> if the input is not connected (HZ), grounded (GD), connected to an
> antenna where it can act as a base (DS - data shield), or measured
> input (MI).
> The sensor rotates measurement across 4 phases (PH0, PH1, PH2, PH3),
> where the inputs are configured and then measured.
> By default, during the first phase, [PH0], CS0 is measured, while CS1
> and CS2 are used as shields. `cat in_proximity0_mode` returns
> "MI,DS,DS".
> I did not make this change configurable: if the hardware designer
> decided to ground or leave an input dangling, it does not make sense
> to set it as a shield. [it can be changed if the need arises].
> Having 4 phases allows different treatment of the input. For instance,
> one phase could be used for object detection,the other for body
> detection. Configuring each phase is not implemented yet.
Ah. I read the emails backwards, this explanation is good - it's a
combination of 'what is wired' and what you are trying to measure.
>
> Add missing documentation in ABI/testing/sysfs-bus-iio-sx9324.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/5] dt-bindings: iio: Add sx9324 binding
2021-10-30 11:18 [PATCH 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
` (2 preceding siblings ...)
2021-10-30 11:18 ` [PATCH 3/5] iio: proximity: Add SX9324 support Gwendal Grignou
@ 2021-10-30 11:18 ` Gwendal Grignou
2021-10-30 17:24 ` Jonathan Cameron
2021-10-30 11:18 ` [PATCH 5/5] iio: sx9324: Add dt_bidding support Gwendal Grignou
4 siblings, 1 reply; 18+ messages in thread
From: Gwendal Grignou @ 2021-10-30 11:18 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Similar to SX9310, add biddings to setup sx9324 hardware properties.
SX9324 is a little different, introduce 4 phases to be configured in 2
pairs over 3 antennas.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
.../iio/proximity/semtech,sx9324.yaml | 141 ++++++++++++++++++
1 file changed, 141 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
new file mode 100644
index 0000000000000..fe9edf15c16d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech's SX9324 capacitive proximity sensor
+
+maintainers:
+ - Gwendal Grignou <gwendal@chromium.org>
+ - Daniel Campello <campello@chromium.org>
+
+description: |
+ Semtech's SX9324 proximity sensor.
+
+properties:
+ compatible:
+ enum:
+ - semtech,sx9324
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ The sole interrupt generated by the device used to announce the
+ preceding reading request has finished and that data is
+ available or that a close/far proximity event has happened.
+ maxItems: 1
+
+ vdd-supply:
+ description: Main power supply
+
+ svdd-supply:
+ description: Host interface power supply
+
+ "#io-channel-cells":
+ const: 1
+
+ semtech,ph0-pin:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description: |
+ Indicates how each CS pin is used during phase 0.
+ Each of the 3 pins have the following value -
+ 0 : unused (high impedance)
+ 1 : measured input
+ 2 : dynamic shield
+ 3 : grounded.
+ For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
+ items:
+ enum: [ 0, 1, 2, 3 ]
+ minItems: 3
+ maxItems: 3
+
+ semtech,ph1-pin:
+ semtech,ph2-pin:
+ semtech,ph3-pin:
+ Same as ph0-pin
+
+ semtech,resolution01
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5, 6, 7]
+ description:
+ Capacitance measurement resolution. For phase 0 and 1.
+ Higher the number, higher the resolution.
+ default: 4
+
+ semtech,resolution23
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5, 6, 7]
+ description:
+ Capacitance measurement resolution. For phase 2 and 3
+ default: 4
+
+ semtech,startup-sensor:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3]
+ default: 0
+ description:
+ Phase used for start-up proximity detection.
+ It is used when we enable a phase to remove static offset and measure
+ only capacitance changes introduced by the user.
+
+ semtech,proxraw-strength01:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5, 6, 7]
+ default: 1
+ description:
+ PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,i
+ and other values represent 1-1/N.
+
+ semtech,proxraw-strength23:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 1, 2, 3, 4, 5, 6, 7]
+ default: 1
+ description:
+ PROXRAW filter strength for phase 2 and 3. A value of 0 represents off,i
+ and other values represent 1-1/N.
+
+ semtech,avg-pos-strength:
+ $ref: /schemas/types.yaml#definitions/uint32
+ enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
+ default: 16
+ description:
+ Average positive filter strength. A value of 0 represents off and
+ UINT_MAX (4294967295) represents infinite. Other values
+ represent 1-1/N.
+
+required:
+ - compatible
+ - reg
+ - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ proximity@28 {
+ compatible = "semtech,sx9310";
+ reg = <0x28>;
+ interrupt-parent = <&pio>;
+ interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
+ vdd-supply = <&pp3300_a>;
+ svdd-supply = <&pp1800_prox>;
+ #io-channel-cells = <1>;
+ semtech,ph0-pin = <1, 2, 3>;
+ semtech,ph1-pin = <3, 2, 1>;
+ semtech,ph2-pin = <1, 2, 3>;
+ semtech,ph3-pin = <3, 2, 1>;
+ semtech,resolution01 = 2;
+ semtech,resolution23 = 2;
+ semtech,startup-sensor = <1>;
+ semtech,proxraw-strength01 = <2>;
+ semtech,proxraw-strength23 = <2>;
+ semtech,avg-pos-strength = <64>;
+ };
+ };
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/5] dt-bindings: iio: Add sx9324 binding
2021-10-30 11:18 ` [PATCH 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
@ 2021-10-30 17:24 ` Jonathan Cameron
2021-11-04 7:16 ` Gwendal Grignou
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2021-10-30 17:24 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, 30 Oct 2021 04:18:26 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> Similar to SX9310, add biddings to setup sx9324 hardware properties.
> SX9324 is a little different, introduce 4 phases to be configured in 2
> pairs over 3 antennas.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
This has a high degree of black magic.
I'm curious - is there a fairly heavy weight userspace library interpreting
the various readings? If so I don't suppose it's available anywhere to look at?
If you can point to any public info on this part that would also be great.
Thanks,
Jonathan
> ---
> .../iio/proximity/semtech,sx9324.yaml | 141 ++++++++++++++++++
> 1 file changed, 141 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> new file mode 100644
> index 0000000000000..fe9edf15c16d4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> @@ -0,0 +1,141 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech's SX9324 capacitive proximity sensor
> +
> +maintainers:
> + - Gwendal Grignou <gwendal@chromium.org>
> + - Daniel Campello <campello@chromium.org>
> +
> +description: |
> + Semtech's SX9324 proximity sensor.
> +
> +properties:
> + compatible:
> + enum:
> + - semtech,sx9324x
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + The sole interrupt generated by the device used to announce the
> + preceding reading request has finished and that data is
> + available or that a close/far proximity event has happened.
No need to say sole given maxItems: 1
Perhaps...
Generated by device to announce preceding read request has finished
and data is available or that a close/far proximity event has happened.
> + maxItems: 1
> +
> + vdd-supply:
> + description: Main power supply
> +
> + svdd-supply:
> + description: Host interface power supply
> +
> + "#io-channel-cells":
> + const: 1
Curious - Do we have consumers of this?
> +
> + semtech,ph0-pin:
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + description: |
> + Indicates how each CS pin is used during phase 0.
> + Each of the 3 pins have the following value -
> + 0 : unused (high impedance)
> + 1 : measured input
> + 2 : dynamic shield
> + 3 : grounded.
> + For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
> + items:
> + enum: [ 0, 1, 2, 3 ]
> + minItems: 3
> + maxItems: 3
> +
> + semtech,ph1-pin:
> + semtech,ph2-pin:
> + semtech,ph3-pin:
> + Same as ph0-pin
I'm curious - why would you chose different combinations? I guess because of
different wiring. If that's the case, is there a documented 'right' choice
for a given wiring. I'm just wondering if we are better off describing
what is connected to these pins and having the driver pick a valid control
sequence. That might simplify things. If not then fair enough.
> +
> + semtech,resolution01
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> + description:
> + Capacitance measurement resolution. For phase 0 and 1.
> + Higher the number, higher the resolution.
Can we not give something more meaningful than high is higher?
I don't have the datasheet for this part and the 9330 that I'm
looking at is rather unhelpful on this. I have no idea how anyone
using that datasheet would figure out what to set this to!
> + default: 4
> +
> + semtech,resolution23
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> + description:
> + Capacitance measurement resolution. For phase 2 and 3
> + default: 4
> +
> + semtech,startup-sensor:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3]
> + default: 0
> + description:
> + Phase used for start-up proximity detection.
> + It is used when we enable a phase to remove static offset and measure
> + only capacitance changes introduced by the user.
> +
> + semtech,proxraw-strength01:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> + default: 1
> + description:
> + PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,i
> + and other values represent 1-1/N.
> +
> + semtech,proxraw-strength23:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> + default: 1
> + description:
> + PROXRAW filter strength for phase 2 and 3. A value of 0 represents off,i
> + and other values represent 1-1/N.
> +
> + semtech,avg-pos-strength:
> + $ref: /schemas/types.yaml#definitions/uint32
> + enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
> + default: 16
> + description:
> + Average positive filter strength. A value of 0 represents off and
> + UINT_MAX (4294967295) represents infinite. Other values
> + represent 1-1/N.
> +
> +required:
> + - compatible
> + - reg
> + - "#io-channel-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + proximity@28 {
> + compatible = "semtech,sx9310";
> + reg = <0x28>;
> + interrupt-parent = <&pio>;
> + interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> + vdd-supply = <&pp3300_a>;
> + svdd-supply = <&pp1800_prox>;
> + #io-channel-cells = <1>;
> + semtech,ph0-pin = <1, 2, 3>;
> + semtech,ph1-pin = <3, 2, 1>;
> + semtech,ph2-pin = <1, 2, 3>;
> + semtech,ph3-pin = <3, 2, 1>;
> + semtech,resolution01 = 2;
> + semtech,resolution23 = 2;
> + semtech,startup-sensor = <1>;
> + semtech,proxraw-strength01 = <2>;
> + semtech,proxraw-strength23 = <2>;
> + semtech,avg-pos-strength = <64>;
> + };
> + };
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/5] dt-bindings: iio: Add sx9324 binding
2021-10-30 17:24 ` Jonathan Cameron
@ 2021-11-04 7:16 ` Gwendal Grignou
2021-11-04 18:13 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Gwendal Grignou @ 2021-11-04 7:16 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Sat, Oct 30, 2021 at 10:20 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 Oct 2021 04:18:26 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > Similar to SX9310, add biddings to setup sx9324 hardware properties.
> > SX9324 is a little different, introduce 4 phases to be configured in 2
> > pairs over 3 antennas.
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> This has a high degree of black magic.
>
> I'm curious - is there a fairly heavy weight userspace library interpreting
> the various readings? If so I don't suppose it's available anywhere to look at?
> If you can point to any public info on this part that would also be great.
Actually, user space does not care about the measurement once the
sensor is set up correctly to match its antennas (gains, thresholds,
hysteresis). They are only used during development and there is a
little of black magic at that point.
Then user spaces care about the events (far/close) generated by the
sensor. See https://chromium.googlesource.com/chromiumos/platform2/+/main/power_manager/powerd/system/user_proximity_watcher.cc#47
>
> Thanks,
>
> Jonathan
>
> > ---
> > .../iio/proximity/semtech,sx9324.yaml | 141 ++++++++++++++++++
> > 1 file changed, 141 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > new file mode 100644
> > index 0000000000000..fe9edf15c16d4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > @@ -0,0 +1,141 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Semtech's SX9324 capacitive proximity sensor
> > +
> > +maintainers:
> > + - Gwendal Grignou <gwendal@chromium.org>
> > + - Daniel Campello <campello@chromium.org>
> > +
> > +description: |
> > + Semtech's SX9324 proximity sensor.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - semtech,sx9324x
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + description:
> > + The sole interrupt generated by the device used to announce the
> > + preceding reading request has finished and that data is
> > + available or that a close/far proximity event has happened.
> No need to say sole given maxItems: 1
> Perhaps...
>
> Generated by device to announce preceding read request has finished
> and data is available or that a close/far proximity event has happened.
>
Done.
> > + maxItems: 1
> > +
> > + vdd-supply:
> > + description: Main power supply
> > +
> > + svdd-supply:
> > + description: Host interface power supply
> > +
> > + "#io-channel-cells":
> > + const: 1
>
> Curious - Do we have consumers of this?
I recopied them from sx9310. It looks standard for i2c devices.
>
> > +
> > + semtech,ph0-pin:
> > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > + description: |
> > + Indicates how each CS pin is used during phase 0.
> > + Each of the 3 pins have the following value -
> > + 0 : unused (high impedance)
> > + 1 : measured input
> > + 2 : dynamic shield
> > + 3 : grounded.
> > + For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
> > + items:
> > + enum: [ 0, 1, 2, 3 ]
> > + minItems: 3
> > + maxItems: 3
> > +
> > + semtech,ph1-pin:
> > + semtech,ph2-pin:
> > + semtech,ph3-pin:
> > + Same as ph0-pin
>
> I'm curious - why would you chose different combinations? I guess because of
> different wiring. If that's the case, is there a documented 'right' choice
> for a given wiring. I'm just wondering if we are better off describing
> what is connected to these pins and having the driver pick a valid control
> sequence. That might simplify things. If not then fair enough.
I explained in Documentation/ABI/testing/sysfs-bus-iio-sx9324 in the
previous patch.
The BIOS would communicate how the input pins are wired.
>
> > +
> > + semtech,resolution01
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > + description:
> > + Capacitance measurement resolution. For phase 0 and 1.
> > + Higher the number, higher the resolution.
>
> Can we not give something more meaningful than high is higher?
> I don't have the datasheet for this part and the 9330 that I'm
> looking at is rather unhelpful on this. I have no idea how anyone
> using that datasheet would figure out what to set this to!
I don't have more info. It is tweaked during setup.
>
> > + default: 4
> > +
> > + semtech,resolution23
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > + description:
> > + Capacitance measurement resolution. For phase 2 and 3
> > + default: 4
> > +
> > + semtech,startup-sensor:
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 1, 2, 3]
> > + default: 0
> > + description:
> > + Phase used for start-up proximity detection.
> > + It is used when we enable a phase to remove static offset and measure
> > + only capacitance changes introduced by the user.
> > +
> > + semtech,proxraw-strength01:
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > + default: 1
> > + description:
> > + PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,i
> > + and other values represent 1-1/N.
> > +
> > + semtech,proxraw-strength23:
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > + default: 1
> > + description:
> > + PROXRAW filter strength for phase 2 and 3. A value of 0 represents off,i
> > + and other values represent 1-1/N.
> > +
> > + semtech,avg-pos-strength:
> > + $ref: /schemas/types.yaml#definitions/uint32
> > + enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
> > + default: 16
> > + description:
> > + Average positive filter strength. A value of 0 represents off and
> > + UINT_MAX (4294967295) represents infinite. Other values
> > + represent 1-1/N.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - "#io-channel-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + proximity@28 {
> > + compatible = "semtech,sx9310";
> > + reg = <0x28>;
> > + interrupt-parent = <&pio>;
> > + interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> > + vdd-supply = <&pp3300_a>;
> > + svdd-supply = <&pp1800_prox>;
> > + #io-channel-cells = <1>;
> > + semtech,ph0-pin = <1, 2, 3>;
> > + semtech,ph1-pin = <3, 2, 1>;
> > + semtech,ph2-pin = <1, 2, 3>;
> > + semtech,ph3-pin = <3, 2, 1>;
> > + semtech,resolution01 = 2;
> > + semtech,resolution23 = 2;
> > + semtech,startup-sensor = <1>;
> > + semtech,proxraw-strength01 = <2>;
> > + semtech,proxraw-strength23 = <2>;
> > + semtech,avg-pos-strength = <64>;
> > + };
> > + };
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/5] dt-bindings: iio: Add sx9324 binding
2021-11-04 7:16 ` Gwendal Grignou
@ 2021-11-04 18:13 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2021-11-04 18:13 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: lars, swboyd, andy.shevchenko, linux-iio
On Thu, 4 Nov 2021 00:16:13 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:
> On Sat, Oct 30, 2021 at 10:20 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 30 Oct 2021 04:18:26 -0700
> > Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > > Similar to SX9310, add biddings to setup sx9324 hardware properties.
> > > SX9324 is a little different, introduce 4 phases to be configured in 2
> > > pairs over 3 antennas.
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > This has a high degree of black magic.
> >
> > I'm curious - is there a fairly heavy weight userspace library interpreting
> > the various readings? If so I don't suppose it's available anywhere to look at?
> > If you can point to any public info on this part that would also be great.
> Actually, user space does not care about the measurement once the
> sensor is set up correctly to match its antennas (gains, thresholds,
> hysteresis). They are only used during development and there is a
> little of black magic at that point.
> Then user spaces care about the events (far/close) generated by the
> sensor. See https://chromium.googlesource.com/chromiumos/platform2/+/main/power_manager/powerd/system/user_proximity_watcher.cc#47
>
Fair enough. I'll just remember this as 'black magic' :)
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > .../iio/proximity/semtech,sx9324.yaml | 141 ++++++++++++++++++
> > > 1 file changed, 141 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > > new file mode 100644
> > > index 0000000000000..fe9edf15c16d4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9324.yaml
> > > @@ -0,0 +1,141 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Semtech's SX9324 capacitive proximity sensor
> > > +
> > > +maintainers:
> > > + - Gwendal Grignou <gwendal@chromium.org>
> > > + - Daniel Campello <campello@chromium.org>
> > > +
> > > +description: |
> > > + Semtech's SX9324 proximity sensor.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - semtech,sx9324x
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + description:
> > > + The sole interrupt generated by the device used to announce the
> > > + preceding reading request has finished and that data is
> > > + available or that a close/far proximity event has happened.
> > No need to say sole given maxItems: 1
> > Perhaps...
> >
> > Generated by device to announce preceding read request has finished
> > and data is available or that a close/far proximity event has happened.
> >
> Done.
> > > + maxItems: 1
> > > +
> > > + vdd-supply:
> > > + description: Main power supply
> > > +
> > > + svdd-supply:
> > > + description: Host interface power supply
> > > +
> > > + "#io-channel-cells":
> > > + const: 1
> >
> > Curious - Do we have consumers of this?
> I recopied them from sx9310. It looks standard for i2c devices.
Should only be the case for devices that have or are likely to have
in kernel IIO consumers. Doesn't do any harm though and might be useful to
someone so fine to leave it.
> >
> > > +
> > > + semtech,ph0-pin:
> > > + $ref: /schemas/types.yaml#/definitions/uint32-array
> > > + description: |
> > > + Indicates how each CS pin is used during phase 0.
> > > + Each of the 3 pins have the following value -
> > > + 0 : unused (high impedance)
> > > + 1 : measured input
> > > + 2 : dynamic shield
> > > + 3 : grounded.
> > > + For instance, CS0 measured, CS1 shield and CS2 ground is [1, 2, 3]
> > > + items:
> > > + enum: [ 0, 1, 2, 3 ]
> > > + minItems: 3
> > > + maxItems: 3
> > > +
> > > + semtech,ph1-pin:
> > > + semtech,ph2-pin:
> > > + semtech,ph3-pin:
> > > + Same as ph0-pin
> >
> > I'm curious - why would you chose different combinations? I guess because of
> > different wiring. If that's the case, is there a documented 'right' choice
> > for a given wiring. I'm just wondering if we are better off describing
> > what is connected to these pins and having the driver pick a valid control
> > sequence. That might simplify things. If not then fair enough.
> I explained in Documentation/ABI/testing/sysfs-bus-iio-sx9324 in the
> previous patch.
> The BIOS would communicate how the input pins are wired.
So my reading here is that isn't quite true. The BIOS is communicating
what state we should put the pins into which is a function of how they
are wired, but not a direct indication of that wiring.
From point of view of someone using this device, I'd really want
to be able to specify pin 1 to this part of the antenna etc and have
the driver work out the settings for each step in the sequence that would
work.
If we don't have that information, or it is complex to map to a sequence then
I can live with what you have here.
> >
> > > +
> > > + semtech,resolution01
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > + description:
> > > + Capacitance measurement resolution. For phase 0 and 1.
> > > + Higher the number, higher the resolution.
> >
> > Can we not give something more meaningful than high is higher?
> > I don't have the datasheet for this part and the 9330 that I'm
> > looking at is rather unhelpful on this. I have no idea how anyone
> > using that datasheet would figure out what to set this to!
> I don't have more info. It is tweaked during setup.
Ah well - magic tweak it is :)
>
> >
> > > + default: 4
> > > +
> > > + semtech,resolution23
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > + description:
> > > + Capacitance measurement resolution. For phase 2 and 3
> > > + default: 4
> > > +
> > > + semtech,startup-sensor:
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 1, 2, 3]
> > > + default: 0
> > > + description:
> > > + Phase used for start-up proximity detection.
> > > + It is used when we enable a phase to remove static offset and measure
> > > + only capacitance changes introduced by the user.
> > > +
> > > + semtech,proxraw-strength01:
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > + default: 1
> > > + description:
> > > + PROXRAW filter strength for phase 0 and 1. A value of 0 represents off,i
> > > + and other values represent 1-1/N.
> > > +
> > > + semtech,proxraw-strength23:
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 1, 2, 3, 4, 5, 6, 7]
> > > + default: 1
> > > + description:
> > > + PROXRAW filter strength for phase 2 and 3. A value of 0 represents off,i
> > > + and other values represent 1-1/N.
> > > +
> > > + semtech,avg-pos-strength:
> > > + $ref: /schemas/types.yaml#definitions/uint32
> > > + enum: [0, 16, 64, 128, 256, 512, 1024, 4294967295]
> > > + default: 16
> > > + description:
> > > + Average positive filter strength. A value of 0 represents off and
> > > + UINT_MAX (4294967295) represents infinite. Other values
> > > + represent 1-1/N.
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - "#io-channel-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/interrupt-controller/irq.h>
> > > + i2c {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + proximity@28 {
> > > + compatible = "semtech,sx9310";
> > > + reg = <0x28>;
> > > + interrupt-parent = <&pio>;
> > > + interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> > > + vdd-supply = <&pp3300_a>;
> > > + svdd-supply = <&pp1800_prox>;
> > > + #io-channel-cells = <1>;
> > > + semtech,ph0-pin = <1, 2, 3>;
> > > + semtech,ph1-pin = <3, 2, 1>;
> > > + semtech,ph2-pin = <1, 2, 3>;
> > > + semtech,ph3-pin = <3, 2, 1>;
> > > + semtech,resolution01 = 2;
> > > + semtech,resolution23 = 2;
> > > + semtech,startup-sensor = <1>;
> > > + semtech,proxraw-strength01 = <2>;
> > > + semtech,proxraw-strength23 = <2>;
> > > + semtech,avg-pos-strength = <64>;
> > > + };
> > > + };
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/5] iio: sx9324: Add dt_bidding support
2021-10-30 11:18 [PATCH 0/5] Expand Semtech SAR Sensors support Gwendal Grignou
` (3 preceding siblings ...)
2021-10-30 11:18 ` [PATCH 4/5] dt-bindings: iio: Add sx9324 binding Gwendal Grignou
@ 2021-10-30 11:18 ` Gwendal Grignou
4 siblings, 0 replies; 18+ messages in thread
From: Gwendal Grignou @ 2021-10-30 11:18 UTC (permalink / raw)
To: jic23, lars, swboyd; +Cc: andy.shevchenko, linux-iio, Gwendal Grignou
Based on bindings/iio/proximity/semtech,sx9324.yaml, implement
retrieving sensor hardware property and alter default values.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
drivers/iio/proximity/sx9324.c | 159 +++++++++++++++++++++++++++++++++
1 file changed, 159 insertions(+)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 41d9c950c5abd..5f2e10d74f058 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -780,6 +780,74 @@ static const struct iio_info sx9324_info = {
.write_event_config = sx_common_write_event_config,
};
+static const struct sx_common_reg_default sx9324_default_regs[] = {
+ { SX9324_REG_IRQ_MSK, 0x00 },
+ { SX9324_REG_IRQ_CFG0, 0x00 },
+ { SX9324_REG_IRQ_CFG1, SX9324_REG_IRQ_CFG1_FAILCOND },
+ { SX9324_REG_IRQ_CFG2, 0x00 },
+ { SX9324_REG_GNRL_CTRL0, SX9324_REG_GNRL_CTRL0_SCANPERIOD_100MS },
+ /*
+ * The lower 4 bits should not be set as it enable sensors measurements.
+ * Turning the detection on before the configuration values are set to
+ * good values can cause the device to return erroneous readings.
+ */
+ { SX9324_REG_GNRL_CTRL1, SX9324_REG_GNRL_CTRL1_PAUSECTRL },
+
+ { SX9324_REG_AFE_CTRL0, 0x00 },
+ { SX9324_REG_AFE_CTRL3, 0x00 },
+ { SX9324_REG_AFE_CTRL4, SX9324_REG_AFE_CTRL4_FREQ_83_33HZ |
+ SX9324_REG_AFE_CTRL4_RES_100 },
+ { SX9324_REG_AFE_CTRL6, 0x00 },
+ { SX9324_REG_AFE_CTRL7, SX9324_REG_AFE_CTRL4_FREQ_83_33HZ |
+ SX9324_REG_AFE_CTRL4_RES_100 },
+
+ /* TODO(gwendal): PHx use chip default or all grounded? */
+ { SX9324_REG_AFE_PH0, 0x29 },
+ { SX9324_REG_AFE_PH1, 0x26 },
+ { SX9324_REG_AFE_PH2, 0x1a },
+ { SX9324_REG_AFE_PH3, 0x16 },
+
+ { SX9324_REG_AFE_CTRL8, SX9324_REG_AFE_CTRL8_RESFILTN_4KOHM },
+ { SX9324_REG_AFE_CTRL9, SX9324_REG_AFE_CTRL9_AGAIN_1 },
+
+ { SX9324_REG_PROX_CTRL0, SX9324_REG_PROX_CTRL0_GAIN_1 |
+ SX9324_REG_PROX_CTRL0_RAWFILT_1P50 },
+ { SX9324_REG_PROX_CTRL1, SX9324_REG_PROX_CTRL0_GAIN_1 |
+ SX9324_REG_PROX_CTRL0_RAWFILT_1P50 },
+ { SX9324_REG_PROX_CTRL2, SX9324_REG_PROX_CTRL2_AVGNEG_THRESH_16K },
+ { SX9324_REG_PROX_CTRL3, SX9324_REG_PROX_CTRL3_AVGDEB_2SAMPLES |
+ SX9324_REG_PROX_CTRL3_AVGPOS_THRESH_16K },
+ { SX9324_REG_PROX_CTRL4, SX9324_REG_PROX_CTRL4_AVGNEG_FILT_1P50 |
+ SX9324_REG_PROX_CTRL3_AVGPOS_FILT_1P256},
+ { SX9324_REG_PROX_CTRL5, 0x00 },
+ { SX9324_REG_PROX_CTRL6, SX9324_REG_PROX_CTRL6_PROXTHRESH_32 },
+ { SX9324_REG_PROX_CTRL7, SX9324_REG_PROX_CTRL6_PROXTHRESH_32 },
+ { SX9324_REG_ADV_CTRL0, 0x00 },
+ { SX9324_REG_ADV_CTRL1, 0x00 },
+ { SX9324_REG_ADV_CTRL2, 0x00 },
+ { SX9324_REG_ADV_CTRL3, 0x00 },
+ { SX9324_REG_ADV_CTRL4, 0x00 },
+ { SX9324_REG_ADV_CTRL5, SX9324_REG_ADV_CTRL5_STARTUP_SENSOR_1 |
+ SX9324_REG_ADV_CTRL5_STARTUP_METHOD_1 },
+ { SX9324_REG_ADV_CTRL6, 0x00 },
+ { SX9324_REG_ADV_CTRL7, 0x00 },
+ { SX9324_REG_ADV_CTRL8, 0x00 },
+ { SX9324_REG_ADV_CTRL9, 0x00 },
+ /* Body/Table threshold */
+ { SX9324_REG_ADV_CTRL10, 0x00 },
+ { SX9324_REG_ADV_CTRL11, 0x00 },
+ { SX9324_REG_ADV_CTRL12, 0x00 },
+ /* TODO(gwendal): SAR currenly disabled */
+ { SX9324_REG_ADV_CTRL13, 0x00 },
+ { SX9324_REG_ADV_CTRL14, 0x00 },
+ { SX9324_REG_ADV_CTRL15, 0x00 },
+ { SX9324_REG_ADV_CTRL16, 0x00 },
+ { SX9324_REG_ADV_CTRL17, 0x00 },
+ { SX9324_REG_ADV_CTRL18, 0x00 },
+ { SX9324_REG_ADV_CTRL19, SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION },
+ { SX9324_REG_ADV_CTRL20, SX9324_REG_ADV_CTRL19_HIGHT_FAILURE_THRESH_SATURATION },
+};
+
/* Activate all channels and perform an initial compensation. */
static int sx9324_init_compensation(struct iio_dev *indio_dev)
{
@@ -804,6 +872,93 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev)
return ret;
}
+static const struct sx_common_reg_default *
+sx9324_get_default_reg(struct device *dev, int idx,
+ struct sx_common_reg_default *reg_def)
+{
+#define SX9324_PIN_DEF "semtech,ph0-pin"
+#define SX9324_RESOLUTION_DEF "semtech,resolution01"
+ unsigned int pin_defs[SX9324_NUM_PINS];
+ char prop[] = SX9324_RESOLUTION_DEF;
+ u32 start = 0, raw = 0, pos = 0;
+ int ret, count, ph, pin;
+
+ memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
+ switch (reg_def->reg) {
+ case SX9324_REG_AFE_PH0:
+ case SX9324_REG_AFE_PH1:
+ case SX9324_REG_AFE_PH2:
+ case SX9324_REG_AFE_PH3:
+ ph = reg_def->reg - SX9324_REG_AFE_PH0;
+ scnprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
+
+ count = device_property_count_u32(dev, prop);
+ if (count != ARRAY_SIZE(pin_defs))
+ break;
+ ret = device_property_read_u32_array(dev, prop, pin_defs,
+ ARRAY_SIZE(pin_defs));
+ for (pin = 0; pin < SX9324_NUM_PINS; pin++)
+ raw |= (pin_defs[pin] << (2 * pin)) &
+ SX9324_REG_AFE_PH0_PIN_MASK(pin);
+ reg_def->def = raw;
+ break;
+ case SX9324_REG_AFE_CTRL4:
+ case SX9324_REG_AFE_CTRL7:
+ if (reg_def->reg == SX9324_REG_AFE_CTRL4)
+ strncpy(prop, "semtech,resolution01", ARRAY_SIZE(prop));
+ else
+ strncpy(prop, "semtech,resolution23", ARRAY_SIZE(prop));
+
+ ret = device_property_read_u32(dev, prop, &raw);
+ if (ret)
+ raw = FIELD_GET(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
+ reg_def->def);
+ else
+ raw = ilog2(raw);
+
+ reg_def->def &= ~SX9324_REG_AFE_CTRL4_RESOLUTION_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
+ raw);
+ break;
+ case SX9324_REG_ADV_CTRL5:
+ ret = device_property_read_u32(dev, "semtech,startup-sensor", &start);
+ if (ret)
+ break;
+
+ reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
+ start);
+ break;
+ case SX9324_REG_PROX_CTRL4:
+ ret = device_property_read_u32(dev, "semtech,avg-neg-strength", &pos);
+ if (ret) {
+ raw = FIELD_GET(SX9324_REG_PROX_CTRL4_AVGNEGFILT_MASK,
+ reg_def->def);
+ } else {
+ /* Powers of 2, except for a gap between 16 and 64 */
+ raw = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
+ }
+ reg_def->def &= ~SX9324_REG_PROX_CTRL4_AVGNEGFILT_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGNEGFILT_MASK,
+ raw);
+
+ ret = device_property_read_u32(dev, "semtech,avg-pos-strength", &pos);
+ if (ret) {
+ raw = FIELD_GET(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
+ reg_def->def);
+ } else {
+ /* Powers of 2, except for a gap between 16 and 64 */
+ raw = clamp(ilog2(pos), 3, 11) - (pos >= 32 ? 4 : 3);
+ }
+ reg_def->def &= ~SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK;
+ reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
+ raw);
+ break;
+ }
+
+ return reg_def;
+}
+
static int sx9324_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
@@ -825,6 +980,9 @@ static int sx9324_probe(struct i2c_client *client)
data->irq_msk_offset = 3;
data->num_channels = SX9324_NUM_CHANNELS;
+ /* Number of default registers to set at init */
+ data->num_default_regs = ARRAY_SIZE(sx9324_default_regs);
+
ret = sx_common_probe_setup(indio_dev, client, &sx9324_regmap_config);
if (ret)
return ret;
@@ -839,6 +997,7 @@ static int sx9324_probe(struct i2c_client *client)
data->read_prox_data = sx9324_read_prox_data;
data->init_compensation = sx9324_init_compensation;
data->wait_for_sample = sx9324_wait_for_sample;
+ data->get_default_reg = sx9324_get_default_reg;
indio_dev->channels = sx9324_channels;
indio_dev->num_channels = ARRAY_SIZE(sx9324_channels);
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread