Hello Oleksij,
Thanks for your review!!
On Sat, 2 Mar 2024 22:35:52 +0100
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
> 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.
Right,
>
> ...
> > @@ -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.
I totally agree that having port abstraction would be more convenient.
Maxime Chevallier is currently working on this and will post it after his
multi-phy series get merged.
Meanwhile, we still need a device pointer for getting the regulator. The
phy->mdio.dev is the only one I can think of as a regulator consumer.
Another idea?
> ...
> > +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...
Does the "vin-supply" property be added at the pse-pi node level or the
pse-controller node level or at the hardware port node level or the manager node
level for the pd692x0?
Maybe better at the pse-pi node level and each PIs of the manager will get the
same regulator?
What do you think?
> Should be tested, but if, instead of "vin-supply", we will use
> "pse-supply" it will make most part of pse_regulator.c obsolete.
Don't know, if it is done at the pse-pi node level it may not break
pse_regulator.c. Not sure about it.
> ....
> > @@ -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.
I don't think so as the regulator_get_exclusive() does the same and refcount it:
https://elixir.bootlin.com/linux/v6.7.8/source/drivers/regulator/core.c#L2268
> > + 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
Oh that's totally true sorry for that mistake.
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
