On Tue, 20 Dec 2022, Andy Shevchenko wrote:
On Tue, Dec 20, 2022 at 08:36:52AM -0800, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.
In general the code here looks good to me, but one thing to discuss due to
comment to the previous patch(es).
...
+ u64 *p;
+
+ p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
+ if (!p)
+ return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
+
+ p++;
+ uart->port.uartclk = *p;
So, here and the below is using always the second u64 from the returned data.
Does it mean:
- we always skip the first u64 from the returned buffer and hence... (see below)
The first u64 of the parameter block, the parameter header, contains a
version field and a next/size field that a parameter consumer might use.
The version field determines the exact layout of the data, and the
next/size field could/should be used to prevent out of bounds accesses.
- we may actually return the second u64 as a plain number (not a pointer) from
(an additional?) API? In such case we would not need to take care about this
p++; lines here and there.
I think an additional API that can be used to fetch an array of u64's
while also checking boundary conditions would be helpful.
- we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?
The length and layout of the parameter data is determined by the parameter
id and version. So the data portion of a parameter is not fixed length.
Thanks for the feedback,
Matthew Gerlach
--
With Best Regards,
Andy Shevchenko
