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

On Thu, Aug 18, 2022 at 10:41 PM Marek Behún [off-list ref] wrote:

On Thu, 18 Aug 2022 22:22:31 +0300
Andy Shevchenko [off-list ref] wrote:

quoted

On Wed, Aug 17, 2022 at 11:09 PM Marek Behún [off-list ref] wrote:

quoted

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

...

quoted

quoted

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

quoted

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy