dt-bindings: iio: adc: ti,ads1015: Add missing ADS1115 compatible string

PATCH Superseded Marek Vasut 5 participants 15 messages Mar 10, 2022 – Mar 20, 2022

Subsystems (2)

Device tree bindings IIO

lore

Share & tools

lore

Marek Vasut

author

2022-03-10 00:34 UTC #0 lore raw

PATCH 1/6 dt-bindings: iio: adc: ti,ads1015: Add missing ADS1115 compatible string

Add missing ti,ads1115 compatible string. This compatible string is supported by the Linux kernel driver and the ads1015 is a 12bit ADC while ads1115 is 16bit ADC. Add the missing compatible string.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

Cc Rob Herring <[email protected]>

Cc [email protected]

---
 Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml +4 -2

index 2c2d01bbc296d..c31c80989cc9a 100644
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/adc/ti,ads1015.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: TI ADS1015 4 channel I2C analog to digital converter
+title: TI ADS1015/ADS1115 4 channel I2C analog to digital converter
 
 maintainers:
   - Daniel Baluta <[email protected]>

@@ -15,7 +15,9 @@ description: |
 
 properties:
   compatible:
-    const: ti,ads1015
+    enum:
+      - ti,ads1015
+      - ti,ads1115
 
   reg:
     maxItems: 1

Signature · 1 line

2.34.1

Marek Vasut

author

2022-03-10 00:34 UTC #1 lore raw

PATCH 2/6 dt-bindings: iio: adc: ti,ads1015: Add TLA2024 compatible string

Add ti,tla2024 compatible string. This device is compatible with ADS1015 except it has no on-chip comparator.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

Cc Rob Herring <[email protected]>

Cc [email protected]

---
 Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml | 1 +
 1 file changed, 1 insertion(+)

Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml +1 -0

index c31c80989cc9a..a3b79438a13a5 100644
@@ -18,6 +18,7 @@ properties:
     enum:
       - ti,ads1015
       - ti,ads1115
+      - ti,tla2024
 
   reg:
     maxItems: 1

Signature · 1 line

2.34.1

Krzysztof Kozlowski

Acked-by on v1 2/6

2022-03-10 12:52 UTC #2 lore raw

Re: [PATCH 2/6] dt-bindings: iio: adc: ti,ads1015: Add TLA2024 compatible string

Quoted 10 lines · Marek Vasut

Add ti,tla2024 compatible string. This device is compatible with ADS1015 except it has no on-chip comparator.
Signed-off-by Marek Vasut <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>
Cc Rob Herring <[email protected]>
Cc [email protected]

Quoted 4 lines

---
 Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml | 1 +
 1 file changed, 1 insertion(+)

Acked-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Marek Vasut

author

2022-03-10 00:34 UTC #3 lore raw

PATCH 3/6 iio: adc: ti-ads1015: Switch to static const writeable ranges table

In reply to Marek Vasut

Switch the driver from code implementing test whether a regmap register is writeable to static const tables describing the test. No functional change.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-ads1015.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iio/adc/ti-ads1015.c +9 ~~-12~~

index 068efbce17103..85932b9dc166a 100644
@@ -273,23 +273,20 @@ static void ads1015_event_channel_disable(struct ads1015_data *data, int chan)
 	data->event_channel = ADS1015_CHANNELS;
 }
 
-static bool ads1015_is_writeable_reg(struct device *dev, unsigned int reg)
-{
-	switch (reg) {
-	case ADS1015_CFG_REG:
-	case ADS1015_LO_THRESH_REG:
-	case ADS1015_HI_THRESH_REG:
-		return true;
-	default:
-		return false;
-	}
-}
+static const struct regmap_range ads1015_writeable_ranges[] = {
+	regmap_reg_range(ADS1015_CFG_REG, ADS1015_HI_THRESH_REG),
+};
+
+static const struct regmap_access_table ads1015_writeable_table = {
+	.yes_ranges = ads1015_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ads1015_writeable_ranges),
+};
 
 static const struct regmap_config ads1015_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
 	.max_register = ADS1015_HI_THRESH_REG,
