On 2022-12-20 18:09, 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) > - 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. > - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words? > I also had the impression that this method of getting and incrementing a pointer to the beginning of the parameter block is a bit more error-prone than necessary. Since parameter blocks are now standardized, wouldn't be easier and safer to wrap the access logic into a helper function like: u16 dfh_get_param_data(struct dfl_device *dfl_dev, u16 param_id, u64 *data) that directly provides a copy of the parameter's data into a pointer provided by the caller and returns the parameter version or an error if not found? Thanks, Marco
