On Sun, 2022-11-06 at 17:51 +0000, Jonathan Cameron wrote: > On Fri, 04 Nov 2022 12:38:07 +0100 > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > On Fri, 2022-11-04 at 11:28 +0200, Antoniu Miclaus wrote: > > > The ADF4377 is a high performance, ultralow jitter, dual output > > > integer-N > > > phased locked loop (PLL) with integrated voltage controlled > > > oscillator > > > (VCO) ideally suited for data converter and mixed signal front > > > end > > > (MxFE) > > > clock applications. > > > > > > Datasheet: > > > https://www.analog.com/media/en/technical-documentation/data-sheets/adf4377.pdf > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> > > Nuno, please crop to the bit you comment on. Doom scrolling isn't > fun ;) > Right, sorry about that :) > A few follow up comments inline. > > Jonathan > > > > --- > > > drivers/iio/frequency/Kconfig | 10 + > > > drivers/iio/frequency/Makefile | 1 + > > > drivers/iio/frequency/adf4377.c | 1154 > > ... > > > +static ssize_t adf4377_read(struct iio_dev *indio_dev, uintptr_t > > > private, > > > + const struct iio_chan_spec *chan, > > > char > > > *buf) > > > +{ > > > + struct adf4377_state *st = iio_priv(indio_dev); > > > + u64 val = 0; > > > + int ret; > > > + > > > + switch ((u32)private) { > > > + case ADF4377_FREQ: > > > + ret = adf4377_get_freq(st, &val); > > > + break; > > > + default: > > > + ret = -EINVAL; > > > + val = 0; > > > + break; > > > + } > > > + > > > + return ret ?: sysfs_emit(buf, "%llu\n", val); > > > > I would also return in place. I've come to prefer it but that's me > > :) > > Definitely if alternative is a ternary! > > > > > + return ret ? : FIELD_GET(ADF4377_MUXOUT_MSK, mode); > > > +} > > > + > > > +static const struct iio_enum adf4377_muxout_enum = { > > > + .items = adf4377_muxout_modes, > > > + .num_items = ARRAY_SIZE(adf4377_muxout_modes), > > > + .get = adf4377_get_muxout_mode, > > > + .set = adf4377_set_muxout_mode, > > > +}; > > > + > > > +#define _ADF4377_EXT_INFO(_name, _shared, _ident) { \ > > > + .name = _name, \ > > > + .read = adf4377_read, \ > > > + .write = adf4377_write, \ > > > + .private = _ident, \ > > > + .shared = _shared, \ > > > + } > > > + > > > +static const struct iio_chan_spec_ext_info adf4377_ext_info[] = > > > { > > > + /* > > > + * Usually we use IIO_CHAN_INFO_FREQUENCY, but there are > > > + * values > 2^32 in order to support the entire frequency > > > range > > > + * in Hz. > > > + */ > > > + _ADF4377_EXT_INFO("frequency", IIO_SHARED_BY_ALL, > > > ADF4377_FREQ), > > > > Can't we have u64 already in IIO_CHAN_INFO_FREQUENCY? I know the > > write > > side is a bit awkward but I think we can make it better. > > hmm. I think we only have s64. If 63 bits is enough then we are good > to go :) > > (in the annals of bad design decisions, thinking years ago that no > one would > go beyond 32 bits... oops). > > > > > > + IIO_ENUM("muxout_select", IIO_SHARED_BY_ALL, > > > &adf4377_muxout_enum), > > > + IIO_ENUM_AVAILABLE("muxout_select", IIO_SHARED_BY_ALL, > > > &adf4377_muxout_enum), > > > + { }, > > > +}; > > > + > > ... > > > > + > > > +static int adf4377_probe(struct spi_device *spi) > > > +{ > > > + struct iio_dev *indio_dev; > > > + struct regmap *regmap; > > > + struct adf4377_state *st; > > > + int ret; > > > + > > > + indio_dev = devm_iio_device_alloc(&spi->dev, > > > sizeof(*st)); > > > + if (!indio_dev) > > > + return -ENOMEM; > > > + > > > + regmap = devm_regmap_init_spi(spi, > > > &adf4377_regmap_config); > > > + if (IS_ERR(regmap)) > > > + return PTR_ERR(regmap); > > > + > > > + st = iio_priv(indio_dev); > > > + > > > + indio_dev->info = &adf4377_info; > > > + indio_dev->name = "adf4377"; > > > + indio_dev->channels = adf4377_channels; > > > + indio_dev->num_channels = ARRAY_SIZE(adf4377_channels); > > > + > > > + st->regmap = regmap; > > > + st->spi = spi; > > > + st->type = spi_get_device_id(spi)->driver_data; > > > > Hmm this is something that came up internally the other day. Are we > > guaranteed that this will always work? For OF I think it is but I'm > > not > > sure about ACPI? At first glance, it seems that it might be ok but > > I > > did not went too deep in the ACPI code. > > Better indeed to not assume it and indeed ACPI can't do this magic, > because > there isn't a match between the actual ACPI ID and the > spi_device_ids. > Not sure what it does with PRP0001 case (where it uses the > of_device_id table). Some comments/questions on the pattern: > > > st->type = device_get_match_id()->driver_data; Are we sure that device_get_match_id() (I guess you meant device_get_match_data()) cannot return NULL? I'm always a bit on the careful side with these dereferences... > if (!st->type) { > const struct spi_device_id *id = > spi_get_device_id(spi); > > if (!id) > return -EINVAL; > > st->type = spi_get_device_id(spi)->driver_data; id->driver_data :) > } - Nuno Sá device_get_match_id()->driver_data;