-	.writeable_reg = ads1015_is_writeable_reg,
+	.wr_table = &ads1015_writeable_table,
 };
 
 static const struct iio_chan_spec ads1015_channels[] = {

Signature · 1 line

2.34.1

Marek Vasut

author

2022-03-10 00:34 UTC #4 lore raw

PATCH 4/6 iio: adc: ti-ads1015: Deduplicate channel macros

In reply to Marek Vasut

These macros differ only in the number of valid bits of each ADC sample and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, ADS1115 is 16bit ADC shifted by 0. No functional change.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-ads1015.c | 86 +++++++++---------------------------
 1 file changed, 22 insertions(+), 64 deletions(-)

drivers/iio/adc/ti-ads1015.c +22 ~~-64~~

index 85932b9dc166a..fc3381ff34710 100644
@@ -134,7 +134,7 @@ static const struct iio_event_spec ads1015_events[] = {
 	},
 };
 
-#define ADS1015_V_CHAN(_chan, _addr) {				\
+#define ADS1015_V_CHAN(_chan, _addr, _realbits, _shift) {	\
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
 	.address = _addr,					\

@@ -145,9 +145,9 @@ static const struct iio_event_spec ads1015_events[] = {
 	.scan_index = _addr,					\
 	.scan_type = {						\
 		.sign = 's',					\
-		.realbits = 12,					\
+		.realbits = (_realbits),			\
 		.storagebits = 16,				\
-		.shift = 4,					\
+		.shift = (_shift),				\
 		.endianness = IIO_CPU,				\
 	},							\
 	.event_spec = ads1015_events,				\

@@ -155,7 +155,7 @@ static const struct iio_event_spec ads1015_events[] = {
 	.datasheet_name = "AIN"#_chan,				\
 }
 
-#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr) {		\
+#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr, _realbits, _shift) { \
 	.type = IIO_VOLTAGE,					\
 	.differential = 1,					\
 	.indexed = 1,						\

@@ -168,51 +168,9 @@ static const struct iio_event_spec ads1015_events[] = {
 	.scan_index = _addr,					\
 	.scan_type = {						\
 		.sign = 's',					\
-		.realbits = 12,					\
-		.storagebits = 16,				\
-		.shift = 4,					\
-		.endianness = IIO_CPU,				\
-	},							\
-	.event_spec = ads1015_events,				\
-	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
-	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
-}
-
-#define ADS1115_V_CHAN(_chan, _addr) {				\
-	.type = IIO_VOLTAGE,					\
-	.indexed = 1,						\
-	.address = _addr,					\
-	.channel = _chan,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
-				BIT(IIO_CHAN_INFO_SCALE) |	\
-				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
-	.scan_index = _addr,					\
-	.scan_type = {						\
-		.sign = 's',					\
-		.realbits = 16,					\
-		.storagebits = 16,				\
-		.endianness = IIO_CPU,				\
-	},							\
-	.event_spec = ads1015_events,				\
-	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
-	.datasheet_name = "AIN"#_chan,				\
-}
-
-#define ADS1115_V_DIFF_CHAN(_chan, _chan2, _addr) {		\
-	.type = IIO_VOLTAGE,					\
-	.differential = 1,					\
-	.indexed = 1,						\
-	.address = _addr,					\
-	.channel = _chan,					\
-	.channel2 = _chan2,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
-				BIT(IIO_CHAN_INFO_SCALE) |	\
-				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
-	.scan_index = _addr,					\
-	.scan_type = {						\
-		.sign = 's',					\
-		.realbits = 16,					\
+		.realbits = (_realbits),			\
 		.storagebits = 16,				\
+		.shift = (_shift),				\
 		.endianness = IIO_CPU,				\
 	},							\
 	.event_spec = ads1015_events,				\

@@ -290,26 +248,26 @@ static const struct regmap_config ads1015_regmap_config = {
 };
 
 static const struct iio_chan_spec ads1015_channels[] = {
-	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1),
-	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3),
-	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3),
-	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3),
-	ADS1015_V_CHAN(0, ADS1015_AIN0),
-	ADS1015_V_CHAN(1, ADS1015_AIN1),
-	ADS1015_V_CHAN(2, ADS1015_AIN2),
-	ADS1015_V_CHAN(3, ADS1015_AIN3),
+	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4),
+	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 12, 4),
+	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 12, 4),
+	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 12, 4),
+	ADS1015_V_CHAN(0, ADS1015_AIN0, 12, 4),
+	ADS1015_V_CHAN(1, ADS1015_AIN1, 12, 4),
+	ADS1015_V_CHAN(2, ADS1015_AIN2, 12, 4),
+	ADS1015_V_CHAN(3, ADS1015_AIN3, 12, 4),
 	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
 };
 
 static const struct iio_chan_spec ads1115_channels[] = {
-	ADS1115_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1),
-	ADS1115_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3),
-	ADS1115_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3),
-	ADS1115_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3),
-	ADS1115_V_CHAN(0, ADS1015_AIN0),
-	ADS1115_V_CHAN(1, ADS1015_AIN1),
-	ADS1115_V_CHAN(2, ADS1015_AIN2),
-	ADS1115_V_CHAN(3, ADS1015_AIN3),
+	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 16, 0),
+	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 16, 0),
+	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 16, 0),
+	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 16, 0),
+	ADS1015_V_CHAN(0, ADS1015_AIN0, 16, 0),
+	ADS1015_V_CHAN(1, ADS1015_AIN1, 16, 0),
+	ADS1015_V_CHAN(2, ADS1015_AIN2, 16, 0),
+	ADS1015_V_CHAN(3, ADS1015_AIN3, 16, 0),
 	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
 };

Signature · 1 line

2.34.1

Andy Shevchenko

2022-03-10 14:33 UTC #5 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 5 lines · Marek Vasut

These macros differ only in the number of valid bits of each ADC sample and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, ADS1115 is 16bit ADC shifted by 0. No functional change.
Signed-off-by Marek Vasut <[email protected]>

...

Quoted 4 lines

Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>

You may consider using --cc parameter in git send-email to avoid this noise in the commit messages.

...

drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 149

Quoted 3 lines

drivers/iio/adc/ti-ads1015.c +1 -1

-		.realbits = 12,					\
+		.realbits = (_realbits),			\
 		.storagebits = 16,				\

This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?

...

drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 172

Quoted 3 lines

drivers/iio/adc/ti-ads1015.c +1 -1

-		.realbits = 16,					\
+		.realbits = (_realbits),			\
 		.storagebits = 16,				\

Ditto.

I see two options:
1) add static assert to make sure realbits <= storagebits;
2) make it also configurable.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Marek Vasut

author

2022-03-10 23:29 UTC #6 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · Marek Vasut
These macros differ only in the number of valid bits of each ADC sample and the shift of those bits, i.e. ADS1015 is 12bit ADC shifted by 4 left, ADS1115 is 16bit ADC shifted by 0. No functional change.
Signed-off-by Marek Vasut <[email protected]>
...

Quoted 7 lines

Quoted 4 lines
Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>
You may consider using --cc parameter in git send-email to avoid this noise in the commit messages.

This is deliberate so I can keep track of who to CC on which patch.

Quoted 8 lines

...
drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 149
Quoted 3 lines
drivers/iio/adc/ti-ads1015.c +1 -1
-		.realbits = 12,					\
+		.realbits = (_realbits),			\
  		.storagebits = 16,				\
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?

When such a chip exists, this can be parametrized as well.

Quoted 10 lines

...
drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 172
Quoted 3 lines
drivers/iio/adc/ti-ads1015.c +1 -1
-		.realbits = 16,					\
+		.realbits = (_realbits),			\
  		.storagebits = 16,				\
Ditto.
I see two options:
1) add static assert to make sure realbits <= storagebits;

Does static_assert work in array of structures (I don't think it does) ?

Quoted 1 line

2) make it also configurable.

That would be unnecessary duplication, this patch is trying to DEduplicate the driver code, not REduplicate it differently.

Andy Shevchenko

2022-03-11 11:30 UTC #7 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 2 lines · Marek Vasut

Quoted 1 line · Andy Shevchenko
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote:

...

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>
You may consider using --cc parameter in git send-email to avoid this noise in the commit messages.
This is deliberate so I can keep track of who to CC on which patch.

You may add the Link tag to lore (which b4 tool can do automatically), so you can always access the email from the archives and track this down. No need to have this in each of the commit messages.

...

Quoted 8 lines

