On Tue, Feb 27, 2024 at 03:42:55PM +0100, Kory Maincent wrote:
> Integrate the regulator framework to the PSE framework for enhanced
> access to features such as voltage, power measurement, and limits, which
> are akin to regulators. Additionally, PSE features like port priorities
> could potentially enhance the regulator framework. Note that this
> integration introduces some implementation complexity, including wrapper
> callbacks and nested locks, but the potential benefits make it worthwhile.
>
> Regulator are using enable counter with specific behavior.
> Two calls to regulator_disable will trigger kernel warnings.
> If the counter exceeds one, regulator_disable call won't disable the
> PSE PI. These behavior isn't suitable for PSE control.
> Added a boolean 'enabled' state to prevent multiple calls to
Please rename rename "enabled" to "admin_state_enabled". This variable
do not reflect real device state, it reflects only user configuration.
...
> @@ -120,15 +118,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> u32 phy_id;
> int rc;
>
> - psec = fwnode_find_pse_control(child);
> - if (IS_ERR(psec))
> - return PTR_ERR(psec);
> -
> mii_ts = fwnode_find_mii_timestamper(child);
> - if (IS_ERR(mii_ts)) {
> - rc = PTR_ERR(mii_ts);
> - goto clean_pse;
> - }
> + if (IS_ERR(mii_ts))
> + return PTR_ERR(mii_ts);
>
> is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
> if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> @@ -161,6 +153,12 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> goto clean_phy;
> }
>
> + psec = dev_find_pse_control(&phy->mdio.dev);
> + if (IS_ERR(psec)) {
> + rc = PTR_ERR(psec);
> + goto unregister_phy;
> + }
> +
I do not think it is a good idea to make PSE controller depend on
phy->mdio.dev. The only reason why we have fwnode_find_pse_control()
here was the missing port abstraction.
...
> +static int
> +devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
> + char *name, int id)
> +{
> + struct regulator_config rconfig = {0};
> + struct regulator_desc *rdesc;
> + struct regulator_dev *rdev;
> +
> + rdesc = devm_kzalloc(pcdev->dev, sizeof(*rdesc), GFP_KERNEL);
> + if (!rdesc)
> + return -ENOMEM;
> +
> + /* Regulator descriptor id have to be the same as its associated
> + * PSE PI id for the well functioning of the PSE controls.
> + */
> + rdesc->id = id;
> + rdesc->name = name;
> + rdesc->type = REGULATOR_CURRENT;
> + rdesc->ops = &pse_pi_ops;
> + rdesc->owner = pcdev->owner;
> +
> + rconfig.dev = pcdev->dev;
> + rconfig.driver_data = pcdev;
> + rconfig.init_data = &pse_pi_initdata;
Please add input supply to track all dependencies:
if (of_property_present(np, "vin-supply"))
config->input_supply = "vin";
May be better to make it not optional...
Should be tested, but if, instead of "vin-supply", we will use
"pse-supply" it will make most part of pse_regulator.c obsolete.
....
> @@ -310,6 +452,20 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index)
> return ERR_PTR(-ENODEV);
> }
>
> + psec->ps = devm_regulator_get_exclusive(dev,
> + rdev_get_name(pcdev->pi[index].rdev));
> + if (IS_ERR(psec->ps)) {
> + kfree(psec);
> + return ERR_CAST(psec->ps);
> + }
> +
> + ret = regulator_is_enabled(psec->ps);
> + if (ret < 0) {
> + kfree(psec);
> + return ERR_PTR(ret);
> + }
> + pcdev->pi[index].enabled = ret;
If I see it correctly, it will prevent us to refcount a request from
user space. So, the runtime PM may suspend PI.
> +
> psec->pcdev = pcdev;
> list_add(&psec->list, &pcdev->pse_control_head);
> psec->id = index;
> @@ -344,7 +500,8 @@ static int psec_id_legacy_xlate(struct pse_controller_dev *pcdev,
> return pse_spec->args[0];
> }
>
> -struct pse_control *of_pse_control_get(struct device_node *node)
> +struct pse_control *of_pse_control_get(struct device *dev,
> + struct device_node *node)
> {
> struct pse_controller_dev *r, *pcdev;
> struct of_phandle_args args;
> @@ -394,7 +551,7 @@ struct pse_control *of_pse_control_get(struct device_node *node)
> }
>
> /* pse_list_mutex also protects the pcdev's pse_control list */
> - psec = pse_control_get_internal(pcdev, psec_id);
> + psec = pse_control_get_internal(dev, pcdev, psec_id);
>
> out:
> mutex_unlock(&pse_list_mutex);
> @@ -443,21 +600,24 @@ int pse_ethtool_set_config(struct pse_control *psec,
> struct netlink_ext_ack *extack,
> const struct pse_control_config *config)
> {
> - const struct pse_controller_ops *ops;
> - int err;
> -
> - ops = psec->pcdev->ops;
> + int err = 0;
>
> - if (!ops->ethtool_set_config) {
> - NL_SET_ERR_MSG(extack,
> - "PSE driver does not configuration");
> - return -EOPNOTSUPP;
> + /* Look at enabled status to not call regulator_enable or
> + * regulator_disable twice creating a regulator counter mismatch
> + */
> + switch (config->c33_admin_control) {
> + case ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED:
> + if (!psec->pcdev->pi[psec->id].enabled)
> + err = regulator_enable(psec->ps);
> + break;
> + case ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED:
> + if (psec->pcdev->pi[psec->id].enabled)
> + err = regulator_disable(psec->ps);
> + break;
> + default:
> + err = -EOPNOTSUPP;
> }
This change seems to break PoDL support
> - mutex_lock(&psec->pcdev->lock);
> - err = ops->ethtool_set_config(psec->pcdev, psec->id, extack, config);
> - mutex_unlock(&psec->pcdev->lock);
> -
> return err;
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
