| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2a6c721b8c27b4631f424e7c68e11a2409005261.camel@gmail.com>
Date: Tue, 15 Nov 2022 13:36:14 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] iio: frequency: adf4377: add support for ADF4377
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@...il.com> 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@...log.com>
>
> 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;
Powered by blists - more mailing lists