| Message ID | 20220817200335.911-3-kabel@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mvebu a3720 comphy: Fix serdes transmit amplitude | expand |
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. ... > + 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.
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. >
On Thu, Aug 18, 2022 at 10:41 PM Marek Behún <kabel@kernel.org> wrote: > > 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. 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.
On Thu, 18 Aug 2022 23:10:09 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > 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). OK > > > 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. 1. there is one branch where I don't need to read the values: when the "-names" property does not exist, the DT binding documentation says that the value property should only contain one value, the default one. So in that case I early return return fwnode_property_read_u32(fwnode, vals_prop, amplitude); 2. I thought that I shouldn't check whether the size of the "tx-p2p-microvolt-names" array is equal to the size of "tx-p2p-microvolt". Rob Herring says (if I understand correctly) that kernel shouldn't validate device-tree, that we have dt-schema for that... Marek
diff --git a/drivers/base/property.c b/drivers/base/property.c index ed6f449f8e5c..34b763436c30 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -921,6 +921,136 @@ int device_get_phy_mode(struct device *dev) } EXPORT_SYMBOL_GPL(device_get_phy_mode); +/** + * fwnode_get_tx_p2p_amplitude - Get peak to peak transmit amplitude for given + * PHY mode + * @fwnode: Pointer to the given node + * @mode: Name of the PHY mode, or "default" / NULL + * @amplitude: Pointer where to store the amplitude + * + * 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". + * + * For example if @mode is "pcie3", we first try searching for value + * corresponding to "pcie3", then to "pcie", and finally to "default". + * + * Return: %0 if the amplitude was read (success), + * %-EINVAL if given arguments are not valid, + * %-ENODATA if the required properties do not have a value, + * %-EPROTO if the property is not an array of strings, + * %-ENXIO if no suitable firmware interface is present, + * %-ENOMEM if out of memory. + */ +int fwnode_get_tx_p2p_amplitude(struct fwnode_handle *fwnode, const char *mode, + u32 *amplitude) +{ + static const char *names_prop = "tx-p2p-microvolt-names", + *vals_prop = "tx-p2p-microvolt"; + const char **names; + int cnt, idx, ret; + u32 *vals; + + 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; + + names = kcalloc(cnt, sizeof(*names), GFP_KERNEL); + if (!names) + return -ENOMEM; + + ret = fwnode_property_read_string_array(fwnode, names_prop, names, cnt); + if (ret < 0) { + kfree(names); + return ret; + } + + if (!mode) + mode = "default"; + + do { + static const char * const gen_table[] = { + "pcie", "usb", "ufs-hs", "dp", "mipi-dphy", + }; + size_t i; + + idx = match_string(names, cnt, mode); + if (idx >= 0) + break; + + /* If mode was not matched, try more generic mode */ + for (i = 0; i < ARRAY_SIZE(gen_table); ++i) { + if (str_has_proper_prefix(mode, gen_table[i])) { + mode = gen_table[i]; + break; + } + } + + /* Or "default" */ + if (i == ARRAY_SIZE(gen_table)) { + if (strcmp(mode, "default")) + mode = "default"; + else + mode = NULL; + } + } while (mode); + + kfree(names); + + if (idx < 0) + return -ENODATA; + + vals = kcalloc(cnt, sizeof(*vals), GFP_KERNEL); + if (!vals) + return -ENOMEM; + + ret = fwnode_property_read_u32_array(fwnode, vals_prop, vals, cnt); + if (ret) + goto out; + + *amplitude = vals[idx]; +out: + kfree(vals); + return ret; +} +EXPORT_SYMBOL_GPL(fwnode_get_tx_p2p_amplitude); + +/** + * device_get_tx_p2p_amplitude - Get peak to peak transmit amplitude for given + * PHY mode + * @dev: Pointer to the given device + * @mode: Name of the PHY mode, or "default" / NULL + * @amplitude: Pointer where to store the amplitude + * + * 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". + * + * For example if @mode is "pcie3", we first try searching for value + * corresponding to "pcie3", then to "pcie", and finally to "default". + * + * Return: %0 if the amplitude was read (success), + * %-EINVAL if given arguments are not valid, + * %-ENODATA if the required properties do not have a value, + * %-EPROTO if the property is not an array of strings, + * %-ENXIO if no suitable firmware interface is present, + * %-ENOMEM if out of memory. + */ +int device_get_tx_p2p_amplitude(struct device *dev, const char *mode, + u32 *amplitude) +{ + return fwnode_get_tx_p2p_amplitude(dev_fwnode(dev), mode, amplitude); +} +EXPORT_SYMBOL_GPL(device_get_tx_p2p_amplitude); + /** * fwnode_iomap - Maps the memory mapped IO for a given fwnode * @fwnode: Pointer to the firmware node diff --git a/include/linux/property.h b/include/linux/property.h index a5b429d623f6..91b12a79e245 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -392,6 +392,11 @@ const void *device_get_match_data(struct device *dev); int device_get_phy_mode(struct device *dev); int fwnode_get_phy_mode(struct fwnode_handle *fwnode); +int fwnode_get_tx_p2p_amplitude(struct fwnode_handle *fwnode, const char *mode, + u32 *amplitude); +int device_get_tx_p2p_amplitude(struct device *dev, const char *mode, + u32 *amplitude); + void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index); struct fwnode_handle *fwnode_graph_get_next_endpoint(
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. Signed-off-by: Marek Behún <kabel@kernel.org> --- Andy et al. can I get Ack for this if this is okay? --- drivers/base/property.c | 130 +++++++++++++++++++++++++++++++++++++++ include/linux/property.h | 5 ++ 2 files changed, 135 insertions(+)