Re: [PATCH v3 1/3] iio: frequency: adf4360: Add support for ADF4360 PLLs

Andy Shevchenko
2020-02-03T14:11:06Z

On Mon, Feb 3, 2020 at 1:47 PM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
> On Sun, 2020-02-02 at 14:45 +0000, Jonathan Cameron wrote:
> > On Tue, 28 Jan 2020 13:13:00 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

Just few comments on the code in case either this will go, or to teach
an author about best practices in LK.

> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/util_macros.h>

...

> > > +enum {
> > > + ADF4360_CTRL,
> > > + ADF4360_RDIV,
> > > + ADF4360_NDIV,

> > > + ADF4360_REG_NUM,

Sounds line no need for comma (is it indeed a terminator line?).

> > > +};

...

> > > +static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
> > > + unsigned int val)
> > > +{
> > > + int ret;
> > > +
> > > + val |= reg;

This is dangerous. Shouldn't be some mask applied?

> > > + st->spi_data[0] = (val >> 16) & 0xff;
> > > + st->spi_data[1] = (val >> 8) & 0xff;
> > > + st->spi_data[2] = val & 0xff;

All ' & 0xff' are redundant.

> > > + ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
> > > + if (ret == 0)
> > > + st->regs[reg] = val;
> > > +
> > > + return ret;

Please, use pattern:
if (ret)
return ret;

...
return 0;

> > > +}

...

> > > +static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
> > > + unsigned int n,
> > > + unsigned int *out_p,
> > > + unsigned int *out_a,
> > > + unsigned int *out_b)
> > > +{
> > > + unsigned int rate = pfd_freq * n;
> > > + unsigned int p, a, b;
> > > +
> > > + /* Make sure divider counter input frequency is low enough */
> > > + p = 8;
> > > + while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
> > > + p *= 2;
> > > +
> > > + /*
> > > + * The range of dividers that can be produced using the dual-modulus
> > > + * pre-scaler is not continuous for values of n < p*(p-1). If we end up
> > > + * with a non supported divider value, pick the next closest one.
> > > + */
> > > + a = n % p;
> > > + b = n / p;

You may avoid divisions if you use shifts.
Currently it's a bit hard to compiler to prove that p is power of 2.

> > > + if (b < 3) {
> > > + b = 3;
> > > + a = 0;
> > > + } else if (a > b) {

Does this guarantee p >= a?

> > > + if (a - b < p - a) {
> > > + a = b;
> > > + } else {
> > > + a = 0;
> > > + b++;
> > > + }
> > > + }

I guess above conditional tree can be a bit improved, although I don't
see right now in what way.

> > > + return p * b + a;
> > > +}

...

> > > + /* ADF4360-0 to AD4370-7 have an optional by two divider */
> > > + if (st->part_id <= ID_ADF4360_7) {
> > > + if (rate < st->vco_min / 2)
> > > + return st->vco_min / 2;
> > > + if (rate < st->vco_min && rate > st->vco_max / 2) {
> > > + if (st->vco_min - rate < rate - st->vco_max / 2)
> > > + return st->vco_min;

> > > + else

Redundant.

> > > + return st->vco_max / 2;
> > > + }

> > > + } else {
> > > + if (rate < st->vco_min)

} else if (...) {

> > > + return st->vco_min;
> > > + }

...

> > > + case ADF4360_POWER_DOWN_REGULATOR:
> > > + if (!st->vdd_reg)
> > > + return -ENODEV;
> > > +
> > > + if (st->chip_en_gpio)
> > > + ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > > + else
> > > + ret = __adf4360_power_down(st,
> > > + ADF4360_POWER_DOWN_SOFT_ASYNC);

Missed error check.

> > > +
> > > + ret = regulator_disable(st->vdd_reg);
> > > + if (ret)
> > > + dev_err(dev, "Supply disable error: %d\n", ret);
> > > + break;
> > > + }

...

> > > + if (ret == 0)
> > > + st->power_down_mode = mode;
> > > +
> > > + return 0;

Looks like 'return ret;' but see one comment at the beginning of the letter.

...

