diff mbox series

[net-next,v4,05/17] net: pse-pd: Introduce PSE types enumeration

Message ID 20240215-feature_poe-v4-5-35bb4c23266c@bootlin.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: Add support for Power over Ethernet (PoE) | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; GEN HAS DIFF 2 files changed, 96 insertions(+), 39 deletions(-);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 991 this patch: 13
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang fail Errors and warnings before: 1007 this patch: 13
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1008 this patch: 13
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kory Maincent Feb. 15, 2024, 4:02 p.m. UTC
Introduce an enumeration to define PSE types (C33 or PoDL),
utilizing a bitfield for potential future support of both types.
Include 'pse_get_types' helper for external access to PSE type info.

This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---

Changes in v2:
- Rename PSE_POE to PSE_C33 to have naming consistency.
- Use "static inline" instead of simple static in the header

Changes in v3:
- Move the pse_type enum in uapi.
- Replace pse_get_types helper by pse_has_podl and pse_has_c33.
---
 drivers/net/pse-pd/pse_core.c      | 12 ++++++++++++
 drivers/net/pse-pd/pse_regulator.c |  1 +
 include/linux/pse-pd/pse.h         | 16 ++++++++++++++++
 include/uapi/linux/pse.h           | 23 +++++++++++++++++++++++
 4 files changed, 52 insertions(+)

Comments

Jakub Kicinski Feb. 15, 2024, 6:58 p.m. UTC | #1
On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote:
> Introduce an enumeration to define PSE types (C33 or PoDL),
> utilizing a bitfield for potential future support of both types.
> Include 'pse_get_types' helper for external access to PSE type info.

I haven't read the series, just noticed this breaks the build:

error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier

but why the separate header? Is it going to be used in other parts of
uAPI than just in ethtool?

> This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>

side-note: no objections to the line but for accounting purposes
(i.e. when we generate development stats) we use the Author / From
line exclusively. So it'd be easier to compute stats of things funded
by Dent if you used:

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

but that's entirely up to you :)
Kory Maincent Feb. 16, 2024, 9:42 a.m. UTC | #2
On Thu, 15 Feb 2024 10:58:46 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote:
> > Introduce an enumeration to define PSE types (C33 or PoDL),
> > utilizing a bitfield for potential future support of both types.
> > Include 'pse_get_types' helper for external access to PSE type info.  
> 
> I haven't read the series, just noticed this breaks the build:
> 
> error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for
> SPDX-License-Identifier

By curiosity how do you get that error? 
Is it with C=1? I didn't faced it with W=1.
C=1 is broken for several architecture like arm64, indeed I forgot to run it.

> but why the separate header? Is it going to be used in other parts of
> uAPI than just in ethtool?

We might use it in pse core if capabilities between PoE and PoDL differ but I
am not sure about it.
Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the enum
values?

> > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>  
> 
> side-note: no objections to the line but for accounting purposes
> (i.e. when we generate development stats) we use the Author / From
> line exclusively. So it'd be easier to compute stats of things funded
> by Dent if you used:
> 
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> but that's entirely up to you :)

Does adding the line side to the SOB in the commit message is sufficient or
should I modify the git send email config?

Regard,
Jakub Kicinski Feb. 17, 2024, 1:36 a.m. UTC | #3
On Fri, 16 Feb 2024 10:42:11 +0100 Köry Maincent wrote:
> > On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote:  
> > > Introduce an enumeration to define PSE types (C33 or PoDL),
> > > utilizing a bitfield for potential future support of both types.
> > > Include 'pse_get_types' helper for external access to PSE type info.    
> > 
> > I haven't read the series, just noticed this breaks the build:
> > 
> > error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for
> > SPDX-License-Identifier  
> 
> By curiosity how do you get that error? 
> Is it with C=1? I didn't faced it with W=1.
> C=1 is broken for several architecture like arm64, indeed I forgot to run it.

Not 100% sure, TBH, I suspect it's somehow enabled by allmodconfig.
I don't think it's a C=1 thing because our clang build doesn't do C=1
and it also hit it.

> > but why the separate header? Is it going to be used in other parts of
> > uAPI than just in ethtool?  
> 
> We might use it in pse core if capabilities between PoE and PoDL differ but I
> am not sure about it.
> Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the enum
> values?

I don't know enough to have an opinion :) Whatever you end up doing,
it's probably worth documenting the reason for the choice in the commit
message?

> > > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org>    
> > 
> > side-note: no objections to the line but for accounting purposes
> > (i.e. when we generate development stats) we use the Author / From
> > line exclusively. So it'd be easier to compute stats of things funded
> > by Dent if you used:
> > 
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > but that's entirely up to you :)  
> 
> Does adding the line side to the SOB in the commit message is sufficient or
> should I modify the git send email config?

