Re: [PATCH net-next v1 4/7] net: pse-pd: add generic PSE driver

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Fri, Aug 19, 2022 at 10:54:14PM +0200, Andrew Lunn wrote:
> > +static int
> > +gen_pse_podl_get_admin_sate(struct pse_controller_dev *pcdev, unsigned long id)
> 
> Should that be state?

ack. fixed.

> > +{
> > +	struct gen_pse_priv *priv = to_gen_pse(pcdev);
> > +
> > +	/* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus
> > +	 * which is provided by the regulator.
> > +	 */
> > +	return priv->admin_state;
> > +}
> > +
> > +static int
> > +gen_pse_podl_set_admin_control(struct pse_controller_dev *pcdev,
> > +			       unsigned long id,
> > +			       enum ethtool_podl_pse_admin_state state)
> > +{
> > +	struct gen_pse_priv *priv = to_gen_pse(pcdev);
> > +	int ret;
> > +
> > +	if (priv->admin_state == state)
> > +		goto set_state;
> 
> return 0; ?

ack. done

> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	priv->admin_state = ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED;
> 
> There is the comment earlier:
> 
> 	/* aPoDLPSEAdminState can be different to aPoDLPSEPowerDetectionStatus
> 	 * which is provided by the regulator.
> 
> Is this because the regulator might of been turned on, but it has
> detected a short and turned itself off? So it is administratively on,
> but off in order to stop the magic smoke escaping?

ack. According to 30.15.1.1.3 aPoDLPSEPowerDetectionStatus, we may have
following:
/**
 * enum ethtool_podl_pse_pw_d_status - power detection status of the PoDL PSE.
 *	IEEE 802.3-2018 30.15.1.1.3 aPoDLPSEPowerDetectionStatus:
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_UNKNOWN: PoDL PSE
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_DISABLED: "The enumeration “disabled” is
 *	asserted true when the PoDL PSE state diagram variable mr_pse_enable is
 *	false"
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_SEARCHING: "The enumeration “searching” is
 *	asserted true when either of the PSE state diagram variables
 *	pi_detecting or pi_classifying is true."
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_DELIVERING: "The enumeration “deliveringPower”
 *	is asserted true when the PoDL PSE state diagram variable pi_powered is
 *	true."
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_SLEEP: "The enumeration “sleep” is asserted
 *	true when the PoDL PSE state diagram variable pi_sleeping is true."
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_IDLE: "The enumeration “idle” is asserted true
 *	when the logical combination of the PoDL PSE state diagram variables
 *	pi_prebiased*!pi_sleeping is true."
 * @ETHTOOL_PODL_PSE_PW_D_STATUS_ERROR: "The enumeration “error” is asserted
 *	true when the PoDL PSE state diagram variable overload_held is true."
 */

Probably all of them, except of ETHTOOL_PODL_PSE_PW_D_STATUS_SEARCHING can be
potentially implemented in this driver on top of regulator framework.

> But what about the other way around? Something has already turned the
> regulator on, e.g. the bootloader. Should the default be
> ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED even thought power is being
> delivered? Do we want to put the regulator into the off state at
> probe, so it is in a well defined state? Or set priv->admin_state to
> whatever regulator_is_enabled() indicates?

Good question. I assume, automotive folks would love be able to enable
regulator in the boot loader on the PSE to let the Powered Device boot parallel
to the PSE.

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 |