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

On Fri, Aug 19, 2022 at 10:54:14PM +0200, Andrew Lunn wrote:

quoted

+static int
+gen_pse_podl_get_admin_sate(struct pse_controller_dev *pcdev, unsigned long id)

Should that be state?

ack. fixed.

quoted

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

quoted

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