Re: [PATCH 3/7] net: phy: sync phy_{write,read,modify}_mmd_indirect with linux

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 10.07.23 14:43, Marco Felsch wrote:
> On 23-07-10, Ahmad Fatoum wrote:
>> On 10.07.23 13:01, Marco Felsch wrote:
>>> On 23-07-10, Ahmad Fatoum wrote:
>>>> Hello Marco,
>>>>
>>>> On 10.07.23 12:49, Marco Felsch wrote:
>>>>> On 23-07-10, Ahmad Fatoum wrote:
>>>>>> On 10.07.23 08:36, Marco Felsch wrote:
>>>>>>> Sync the barebox phy_{write,read,modify}_mmd_indirect API with the Linux
>>>>>>> phy_{write,read,modify}_mmd API to make it easier and less error prone
>>>>>>> to port phy drivers. This also fixes the r8169 driver since this driver
>>>>>>> did not adapt the parameters while porting from Linux.
>>>>>>>
>>>>>>> The sync also aligns the function parameter `prtad` as well as the
>>>>>>> function documentation.
>>>>>>
>>>>>> This breaks out of tree board code in a subtle and annoying manner.
>>>>>
>>>>> Do we really need to care about out all-of-tree drivers/boards? If so,
>>>>> we need to ensure to not break any public function.
>>>>
>>>> Out-of-tree drivers not too much probably, but out-of-tree boards
>>>> are unfortunately the norm. Breaking the build can be justified,
>>>> but silent breaking by swapping arguments is not nice.
>>>
>>> And how do you ensure that a public API is only used by drivers? Don't
>>> get me wrong I got your point but if you update an out-of-tree board to
>>> a newer barebox version it may happen that something break. Therefore
>>> manufacturers should upstream there code to reduce the maintenance
>>> effort to a minimum :) I know we don't live in an ideal world :/
>>
>> Some breakage is ultimately unavoidable, but we can try to not needlessly
>> introduce silent breakage. Here we have a nice way out: We import the
>> Linux function with the Linux name and use that for upstream code. The old
>> function is marked deprecated and removed later on. Do you see an issue
>> with that?
> 
> The only problem I see is, that the Linux API does support C45 and C22
> and we don't. As said, we can implement it and make the C45 support a
> stub. From user perspective I would assume that the new API is working
> for both classes (like Linux). Therefore I kept the _indirect suffix
> since this makes it clear from API pov.

Error message on C45 sounds good to me. Swapping parameters without
function rename doesn't.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |