diff mbox series

[v6,6/9] iio: adc: ad7173: add support for special inputs

Message ID 20240606-ad4111-v6-6-573981fb3e2e@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for AD411x | expand

Commit Message

Dumitru Ceclan via B4 Relay June 6, 2024, 4:07 p.m. UTC 
From: Dumitru Ceclan <dumitru.ceclan@analog.com>

 Add support for selecting REF+ and REF- inputs on all models.
 Add support for selecting ((AVDD1 − AVSS)/5) inputs
  on supported models.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Nuno Sá June 7, 2024, 9:06 a.m. UTC | #1 
On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
>  Add support for selecting REF+ and REF- inputs on all models.
>  Add support for selecting ((AVDD1 − AVSS)/5) inputs
>   on supported models.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 4040edbd1c32..d16fa081a285 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -66,6 +66,13 @@
>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>  #define AD7173_AIN_TEMP_POS	17
>  #define AD7173_AIN_TEMP_NEG	18
> +#define AD7173_AIN_POW_MON_POS	19
> +#define AD7173_AIN_POW_MON_NEG	20
> +#define AD7173_AIN_REF_POS	21
> +#define AD7173_AIN_REF_NEG	22
> +
> +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
> +					(x) == AD7173_AIN_REF_NEG)
>  
>  #define AD7172_2_ID			0x00d0
>  #define AD7175_ID			0x0cd0
> @@ -146,6 +153,8 @@ struct ad7173_device_info {
>  	unsigned int id;
>  	char *name;
>  	bool has_temp;
> +	/* ((AVDD1 − AVSS)/5) */
> +	bool has_pow_supply_monitoring;
>  	bool has_input_buf;
>  	bool has_int_ref;
>  	bool has_ref2;
> @@ -216,6 +225,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_temp = true,
>  		.has_input_buf = true,
>  		.has_int_ref = true,
> +		.has_pow_supply_monitoring = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -230,6 +240,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_temp = false,
>  		.has_input_buf = true,
>  		.has_ref2 = true,
> +		.has_pow_supply_monitoring = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -245,6 +256,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_input_buf = true,
>  		.has_int_ref = true,
>  		.has_ref2 = true,
> +		.has_pow_supply_monitoring = false,

No need to set the 'false' cases...


- Nuno Sá
Ceclan, Dumitru June 7, 2024, 9:34 a.m. UTC | #2 
On 07/06/2024 12:06, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>>  Add support for selecting REF+ and REF- inputs on all models.
>>  Add support for selecting ((AVDD1 − AVSS)/5) inputs
>>   on supported models.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 4040edbd1c32..d16fa081a285 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -66,6 +66,13 @@
>>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>>  #define AD7173_AIN_TEMP_POS	17
>>  #define AD7173_AIN_TEMP_NEG	18
>> +#define AD7173_AIN_POW_MON_POS	19
>> +#define AD7173_AIN_POW_MON_NEG	20
>> +#define AD7173_AIN_REF_POS	21
>> +#define AD7173_AIN_REF_NEG	22
>> +
>> +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
>> +					(x) == AD7173_AIN_REF_NEG)
>>  
>>  #define AD7172_2_ID			0x00d0
>>  #define AD7175_ID			0x0cd0
>> @@ -146,6 +153,8 @@ struct ad7173_device_info {
>>  	unsigned int id;
>>  	char *name;
>>  	bool has_temp;
>> +	/* ((AVDD1 − AVSS)/5) */
>> +	bool has_pow_supply_monitoring;
>>  	bool has_input_buf;
>>  	bool has_int_ref;
>>  	bool has_ref2;
>> @@ -216,6 +225,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_temp = true,
>>  		.has_input_buf = true,
>>  		.has_int_ref = true,
>> +		.has_pow_supply_monitoring = true,
>>  		.clock = 2 * HZ_PER_MHZ,
>>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> @@ -230,6 +240,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_temp = false,
>>  		.has_input_buf = true,
>>  		.has_ref2 = true,
>> +		.has_pow_supply_monitoring = true,
>>  		.clock = 2 * HZ_PER_MHZ,
>>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> @@ -245,6 +256,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_input_buf = true,
>>  		.has_int_ref = true,
>>  		.has_ref2 = true,
>> +		.has_pow_supply_monitoring = false,
> 
> No need to set the 'false' cases...
> 
> 
> - Nuno Sá

This was suggested by David Lechner to ensure consistency with has_temp
regarding another field, I considered that it would apply here as well.
https://lore.kernel.org/all/CAMknhBGaJxXvsQ8cZkgDsKLVjOY5y2pzox-99hdOCrUaoZdsxg@mail.gmail.com/

This would also increase visibility towards what features does a specific
model support as it is clearly stated with "= false" rather than looking
for what fields are not set within the struct.
Nuno Sá June 7, 2024, 10:29 a.m. UTC | #3 
On Fri, 2024-06-07 at 12:34 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:06, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > >  Add support for selecting REF+ and REF- inputs on all models.
> > >  Add support for selecting ((AVDD1 − AVSS)/5) inputs
> > >   on supported models.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
> > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 4040edbd1c32..d16fa081a285 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > @@ -66,6 +66,13 @@
> > >  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
> > >  #define AD7173_AIN_TEMP_POS	17
> > >  #define AD7173_AIN_TEMP_NEG	18
> > > +#define AD7173_AIN_POW_MON_POS	19
> > > +#define AD7173_AIN_POW_MON_NEG	20
> > > +#define AD7173_AIN_REF_POS	21
> > > +#define AD7173_AIN_REF_NEG	22
> > > +
> > > +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
> > > +					(x) == AD7173_AIN_REF_NEG)
> > >  
> > >  #define AD7172_2_ID			0x00d0
> > >  #define AD7175_ID			0x0cd0
> > > @@ -146,6 +153,8 @@ struct ad7173_device_info {
> > >  	unsigned int id;
> > >  	char *name;
> > >  	bool has_temp;
> > > +	/* ((AVDD1 − AVSS)/5) */
> > > +	bool has_pow_supply_monitoring;
> > >  	bool has_input_buf;
> > >  	bool has_int_ref;
> > >  	bool has_ref2;
> > > @@ -216,6 +225,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_temp = true,
> > >  		.has_input_buf = true,
> > >  		.has_int_ref = true,
> > > +		.has_pow_supply_monitoring = true,
> > >  		.clock = 2 * HZ_PER_MHZ,
> > >  		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > >  		.num_sinc5_data_rates =
> > > ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > @@ -230,6 +240,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_temp = false,
> > >  		.has_input_buf = true,
> > >  		.has_ref2 = true,
> > > +		.has_pow_supply_monitoring = true,
> > >  		.clock = 2 * HZ_PER_MHZ,
> > >  		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > >  		.num_sinc5_data_rates =
> > > ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > @@ -245,6 +256,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_input_buf = true,
> > >  		.has_int_ref = true,
> > >  		.has_ref2 = true,
> > > +		.has_pow_supply_monitoring = false,
> > 
> > No need to set the 'false' cases...
> > 
> > 
> > - Nuno Sá
> 
> This was suggested by David Lechner to ensure consistency with has_temp
> regarding another field, I considered that it would apply here as well.
> https://lore.kernel.org/all/CAMknhBGaJxXvsQ8cZkgDsKLVjOY5y2pzox-99hdOCrUaoZdsxg@mail.gmail.com/
> 

Well, I would argue that the has_temp flag being set to 0 is also unneeded and
can be removed (in another patch).
> This would also increase visibility towards what features does a specific
> model support as it is clearly stated with "= false" rather than looking
> for what fields are not set within the struct.

IMO, the omission of the flag is already pretty clear that the feature is not
available. Typically we don't initialize things that do not need to be
initialized (less LOC)...

- Nuno Sá
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 4040edbd1c32..d16fa081a285 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -66,6 +66,13 @@ 
 	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
 #define AD7173_AIN_TEMP_POS	17
 #define AD7173_AIN_TEMP_NEG	18
+#define AD7173_AIN_POW_MON_POS	19
+#define AD7173_AIN_POW_MON_NEG	20
+#define AD7173_AIN_REF_POS	21
+#define AD7173_AIN_REF_NEG	22
+
+#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
+					(x) == AD7173_AIN_REF_NEG)
 
 #define AD7172_2_ID			0x00d0
 #define AD7175_ID			0x0cd0
