Re: [PATCH RFC v2 2/9] iio: frequency: ad9910: initial driver implementation

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Mar 23, 2026 at 10:34:37AM +0000, Rodrigo Alencar wrote:
> On 26/03/22 04:50PM, Jonathan Cameron wrote:
> > On Wed, 18 Mar 2026 17:56:02 +0000
> > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote:

...

> > > +#include <linux/array_size.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > 
> > Generally can avoid including device.h in favour of more specific
> > headers.  There are a few exceptions where we can't such as actual
> > dereferencing of struct device, but I don't recall seeing a case in here.
> 
> I understood that the usage of devm_add_action_or_reset() would justify
> the header.

It's in the device/devres.h.

...

> > > +	st->data.pll_enabled = device_property_read_bool(dev, "adi,pll-enable");
> > > +	if (st->data.pll_enabled) {
> > > +		tmp = AD9910_ICP_MAX_uA;
> > 
> > Defaulting to max current seems unusual.

Agree.

> >  What's the motivation? Normal instinct is go minimum if no other info.
> 
> ICP_MAX_uA leads to 111 in the CFR3_ICP field, which is the default value
> when the device resets or when it powers on. I suppose that if we are not
> touching that property, there would be no reason to change that.

I believe we should think different, id est about potential damages or
current drain. I would expect a minimum or hi-impedance (power off) state
of the related part of the device.

-- 
With Best Regards,
Andy Shevchenko