Re: [PATCH 10/10] iio: dac: support the ad9739a RF DAC

[Jonathan Cameron <jic23@kernel.org>]

> > > +static int ad9739a_oper_mode_set(struct iio_dev *indio_dev,
> > > +				 const struct iio_chan_spec *chan, u32 mode)
> > > +{
> > > +	struct ad9739a_state *st = iio_priv(indio_dev);
> > > +
> > > +	if (mode == AD9739A_MIXED_MODE - 1)
> > > +		mode = AD9739A_MIXED_MODE;  
> > 
> > Why?  Feels like a comment is needed. Or a more obvious conversion function.
> >   
> 
> To match what we want to write in the register... With just two values it's too
> simple that opt not to have any helper function or table. Would you be fine with a
> comment?

yes

> 
> > > +
> > > +	return regmap_update_bits(st->regmap, AD9739A_REG_DEC_CNT,
> > > +				  AD9739A_DAC_DEC, mode);
> > > +}
> > > +
> > > +static int ad9739a_read_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan,
> > > +			    int *val, int *val2, long mask)
> > > +{
> > > +	struct ad9739a_state *st = iio_priv(indio_dev);
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > > +		*val = st->sample_rate;
> > > +		*val2 = 0;
> > > +		return IIO_VAL_INT_64;  
> > 
> > Big numbers :)  
> 
> My setup is using 2.5Ghz which is big enough to overflow INT but would work on UINT.

I like big numbers so it's fine doing this. Just unusual to force
val2 to 0 so it made me look closer and appreciate just how big these were getting ;)
> > > +	if (id != AD9739A_ID)
> > > +		return dev_err_probe(dev, -ENODEV, "Unrecognized CHIP_ID 0x%X",
> > > +				     id);  
> > Do we have to give up here?  Could it be a compatible future part?
> > If so we should fallback on what firmware told us it was + perhaps a
> > dev_info() to say we don't recognise the ID register value.
> >   
> 
> I typically prefer to really give up in these cases but no strong opinion... Can turn
> this into a dev_warn()...

DT maintainers generally advise carrying on and trusting the firmware.
I used to agree with you that paranoia was good but I can see there point
and we do have cases where this happened in real parts.

Jonathan

> 
> - Nuno Sá
>