[PATCH v8 4/9] phy: fsl: Add Lynx 10G SerDes driver

[Sean Anderson]

On 10/29/22 05:11, Bagas Sanjaya wrote:
> On Thu, Oct 27, 2022 at 03:11:08PM -0400, Sean Anderson wrote:
>>  .. only::  subproject and html
>> diff --git a/Documentation/driver-api/phy/lynx_10g.rst b/Documentation/driver-api/phy/lynx_10g.rst
>> new file mode 100644
>> index 000000000000..ebbf4dd86726
>> --- /dev/null
>> +++ b/Documentation/driver-api/phy/lynx_10g.rst
>> @@ -0,0 +1,58 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +===========================
>> +Lynx 10G Phy (QorIQ SerDes)
>> +===========================
>> +
>> +Using this phy
>> +--------------
>> +
>> +:c:func:`phy_get` just gets (or creates) a new :c:type:`phy` with the lanes
>> +described in the phandle. :c:func:`phy_init` is what actually reserves the
>> +lanes for use. Unlike some other drivers, when the phy is created, there is no
>> +default protocol. :c:func:`phy_set_mode <phy_set_mode_ext>` must be called in
>> +order to set the protocol.
>> +
>> +Supporting SoCs
>> +---------------
>> +
>> +Each new SoC needs a :c:type:`struct lynx_conf <lynx_conf>`, containing the
>> +number of lanes in each device, the endianness of the device, and the helper
>> +functions to use when selecting protocol controllers. For example, the
>> +configuration for the LS1046A is::
> 
> Did you mean struct lynx_cfg as in below snippet?

Yes.

>> +
>> +    static const struct lynx_cfg ls1046a_cfg = {
>> +        .lanes = 4,
>> +        .endian = REGMAP_ENDIAN_BIG,
>> +        .mode_conflict = lynx_ls_mode_conflict,
>> +        .mode_apply = lynx_ls_mode_apply,
>> +        .mode_init = lynx_ls_mode_init,
>> +    };
>> +
>> +The ``mode_`` functions will generally be common to all SoCs in a series (e.g.
>> +all Layerscape SoCs or all T-series SoCs).
>> +
>> +In addition, you will need to add a device node as documented in
>> +``Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml``. This lets the
>> +driver know which lanes are available to configure.
>> +
>> +Supporting Protocols
>> +--------------------
>> +
>> +Each protocol is a combination of values which must be programmed into the lane
>> +registers. To add a new protocol, first add it to :c:type:`enum lynx_protocol
>> +<lynx_protocol>`. Add a new entry to `lynx_proto_params`, and populate the
>> +appropriate fields. Modify `lynx_lookup_proto` to map the :c:type:`enum
>> +phy_mode <phy_mode>` to :c:type:`enum lynx_protocol <lynx_protocol>`. Update
>> +the ``mode_conflict``, ``mode_apply``, and ``mode_init`` helpers are updated to
>> +support your protocol.
>> +
> 
> These lynx_ keywords should be in double backticks to be consistent
> (rendered as inline code).
> 

OK

> Also, don't forget to add conjunctions:
> 
> "... Then modify ``lynx_lookup_proto`` ... Finally, update the ...
> helpers ..."

Personally, I like to be conservative with connectives when describing
sequences. I do agree that a "finally" would help here.

--Sean

>> +You may need to modify :c:func:`lynx_set_mode` in order to support your
>> +protocol. This can happen when you have added members to :c:type:`struct
>> +lynx_proto_params <lynx_proto_params>`. It can also happen if you have specific
>> +clocking requirements, or protocol-specific registers to program.
>> +
>> +Internal API Reference
>> +----------------------
>> +
>> +.. kernel-doc:: drivers/phy/freescale/phy-fsl-lynx-10g.c
> 
> Otherwise LGTM, thanks.
>