> > > +static ssize_t adf4360_read(struct iio_dev *indio_dev,
> > > + uintptr_t private,
> > > + const struct iio_chan_spec *chan,
> > > + char *buf)
> > > +{
> > > + struct adf4360_state *st = iio_priv(indio_dev);
> > > + unsigned long val;

> > > + int ret = 0;

Redundant variable.

> > > +
> > > + switch ((u32)private) {
> > > + case ADF4360_FREQ_REFIN:
> > > + val = st->clkin_freq;
> > > + break;
> > > + case ADF4360_MTLD:
> > > + val = st->mtld;
> > > + break;
> > > + case ADF4360_FREQ_PFD:
> > > + val = st->pfd_freq;
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + val = 0;
> > > + }
> > > +
> > > + return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
> > > +}

...

> > > +static ssize_t adf4360_write(struct iio_dev *indio_dev,
> > > + uintptr_t private,
> > > + const struct iio_chan_spec *chan,
> > > + const char *buf, size_t len)
> > > +{
> > > + struct adf4360_state *st = iio_priv(indio_dev);
> > > + unsigned long readin, tmp;
> > > + bool mtld;
> > > + int ret = 0;
> > > +
> > > + mutex_lock(&st->lock);
> > > + switch ((u32)private) {

Strange casting. Why?

> > > + if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {

Too many parentheses.

> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + if (st->clkin) {
> > > + tmp = clk_round_rate(st->clkin, readin);
> > > + if (tmp != readin) {
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + ret = clk_set_rate(st->clkin, tmp);
> > > + if (ret)
> > > + break;
> > A bit odd to directly provide an interface to control and entirely different
> > bit of hardware. If there are specific demands on the input clock as a
> > result
> > of something to do with the outputs, then fair enough. Direct tweaking like
> > this seems like a very odd interface.
> >
> > > + }
> > > +
> > > + st->clkin_freq = readin;
> > > + break;

> > > + case ADF4360_FREQ_PFD:
> > > + ret = kstrtoul(buf, 10, &readin);
> > > + if (ret)
> > > + break;
> > > +

> > > + if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {

Ditto.

> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + st->pfd_freq = readin;
> > > + break;
> > > + default:
> > > + ret = -EINVAL;

Nevertheless 'break;' is good to have even here.

> > > + }

> > > +
> > > + if (ret == 0)

Maybe this, or maybe

if (ret)
goto out_unlock;

> > > + ret = adf4360_set_freq(st, st->freq_req);
> > > + mutex_unlock(&st->lock);
> > > +
> > > + return ret ? ret : len;
> > > +}

...

> > > + int ret = 0;

Redundant assignment.

> > > + mutex_lock(&st->lock);
> > > + writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > > + writeval |= ADF4360_MUXOUT(mode & 0x7);
> > > + ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);

> > > + if (ret == 0)
> > > + st->muxout_mode = mode & 0x7;
> > > + mutex_unlock(&st->lock);

Similar comment to the return check style as above.

> > > + return ret;
> > > +}

...

> > > +static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {

> > > + IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
> > > + { },

No need to have comma for terminator line.

> > > +};

...

> > > + struct adf4360_state *st = iio_priv(indio_dev);
> > > + bool lk_det;
> > > +
> > > + switch (mask) {
> > > + case IIO_CHAN_INFO_FREQUENCY:
> > > + if (adf4360_is_powerdown(st))
> > > + return -EBUSY;
> > > +
> > > + lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
> > > + st->muxout_mode;
> > > + if (lk_det && st->muxout_gpio) {
> > > + if (!gpiod_get_value(st->muxout_gpio)) {
> > > + dev_dbg(&st->spi->dev, "PLL un-locked\n");
> > > + return -EBUSY;
> > > + }
> > > + }
> > > +
> > > + *val = st->freq_req;

> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }

> > > +
> > > + return 0;

How this is possible?

...

> > > + default:
> > > + ret = -EINVAL;

break;

...

> > > + struct adf4360_state *st = iio_priv(indio_dev);

> > > + int ret = 0;

Would be better to have this assignment...

> > > +
> > > + if (reg >= ADF4360_REG_NUM)
> > > + return -EFAULT;
> > > +
> > > + mutex_lock(&st->lock);
> > > + if (readval) {
> > > + *readval = st->regs[reg];

...here.

> > > + } else {
> > > + writeval &= 0xFFFFFC;

What this magic does? Make a define using GENMASK().

> > > + ret = adf4360_write_reg(st, reg, writeval);
> > > + }
> > > + mutex_unlock(&st->lock);
> > > +
> > > + return ret;
> > > +}

