Re: [PATCH linux-phy v2 2/4] device property: Add {fwnode/device}_get_tx_p2p_amplitude()

[Andy Shevchenko]

On Wed, Aug 17, 2022 at 11:09 PM Marek Behún <kabel@xxxxxxxxxx> 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