I think you can sed -i s/// the patches? When the From in the email
file doesn't match your git config IIUC git will include the from line
in the body and pick it up from them. IOW it will work. The scripts look
at git author so s-o-b won't do much.
Kory Maincent Feb. 19, 2024, 3:04 p.m. UTC | #4
On Fri, 16 Feb 2024 17:36:38 -0800
Jakub Kicinski <kuba@kernel.org> wrote:
> > > but why the separate header? Is it going to be used in other parts of
> > > uAPI than just in ethtool?    
> > 
> > We might use it in pse core if capabilities between PoE and PoDL differ but
> > I am not sure about it.
> > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the
> > enum values?  
> 
> I don't know enough to have an opinion :) Whatever you end up doing,
> it's probably worth documenting the reason for the choice in the commit
> message?

Mmh, I am still not sure of the best choice on this. I think I will move it to
ethtool as you suggested.

> > > > This patch is sponsored by Dent Project
> > > > <dentproject@linuxfoundation.org>      
> > > 
> > > side-note: no objections to the line but for accounting purposes
> > > (i.e. when we generate development stats) we use the Author / From
> > > line exclusively. So it'd be easier to compute stats of things funded
> > > by Dent if you used:
> > > 
> > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > > 
> > > but that's entirely up to you :)    
> > 
> > Does adding the line side to the SOB in the commit message is sufficient or
> > should I modify the git send email config?  
> 
> I think you can sed -i s/// the patches? When the From in the email
> file doesn't match your git config IIUC git will include the from line
> in the body and pick it up from them. IOW it will work. The scripts look
> at git author so s-o-b won't do much.

Ok, I will stick to the simple sentence then. ;)
Thanks for the information!

Regards,
Andrew Lunn Feb. 19, 2024, 3:44 p.m. UTC | #5
On Mon, Feb 19, 2024 at 04:04:56PM +0100, Köry Maincent wrote:
> On Fri, 16 Feb 2024 17:36:38 -0800
> Jakub Kicinski <kuba@kernel.org> wrote:
> > > > but why the separate header? Is it going to be used in other parts of
> > > > uAPI than just in ethtool?    
> > > 
> > > We might use it in pse core if capabilities between PoE and PoDL differ but
> > > I am not sure about it.
> > > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the
> > > enum values?  
> > 
> > I don't know enough to have an opinion :) Whatever you end up doing,
> > it's probably worth documenting the reason for the choice in the commit
> > message?
> 
> Mmh, I am still not sure of the best choice on this. I think I will move it to
> ethtool as you suggested.

kAPI is hard to change. So it is worth thinking about it.

Can you think of any possible kAPI not using ethtool netlink? Its not
going to be ioctl. We generally don't export new things in /sysfs if
we have netlink, etc.

So to me, it is only going to be used be the ethtool API, so i would
follow the usual conventions for ethtool.

   Andrew
Rob Herring (Arm) Feb. 21, 2024, 2:36 p.m. UTC | #6
On Fri, Feb 16, 2024 at 05:36:38PM -0800, Jakub Kicinski wrote:
> On Fri, 16 Feb 2024 10:42:11 +0100 Köry Maincent wrote:
> > > On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote:  
> > > > Introduce an enumeration to define PSE types (C33 or PoDL),
> > > > utilizing a bitfield for potential future support of both types.
> > > > Include 'pse_get_types' helper for external access to PSE type info.    
> > > 
> > > I haven't read the series, just noticed this breaks the build:
> > > 
> > > error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for
> > > SPDX-License-Identifier  
> > 
> > By curiosity how do you get that error? 
> > Is it with C=1? I didn't faced it with W=1.
> > C=1 is broken for several architecture like arm64, indeed I forgot to run it.
> 
> Not 100% sure, TBH, I suspect it's somehow enabled by allmodconfig.
> I don't think it's a C=1 thing because our clang build doesn't do C=1
> and it also hit it.

Probably from scripts/spdxcheck.py? IIRC, it has some dependencies which 
have to be installed to enable it.

Rob
Kory Maincent Feb. 21, 2024, 2:40 p.m. UTC | #7
On Mon, 19 Feb 2024 16:44:34 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Feb 19, 2024 at 04:04:56PM +0100, Köry Maincent wrote:
> > On Fri, 16 Feb 2024 17:36:38 -0800
> > Jakub Kicinski <kuba@kernel.org> wrote:  
> > > > > but why the separate header? Is it going to be used in other parts of
> > > > > uAPI than just in ethtool?      
> > > > 
> > > > We might use it in pse core if capabilities between PoE and PoDL differ
> > > > but I am not sure about it.
> > > > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to
> > > > the enum values?    
> > > 
> > > I don't know enough to have an opinion :) Whatever you end up doing,
> > > it's probably worth documenting the reason for the choice in the commit
> > > message?  
> > 
> > Mmh, I am still not sure of the best choice on this. I think I will move it
> > to ethtool as you suggested.  
> 
> kAPI is hard to change. So it is worth thinking about it.
> 
> Can you think of any possible kAPI not using ethtool netlink? Its not
> going to be ioctl. We generally don't export new things in /sysfs if
> we have netlink, etc.
> 
> So to me, it is only going to be used be the ethtool API, so i would
> follow the usual conventions for ethtool.