...

> > > + /* ADF4360 PLLs are write only devices, try to probe using GPIO. */
> > > + for (i = 0; i < 4; i++) {
> > > + if (i & 1)
> > > + val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
> > > + else
> > > + val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
> > > +
> > > + ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = gpiod_get_value(st->muxout_gpio);

> > > + if (ret ^ (i & 1)) {

I guess == or != is better than ^ here.

> > > + dev_err(dev, "Probe failed (muxout)");
> > > + return -ENODEV;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}

...

> > Hmm. This makes me wonder why this is an IIO driver rather than a clk
> > driver? Definitely needs some more information in the patch description
> > or a cover letter.

+1.

...

> > > + ret = device_property_read_string(dev, "clock-output-names",
> > > + &st->clk_out_name);

Your driver is OF dependent, why to bother with device property API?

> > > + if ((ret < 0) && dev->of_node)

Oh, this is bad. Why do you need to check for OF node at all?!

> > > + st->clk_out_name = dev->of_node->name;

...

> > > + /*
> > > + * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
> > > + * range using an external inductor. These properties describe
> > > + * the range selected by the external inductor.
> > > + */
> > > + ret = device_property_read_u32(dev,
> > > + "adi,vco-minimum-frequency-hz",
> > > + &tmp);
> > > + if (ret == 0)
> > > + st->vco_min = max(st->info->vco_min, tmp);
> > > + else
> > > + st->vco_min = st->info->vco_min;

Why to use tmp at all?

&st->vco_min);
if (ret)
st->vco_min = st->info->vco_min;

st->vco_min = max(st->info->vco_min, st->vco_min);

> > > + ret = device_property_read_u32(dev,
> > > + "adi,vco-maximum-frequency-hz",
> > > + &tmp);
> > > + if (ret == 0)
> > > + st->vco_max = min(st->info->vco_max, tmp);
> > > + else
> > > + st->vco_max = st->info->vco_max;

Ditto.

> > > + ret = device_property_read_u32(dev,
> > > + "adi,loop-filter-pfd-frequency-hz",
> > > + &tmp);
> > > + if (ret == 0) {
> > > + st->pfd_freq = tmp;

Ditto!

> > > + } else {
> > > + dev_err(dev, "PFD frequency property missing\n");
> > > + return ret;
> > > + }

> > > +
> > > + ret = device_property_read_u32(dev,
> > > + "adi,loop-filter-charge-pump-current-microamp",
> > > + &tmp);
> > > + if (ret == 0) {
> > > + st->cpi = find_closest(tmp, adf4360_cpi_modes,
> > > + ARRAY_SIZE(adf4360_cpi_modes));

Ditto!

> > > + } else {
> > > + dev_err(dev, "CPI property missing\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
> > > + if (ret == 0)
> > > + st->freq_req = tmp;
> > > + else
> > > + st->freq_req = st->vco_min;

Ditto.

> > > + ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
> > > + &tmp);
> > > + if (ret == 0)
> > > + st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
> > > + ARRAY_SIZE(adf4360_power_lvl_microamp));
> > > + else
> > > + st->power_level = ADF4360_PL_5;

Ditto.

...

> > > + if (spi->dev.of_node)
> > > + indio_dev->name = spi->dev.of_node->name;

Use %pOFn instead

> > > + else
> > > + indio_dev->name = spi_get_device_id(spi)->name;

...

> > > +static const struct of_device_id adf4360_of_match[] = {
> > > + { .compatible = "adi,adf4360-0", },
> > > + { .compatible = "adi,adf4360-1", },
> > > + { .compatible = "adi,adf4360-2", },
> > > + { .compatible = "adi,adf4360-3", },
> > > + { .compatible = "adi,adf4360-4", },
> > > + { .compatible = "adi,adf4360-5", },
> > > + { .compatible = "adi,adf4360-6", },
> > > + { .compatible = "adi,adf4360-7", },
> > > + { .compatible = "adi,adf4360-8", },
> > > + { .compatible = "adi,adf4360-9", },

> > > + {},

No comma here.

> > > +};

--
With Best Regards,
Andy Shevchenko