@@ -146,6 +153,8 @@  struct ad7173_device_info {
 	unsigned int id;
 	char *name;
 	bool has_temp;
+	/* ((AVDD1 − AVSS)/5) */
+	bool has_pow_supply_monitoring;
 	bool has_input_buf;
 	bool has_int_ref;
 	bool has_ref2;
@@ -216,6 +225,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -230,6 +240,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -245,6 +256,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = false,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -259,6 +271,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -274,6 +287,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -288,6 +302,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = false,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = false,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -302,6 +317,7 @@  static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.odr_start_value = AD7177_ODR_START_VALUE,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
@@ -914,9 +930,18 @@  static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 					      unsigned int ain0, unsigned int ain1)
 {
 	struct device *dev = &st->sd.spi->dev;
+	bool special_input0, special_input1;
+
+	/* (AVDD1-AVSS)/5 power supply monitoring */
+	if (ain0 == AD7173_AIN_POW_MON_POS && ain1 == AD7173_AIN_POW_MON_NEG &&
+	    st->info->has_pow_supply_monitoring)
+		return 0;
+
+	special_input0 = AD7173_IS_REF_INPUT(ain0);
+	special_input1 = AD7173_IS_REF_INPUT(ain1);
 
-	if (ain0 >= st->info->num_inputs ||
-	    ain1 >= st->info->num_inputs)
+	if ((ain0 >= st->info->num_inputs && !special_input0) ||
+	    (ain1 >= st->info->num_inputs && !special_input1))
 		return dev_err_probe(dev, -EINVAL,
 				     "Input pin number out of range for pair (%d %d).\n",
 				     ain0, ain1);