linux-kernel - Re: [PATCH linux-phy v2 2/4] device property: Add {fwnode/device}_get_tx_p2p

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Aug 2022 22:22:31 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Marek Behún <kabel@...nel.org>
Cc:     Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        Linux Phy <linux-phy@...ts.infradead.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Kees Cook <keescook@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Pali Rohár <pali@...nel.org>,
        josef.schlehofer@....cz
Subject: Re: [PATCH linux-phy v2 2/4] device property: Add {fwnode/device}_get_tx_p2p_amplitude()

On Wed, Aug 17, 2022 at 11:09 PM Marek Behún <kabel@...nel.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 = fwnode_property_string_array_count(fwnode, names_prop);
> +       if (!cnt || cnt == -EINVAL)
> +               /*
> +                * If the names property does not exist or is empty, we expect
> +                * the values property to contain only one, default value.
> +                */
> +               return fwnode_property_read_u32(fwnode, vals_prop, amplitude);
> +       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 PHY mode
> + * by parsing the 'tx-p2p-microvolt' and 'tx-p2p-microvolt-names' properties.
> + * 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.

-- 
With Best Regards,
Andy Shevchenko