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

[Bagas Sanjaya <bagasdotme@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3220 bytes --]

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?

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

Also, don't forget to add conjunctions:

"... Then modify ``lynx_lookup_proto`` ... Finally, update the ...
helpers ..."

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

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]