Re: [PATCH 2/2] net: phy: dp83869: Add TI dp83869 phy

[Heiner Kallweit]

On 05.11.2019 21:26, Dan Murphy wrote:
> Heiner
> 
> On 11/5/19 2:20 PM, Heiner Kallweit wrote:
>> On 05.11.2019 21:02, Dan Murphy wrote:
>>> Heiner
>>>
>>> On 11/5/19 1:55 PM, Heiner Kallweit wrote:
>>>> On 05.11.2019 19:18, Dan Murphy wrote:
>>>>> Add support for the TI DP83869 Gigabit ethernet phy
>>>>> device.
>>>>>
>>>>> The DP83869 is a robust, low power, fully featured
>>>>> Physical Layer transceiver with integrated PMD
>>>>> sublayers to support 10BASE-T, 100BASE-TX and
>>>>> 1000BASE-T Ethernet protocols.
>>>>>
>>>>> Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
>>>>> ---
>>>>> ÂÂ drivers/net/phy/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 6 +
>>>>> ÂÂ drivers/net/phy/MakefileÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>>>> ÂÂ drivers/net/phy/dp83869.cÂÂÂÂÂÂÂÂÂÂÂ | 439 +++++++++++++++++++++++++++
>>>>> ÂÂ include/dt-bindings/net/ti-dp83869.h |Â 43 +++
>>>>> ÂÂ 4 files changed, 489 insertions(+)
>>>>> ÂÂ create mode 100644 drivers/net/phy/dp83869.c
>>>>> ÂÂ create mode 100644 include/dt-bindings/net/ti-dp83869.h
>> [...]
>>
>>>>> +static int op_mode;
>>>>> +
>>>>> +module_param(op_mode, int, 0644);
>>>>> +MODULE_PARM_DESC(op_mode, "The operational mode of the PHY");
>>>>> +
>>>> A module parameter isn't the preferred option here.
>>>> You could have more than one such PHY in different configurations.
>>>> Other drivers like the Marvell one use the interface mode to
>>>> check for the desired mode. Or you could read it from DT.
>>>>
>>> We do read the initial mode from the DT but there was a request to be able to change the mode during runtime.
>> Maybe we need to understand the use case better to be able to advise.
>> Will this be needed in production? Or was it requested as debug feature?
>> There's the option to set PHY registers from userspace, e.g. with phytool.
>> This could be used for reconfiguring the PHY.
> 
> This was a customer request that they be able to modify the op_mode from user space.
> 
> This was all I was given for a requirement. The customers use case was proprietary.
> 
If the module parameter serves just a (unknown) proprietary use case,
then it should not be in mainline. You could provide a patch with this
parameter separately to the client.

> Dan
> 
> 
>> Heiner
>>
>>
>>
>> [...]
>