[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Marek Vasut]

On 3/31/20 3:24 PM, Simon Glass wrote:
> Hi Marek,
> 
> On Mon, 30 Mar 2020 at 18:39, Marek Vasut <marex at denx.de> wrote:
>>
>> On 3/31/20 1:31 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex at denx.de> wrote:
>>>>
>>>> On 3/22/20 4:17 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex at denx.de> wrote:
>>>>>>
>>>>>> On 3/22/20 3:08 AM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> I think at this point we've covered all the ground and mentioned the
>>>>>>> pros and cons of each method, so I'll leave the discussion where it
>>>>>>> is.
>>>>>>
>>>>>> Great, so let's remove the struct-based access from the driver and use
>>>>>> regular #define REGISTER 0xoffset.
>>>>>
>>>>> I think any individual decision depends on the pros and cons we
>>>>> outlined in our discussion. I don't have any information to suggest
>>>>> that the Mediatek XHCI driver has any of the variations you talked
>>>>> about in your worst-case scenarios, so I can't comment on that. I am
>>>>> more concerned about this as a general rule as I feel that the
>>>>> struct-based approach is generally best for U-Boot, except for the
>>>>> cases you highlighted:
>>>>>
>>>>> - where the registers appear at different offsets in different
>>>>> hardware revisions served by the same driver
>>>>> - where the driver only uses a small subset of the registers and it is
>>>>> not worth defining a struct to cover them all, with associated empty
>>>>> regions, etc.
>>>>>
>>>>> Anything else?
>>>>
>>>> It's also very difficult to easily figure out the address of a register
>>>> that's buried somewhere down in a long structure, possibly with embedded
>>>> sub-structures.
>>>
>>> OK I have updated the coding style page with all of this.
>>
>> Which page ?
> 
> https://www.denx.de/wiki/U-Boot/CodingStyle

" U-Boot typically uses a C structure to map out the registers in an I/O
region, rather than offsets. The reasons for this are: " is misleading
and suggests that using structures is the best practice. This should be
reworded to make it clear both options are equally valid.

> Separately, I sent a patch a while back which updated checkpatch for
> U-Boot. I got some pushback

Link ?

I am very much opposed to this struct-based approach, so there is my
pushback too. I think we should NOT do it.

>, but I think that was wrong and we should
> do it. For example I am saying many of the same things in code reviews
> and many of them could be caught by the script. Examples include using
> if() instead of #if where possible.
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut