[v7,1/2] dt-bindings: adc: add AD7173

The AD7173 family offer a complete integrated Sigma-Delta ADC solution
which can be used in high precision, low noise single channel applications
or higher speed multiplexed applications. The Sigma-Delta ADC is intended
primarily for measurement of signals close to DC but also delivers
outstanding performance with input bandwidths out to ~10kHz.

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
 .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml

On 12/5/23 15:42, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.
> 


V6 -> V7

 - format Kconfig supported models using '-'

 - include types.h instead of stddef.h

 - change device_info->num_gpios type to u8

 - reorder fields in ad7173_state, place ad_sigma_delta

 - reorder fields in ad7173_device_info, group by type

 - add default case in read_raw()

 - rename ad7173_debug() to ad7173_debug_reg_access()

 - remove explicit u8 cast

 - remove return 0; for offset in read_raw()

 - add '\n' to error messages

 - reorder probe variables -> reversed xmas tree

 - remove redundant inner commas in of_match and id_table

 - simplify selected reference code by setting default value and
ignoring fwnode_property_read_string() error

 - use match_string() for finding selected reference

 - err on no channels specified

 - add missing fwnode_handle_put(child) on err return branches

 - remove spi_set_drvdata() from probe

 - remove offset variable from find_live_config(), not used

 - add comment showing a generic LRU implementation might be used if one
will exist



 - MISC:  add blank line to chanel_config struct, trailing comma
ref_sel_str[],
 remove blank line in update_scan_mode(), add spaces around '/'

On 12/5/23 15:42, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
> 

Bindings V6 -> V7
 - remove redundant bipolar attribute
 - add reference-select example

On Tue, 05 Dec 2023 15:42:20 +0200, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml: properties:required: ['compatible', 'reg', 'interrupts'] is not of type 'object', 'boolean'
	from schema $id: http://json-schema.org/draft-07/schema#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml: properties: 'required' should not be valid under {'$ref': '#/definitions/json-schema-prop-names'}
	hint: A json-schema keyword was found instead of a DT property name.
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml: properties:required: ['compatible', 'reg', 'interrupts'] is not of type 'object', 'boolean'
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231205134223.17335-1-mitrutzceclan@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:
>
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel
> applications or higher speed multiplexed applications. The Sigma-Delta
> ADC is intended primarily for measurement of signals close to DC but also
> delivers outstanding performance with input bandwidths out to ~10kHz.

...

> +config AD7173
> +       tristate "Analog Devices AD7173 driver"
> +       depends on SPI_MASTER
> +       select AD_SIGMA_DELTA
> +       select GPIO_REGMAP if GPIOLIB
> +       select REGMAP_SPI if GPIOLIB
> +       help
> +         Say yes here to build support for Analog Devices AD7173 and similar ADC
> +         Currently supported models:
> +          - AD7172-2,
> +          - AD7173-8,
> +          - AD7175-2,
> +          - AD7176-2

Drop commas (sorry if I was not clear enough).


> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitmap.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>

> +#include <linux/gpio/driver.h>

This...

> +#include <linux/idr.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

> +#include <linux/gpio/regmap.h>

...and this are too far from each other. I believe you should count on G order.

> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +#define AD7173_GPO12_DATA(x)   BIT(x)

x + 0 ?

> +#define AD7173_GPO23_DATA(x)   BIT(x + 4)
> +#define AD7173_GPO_DATA(x)     (x < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))

...

> +               dev_warn(&st->sd.spi->dev,

Here...

> +                        "Unexpected device id: 0x%04X, expected: 0x%04X\n",
> +                        id, st->info->id);
> +
> +       st->adc_mode |= AD7173_ADC_MODE_SING_CYC;
> +       st->interface_mode = 0x0;
> +
> +       st->config_usage_counter = 0;
> +       st->config_cnts = devm_kcalloc(indio_dev->dev.parent,

...and here are different pointers in use, can you unify to use the
physical device pointer (as per above) here as well?

> +                                      st->info->num_configs, sizeof(u64),
> +                                      GFP_KERNEL);
> +       if (!st->config_cnts)
> +               return -ENOMEM;

...

