| 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: <20221106183515.6ab0b435@jic23-huawei>
Date: Sun, 6 Nov 2022 18:35:15 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: <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 Fri, 4 Nov 2022 11:28:00 +0200
Antoniu Miclaus <antoniu.miclaus@...log.com> 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>
Mainly because I can't remember the argument, though I clearly accepted it in
the past ;) Why do we have these PLL devices in IIO rather than via the clk framework?
A few comments inline to add to those of the other reviewers - I've tried not
to overlap too much!
> ---
> drivers/iio/frequency/Kconfig | 10 +
> drivers/iio/frequency/Makefile | 1 +
> drivers/iio/frequency/adf4377.c | 1154 +++++++++++++++++++++++++++++++
> 3 files changed, 1165 insertions(+)
> create mode 100644 drivers/iio/frequency/adf4377.c
>
...
> diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c
> new file mode 100644
> index 000000000000..1901dde1003e
> --- /dev/null
> +++ b/drivers/iio/frequency/adf4377.c
> @@ -0,0 +1,1154 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADF4377 driver
> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +#include <linux/gpio/consumer.h>
Why singled out for it's own block?
That would kind of make sense for the iio headers because this is an IIO
driver but not this one.
> +
> +/* ADF4377 REG0000 Map */
Normally we'd somehow encode which register it is
within the field defines. Seems this particular part
doesn't bother with register names. Maybe..
#define ADF4377_0000_SOFT_RESET_R_MSK
etc? Ideally then dropping the comments, perhaps with the
exception of one at the top saying the datasheet doesn't give
any registers names so we have to use the address.
> +#define ADF4377_SOFT_RESET_R_MSK BIT(7)
> +#define ADF4377_LSB_FIRST_R_MSK BIT(6)
> +#define ADF4377_ADDRESS_ASC_R_MSK BIT(5)
> +#define ADF4377_SDO_ACTIVE_R_MSK BIT(4)
> +#define ADF4377_SDO_ACTIVE_MSK BIT(3)
> +#define ADF4377_ADDRESS_ASC_MSK BIT(2)
> +#define ADF4377_LSB_FIRST_MSK BIT(1)
> +#define ADF4377_SOFT_RESET_MSK BIT(0)
> +
> +/* ADF4377 REG0000 Bit Definition */
> +#define ADF4377_SDO_ACTIVE_SPI_3W 0x0
> +#define ADF4377_SDO_ACTIVE_SPI_4W 0x1
These would need the register address prefix as well.
I'd also be tempted to just ignore the docs and call that
bit ADF4377_0000_4_WIRE
That way you won't need to define the values of the field.
Might be able to do similar with some of the others.
> +
> +#define ADF4377_ADDR_ASC_AUTO_DECR 0x0
> +#define ADF4377_ADDR_ASC_AUTO_INCR 0x1
> +
> +#define ADF4377_LSB_FIRST_MSB 0x0
> +#define ADF4377_LSB_FIRST_LSB 0x1
Again, you can probably make this a flag with the 1 value and 0 value
having obvious meaning.
> +
> +#define ADF4377_SOFT_RESET_N_OP 0x0
I'd define the bit as SOFT_RESET_EN then value of 1 and 0 is obvious
without needing defines.
Same might be doable for later fields, but I haven't looked closely.
> +#define ADF4377_SOFT_RESET_EN 0x1
> +
> +/* ADF4377 REG0001 Map */
> +#define ADF4377_SINGLE_INSTR_MSK BIT(7)
> +#define ADF4377_MASTER_RB_CTRL_MSK BIT(5)
> +
> +/* ADF4377 REG0001 Bit Definition */
> +#define ADF4377_SPI_STREAM_EN 0x0
> +#define ADF4377_SPI_STREAM_DIS 0x1
Just call the bit SPI_STREAM_DIS and use FIELD_PREP(ADF4377_00001_SPI_STREAM_DIS, 0) etc.
> +
> +#define ADF4377_RB_SLAVE_REG 0x0
subordinate on datasheet.
> +#define ADF4377_RB_MASTER_REG 0x1
main on datasheet.
> +
...
> +/* ADF4377 REG001A Map */
> +#define ADF4377_PD_ALL_MSK BIT(7)
> +#define ADF4377_PD_RDIV_MSK BIT(6)
> +#define ADF4377_PD_NDIV_MSK BIT(5)
> +#define ADF4377_PD_VCO_MSK BIT(4)
> +#define ADF4377_PD_LD_MSK BIT(3)
> +#define ADF4377_PD_PFDCP_MSK BIT(2)
> +#define ADF4377_PD_CLKOUT1_MSK BIT(1)
> +#define ADF4377_PD_CLKOUT2_MSK BIT(0)
> +
> +/* ADF4377 REG001A Bit Definition */
> +#define ADF4377_PD_ALL_N_OP 0x0
Don't bother with normal operation defines. Also
just define the mask.
#define ADF4377_001A_PD_ALL BIT(7)
FIELD_PREP(ADF4377_001A_PD_ALL, 0 or 1)
is then clear with fewer defines and short lines at call site.
Anyhow, I've picked out a few examples. Similar approaches may
make other parts of the code more readable.
> +#define ADF4377_PD_ALL_PD 0x1
> +
...
> +struct adf4377_state {
> + struct spi_device *spi;
> + enum adf4377_dev_type type;
> + struct regmap *regmap;
> + struct clk *clkin;
> + /* Protect against concurrent accesses to the device and data content */
> + struct mutex lock;
> + struct notifier_block nb;
> + /* Reference Divider */
> + unsigned int ref_div_factor;
> + /* PFD Frequency */
> + unsigned int f_pfd;
> + /* Input Reference Clock */
> + unsigned int clkin_freq;
> + /* CLKOUT Divider */
> + u8 clkout_div_sel;
> + /* Feedback Divider (N) */
> + u16 n_int;
> + u16 synth_lock_timeout;
> + u16 vco_alc_timeout;
> + u16 adc_clk_div;
> + u16 vco_band_div;
> + u8 dclk_div1;
> + u8 dclk_div2;
> + u8 dclk_mode;
> + unsigned int f_div_rclk;
> + struct gpio_desc *gpio_ce;
> + struct gpio_desc *gpio_enclk1;
> + struct gpio_desc *gpio_enclk2;
> + u8 buf[2] ____cacheline_aligned;
__aligned(IIO_DMA_MINALIGN)
Cacheline line alignment isn't enough on few obsure ARM cores where
that reflects the l1 cacheline, but coherency needs handling at
a slower cache level which has longer cachelines.
> +};
> +
> +static int adf4377_set_default(struct adf4377_state *st)
regmap has infrastructure to do this from a table. Use that rather
than hand coding it. If there is a strong reason to do otherwise
then add a comment here.
...
> +
> +int adf4377_set_freq(struct adf4377_state *st, u64 freq)
This is awfully involved. Perhaps a comment or spec reference for the register
write sequence?
> +{
> + unsigned int read_val;
> + u64 f_vco;
> + int ret;
> +
> + ret = regmap_update_bits(st->regmap, 0x1C, ADF4377_EN_DNCLK_MSK | ADF4377_EN_DRCLK_MSK,
> + FIELD_PREP(ADF4377_EN_DNCLK_MSK, ADF4377_EN_DNCLK_ON) |
> + FIELD_PREP(ADF4377_EN_DRCLK_MSK, ADF4377_EN_DRCLK_ON));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, 0x11, ADF4377_EN_AUTOCAL_MSK | ADF4377_DCLK_DIV2_MSK,
> + FIELD_PREP(ADF4377_EN_AUTOCAL_MSK, ADF4377_VCO_CALIB_EN) |
> + FIELD_PREP(ADF4377_DCLK_DIV2_MSK, st->dclk_div2));
> + if (ret)
> + return ret;
> +
... lots more ...
> +
> +static void adf4377_gpio_init(struct adf4377_state *st)
> +{
> + if (st->gpio_ce) {
> + gpiod_set_value(st->gpio_ce, 1);
This was driven high at the gpiod_get_optional() so we are leaving it the same. Maybe makes
sense to drive it low at request and high here so as to force a reset?
> +
> + /* Delay for SPI register bits to settle to their power-on reset state */
> + usleep_range(200, 250);
> + }
> +
> + if (st->gpio_enclk1)
> + gpiod_set_value(st->gpio_enclk1, 1);
So is the assumption here that if we don't have control of these they are all tied to 1?
Perhaps a comment to say that if so...
Is turning these off a power saving thing? If so I'd expect that in the remove() path
probably via a devm_add_action_or_reset() type call.
> +
> + if (st->gpio_enclk2 && st->type == ADF4377)
> + gpiod_set_value(st->gpio_enclk2, 1);
> +}
> +
> +static int adf4377_init(struct adf4377_state *st)
> +{
> + int ret;
> +
> + /* GPIO Inititalization */
> + adf4377_gpio_init(st);
> +
> + /* Software reset */
> + ret = adf4377_soft_reset(st);
If I read the datasheet right, we could have already done a hardware reset.
Normally we'd not bother doing both.
> + if (ret)
> + return ret;
> +
> + /* Set Default Registers */
Name of function rather removes need for the comment. Same for the other two above.
> + ret = adf4377_set_default(st);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, 0x15, ADF4377_CP_I_MSK,
Unlike the above calls, not so obvious what this is doing so a comment would be helpful.
> + FIELD_PREP(ADF4377_CP_I_MSK, ADF4377_CP_10MA1));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, 0x00,
> + ADF4377_SDO_ACTIVE_MSK | ADF4377_SDO_ACTIVE_R_MSK,
> + FIELD_PREP(ADF4377_SDO_ACTIVE_MSK, ADF4377_SDO_ACTIVE_SPI_4W) |
> + FIELD_PREP(ADF4377_SDO_ACTIVE_R_MSK, ADF4377_SDO_ACTIVE_SPI_4W));
> + if (ret)
> + return ret;
> +
> + st->clkin_freq = clk_get_rate(st->clkin);
> +
> + /* Power Up */
> + ret = regmap_write(st->regmap, 0x1a,
> + FIELD_PREP(ADF4377_PD_ALL_MSK, ADF4377_PD_ALL_N_OP) |
> + FIELD_PREP(ADF4377_PD_RDIV_MSK, ADF4377_PD_RDIV_N_OP) |
> + FIELD_PREP(ADF4377_PD_NDIV_MSK, ADF4377_PD_NDIV_N_OP) |
> + FIELD_PREP(ADF4377_PD_VCO_MSK, ADF4377_PD_VCO_N_OP) |
> + FIELD_PREP(ADF4377_PD_LD_MSK, ADF4377_PD_LD_N_OP) |
> + FIELD_PREP(ADF4377_PD_PFDCP_MSK, ADF4377_PD_PFDCP_N_OP) |
> + FIELD_PREP(ADF4377_PD_CLKOUT1_MSK, ADF4377_PD_CLKOUT1_N_OP) |
> + FIELD_PREP(ADF4377_PD_CLKOUT2_MSK, ADF4377_PD_CLKOUT2_N_OP));
> + if (ret)
> + return ret;
> +
> + /* Compute PFD */
> + st->ref_div_factor = 0;
> + do {
> + st->ref_div_factor++;
> + st->f_pfd = st->clkin_freq / st->ref_div_factor;
> + } while (st->f_pfd > ADF4377_MAX_FREQ_PFD);
> +
> + if (st->f_pfd > ADF4377_MAX_FREQ_PFD || st->f_pfd < ADF4377_MIN_FREQ_PFD)
> + return -EINVAL;
> +
> + st->f_div_rclk = st->f_pfd;
> +
> + if (st->f_pfd <= ADF4377_FREQ_PFD_80MHZ) {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_1;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_1;
> + st->dclk_mode = ADF4377_DCLK_MODE_DIS;
> + } else if (st->f_pfd <= ADF4377_FREQ_PFD_125MHZ) {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_1;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_1;
> + st->dclk_mode = ADF4377_DCLK_MODE_EN;
> + } else if (st->f_pfd <= ADF4377_FREQ_PFD_160MHZ) {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_2;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_1;
> + st->dclk_mode = ADF4377_DCLK_MODE_DIS;
> + st->f_div_rclk /= 2;
> + } else if (st->f_pfd <= ADF4377_FREQ_PFD_250MHZ) {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_2;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_1;
> + st->dclk_mode = ADF4377_DCLK_MODE_EN;
> + st->f_div_rclk /= 2;
> + } else if (st->f_pfd <= ADF4377_FREQ_PFD_320MHZ) {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_2;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_2;
> + st->dclk_mode = ADF4377_DCLK_MODE_DIS;
> + st->f_div_rclk /= 4;
> + } else {
> + st->dclk_div1 = ADF4377_DCLK_DIV1_2;
> + st->dclk_div2 = ADF4377_DCLK_DIV2_2;
> + st->dclk_mode = ADF4377_DCLK_MODE_EN;
> + st->f_div_rclk /= 4;
> + }
> +
> + st->synth_lock_timeout = DIV_ROUND_UP(st->f_div_rclk, 50000);
> + st->vco_alc_timeout = DIV_ROUND_UP(st->f_div_rclk, 20000);
> + st->vco_band_div = DIV_ROUND_UP(st->f_div_rclk, 150000 * 16 * (1 << st->dclk_mode));
> + st->adc_clk_div = DIV_ROUND_UP((st->f_div_rclk / 400000 - 2), 4);
> +
> + return 0;
> +}
> +
> +
> +static const struct spi_device_id adf4377_id[] = {
> + { "adf4377", ADF4377 },
> + { "adf4378", ADF4378 },
> + {},
As below.
> +};
> +MODULE_DEVICE_TABLE(spi, adf4377_id);
> +
> +static const struct of_device_id adf4377_of_match[] = {
> + { .compatible = "adi,adf4377" },
> + { .compatible = "adi,adf4378" },
> + {},
No comma on NULL terminators. We shouldn't add anything after
them, so good to make that explicit.
> +};
> +MODULE_DEVICE_TABLE(of, adf4377_of_match);
> +
> +static struct spi_driver adf4377_driver = {
> + .driver = {
> + .name = "adf4377",
> + .of_match_table = adf4377_of_match,
> + },
> + .probe = adf4377_probe,
> + .id_table = adf4377_id,
> +};
> +module_spi_driver(adf4377_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@...log.com>");
> +MODULE_DESCRIPTION("Analog Devices ADF4377");
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists