Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Dec 2022 14:37:02 -0800 (PST)
From:   matthew.gerlach@linux.intel.com
To:     Marco Pagani <marpagan@redhat.com>
cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        hao.wu@intel.com, yilun.xu@intel.com, russell.h.weight@intel.com,
        basheer.ahmed.muddebihal@intel.com, trix@redhat.com,
        mdf@kernel.org, linux-fpga@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        tianfei.zhang@intel.com, corbet@lwn.net,
        gregkh@linuxfoundation.org, linux-serial@vger.kernel.org,
        jirislaby@kernel.org, geert+renesas@glider.be,
        niklas.soderlund+renesas@ragnatech.se, macro@orcam.me.uk,
        johan@kernel.org, lukas@wunner.de, ilpo.jarvinen@linux.intel.com,
        bagasdotme@gmail.com
Subject: Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera
 16550.
In-Reply-To: <70c7daf8-379e-1479-aba5-4476c4a93b9f@redhat.com>
Message-ID: <alpine.DEB.2.22.394.2212211434520.570436@rhweight-WRK1>
References: <20221220163652.499831-1-matthew.gerlach@linux.intel.com> <20221220163652.499831-5-matthew.gerlach@linux.intel.com> <Y6HsQJQMDnHgTesF@smile.fi.intel.com> <d34b021c-eec4-905e-f352-734db2d8338a@redhat.com>
 <70c7daf8-379e-1479-aba5-4476c4a93b9f@redhat.com>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Precedence: bulk


On Wed, 21 Dec 2022, Marco Pagani wrote:

>
> On 2022-12-21 18:26, Marco Pagani wrote:
>>
>>
>> On 2022-12-20 18:09, Andy Shevchenko wrote:
>>> On Tue, Dec 20, 2022 at 08:36:52AM -0800, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> 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?
>
> Please ignore the last part of my reply. The diagram in the documentation made
> me think that parameter data are always 64-bit wide. Since the parameter data
> "payload" size depends on the version and ID, an eventual helper function could
> then return a pointer to the data payload and the version number to the caller.

Certainly helper functions should be created to make accessing the actual 
parameter easier and safer.

Thanks for the feedback,
Matthew Gerlach

>
> Thanks,
> Marco
>
>
>