iio: frequency: New driver for HMC7044 #141

dbogdan · 2018-07-04T14:35:07Z

HMC7044 is a high performance, dual-loop, integer-N jitter attenuator
capable of performing reference selection and generation of ultralow
phase noise frequencies for high speed data converters with either
parallel or serial (JESD204B type) interfaces.

Signed-off-by: Dragos Bogdan [email protected]

lclausen-adi

Looks very good, well done.

One thing you should review though is the locking (or rather the lack of it). There are code paths that can run concurrently that need locking.

lclausen-adi · 2018-07-04T16:13:03Z

drivers/iio/frequency/hmc7044.c

+		div = clamp(div, 1, 4094);
+		/* Supported odd divide ratios are 1, 3, and 5 */
+		if ((div != 1) && (div != 3) && (div != 5))
+			div = div - (div % 2);


We should make sure that still pick the divider that gives the closest result. E.g. if the ratio of pll2 and output frequency is lets say 7.4 then DIV_ROUND_CLOSEST() picks 7. Then we clear the LSB and end up with 6. But in this case 8 would have been the better choice.

I think DIV_ROUND_CLOSEST(hmc->pll2_freq, val * 2) * 2 will produce the correct result.
Maybe there is a better way.

lclausen-adi · 2018-07-04T16:19:54Z

drivers/iio/frequency/hmc7044.c

+
+#define HMC7044_LOW_VCO_MIN	2150000
+#define HMC7044_LOW_VCO_MAX	2880000
+#define HMC7044_HIGH_VCO_MIN	2400000


datasheet has 2650 here.

lclausen-adi · 2018-07-04T16:21:42Z

drivers/iio/frequency/hmc7044.c

+	int div;
+
+	div = DIV_ROUND_CLOSEST(hmc->pll2_freq, rate);
+	div = clamp(div, 1, 4094);


We should avoid the duplicated code here and in write_raw(). Best is to factor it out into a helper function.

lclausen-adi · 2018-07-04T16:25:35Z

drivers/iio/frequency/hmc7044.c

+	hmc->clk_data.clks = hmc->clks;
+	hmc->clk_data.clk_num = HMC7044_NUM_CHAN;
+
+	ret = of_clk_add_provider(hmc->spi->dev.of_node,


Just return of_clk...

lclausen-adi · 2018-07-04T16:27:27Z

drivers/iio/frequency/hmc7044.c

+	}
+
+	return -EINVAL;
+


Extra newline

lclausen-adi · 2018-07-04T16:28:36Z

drivers/iio/frequency/hmc7044.c

+	int i;
+
+	for (i = 0; i < hmc->num_channels; i++) {
+		if (hmc->channels[i].num == chan->channel)


can we just store the index in the address field of the channel instead of having to loop over the array?

lclausen-adi · 2018-07-04T16:32:14Z

drivers/iio/frequency/hmc7044.c

+		hmc->iio_channels[i].info_mask_separate =
+			BIT(IIO_CHAN_INFO_FREQUENCY);
+
+		ret = hmc7044_clk_register(indio_dev, chan->num);


We should only register the clocks after all registers have written and the sync has been performed. To avoid race conditions.

lclausen-adi · 2018-07-04T16:32:52Z

drivers/iio/frequency/hmc7044.c

+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);


return iio_device_register()

devm_iio_device_register() could be used as well

or hmm ; is the order of unregister important here in the hmc7044_remove() ?
in that case it may be fine to leave this as is;

also, why not use devm_iio_device_register() ?

I think that the un-registration order is important... I might be wrong.

it is important

lclausen-adi · 2018-07-04T16:35:09Z

Documentation/devicetree/bindings/iio/frequency/hmc7044.txt

+		of the master timer, which controls synchronization and
+		pulse generator events. It should be set to a submultiple
+		of the lowest output SYSREF frequency, no faster than 4
+		MHz. If this is, 1024 will be used by default.


If this is what? ;)

commodo · 2018-07-05T06:40:18Z

drivers/iio/frequency/hmc7044.c

+	buf[1] = cmd & 0xFF;
+	buf[2] = val;
+
+	ret = spi_write(hmc->spi, buf, 3);


you could return directly here with return spi_write(hmc->spi, buf, ARRAY_SIZE(buf));

stefpopa · 2018-07-05T07:45:48Z

Documentation/devicetree/bindings/iio/frequency/hmc7044.txt

@@ -0,0 +1,111 @@
+Analog Devices HMC7044 device driver


Bindings should be in a different commit

Fixed this.

stefpopa

Looks very good overall

stefpopa · 2018-07-05T07:57:15Z

drivers/iio/frequency/hmc7044.c

+ * Copyright 2018 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */


You should use the SPDX-License-Identifer at the top following Documentation/process/license-rules.rst
Something like this: // SPDX-License-Identifier: GPL-2.0

stefpopa · 2018-07-05T08:01:46Z

drivers/iio/frequency/hmc7044.c

+	u32			clkin_freq[4];
+	u32			vcxo_freq;
+	u32			pll2_freq;
+	unsigned int		pll1_loop_bw;


Nitpick: you are mixing u32 and unsigned int;

I've decided to do this to avoid some extra casts - of of_property_read_u32() returns an u32 while the clock framework uses "unsigned long" types.

stefpopa · 2018-07-05T08:03:12Z

drivers/iio/frequency/hmc7044.c

+
+static int hmc7044_write(struct iio_dev *indio_dev,
+			 unsigned int reg,
+			 unsigned int  val)


nitpick: you have a double space here

stefpopa · 2018-07-05T08:08:11Z

drivers/iio/frequency/hmc7044.c

