[net-next,v4,05/17] net: pse-pd: Introduce PSE types enumeration
Commit Message
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
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 :)
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,
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.
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,
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
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
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,
@@ -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);
@@ -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",
@@ -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
new file mode 100644
@@ -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 */