> +       case IIO_CHAN_INFO_SCALE:
> +               if (chan->type == IIO_TEMP) {
> +                       *val = 250000000;

MEGA?

> +                       *val2 = 800273203; /* (2^24 * 477) / 10 */

Why not write it as is:

                       *val2 = (BIT(24) * 477) / 10;

?

It will be more robust as we don't expect compiler to give different
results here

> +                       return IIO_VAL_FRACTIONAL;
> +               } else {
> +                       *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
> +                       *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
> +                       return IIO_VAL_FRACTIONAL_LOG2;
> +               }

...

> +       /* If a regulator is not available, it will be set to a dummy regulator.
> +        * Each channel reference is checked with regulator_get_voltage() before
> +        *  setting attributes so if any channel uses a dummy supply the driver
> +        *  probe will fail.
> +        */

/*
 * Multi-line comments should follow this style. Find the
 * difference.
 */

...

> +               ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
> +
> +               fwnode_property_read_string(child, "adi,reference-select",
> +                                           &ref_label);
> +               ref_sel = match_string(ad7173_ref_sel_str,
> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
> +               if (ref_sel < 0) {

Can we use fwnode_property_match_property_string()?

> +                       fwnode_handle_put(child);
> +                       return dev_err_probe(dev, -EINVAL,
> +                                            "Invalid channel reference name %s\n",
> +                                            ref_label);
> +               }

On Tue, Dec 05, 2023 at 05:28:34PM +0200, Andy Shevchenko wrote:
> On Tue, Dec 5, 2023 at 3:46 PM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

...

> > +       case IIO_CHAN_INFO_SCALE:
> > +               if (chan->type == IIO_TEMP) {
> > +                       *val = 250000000;
> 
> MEGA?

Or MICRO?

On 12/5/23 17:28, Andy Shevchenko wrote:
>> +               ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
>> +
>> +               fwnode_property_read_string(child, "adi,reference-select",
>> +                                           &ref_label);
>> +               ref_sel = match_string(ad7173_ref_sel_str,
>> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
>> +               if (ref_sel < 0) {
> Can we use fwnode_property_match_property_string()?

fwnode_property_match_string() searches a given string in a device-tree
string array and returns the index. I do not think that this function
fits here as the DT attribute is a single string.

On Tue, Dec 05, 2023 at 03:42:20PM +0200, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
> ---
>  .../bindings/iio/adc/adi,ad7173.yaml          | 170 ++++++++++++++++++
>  1 file changed, 170 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> new file mode 100644
> index 000000000000..087820a0cf48
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -0,0 +1,170 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7173 ADC
> +
> +maintainers:
> +  - Ceclan Dumitru <dumitru.ceclan@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7172-2
> +      - adi,ad7173-8
> +      - adi,ad7175-2
> +      - adi,ad7176-2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  spi-max-frequency:
> +    maximum: 20000000
> +
> +  refin-supply:
> +    description: external reference supply, can be used as reference for conversion.
> +
> +  refin2-supply:
> +    description: external reference supply, can be used as reference for conversion.
> +
> +  avdd-supply:
> +    description: avdd supply, can be used as reference for conversion.
> +

> +  required:
> +    - compatible
> +    - reg
> +    - interrupts

This is at the wrong level of indent (as Rob's bot pointed out) and
should come after patternProperties too.

Otherwise, this looks okay to me.
Thanks,
Conor.

On 12/5/23 18:25, Conor Dooley wrote:
> On Tue, Dec 05, 2023 at 03:42:20PM +0200, Dumitru Ceclan wrote:
>> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
>> which can be used in high precision, low noise single channel applications
>> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
>> primarily for measurement of signals close to DC but also delivers
>> outstanding performance with input bandwidths out to ~10kHz.
>>
...
>> +  required:
>> +    - compatible
>> +    - reg
>> +    - interrupts
> 
> This is at the wrong level of indent (as Rob's bot pointed out) and
> should come after patternProperties too.
> 

Yep, it was fixed in V6, forgot to include it

On Tue, Dec 05, 2023 at 06:12:18PM +0200, Ceclan Dumitru wrote:
> On 12/5/23 17:28, Andy Shevchenko wrote:
> >> +               ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
> >> +
> >> +               fwnode_property_read_string(child, "adi,reference-select",
> >> +                                           &ref_label);
> >> +               ref_sel = match_string(ad7173_ref_sel_str,
> >> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
> >> +               if (ref_sel < 0) {
> > Can we use fwnode_property_match_property_string()?
> 
> fwnode_property_match_string() searches a given string in a device-tree
> string array and returns the index. I do not think that this function
> fits here as the DT attribute is a single string.

I'm not talking about that. I mentioned different API call.

/**
 * fwnode_property_match_property_string - find a property string value in an array and return index
 * @fwnode: Firmware node to get the property of
 * @propname: Name of the property holding the string value
 * @array: String array to search in
 * @n: Size of the @array
 *
 * Find a property string value in a given @array and if it is found return
 * the index back.
 *
 * Return: index, starting from %0, if the string value was found in the @array (success),
 *         %-ENOENT when the string value was not found in the @array,
 *         %-EINVAL if given arguments are not valid,
 *         %-ENODATA if the property does not have a value,
 *         %-EPROTO or %-EILSEQ if the property is not a string,
 *         %-ENXIO if no suitable firmware interface is present.
 */

On 12/5/23 19:18, Andy Shevchenko wrote:
> On Tue, Dec 05, 2023 at 06:12:18PM +0200, Ceclan Dumitru wrote:
>> On 12/5/23 17:28, Andy Shevchenko wrote:
>>>> +               ref_label = 
>>>> + ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
>>>> +
>>>> +               fwnode_property_read_string(child, "adi,reference-select",
>>>> +                                           &ref_label);
>>>> +               ref_sel = match_string(ad7173_ref_sel_str,
>>>> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
>>>> +               if (ref_sel < 0) {
>>> Can we use fwnode_property_match_property_string()?
>>
>> fwnode_property_match_string() searches a given string in a 
>> device-tree string array and returns the index. I do not think that 
>> this function fits here as the DT attribute is a single string.
> 
> I'm not talking about that. I mentioned different API call.
> 
> /**
>  * fwnode_property_match_property_string - find a property string value in an array and return index
>  * @fwnode: Firmware node to get the property of
>  * @propname: Name of the property holding the string value
>  * @array: String array to search in
>  * @n: Size of the @array
>  *
>  * Find a property string value in a given @array and if it is found return
>  * the index back.
>  *
>  * Return: index, starting from %0, if the string value was found in the @array (success),
>  *         %-ENOENT when the string value was not found in the @array,
>  *         %-EINVAL if given arguments are not valid,
>  *         %-ENODATA if the property does not have a value,
>  *         %-EPROTO or %-EILSEQ if the property is not a string,
>  *         %-ENXIO if no suitable firmware interface is present.
>  */
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Yep, it can be used. I cannot seem to find it upstream but found the
thread, when the patch containing that function will be applied I'll
change to that.

On Tue, 5 Dec 2023 19:18:12 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Dec 05, 2023 at 06:12:18PM +0200, Ceclan Dumitru wrote:
> > On 12/5/23 17:28, Andy Shevchenko wrote:  
> > >> +               ref_label = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF];
> > >> +
> > >> +               fwnode_property_read_string(child, "adi,reference-select",
> > >> +                                           &ref_label);
> > >> +               ref_sel = match_string(ad7173_ref_sel_str,
> > >> +                                      ARRAY_SIZE(ad7173_ref_sel_str), ref_label);
> > >> +               if (ref_sel < 0) {  
> > > Can we use fwnode_property_match_property_string()?  
> > 
> > fwnode_property_match_string() searches a given string in a device-tree
> > string array and returns the index. I do not think that this function
> > fits here as the DT attribute is a single string.  
> 
> I'm not talking about that. I mentioned different API call.
> 
> /**
>  * fwnode_property_match_property_string - find a property string value in an array and return index
>  * @fwnode: Firmware node to get the property of
>  * @propname: Name of the property holding the string value
>  * @array: String array to search in
>  * @n: Size of the @array
>  *
>  * Find a property string value in a given @array and if it is found return
>  * the index back.
>  *
>  * Return: index, starting from %0, if the string value was found in the @array (success),
>  *         %-ENOENT when the string value was not found in the @array,
>  *         %-EINVAL if given arguments are not valid,
>  *         %-ENODATA if the property does not have a value,
>  *         %-EPROTO or %-EILSEQ if the property is not a string,
>  *         %-ENXIO if no suitable firmware interface is present.
>  */
> 
Which is in the togreg branch of iio.git (was a patch from Andy that I've queued up)

Jonathan