Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220817200335.911-1-kabel@kernel.org> <20220817200335.911-3-kabel@kernel.org>
In-Reply-To: <20220817200335.911-3-kabel@kernel.org>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 18 Aug 2022 22:22:31 +0300
Message-ID: <CAHp75Vcn1MXEB_+kJuR3qY8qxi6oq0pEFFSFrmKv2-tE-YP-zA@mail.gmail.com>
Subject: Re: [PATCH linux-phy v2 2/4] device property: Add {fwnode/device}_get_tx_p2p_amplitude()
To:     =?UTF-8?B?TWFyZWsgQmVow7pu?= <kabel@kernel.org>
Cc:     Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Linux Phy <linux-phy@lists.infradead.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Daniel Scally <djrscally@gmail.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Kees Cook <keescook@chromium.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>,
        josef.schlehofer@nic.cz
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 17, 2022 at 11:09 PM Marek Beh=C3=BAn <kabel@kernel.org> wrote:
>
> Add functions fwnode_get_tx_p2p_amplitude() and
> device_get_tx_p2p_amplitude() that parse the 'tx-p2p-microvolt' and
> 'tx-p2p-microvolt-names' properties and return peak to peak transmit
> amplitude in microvolts for given PHY mode.
>
> The functions search for mode name in 'tx-p2p-microvolt-names' property,
> and return amplitude at the corresponding index in the 'tx-p2p-microvolt'
> property.
>
> If given mode is not matched in 'tx-p2p-microvolt-names' array, the mode
> name is generalized (for example "pcie3" -> "pcie" -> "default", or
> "usb-ss" -> "usb" -> "default").
>
> If the 'tx-p2p-microvolt-names' is not present, the 'tx-p2p-microvolt'
> property is expected to contain only one value, which is considered
> default, and will be returned for any mode.

It's very specific to a domain. NAK for putting it to the generic
code, otherwise explain how it can be useful outside of the PHY world.

...

> +       cnt =3D fwnode_property_string_array_count(fwnode, names_prop);
> +       if (!cnt || cnt =3D=3D -EINVAL)
> +               /*
> +                * If the names property does not exist or is empty, we e=
xpect
> +                * the values property to contain only one, default value=
.
> +                */
> +               return fwnode_property_read_u32(fwnode, vals_prop, amplit=
ude);
> +       else if (cnt < 0)
> +               return cnt;

You may count the values and read them all, and then check the names
and compare count to the read values. In such a case you don't need
too many (overlapped) checks. I think the current implementation is
far from being optimal. Take your time and try to get rid of 20% of
lines in this function. I believe it's doable.

...

> + * Gets the peak to peak transmit amplitude in microvolts for a given PH=
Y mode
> + * by parsing the 'tx-p2p-microvolt' and 'tx-p2p-microvolt-names' proper=
ties.
> + * If amplitude is not specified for @mode exactly, tries a more generic=
 mode,
> + * and if that isn't specified, tries "default".

Gets --> Get
tries --> try

Otherwise add a subject to the sentences.

--=20
With Best Regards,
Andy Shevchenko