+	else
+		high_vco_en = false;
+
+


extra line here

stefpopa · 2018-07-05T08:14:12Z

drivers/iio/frequency/hmc7044.c

+	return 0;
+}
+
+static int hmc7044_setup(struct iio_dev *indio_dev)


I am thinking if it wouldn't be better to split this function into smaller pieces. It took a long time to scroll to the end of it :)

Since no part of this function is called somewhere else, I wouldn't split it. However, if the majority will want this, I will do it. :)

stefpopa · 2018-07-05T08:22:10Z

drivers/iio/frequency/hmc7044.c

+
+	hmc->pll1_loop_bw = 200;
+	of_property_read_u32(np, "adi,pll1-loop-bandwidth-hz",
+			     &hmc->pll1_loop_bw);


Another way to deal with optional properties would be something like this:
ret = of_property_read_...();
if (ret) {
...
} else {
...
}

stefpopa · 2018-07-05T08:25:45Z

drivers/iio/frequency/hmc7044.c

+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);


also, why not use devm_iio_device_register() ?

mirceacaprioru · 2018-07-05T08:48:52Z

drivers/iio/frequency/hmc7044.c

+ */
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>


Could you sort this includes to have a logic order:
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/device.h>

dbogdan · 2018-07-05T13:02:15Z

v2:

Add mutex locking;
Calculation of output dividers was revised;
Fix HMC7044_HIGH_VCO_MIN value;
Store the iio_channel index to address field of the iio_chan_spec;
Register the clocks after the part was initialized;
Remove unnecessary includes;
Remove extra spaces;
Split the commit in two: a dedicated one for the documentation.

stefpopa

LGTM

lclausen-adi · 2018-07-05T13:25:14Z

drivers/iio/frequency/hmc7044.c

+
+	div = DIV_ROUND_CLOSEST(parent_rate, rate);
+	/* Supported odd divide ratios are 1, 3, and 5 */
+	if ((div != 1) && (div != 3) && (div != 5)) {


we should only re-calc if the div is uneven.

lclausen-adi · 2018-07-05T13:27:21Z

drivers/iio/frequency/hmc7044.c

+	char name[SPI_NAME_SIZE + 8];
+
+	sprintf(name, "%s_out%d", indio_dev->name, num);
+


Same comment as before. Should read clock-output-names from devicetree.

This is a good remarked - fixed this.

lclausen-adi · 2018-07-05T13:29:02Z

drivers/iio/frequency/hmc7044.c

+#define HMC7044_OUT_DIV_MIN	1
+#define HMC7044_OUT_DIV_MAX	4094
+
+struct output {


I'd prefer that all the structs have a proper hmc7044_ prefix.

dbogdan · 2018-07-05T16:09:11Z

v3:

add hmc7044_ prefix to output and chan_spec structures;
compute the divider only for the odd numbers;
get the clock-output-names from dts.

commodo · 2018-07-06T07:17:01Z

drivers/iio/frequency/hmc7044.c

+
+	address = to_output(hw)->address;
+	chan = &hmc->iio_channels[address];
+	if (!chan)


nitpick: this check looks redundant; you aren't likely going to get null here;
you could check if address is in range and return -EINVAL if it isn't, but from the looks of the code, it's likely that it's not going to happen

this also applies to other places in the code;

Fixed this. Thanks.

HMC7044 is a high performance, dual-loop, integer-N jitter attenuator capable of performing reference selection and generation of ultralow phase noise frequencies for high speed data converters with either parallel or serial (JESD204B type) interfaces. Signed-off-by: Dragos Bogdan <[email protected]>

commodo

LGTM
Will merge later today if no objections

dbogdan requested review from mhennerich, lclausen-adi, commodo, mirceacaprioru and stefpopa July 4, 2018 14:35

lclausen-adi suggested changes Jul 4, 2018

View reviewed changes

commodo reviewed Jul 5, 2018

View reviewed changes

stefpopa reviewed Jul 5, 2018

View reviewed changes

mirceacaprioru reviewed Jul 5, 2018

View reviewed changes

dbogdan force-pushed the hmc7044 branch 6 times, most recently from 9197e8a to 3e01e56 Compare July 5, 2018 12:45

stefpopa approved these changes Jul 5, 2018

View reviewed changes

mhennerich approved these changes Jul 5, 2018

View reviewed changes

lclausen-adi approved these changes Jul 5, 2018

View reviewed changes

dbogdan force-pushed the hmc7044 branch 2 times, most recently from dbdbf65 to bacf9c4 Compare July 5, 2018 16:06

dbogdan force-pushed the hmc7044 branch from bacf9c4 to 65ec5ab Compare July 5, 2018 20:31

commodo reviewed Jul 6, 2018

View reviewed changes

dbogdan added 2 commits July 6, 2018 10:51

dbogdan force-pushed the hmc7044 branch from 65ec5ab to b04b239 Compare July 6, 2018 07:51

commodo approved these changes Jul 6, 2018

View reviewed changes

commodo merged commit 00cccff into altera_4.9 Jul 6, 2018

commodo deleted the hmc7044 branch July 6, 2018 10:20

		char name[SPI_NAME_SIZE + 8];

		sprintf(name, "%s_out%d", indio_dev->name, num);

iio: frequency: New driver for HMC7044 #141

iio: frequency: New driver for HMC7044 #141

Conversation

dbogdan commented Jul 4, 2018

lclausen-adi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefpopa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbogdan commented Jul 5, 2018

stefpopa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbogdan commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

commodo left a comment

Choose a reason for hiding this comment

stefpopa left a comment •

edited

Loading