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

  From: Dumitru Ceclan <mitrutzceclan@gmail.com>

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>
---
V5->V6
 - Moved global required property to proper placement

 .../bindings/iio/adc/adi,ad7173.yaml          | 178 ++++++++++++++++++
 1 file changed, 178 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
  

On Thu, Nov 23, 2023 at 05:23:22PM +0200, mitrutzceclan wrote:
> From: Dumitru Ceclan <mitrutzceclan@gmail.com>

Thank you for the update!
My comments below.

> 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.
> 
> 

One blank line is enough here.

> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

...

> V5->V6
>  - No changes

Don't issue patches too often (minimum gap between versions is 24h).

...

> +	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

I would use

 - FOO
 - BAR

style that will reduce amount of potential churn if you need to add an entry at
the end of this list.

> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad7173.

...

> +#include <linux/stddef.h>

You probably meant types.h here (it will include stddef, at least most of
the code relies on that), which is currently absent.

...

> +struct ad7173_device_info {
> +	char *name;
> +	unsigned int id;
> +	unsigned int num_inputs;
> +	unsigned int num_configs;
> +	unsigned int num_channels;

> +	unsigned char num_gpios;

I would use u8 as you have done for cfg_slot, for example. As it holds a number
and not a real character.

> +	bool has_temp;
> +	unsigned int clock;
> +
> +	const unsigned int *sinc5_data_rates;
> +	unsigned int num_sinc5_data_rates;
> +};

...

> +struct ad7173_channel_config {
> +	u8 cfg_slot;
> +	bool live;

Perhaps a blank line?

> +	/* Following fields are used to compare equality. */
> +	struct_group(config_props,
> +		bool bipolar;
> +		bool input_buf;
> +		u8 odr;
> +		u8 ref_sel;
> +	);
> +};

...

> +struct ad7173_state {
> +	const struct ad7173_device_info *info;
> +	struct ad_sigma_delta sd;

It might be better to embed that struct first. In any case you always can
consult with `pahole` tool for data structure layouts.

> +	struct ad7173_channel *channels;
> +	struct regulator_bulk_data regulators[3];
> +	unsigned int adc_mode;
> +	unsigned int interface_mode;
> +	unsigned int num_channels;
> +	struct ida cfg_slots_status;
> +	unsigned long long config_usage_counter;
> +	unsigned long long *config_cnts;
> +#if IS_ENABLED(CONFIG_GPIOLIB)
> +	struct regmap *reg_gpiocon_regmap;
> +	struct gpio_regmap *gpio_regmap;
> +#endif
> +};

...

> +static const char *const ad7173_ref_sel_str[] = {
> +	[AD7173_SETUP_REF_SEL_EXT_REF]    = "refin",
> +	[AD7173_SETUP_REF_SEL_EXT_REF2]   = "refin2",
> +	[AD7173_SETUP_REF_SEL_INT_REF]    = "refout-avss",

> +	[AD7173_SETUP_REF_SEL_AVDD1_AVSS] = "avdd"

Leave trailing comma here as well.

> +};

...

> +	struct device *dev = &st->sd.spi->dev;

For example, here st->sd become a no-op at compile time (see above about
placing sd to be the first member). The code generation can be checked
(for the size) by bloat-o-meter.

...

> +static int ad7173_free_config_slot_lru(struct ad7173_state *st)

> +static int ad7173_load_config(struct ad7173_state *st,
> +			      struct ad7173_channel_config *cfg)

Have you checked, btw, list_lru.h? Maybe all this can be simply changed by
using existing library?

...

> +	return vref / (MICRO/MILLI);

Wouldn't MILLI in the denominator just suffice?

...

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}

> +	return -EINVAL;

Make this 'default' case.

...

> +static int ad7173_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	int i, ret;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		if (test_bit(i, scan_mask))
> +			ret = ad7173_set_channel(&st->sd, i);
> +		else
> +			ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(i), 2, 0);

> +

Unneeded blank line.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

> +static int ad7173_debug(struct iio_dev *indio_dev, unsigned int reg,
> +			unsigned int writeval, unsigned int *readval)

Hmm... The function suggests it debugs something or helps with debugging
something. Without actual description is hard to understand the purpose.
Can you add a top comment on this function with explanations?

...

> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);

One line?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");

...

> +			return dev_err_probe(dev, -EINVAL,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);

Seems broken indentation.

...

> +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> +		if (!ret) {
> +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> +					ref_sel = i;
> +					break;
> +				}

> +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
> +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);

Too long line.

> +		} else if (ret != -EINVAL) {
> +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> +		}


Use standard pattern and it will be easier to see that 'else' is redundant.

		if (ret == -EINVAL) // However I don't like this handling of
				    // properties, but up to you and maintainer
			ret = 0;
		if (ret)
			return dev_err_probe(...);


BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?

...

> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);

Why casting?

> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot use reference %u", ref_sel);

...

> +			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8");

Missing \n. Check all your messages that they are terminated with \n.

...

> +	struct ad7173_state *st;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	int ret;

Reversed xmas tree order?

	struct device *dev = &spi->dev;
	struct iio_dev *indio_dev;
	struct ad7173_state *st;
	int ret;

...

> +static const struct of_device_id ad7173_of_match[] = {
> +	{ .compatible = "adi,ad7172-2",
> +	  .data = &ad7173_device_info[ID_AD7172_2], },
> +	{ .compatible = "adi,ad7173-8",
> +	  .data = &ad7173_device_info[ID_AD7173_8], },
> +	{ .compatible = "adi,ad7175-2",
> +	  .data = &ad7173_device_info[ID_AD7175_2], },
> +	{ .compatible = "adi,ad7176-2",
> +	  .data = &ad7173_device_info[ID_AD7176_2], },

Last inner commas are not needed.

> +	{ }
> +};

...

> +static const struct spi_device_id ad7173_id_table[] = {
> +	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2], },
> +	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8], },
> +	{ "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2], },
> +	{ "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2], },

Ditto.

> +	{ }
> +};
  

Yo,

This looks a lot better IMO, thanks.

On Thu, Nov 23, 2023 at 05:23:21PM +0200, mitrutzceclan wrote:
> +patternProperties:
> +  "^channel@[0-9a-f]$":
> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false

> +      bipolar:
> +        type: boolean

This should be defined in adc.yaml, so duping it here is not needed?

> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          not:
> +            contains:
> +              const: adi,ad7173-8
> +    then:
> +      properties:
> +        refin2-supply: false
> +      patternProperties:
> +        "^channel@[0-9a-f]$":
> +          properties:
> +            adi,reference-select:
> +              enum:
> +                - refin
> +                - refout-avss
> +                - avdd

I assume you tested that this restriction works as intended?

Cheers,
Conor.
  

> 
> ...
> 
> > +	return vref / (MICRO/MILLI);  
> 
> Wouldn't MILLI in the denominator just suffice?

Just a quick comment here. Given this is converting from micro to milli units
I'd consider the maths here be acting as documentation of that which would be lost if
/MILLI only used.  Need spaces around the / though


> 
> ...
> 
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		reg = st->channels[chan->address].cfg.odr;
> > +
> > +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> > +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> > +
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	}  
> 
> > +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> > +		if (!ret) {
> > +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> > +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> > +					ref_sel = i;
> > +					break;
> > +				}  
> 
> > +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
> > +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);  
> 
> Too long line.
> 
> > +		} else if (ret != -EINVAL) {
> > +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> > +		}  
> 
> 
> Use standard pattern and it will be easier to see that 'else' is redundant.
> 
> 		if (ret == -EINVAL) // However I don't like this handling of
> 				    // properties, but up to you and maintainer

Personally I'd check for existence of property first and only try reading if it
exists.  Avoid dance with resetting ret to 0.

> 			ret = 0;
> 		if (ret)
> 			return dev_err_probe(...);
> 
> 
> BUT. Isn't it a home grown variant of fwnode_property_match_property_string()?

true enough...  I'd still add an existence check first given this one is optional.

Jonathan
  

On Thu, 23 Nov 2023 17:23:21 +0200
mitrutzceclan <mitrutzceclan@gmail.com> wrote:

> From: Dumitru Ceclan <mitrutzceclan@gmail.com>
> 
> 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>

An example of reference selection would be good to have. Otherwise
looks fine to me, but I'll wait for the binding experts to take another
look.
  