Quoted 6 lines
drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 149
Quoted 3 lines
drivers/iio/adc/ti-ads1015.c +1 -1
-            .realbits = 12,                                 \
+            .realbits = (_realbits),                        \
             .storagebits = 16,                              \
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?
When such a chip exists, this can be parametrized as well.

Yes, My point is that it's error prone.

...

Quoted 4 lines

Quoted 2 lines
I see two options:
1) add static assert to make sure realbits <= storagebits;
Does static_assert work in array of structures (I don't think it does) ?

You can check, but IIRC some of the macros have it. Don't remember the details, though.

Quoted 4 lines

Quoted 1 line
2) make it also configurable.
That would be unnecessary duplication, this patch is trying to DEduplicate the driver code, not REduplicate it differently.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Marek Vasut

author

2022-03-11 11:33 UTC #8 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 14 lines · Andy Shevchenko

Quoted 2 lines · Marek Vasut
Quoted 1 line · Andy Shevchenko
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote:
...
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>
You may consider using --cc parameter in git send-email to avoid this noise in the commit messages.

Quoted 6 lines

Quoted 1 line
This is deliberate so I can keep track of who to CC on which patch.
You may add the Link tag to lore (which b4 tool can do automatically), so you can always access the email from the archives and track this down. No need to have this in each of the commit messages.

This is used by git-send-email to put the right people on Cc, not by Lore.

Quoted 6 lines

...

Quoted 4 lines

Quoted 3 lines

drivers/iio/adc/ti-ads1015.c +1 -1

-            .realbits = 12,                                 \
+            .realbits = (_realbits),                        \
              .storagebits = 16,                              \

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?
When such a chip exists, this can be parametrized as well.
Yes, My point is that it's error prone.

Won't IIO core warn if realbits > storagebits ?

Quoted 9 lines

