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

["Marek Behún" <kabel@kernel.org>]

On Thu, 18 Aug 2022 22:22:31 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

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

The property may be also useful for drivers that don't depend on
generic PHY subsystem. At least the mv88e6xxx DSA driver already reads
the property [1] although it simply uses of_property_read_u32(),
because it does not expect more complicated definition yet.

There are three subsystem which may want to use this function: generic
PHY, ethernet PHY and DSA. Since ethernet PHY subsystem nor DSA
subsystem do not depend on generic PHY, I thought putting it in base
would be sensible.

If you think it should be in generic PHY subsystem anyway, and that
other drivers needing it should depend on GENERIC_PHY, I can move it.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/chip.c?h=v6.0-rc1#n3504

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

What do you mean? Count the values and read them all via one
call to fwnode_property_string_array_count() ?

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