On Thu, 23 Nov 2023 17:23:22 +0200
mitrutzceclan <mitrutzceclan@gmail.com> wrote:

> From: Dumitru Ceclan <mitrutzceclan@gmail.com>
> 
> 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.
> 
> 
> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>

Hi.

A few minor things from me.

thanks,

Jonathan

> new file mode 100644
> index 000000000000..ca307d406cee
> --- /dev/null
> +++ b/drivers/iio/adc/ad7173.c
> @@ -0,0 +1,962 @@

...

> +
> +static int ad7173_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct ad7173_channel *ch = &st->channels[chan->address];
> +	unsigned int reg;
> +	int ret;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ad_sigma_delta_single_conversion(indio_dev, chan, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* disable channel after single conversion */
> +		ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 250000000;
> +			*val2 = 800273203; /* (2^24 * 477) / 10 */
> +			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;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type == IIO_TEMP) {
> +			*val = -874379;
> +		} else {
> +			if (ch->cfg.bipolar)
> +				*val = -BIT(chan->scan_type.realbits - 1);
> +			else
> +				*val = 0;
Ideally if the channel isn't bipolar, don't expose the offset at all. Default
assumption if there is no such file is 0 offset. 
If that's fiddly to do then fine to keep the file though.

> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		reg = st->channels[chan->address].cfg.odr;
> +
> +		*val = st->info->sinc5_data_rates[reg] / MILLI;
> +		*val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI);
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;


...


> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> +{
> +	struct ad7173_channel *channels_st_priv_arr, *chan_st_priv;
> +	struct ad7173_state *st = iio_priv(indio_dev);
> +	struct device *dev = indio_dev->dev.parent;
> +	struct iio_chan_spec *chan_arr, *chan;
> +	struct fwnode_handle *child;
> +	unsigned int ain[2], chan_index = 0;
> +	unsigned int num_channels;
> +	const char *ref_label;
> +	u32 ref_sel;
> +	int i, ret;
> +
> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);

If only some of them are supplied, the driver works fine as long as no channel is using them?
If so should probably get the optionally then check for availability of the ones you
want.  I'm sure someone will just wire up ref.  Incidentally, ref2 isn't there for all chips
I think, so we should not get it on the ones where it can't exist.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add regulators disable action\n");
> +
> +	num_channels = device_get_child_node_count(dev);
> +
> +	if (st->info->has_temp)
> +		num_channels++;
> +
> +	if (num_channels == 0)
> +		return 0;

Isn't it all a bit pointless if this happens?  So maybe an error makes more sense?

> +	st->num_channels = num_channels;
> +
> +	chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels, GFP_KERNEL);
> +	if (!chan_arr)
> +		return -ENOMEM;
> +
> +	channels_st_priv_arr = devm_kcalloc(dev, num_channels,
> +					    sizeof(*channels_st_priv_arr),
> +					    GFP_KERNEL);
> +	if (!channels_st_priv_arr)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = chan_arr;
> +	indio_dev->num_channels = num_channels;
> +	st->channels = channels_st_priv_arr;
> +
> +	if (st->info->has_temp) {
> +		chan_arr[chan_index] = ad7173_temp_iio_channel_template;
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		chan_st_priv->ain =
> +			AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2);
> +		chan_st_priv->cfg.bipolar = false;
> +		chan_st_priv->cfg.input_buf = true;
> +		chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> +
> +		chan_index++;
> +	}
> +
> +	device_for_each_child_node(dev, child) {
> +		chan = &chan_arr[chan_index];
> +		chan_st_priv = &channels_st_priv_arr[chan_index];
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     ain, ARRAY_SIZE(ain));
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (ain[0] >= st->info->num_inputs ||
> +		    ain[1] >= st->info->num_inputs) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +				"Input pin number out of range for pair (%d %d).", ain[0], ain[1]);
> +		}
> +
> +		ref_sel = AD7173_SETUP_REF_SEL_INT_REF;
> +		ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label);
> +		if (!ret) {
> +			for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++)
> +				if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) {
> +					ref_sel = i;
> +					break;
> +				}
> +
> +			if (i == ARRAY_SIZE(ad7173_ref_sel_str))
Missing fwnode_handle_put(child). Same for other error paths in th device_for_each_child_node() 
loop. I wonder if that can get cleaned up with the cleanup.h magic.  Anyhow, that's a question
for another day.

