| 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: <b201064d6ab1f85709d99042bf4c77ac90d58a73.camel@gmail.com>
Date: Fri, 20 Sep 2024 15:10:55 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Angelo Dureghello <adureghello@...libre.com>, Lars-Peter Clausen
<lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, Nuno
Sa <nuno.sa@...log.com>, Jonathan Cameron <jic23@...nel.org>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, dlechner@...libre.com
Subject: Re: [PATCH v3 06/10] iio: backend: adi-axi-dac: extend features
On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@...libre.com>
>
> Extend AXI-DAC backend with new features required to interface
> to the ad3552r DAC. Mainly, a new compatible string is added to
> support the ad3552r-axi DAC IP, very similar to the generic DAC
> IP but with some customizations to work with the ad3552r.
>
> Then, a serie of generic functions has been added to match with
> ad3552r needs. Function names has been kept generic as much as
> possible, to allow re-utilization from other frontend drivers.
>
> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> ---
The commit title is not ok... iio: dac: adi-axi-dac: ...
> drivers/iio/dac/adi-axi-dac.c | 274 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 265 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index b8b4171b8043..3ca3a14c575b 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -44,11 +44,34 @@
> #define AXI_DAC_RSTN_MMCM_RSTN BIT(1)
> #define AXI_DAC_RSTN_RSTN BIT(0)
> #define AXI_DAC_REG_CNTRL_1 0x0044
> +#define AXI_DAC_EXT_SYNC_ARM BIT(1)
> +#define AXI_DAC_EXT_SYNC_DISARM BIT(2)
> #define AXI_DAC_SYNC BIT(0)
> #define AXI_DAC_REG_CNTRL_2 0x0048
> +#define AXI_DAC_SDR_DDR_N BIT(16)
> +#define AXI_DAC_SYMB_8B BIT(14)
> #define ADI_DAC_R1_MODE BIT(5)
> +#define AXI_DAC_UNSIGNED_DATA BIT(4)
> +#define AXI_DAC_REG_STATUS_1 0x54
> +#define AXI_DAC_REG_STATUS_2 0x58
> #define AXI_DAC_DRP_STATUS 0x0074
> #define AXI_DAC_DRP_LOCKED BIT(17)
> +#define AXI_DAC_CNTRL_DATA_RD 0x0080
> +#define AXI_DAC_DATA_RD_8 GENMASK(7, 0)
> +#define AXI_DAC_DATA_RD_16 GENMASK(15, 0)
> +#define AXI_DAC_CNTRL_DATA_WR 0x0084
> +#define AXI_DAC_DATA_WR_8 GENMASK(23, 16)
> +#define AXI_DAC_DATA_WR_16 GENMASK(23, 8)
> +#define AXI_DAC_UI_STATUS 0x0088
> +#define AXI_DAC_BUSY BIT(4)
> +#define AXI_DAC_REG_CUSTOM_CTRL 0x008C
> +#define AXI_DAC_ADDRESS GENMASK(31, 24)
> +#define AXI_DAC_SYNCED_TRANSFER BIT(2)
> +#define AXI_DAC_STREAM BIT(1)
> +#define AXI_DAC_TRANSFER_DATA BIT(0)
> +
> +#define AXI_DAC_STREAM_ENABLE (AXI_DAC_TRANSFER_DATA | AXI_DAC_STREAM)
> +
> /* DAC Channel controls */
> #define AXI_DAC_REG_CHAN_CNTRL_1(c) (0x0400 + (c) * 0x40)
> #define AXI_DAC_REG_CHAN_CNTRL_3(c) (0x0408 + (c) * 0x40)
> @@ -62,11 +85,25 @@
> #define AXI_DAC_REG_CHAN_CNTRL_7(c) (0x0418 + (c) * 0x40)
> #define AXI_DAC_DATA_SEL GENMASK(3, 0)
>
> +#define AXI_DAC_RD_ADDR(x) (BIT(7) | (x))
> +
> /* 360 degrees in rad */
> #define AXI_DAC_2_PI_MEGA 6283190
> +
> enum {
> AXI_DAC_DATA_INTERNAL_TONE,
> AXI_DAC_DATA_DMA = 2,
> + AXI_DAC_DATA_INTERNAL_RAMP_16BIT = 11,
> +};
> +
> +enum {
> + AXI_DAC_BUS_TYPE_NONE,
> + AXI_DAC_BUS_TYPE_DDR_QSPI,
> +};
> +
> +struct axi_dac_info {
> + unsigned int version;
> + int bus_type;
Remove the bus_type... For now, let's just assume it's this QSPI implementation.
Let's worry with different buses when we actually have them. For now, let's implement
this one but only for the new compatible. Maybe have a boolean in here like
`bus_controller`. You'll also need a backend info structure per dac_info structure.
More on that below...
Also it makes sense to have a const char *name variable. If it has a different
compatible, we should name it accordingly.
> };
>
> struct axi_dac_state {
> @@ -77,6 +114,7 @@ struct axi_dac_state {
> * data/variables.
> */
> struct mutex lock;
> + const struct axi_dac_info *info;
> u64 dac_clk;
> u32 reg_config;
> bool int_tone;
> @@ -461,6 +499,11 @@ static int axi_dac_data_source_set(struct iio_backend *back,
> unsigned int chan,
> return regmap_update_bits(st->regmap,
> AXI_DAC_REG_CHAN_CNTRL_7(chan),
> AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
> + case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> + return regmap_update_bits(st->regmap,
> + AXI_DAC_REG_CHAN_CNTRL_7(chan),
> + AXI_DAC_DATA_SEL,
> + AXI_DAC_DATA_INTERNAL_RAMP_16BIT);
> default:
> return -EINVAL;
> }
> @@ -518,9 +561,206 @@ static int axi_dac_reg_access(struct iio_backend *back,
> unsigned int reg,
> return regmap_write(st->regmap, reg, writeval);
> }
>
> +static int axi_dac_ext_sync_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
> + AXI_DAC_EXT_SYNC_ARM);
> +}
> +
> +static int axi_dac_ext_sync_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_1,
> + AXI_DAC_EXT_SYNC_DISARM);
> +}
> +
> +static int axi_dac_ddr_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SDR_DDR_N);
> +}
> +
> +static int axi_dac_ddr_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SDR_DDR_N);
> +}
> +
> +static int axi_dac_buffer_enable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_STREAM_ENABLE);
> +}
> +
> +static int axi_dac_buffer_disable(struct iio_backend *back)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_STREAM_ENABLE);
> +}
> +
> +static int axi_dac_data_transfer_addr(struct iio_backend *back, u32 address)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + /*
> + * Sample register address, when the DAC is configured, or stream
> + * start address when the FSM is in stream state.
> + */
> + return regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_ADDRESS,
> + FIELD_PREP(AXI_DAC_ADDRESS, address));
> +}
> +
> +static int axi_dac_data_format_set(struct iio_backend *back, unsigned int ch,
> + const struct iio_backend_data_fmt *data)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + if (data->type == IIO_BACKEND_DATA_UNSIGNED)
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_UNSIGNED_DATA);
> +
> + return -EINVAL;
nit: I would prefer error handling. Or assuming we might have additional types in the
future, we can also make it a switch() case...
> +}
> +
> +static int axi_dac_read_raw(struct iio_backend *back,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> + int err;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY: {
> + int clk_in, reg;
> +
> + /*
> + * As from AXI IP documentation,
> + * returning the SCLK depending on the stream mode.
> + */
This comment is very specific to the implementation. Make sure to refer to the actual
ip on it (axi-ad3552r).
> + clk_in = clk_get_rate(clk_get(st->dev, 0));
> +
Nope.. You have several problems in here:
1) clk_get() every time the function get's called. This has leaks;
2) AFAIR, this IP already get's the AXI bus clock to enable it. You'll have to name
your clocks;
3) The clk_get() needs to be done on probe() (use the devm_ variant). And it is only
mandatory for the axi_ad3552r implementation.
4) You're not enabling the clock (you can have it done in one call).
5) You need to return -EOPNOTSUPP or similar in case there's no support for this. We
have a dedicated compatible so we can make use of it (maybe for now the
bus_controller boolean is enough - again, let's assume it's qspi).
6) I don't think the refclk is expected to change so you can likely get on probe().
> + err = regmap_read(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, ®);
> + if (err)
> + return err;
> +
> + if (reg & AXI_DAC_STREAM)
> + *val = clk_in / 2;
> + else
> + *val = clk_in / 8;
> +
> + return IIO_VAL_INT;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg,
> + unsigned int val, size_t data_size)
> +{
Be consistent... either two `u32` or two `unsigned int`
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 ival;
> +
> + if (data_size == 2)
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_16, val);
> + else
> + ival = FIELD_PREP(AXI_DAC_DATA_WR_8, val);
> +
> + ret = regmap_write(st->regmap, AXI_DAC_CNTRL_DATA_WR, ival);
> + if (ret)
> + return ret;
> +
> + /*
> + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
> + * the data size. So keeping data size control here only,
> + * since data size is mandatory for the current transfer.
> + * DDR state handled separately by specific backend calls,
> + * generally all raw register writes are SDR.
> + */
> + if (data_size == 1)
> + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + else
> + ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> + AXI_DAC_SYMB_8B);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_ADDRESS,
> + FIELD_PREP(AXI_DAC_ADDRESS, reg));
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA,
> + AXI_DAC_TRANSFER_DATA);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap,
> + AXI_DAC_REG_CUSTOM_CTRL, ival,
> + ival & AXI_DAC_TRANSFER_DATA,
> + 10, 100 * KILO);
> + if (ret)
> + return ret;
> +
> + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> + AXI_DAC_TRANSFER_DATA);
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg,
> + unsigned int *val, size_t data_size)
> +{
ditto. Not sure if the backend ops are like this. If yes, same comment.
> + struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> + switch (st->info->bus_type) {
> + case AXI_DAC_BUS_TYPE_DDR_QSPI: {
> + int ret;
> + u32 bval;
> +
> + ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0,
> + data_size);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS,
> + bval, bval != AXI_DAC_BUSY,
> + 10, 100);
> + if (ret)
> + return ret;
> +
> + return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val);
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
Both the above look mostly good... Just get rid of the bus_type id :)
> static const struct iio_backend_ops axi_dac_generic_ops = {
> .enable = axi_dac_enable,
> .disable = axi_dac_disable,
> + .read_raw = axi_dac_read_raw,
> .request_buffer = axi_dac_request_buffer,
> .free_buffer = axi_dac_free_buffer,
> .extend_chan_spec = axi_dac_extend_chan,
> @@ -528,6 +768,14 @@ static const struct iio_backend_ops axi_dac_generic_ops = {
> .ext_info_get = axi_dac_ext_info_get,
> .data_source_set = axi_dac_data_source_set,
> .set_sample_rate = axi_dac_set_sample_rate,
> + .ext_sync_enable = axi_dac_ext_sync_enable,
> + .ext_sync_disable = axi_dac_ext_sync_disable,
> + .ddr_enable = axi_dac_ddr_enable,
> + .ddr_disable = axi_dac_ddr_disable,
> + .buffer_enable = axi_dac_buffer_enable,
> + .buffer_disable = axi_dac_buffer_disable,
> + .data_format_set = axi_dac_data_format_set,
> + .data_transfer_addr = axi_dac_data_transfer_addr,
> .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access),
> };
Make sure to define a new struct iio_backend_ops with the ops this design needs and
leave the above untouched.
- Nuno Sá
Powered by blists - more mailing lists