| 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: <fd68cda2-f523-49fd-943b-c07dbb461799@baylibre.com>
Date: Mon, 2 Sep 2024 18:04:51 +0200
From: Angelo Dureghello <adureghello@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Olivier Moysan <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
dlechner@...libre.com
Subject: Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features
On 31/08/24 1:34 PM, Jonathan Cameron wrote:
> On Thu, 29 Aug 2024 14:32:01 +0200
> Angelo Dureghello <adureghello@...libre.com> wrote:
>
>> From: Angelo Dureghello <adureghello@...libre.com>
>>
>> Extend DAC backend with new features required for the AXI driver
>> version for the a3552r DAC.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> Hi Angelo
> Minor comments inline.
>>
>> static int axi_dac_enable(struct iio_backend *back)
>> @@ -460,7 +493,13 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
>> case IIO_BACKEND_EXTERNAL:
>> return regmap_update_bits(st->regmap,
>> AXI_DAC_REG_CHAN_CNTRL_7(chan),
>> - AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
>> + AXI_DAC_DATA_SEL,
>> + AXI_DAC_DATA_DMA);
> Unrelated change. If you want to change this, separate patch.
Thanks, fixed.
>
>> + case IIO_BACKEND_INTERNAL_RAMP_16:
>> + return regmap_update_bits(st->regmap,
>> + AXI_DAC_REG_CHAN_CNTRL_7(chan),
>> + AXI_DAC_DATA_SEL,
>> + AXI_DAC_DATA_INTERNAL_RAMP_16);
>> default:
>> return -EINVAL;
>> }
>> @@ -518,9 +557,204 @@ static int axi_dac_reg_access(struct iio_backend *back, unsigned int reg,
>> return regmap_write(st->regmap, reg, writeval);
>> }
>>
>> +
>> +static int axi_dac_bus_reg_write(struct iio_backend *back,
>> + u32 reg, void *val, size_t size)
> Maybe just pass an unsigned int for val?
> So follow what regmap does? You will still need the size, but it
> will just be configuration related rather than affecting the type
> of val.
>
void * was used since data size in the future may vary depending
on the bus physical interface.
Actually, a reg bus write involves several AXI regmap operations.
>
>> +{
>> + struct axi_dac_state *st = iio_backend_get_priv(back);
>> +
>> + if (!st->bus_type)
>> + return -EOPNOTSUPP;
>> +
>> + if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {
> As below, I'd use a switch and factor out this block as a separate
> bus specific function.
Ok, changed.
>
>> + int ret;
>> + u32 ival;
>> +
>> + if (size != 1 && size != 2)
>> + return -EINVAL;
>> +
>> + switch (size) {
>> + case 1:
>> + ival = FIELD_PREP(AXI_DAC_DATA_WR_8, *(u8 *)val);
>> + break;
>> + case 2:
>> + ival = FIELD_PREP(AXI_DAC_DATA_WR_16, *(u16 *)val);
>> + break;
>> + default:
>> + return -EINVAL;
> Hopefully compiler won't need this and the above. I'd drop the size != 1..
> check in favour of just doing it in this switch.
>
sure, done.
>> + }
>> +
>> + 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 to the current transfer.
>> + * DDR state handled separately by specific backend calls,
>> + * generally all raw register writes are SDR.
>> + */
>> + if (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);
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int axi_dac_bus_reg_read(struct iio_backend *back,
>> + u32 reg, void *val, size_t size)
> As for write, I'd just use an unsigned int * for val like
> regmap does.
Ok, so initial choice was unsigned int, further thinking of
possible future busses drive the choice to void *.
Let me know, i can switch to unsigned int in case.
>
>
>> +{
>> + struct axi_dac_state *st = iio_backend_get_priv(back);
>> +
>> + if (!st->bus_type)
>> + return -EOPNOTSUPP;
>> +
>> + if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {
> It got mentioned in binding review but if this isn't QSPI, even
> if similar don't call it that.
It's a bit difficult to find a different name, physically,
it is a QSPI, 4 lanes + clock + cs, and datasheet is naming it Quad SPI.
But looking the data protocol, it's a bit different.
QSPI has instruction, address and data.
Here we have just ADDR and DATA.
What about ADI_QSPI ?
> Maybe use a switch from the start give it will make sense
> anyway the moment there is a second bus type.
ok, used a switch in the write too.
> I'd be tempted to factor the rest of this block out.
> I guess expectation is we'll see more bus types so that factoring
> out will be needed soon anyway.
>
>
>> + int ret;
>> + u32 bval;
> u32 bval = 0;
>> +
>> + if (size != 1 && size != 2)
>> + return -EINVAL;
>> +
>> + bval = 0;
>> + ret = axi_dac_bus_reg_write(back,
>> + AXI_DAC_RD_ADDR(reg), &bval, size);
> Ugly wrap. Move more stuff on to first line.
ok done.
>
>> + 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);
>> + }
>> +
>> + return -EINVAL;
>> +}
Thanks,
regards,
Angelo
--
,,, Angelo Dureghello
:: :. BayLibre -runtime team- Developer
:`___:
`____:
Powered by blists - more mailing lists