> +				return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label);


> +		} else if (ret != -EINVAL) {
Up to you, but you could be lazier and set ref_label = ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF] before
trying to read it then don't bother handling the fwnode property read error at all
That's common practice when there is a default.

It would unnecessarily run the string match when the property wasn't there, but that's not too bad
for the code simplification it would give I think

> +			return dev_err_probe(dev, ret, "Invalid channel reference value");
> +		}
> +
> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Cannot use reference %u", ref_sel);
> +		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 &&
> +		    st->info->id != AD7173_ID)
> +			return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8");

Why all the way down here?  I'd do this a few lines earlier so we haven't already used the bonkers
value.

> +		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
> +			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> +		chan_st_priv->cfg.ref_sel = ref_sel;
> +
> +		*chan = ad7173_channel_template;
> +		chan->address = chan_index;
> +		chan->scan_index = chan_index;
> +		chan->channel = ain[0];
> +		chan->channel2 = ain[1];
> +		chan->differential = true;
> +
> +		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> +		chan_st_priv->chan_reg = chan_index;
> +		chan_st_priv->cfg.input_buf = true;
> +		chan_st_priv->cfg.odr = 0;
> +
> +		chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar");
> +		if (chan_st_priv->cfg.bipolar)
> +			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
> +
> +		chan_index++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7173_probe(struct spi_device *spi)
> +{
> +	struct ad7173_state *st;
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->info = device_get_match_data(dev);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	ida_init(&st->cfg_slots_status);
> +	ret = devm_add_action_or_reset(dev, ad7173_ida_destroy, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad7173_info;
> +
> +	spi->mode = SPI_MODE_3;
> +
> +	ad7173_sigma_delta_info.num_slots = st->info->num_configs;
> +	ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, indio_dev);

Used?  I'm not immediately seeing where. If not, don't set it.

Thanks,

Jonathan
  

On 11/23/23 20:02, Conor Dooley wrote:
> On Thu, Nov 23, 2023 at 05:23:21PM +0200, mitrutzceclan wrote:
>> +allOf:
>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          not:
>> +            contains:
>> +              const: adi,ad7173-8
>> +    then:
>> +      properties:
>> +        refin2-supply: false
>> +      patternProperties:
>> +        "^channel@[0-9a-f]$":
>> +          properties:
>> +            adi,reference-select:
>> +              enum:
>> +                - refin
>> +                - refout-avss
>> +                - avdd
> 
> I assume you tested that this restriction works as intended?
> 

Yes, for both reference and reference-select
  

On Mon, 4 Dec 2023 18:49:21 +0200
Ceclan Dumitru <mitrutzceclan@gmail.com> wrote:

> On 11/25/23 19:21, Jonathan Cameron wrote:
> > On Thu, 23 Nov 2023 17:23:22 +0200
> > mitrutzceclan <mitrutzceclan@gmail.com> wrote:
> >   
> >> From: Dumitru Ceclan <mitrutzceclan@gmail.com>  
> 
> ...
> >> +	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
> >> +	st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2];
> >> +	st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS];
> >> +
> >> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators),
> >> +				      st->regulators);  
> > 
> > If only some of them are supplied, the driver works fine as long as no channel is using them?
> > If so should probably get the optionally then check for availability of the ones you
> > want.  I'm sure someone will just wire up ref.  Incidentally, ref2 isn't there for all chips
> > I think, so we should not get it on the ones where it can't exist.
> >   
> 
> 
> This sets a dummy regulator in place if no proper supply is found. Then
> the call regulator_get_voltage() on the dummy will fail. About getting
> ref2, sure, I'll set the string only with the right ID.
> 
> ...
> >> +		ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel);
> >> +		if (ret < 0)
> >> +			return dev_err_probe(dev, ret,
> >> +					     "Cannot use reference %u", ref_sel);  
> 
> Here the probe would not continue if a channel selects a supply that is
> not available in the DT.
Ok. It's a little ugly though - so maybe a comment at the bulk_get to say
it will fail cleanly a bit later.