Oops sorry forgot to reply to you.
Indeed I reached to the same conclusion on my side.

Regards,
diff mbox series

Patch

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 146b81f08a89..090e04c32f9e 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -312,3 +312,15 @@  int pse_ethtool_set_config(struct pse_control *psec,
 	return err;
 }
 EXPORT_SYMBOL_GPL(pse_ethtool_set_config);
+
+bool pse_has_podl(struct pse_control *psec)
+{
+	return psec->pcdev->types & PSE_PODL;
+}
+EXPORT_SYMBOL_GPL(pse_has_podl);
+
+bool pse_has_c33(struct pse_control *psec)
+{
+	return psec->pcdev->types & PSE_C33;
+}
+EXPORT_SYMBOL_GPL(pse_has_c33);
diff --git a/drivers/net/pse-pd/pse_regulator.c b/drivers/net/pse-pd/pse_regulator.c
index 1dedf4de296e..e34ab8526067 100644
--- a/drivers/net/pse-pd/pse_regulator.c
+++ b/drivers/net/pse-pd/pse_regulator.c
@@ -116,6 +116,7 @@  pse_reg_probe(struct platform_device *pdev)
 	priv->pcdev.owner = THIS_MODULE;
 	priv->pcdev.ops = &pse_reg_ops;
 	priv->pcdev.dev = dev;
+	priv->pcdev.types = PSE_PODL;
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
 	if (ret) {
 		dev_err(dev, "failed to register PSE controller (%pe)\n",
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index be4e5754eb24..f006cbdf8b3b 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -8,6 +8,7 @@ 
 #include <linux/ethtool.h>
 #include <linux/list.h>
 #include <uapi/linux/ethtool.h>
+#include <uapi/linux/pse.h>
 
 struct phy_device;
 struct pse_controller_dev;
@@ -77,6 +78,7 @@  struct pse_control;
  *            device tree to id as given to the PSE control ops
  * @nr_lines: number of PSE controls in this controller device
  * @lock: Mutex for serialization access to the PSE controller
+ * @types: types of the PSE controller
  */
 struct pse_controller_dev {
 	const struct pse_controller_ops *ops;
@@ -89,6 +91,7 @@  struct pse_controller_dev {
 			const struct of_phandle_args *pse_spec);
 	unsigned int nr_lines;
 	struct mutex lock;
+	u32 types;
 };
 
 #if IS_ENABLED(CONFIG_PSE_CONTROLLER)
@@ -108,6 +111,9 @@  int pse_ethtool_set_config(struct pse_control *psec,
 			   struct netlink_ext_ack *extack,
 			   const struct pse_control_config *config);
 
+bool pse_has_podl(struct pse_control *psec);
+bool pse_has_c33(struct pse_control *psec);
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node)
@@ -133,6 +139,16 @@  static inline int pse_ethtool_set_config(struct pse_control *psec,
 	return -ENOTSUPP;
 }
 
+static inline bool pse_has_podl(struct pse_control *psec)
+{
+	return false;
+}
+
+static inline bool pse_has_c33(struct pse_control *psec)
+{
+	return false;
+}
+
 #endif
 
 #endif
diff --git a/include/uapi/linux/pse.h b/include/uapi/linux/pse.h
new file mode 100644
index 000000000000..ebd9b4be9d9d
--- /dev/null
+++ b/include/uapi/linux/pse.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Userspace API for Power Sourcing Equipment
+ *
+ * Copyright (c) 2023 Bootlin, Kory Maincent <kory.maincent@bootlin.com>
+ */
+#ifndef _PSE_CONTROLLER_H
+#define _PSE_CONTROLLER_H
+
+/**
+ * enum - Types of PSE controller.
+ *
+ * @PSE_UNKNOWN: Type of PSE controller is unknown
+ * @PSE_PODL: PSE controller which support PoDL
+ * @PSE_C33: PSE controller which support Clause 33 (PoE)
+ */
+enum {
+	PSE_UNKNOWN =	1 << 0,
+	PSE_PODL =	1 << 1,
+	PSE_C33 =	1 << 2,
+};
+
+#endif /* _PSE_CONTROLLER_H */