Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163

[David Lechner]

On 6/24/26 3:30 AM, Lukas wrote:
> Thanks for the review. As i said this is my first time submitting a
> patch. I have looked at already existing spi dac drivers for reference
> but i seemed to have missed quite a lot. But the comments are greatly
> appreciated.
> 
> On Wed, Jun 24, 2026 at 12:56:15AM +0600, Siratul Islam wrote:
>> A link to the datasheet here would be nice.
> 
> I will try to add all the small suggestions i dont mention explicitly,
> like style issues or using guard instead of manual lock/unlock to v2.
> 
>>> +
>>> +	if (st->internal_ref) {
>>> +		st->vref_uv = 2500000; /* 2.5V internal reference */
>> A note on where this value came from or why this was chosen, or a reference to datasheet would be better.
> 
> I think i would add the suggestion from David Lechner to remove the
> internal_ref property completly and add "the way of doing optional
> voltage references". This includes using the macro
> DAC8163_INTERNAL_REF_mV. Would this be acceptable?
> 
>> You have a CMD_SOFT_RST defined but not used. Should this be used to reset before doing any configuration?
> 
> Yes this is a command which isnt used at this point. But maybe it makes
> sense to reset the DAC first when probing.

In general we tend to reset IIO devices during probe. DACs can be an exception
though since they are output devices and resetting it could change the output.
This device is quite simple anyway, so reset probably isn't needed.

> 
> Best regards
> Lukas