Re: [PATCH net-next 2/9] ethtool: Expand Ethernet Power Equipment with PoE alongside PoDL

[Köry Maincent]

+Oleksij

Sorry forgot to CC you the series.
Maybe you should add yourself to the MAINTAINERS of pse-pd drivers subsystem?

On Sat, 18 Nov 2023 18:38:43 +0100
Andrew Lunn <andrew@xxxxxxx> wrote:

> On Thu, Nov 16, 2023 at 03:01:34PM +0100, Kory Maincent wrote:
> > In the current PSE interface for Ethernet Power Equipment, support is
> > limited to PoDL. This patch extends the interface to accommodate the
> > objects specified in IEEE 802.3-2022 145.2 for Power sourcing
> > Equipment (PSE).  
> 
> Sorry for taking a while getting to these patches. Plumbers and other
> patches have been keeping me busy.

Don't worry you are doing a great job as a net maintainer and I won't raise any
remarks on delay considering how you are doing your job.
Thanks again for your review!!

> I'm trying to get my head around naming... Is there some sort of
> hierarchy? Is PSE the generic concept for putting power down the
> cable? Then you have the sub-type PoDL, and the sub-type PoE?

In fact as we discussed with Oleksij I decided to keep the naming as close as
possible to the IEEE 802.3 standard.
On the standard the PODL is naming like this aPoDLPSE* (ex: aPoDLPSEAdminState)
and the PSE is naming like this aPSE* (ex: aPSEAdminState) without any PoE
prefix. Maybe it is due to PoE being supported before PoDL and they didn't
expect the PoDL part.

> >  struct pse_control_config {
> >  	enum ethtool_podl_pse_admin_state podl_admin_control;
> > +	enum ethtool_pse_admin_state admin_control;  
> 
> When i look at this, it seems to me admin_control should be generic
> across all schemes which put power down the cable, and
> podl_admin_control is specific to how PoDL puts power down the cable.
>
> Since you appear to be adding support for a second way to put power
> down the cable, i would expect something like poe_admin_control being
> added here. But maybe that is in a later patch?

No as said above admin_control is for PoE and podl_admin_control is for PoDL.
Maybe you prefer to use poe_admin_control, and add poe prefix in the poe
variables. It will differ a bit from the IEEE standard naming but I agreed that
it would be more understandable in the development part.

I am open to the change.
Oleksij do you agree?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com