On Mon, Feb 3, 2020 at 1:47 PM Ardelean, Alexandru
<alexandru.Ardelean@xxxxxxxxxx> 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@xxxxxxxxxx> 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
