On 10/29/22 05:11, Bagas Sanjaya wrote:

On Thu, Oct 27, 2022 at 03:11:08PM -0400, Sean Anderson wrote:

quoted

 .. 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.

quoted

+
+    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

quoted

+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.