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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 24 Jun 2026 09:18:00 -0500
David Lechner <dlechner@baylibre.com> wrote:

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

If this one is write only (based on another comment - I haven't read the code
yet), then I think we might need to reset anyway so that we can bring state
in sync and allow readback of the current channel values.

> 
> > 
> > Best regards
> > Lukas  
>