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

[Andy Shevchenko]

On Thu, Aug 18, 2022 at 10:41 PM Marek Behún <kabel@xxxxxxxxxx> wrote:
>
> On Thu, 18 Aug 2022 22:22:31 +0300
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> > 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.
>
> 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.

Yes, I have no objection to put it there, just that the above
justification doesn't allow it to be in the generic code (yes, we may
still have some awkward APIs in the property.c and ideally they should
be moved to the respective subsystems).

> [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() ?

No, you obviously may not read them via string_array APIs, esp. one
that is related to counting.

Count the vals first, read them all (it seems you need it in all
branches of your flow). Then count names and compare them to the
number of values, and so on... Also try to assign "default" only once.

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

-- 
With Best Regards,
Andy Shevchenko