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

[David Lechner <dlechner@baylibre.com>]

On 4/10/25 3:39 AM, Paller, Kim Seer wrote:
> 
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Tuesday, April 8, 2025 10:06 PM
>> To: Paller, Kim Seer <KimSeer.Paller@analog.com>; Jonathan Cameron
>> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Hennerich,
>> Michael <Michael.Hennerich@analog.com>; Rob Herring <robh@kernel.org>;
>> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
>> <conor+dt@kernel.org>
>> Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devicetree@vger.kernel.org
>> 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.

Sorry, I don't know why I didn't see it the first 2 times I looked. :-(
It is there.

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

Very good.