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

[Andy Shevchenko <andy@kernel.org>]

On Wed, Apr 23, 2025 at 07:53:37AM +0000, Paller, Kim Seer wrote:
> > From: Andy Shevchenko <andy@kernel.org>
> > Sent: Tuesday, April 22, 2025 11:11 PM
> > On Mon, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller wrote:

First of all, there are a lot of comments left without replies while some of
them commented as "agree I will follow your advice". This is confusing. The
rule of thumb is to not reply with positive at all, just only for the things
that you want to clarify. And with that remove a lot of unneeded (you agree
with) context!

...

> > > +		st->vref_mv = range_multiplier * vref / 1000;
> > 
> > MILLI?
> 
> Yes this is milli, will change this also to vref_mV
> 
> 	st->vref_mV = range_multiplier * vref_mV / 1000;

Ah, I was not clear enough, MILLI in capital letters is defined constant which
you may use instead of 1000.

...

> > > +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> > > +	if (vref < 0 && vref != -ENODEV)
> > > +		return vref;
> > > +
> > > +	has_external_vref = vref != -ENODEV;
> > 
> > Wouldn't be better just make this 0 when it's == -ENODEV and check just the
> > value without having this additional boolean variable (note, I haven't checked
> > the meaning of Vref == 0 in case it's possible in real life and hardware behaves
> > adequately)?
> 
> I think it could be simpler to set vref to 0 when it's -ENODEV and check its value directly
> without having additional boolean variable. I'll try this approach.

But double check that hardware doesn't support Vref == 0 in real life.

-- 
With Best Regards,
Andy Shevchenko