...
Quoted 4 lines
Quoted 2 lines
I see two options:
1) add static assert to make sure realbits <= storagebits;
Does static_assert work in array of structures (I don't think it does) ?
You can check, but IIRC some of the macros have it. Don't remember the details, though.

I already checked before replying, hence my question, as I didn't find a way to make it work.

Andy Shevchenko

2022-03-11 17:04 UTC #9 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 4 lines · Marek Vasut

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Marek Vasut
Quoted 1 line · Andy Shevchenko
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote:

...

Quoted 4 lines

Quoted 3 lines

drivers/iio/adc/ti-ads1015.c +1 -1

-            .realbits = 12,                                 \
+            .realbits = (_realbits),                        \
              .storagebits = 16,                              \

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?
When such a chip exists, this can be parametrized as well.
Yes, My point is that it's error prone.
Won't IIO core warn if realbits > storagebits ?

If it's the case, then it's very good!

...

Quoted 10 lines

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I see two options:
1) add static assert to make sure realbits <= storagebits;
Does static_assert work in array of structures (I don't think it does) ?
You can check, but IIRC some of the macros have it. Don't remember the details, though.
I already checked before replying, hence my question, as I didn't find a way to make it work.

It seems that current use cases have it either in functions or in the expressions as ({...}). I dunno if the result of ({...}) can be a data structure or compound literal.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Marek Vasut

author

2022-03-11 18:52 UTC #10 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 15 lines · Andy Shevchenko

Quoted 4 lines · Marek Vasut
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Marek Vasut
Quoted 1 line · Andy Shevchenko
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote:
...
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/iio/adc/ti-ads1015.c static const struct iio_event_spec ads1015_events[] = { · line 149
Quoted 3 lines
drivers/iio/adc/ti-ads1015.c +1 -1
-            .realbits = 12,                                 \
+            .realbits = (_realbits),                        \
               .storagebits = 16,                              \
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
When such a chip exists, this can be parametrized as well.
Yes, My point is that it's error prone.
Won't IIO core warn if realbits > storagebits ?
If it's the case, then it's very good!

No, apparently it won't .

Quoted 10 lines

...
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I see two options:
1) add static assert to make sure realbits <= storagebits;
Does static_assert work in array of structures (I don't think it does) ?
You can check, but IIRC some of the macros have it. Don't remember the details, though.

Quoted 6 lines

Quoted 2 lines
I already checked before replying, hence my question, as I didn't find a way to make it work.
It seems that current use cases have it either in functions or in the expressions as ({...}). I dunno if the result of ({...}) can be a data structure or compound literal.

I added a patch to v2, but ugh, it isn't nice:

[PATCH v2 7/7] iio: adc: ti-ads1015: Add static assert to test if shifted realbits fit into storagebits

Jonathan Cameron

maintainer

2022-03-20 13:15 UTC #11 lore raw

Re: [PATCH 4/6] iio: adc: ti-ads1015: Deduplicate channel macros

Quoted 18 lines · On Fri, 11 Mar 2022 19:51:56 +0100 Marek Vasut <[email protected]> wrote:

Quoted 17 lines · Andy Shevchenko
Quoted 4 lines · Marek Vasut
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Marek Vasut
Quoted 1 line · Andy Shevchenko
On Thu, Mar 10, 2022 at 01:34:00AM +0100, Marek Vasut wrote:
...
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +1 -1
-            .realbits = 12,                                 \
+            .realbits = (_realbits),                        \
               .storagebits = 16,                              \  
This seems inconsistent a bit. What if the next chip wants to have more than 16 bits in realbits?
When such a chip exists, this can be parametrized as well.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Yes, My point is that it's error prone.
Won't IIO core warn if realbits > storagebits ?
If it's the case, then it's very good!
No, apparently it won't .

Easy to add I think and a good idea. Though can only be a runtime check obviously.

Put a verification check in iio_buffer_add_channel_sys() which is registering the _type attr used to get access to this info from user space.

Jonathan

Quoted 22 lines

Quoted 11 lines
...
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I see two options:
1) add static assert to make sure realbits <= storagebits;
Does static_assert work in array of structures (I don't think it does) ?
You can check, but IIRC some of the macros have it. Don't remember the details, though.
Quoted 6 lines
Quoted 2 lines
I already checked before replying, hence my question, as I didn't find a way to make it work.
It seems that current use cases have it either in functions or in the expressions as ({...}). I dunno if the result of ({...}) can be a data structure or compound literal.
I added a patch to v2, but ugh, it isn't nice:
[PATCH v2 7/7] iio: adc: ti-ads1015: Add static assert to test if shifted realbits fit into storagebits

Marek Vasut

author

2022-03-10 00:34 UTC #12 lore raw

PATCH 5/6 iio: adc: ti-ads1015: Make channel event_spec optional

In reply to Marek Vasut

Pass event_spec and num_event_specs to ADS1015_V_CHAN and ADS1015_V_DIFF_CHAN macros, to make it possible to pass no event_spec at all for chips which have no comparator and thus no events. No functional change.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-ads1015.c | 60 +++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 22 deletions(-)

drivers/iio/adc/ti-ads1015.c +38 ~~-22~~

index fc3381ff34710..7d0c0552f425c 100644
@@ -134,7 +134,7 @@ static const struct iio_event_spec ads1015_events[] = {
 	},
 };
 
-#define ADS1015_V_CHAN(_chan, _addr, _realbits, _shift) {	\
+#define ADS1015_V_CHAN(_chan, _addr, _realbits, _shift, _event_spec, _num_event_specs) { \
 	.type = IIO_VOLTAGE,					\
 	.indexed = 1,						\
 	.address = _addr,					\

@@ -150,12 +150,12 @@ static const struct iio_event_spec ads1015_events[] = {
 		.shift = (_shift),				\
 		.endianness = IIO_CPU,				\
 	},							\
-	.event_spec = ads1015_events,				\
-	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
+	.event_spec = (_event_spec),				\
+	.num_event_specs = (_num_event_specs),			\
 	.datasheet_name = "AIN"#_chan,				\
 }
 
-#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr, _realbits, _shift) { \
+#define ADS1015_V_DIFF_CHAN(_chan, _chan2, _addr, _realbits, _shift, _event_spec, _num_event_specs) { \
 	.type = IIO_VOLTAGE,					\
 	.differential = 1,					\
 	.indexed = 1,						\

@@ -173,8 +173,8 @@ static const struct iio_event_spec ads1015_events[] = {
 		.shift = (_shift),				\
 		.endianness = IIO_CPU,				\
 	},							\
-	.event_spec = ads1015_events,				\
-	.num_event_specs = ARRAY_SIZE(ads1015_events),		\
+	.event_spec = (_event_spec),				\
+	.num_event_specs = (_num_event_specs),			\
 	.datasheet_name = "AIN"#_chan"-AIN"#_chan2,		\
 }

