RE: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Paller, Kim Seer]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Tuesday, April 8, 2025 10:06 PM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Hennerich,
> Michael <Michael.Hennerich@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
> AD3531R
> 
> [External]
> 
> On 4/7/25 3:01 AM, Paller, Kim Seer wrote:
> >>> +	for (i = 0; i < chip_info->num_channels; i++)
> >>> +		st->chan[i].powerdown_mode =
> >> AD3530R_32KOHM_POWERDOWN_MODE;
> >>> +
> >>> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac",
> >> GPIOD_OUT_HIGH);
> >>> +	if (IS_ERR(st->ldac_gpio))
> >>> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> >>> +				     "Failed to get ldac GPIO\n");
> >>> +
> >>> +	if (device_property_present(dev, "adi,double-output-range")) {
> >>
> >> This is not documented in the DT bindings.
> >>
> >> This could instead be implemented by making the out_voltage_scale
> attribute
> >> writeable.
> >>
> >> If we really do need it in the devicetree because we want to protect against
> >> someone accidentally setting it too high, then the property should be the
> actual
> >> multipler value with an enum specifier of 1, 2 and a default of 1 rather than
> a
> >> flag (e.g. adi,output-multipler).
> >
> > Thank you for the feedback. This should be adi,range-double, which is already
> > documented in the DT bindings and is also used as a boolean type in other
> drivers.
> > I just forgot to update it here. This is a one-bit configuration that doubles the
> > output range (multiplier of 2).  Should I proceed with the suggested
> approach?
> >
> 
> I see adi,range-double in
> Documentation/devicetree/bindings/iio/adc/adi,ad7923.yaml
> 
> We would need to add the same in the new .yaml file added in this
> series along with a justification in the commit message of why this
> needs to be set in the devicetree rather than at runtime.

I can confirm that the adi,range-double property is already present in the adi,ad3530r.yaml
file as a boolean type, and consistent with its usage in adi,ad7923.yaml.

> 
> Ideally, we would ask the applications engineer for this chip to
> find out how real users would want to use this feature to make sure
> setting it in the devicetree aligns with that and is not too
> restrictive.

The apps engineer for this chip indicated that the boolean type format aligns better with
how users are expected to configure this feature, especially for users who may not be
familiar with specific range values or enum specifiers.