@@ -248,26 +248,42 @@ static const struct regmap_config ads1015_regmap_config = {
 };
 
 static const struct iio_chan_spec ads1015_channels[] = {
-	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4),
-	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 12, 4),
-	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 12, 4),
-	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 12, 4),
-	ADS1015_V_CHAN(0, ADS1015_AIN0, 12, 4),
-	ADS1015_V_CHAN(1, ADS1015_AIN1, 12, 4),
-	ADS1015_V_CHAN(2, ADS1015_AIN2, 12, 4),
-	ADS1015_V_CHAN(3, ADS1015_AIN3, 12, 4),
+	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 12, 4,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 12, 4,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 12, 4,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(0, ADS1015_AIN0, 12, 4,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(1, ADS1015_AIN1, 12, 4,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(2, ADS1015_AIN2, 12, 4,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(3, ADS1015_AIN3, 12, 4,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
 	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
 };
 
 static const struct iio_chan_spec ads1115_channels[] = {
-	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 16, 0),
-	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 16, 0),
-	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 16, 0),
-	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 16, 0),
-	ADS1015_V_CHAN(0, ADS1015_AIN0, 16, 0),
-	ADS1015_V_CHAN(1, ADS1015_AIN1, 16, 0),
-	ADS1015_V_CHAN(2, ADS1015_AIN2, 16, 0),
-	ADS1015_V_CHAN(3, ADS1015_AIN3, 16, 0),
+	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 16, 0,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 16, 0,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 16, 0,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 16, 0,
+			    ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(0, ADS1015_AIN0, 16, 0,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(1, ADS1015_AIN1, 16, 0,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(2, ADS1015_AIN2, 16, 0,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
+	ADS1015_V_CHAN(3, ADS1015_AIN3, 16, 0,
+		       ads1015_events, ARRAY_SIZE(ads1015_events)),
 	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
 };

Signature · 1 line

2.34.1

Marek Vasut

author

2022-03-10 00:34 UTC #13 lore raw

PATCH 6/6 iio: adc: ti-ads1015: Add TLA2024 support

In reply to Marek Vasut

Add support for TI TLA2024 ADC. This chip is compatible with ADS1015 except it has no comparator in it, hence the comparator configuration bits are missing in Configuration Register and the Hi_Thresh/Lo_Thresh registers are missing as well and so is event support.

Signed-off-by Marek Vasut <[email protected]>

Cc Andy Shevchenko <[email protected]>

Cc Daniel Baluta <[email protected]>

Cc Jonathan Cameron <[email protected]>

---
 drivers/iio/adc/ti-ads1015.c | 53 ++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

drivers/iio/adc/ti-ads1015.c +51 -2

index 7d0c0552f425c..dd36fe0a45cba 100644
@@ -80,6 +80,7 @@ enum chip_ids {
 	ADSXXXX = 0,
 	ADS1015,
 	ADS1115,
+	TLA2024,
 };
 
 enum ads1015_channels {

@@ -247,6 +248,22 @@ static const struct regmap_config ads1015_regmap_config = {
 	.wr_table = &ads1015_writeable_table,
 };
 
+static const struct regmap_range tla2024_writeable_ranges[] = {
+	regmap_reg_range(ADS1015_CFG_REG, ADS1015_CFG_REG),
+};
+
+static const struct regmap_access_table tla2024_writeable_table = {
+	.yes_ranges = tla2024_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(tla2024_writeable_ranges),
+};
+
+static const struct regmap_config tla2024_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = ADS1015_CFG_REG,
+	.wr_table = &tla2024_writeable_table,
+};
+
 static const struct iio_chan_spec ads1015_channels[] = {
 	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4,
 			    ads1015_events, ARRAY_SIZE(ads1015_events)),

@@ -287,6 +304,19 @@ static const struct iio_chan_spec ads1115_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
 };
 
+static const struct iio_chan_spec tla2024_channels[] = {
+	ADS1015_V_DIFF_CHAN(0, 1, ADS1015_AIN0_AIN1, 12, 4, NULL, 0),
+	ADS1015_V_DIFF_CHAN(0, 3, ADS1015_AIN0_AIN3, 12, 4, NULL, 0),
+	ADS1015_V_DIFF_CHAN(1, 3, ADS1015_AIN1_AIN3, 12, 4, NULL, 0),
+	ADS1015_V_DIFF_CHAN(2, 3, ADS1015_AIN2_AIN3, 12, 4, NULL, 0),
+	ADS1015_V_CHAN(0, ADS1015_AIN0, 12, 4, NULL, 0),
+	ADS1015_V_CHAN(1, ADS1015_AIN1, 12, 4, NULL, 0),
+	ADS1015_V_CHAN(2, ADS1015_AIN2, 12, 4, NULL, 0),
+	ADS1015_V_CHAN(3, ADS1015_AIN3, 12, 4, NULL, 0),
+	IIO_CHAN_SOFT_TIMESTAMP(ADS1015_TIMESTAMP),
+};
+
+
 #ifdef CONFIG_PM
 static int ads1015_set_power_state(struct ads1015_data *data, bool on)
 {

@@ -823,6 +853,12 @@ static const struct iio_info ads1115_info = {
 	.attrs          = &ads1115_attribute_group,
 };
 
+static const struct iio_info tla2024_info = {
+	.read_raw	= ads1015_read_raw,
+	.write_raw	= ads1015_write_raw,
+	.attrs          = &ads1115_attribute_group,
+};
+
 static int ads1015_client_get_channels_config(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);

@@ -937,6 +973,12 @@ static int ads1015_probe(struct i2c_client *client,
 		indio_dev->info = &ads1115_info;
 		data->data_rate = (unsigned int *) &ads1115_data_rate;
 		break;
+	case TLA2024:
+		indio_dev->channels = tla2024_channels;
+		indio_dev->num_channels = ARRAY_SIZE(tla2024_channels);
+		indio_dev->info = &tla2024_info;
+		data->data_rate = (unsigned int *) &ads1015_data_rate;
+		break;
 	default:
 		dev_err(&client->dev, "Unknown chip %d\n", chip);
 		return -EINVAL;

@@ -957,7 +999,9 @@ static int ads1015_probe(struct i2c_client *client,
 	/* we need to keep this ABI the same as used by hwmon ADS1015 driver */
 	ads1015_get_channels_config(client);
 
-	data->regmap = devm_regmap_init_i2c(client, &ads1015_regmap_config);
+	data->regmap = devm_regmap_init_i2c(client, (chip == TLA2024) ?
+					    &tla2024_regmap_config :
+					    &ads1015_regmap_config);
 	if (IS_ERR(data->regmap)) {
 		dev_err(&client->dev, "Failed to allocate register map\n");
 		return PTR_ERR(data->regmap);

@@ -971,7 +1015,7 @@ static int ads1015_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	if (client->irq) {
+	if (client->irq && chip != TLA2024) {
 		unsigned long irq_trig =
 			irqd_get_trigger_type(irq_get_irq_data(client->irq));
 		unsigned int cfg_comp_mask = ADS1015_CFG_COMP_QUE_MASK |

@@ -1073,6 +1117,7 @@ static const struct dev_pm_ops ads1015_pm_ops = {
 static const struct i2c_device_id ads1015_id[] = {
 	{"ads1015", ADS1015},
 	{"ads1115", ADS1115},
+	{"tla2024", TLA2024},
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, ads1015_id);

@@ -1086,6 +1131,10 @@ static const struct of_device_id ads1015_of_match[] = {
 		.compatible = "ti,ads1115",
 		.data = (void *)ADS1115
 	},
+	{
+		.compatible = "ti,tla2024",
+		.data = (void *)TLA2024
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, ads1015_of_match);

Signature · 1 line

2.34.1

Krzysztof Kozlowski

Acked-by on v1 1/6

2022-03-10 12:51 UTC #14 lore raw

In reply to Marek Vasut

Quoted 11 lines · Marek Vasut

Add missing ti,ads1115 compatible string. This compatible string is supported by the Linux kernel driver and the ads1015 is a 12bit ADC while ads1115 is 16bit ADC. Add the missing compatible string.
Signed-off-by Marek Vasut <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Daniel Baluta <[email protected]>
Cc Jonathan Cameron <[email protected]>
Cc Rob Herring <[email protected]>
Cc [email protected]

Quoted 4 lines

---
 Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Acked-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof