I see a printk when booting on chromebooks that don't have the
PCHG device. This series extracts the count function from the PCHG
driver and uses it in the mfd driver to skip registration of the PCHG
device if there aren't any charger ports. This gets rid of the message
at boot and removes one struct device from my system.
Stephen Boyd (3):
platform/chrome: cros_ec_proto: Add peripheral charger count API
mfd: cros_ec_dev: Only register PCHG device if present
power: supply: PCHG: Use cros_ec_pchg_port_count()
drivers/mfd/cros_ec_dev.c | 16 ++++++++++
drivers/platform/chrome/cros_ec_proto.c | 31 +++++++++++++++++++
.../power/supply/cros_peripheral_charger.c | 29 +----------------
include/linux/platform_data/cros_ec_proto.h | 1 +
4 files changed, 49 insertions(+), 28 deletions(-)
Cc: Lee Jones <[email protected]>
Cc: Daisuke Nojiri <[email protected]>
Cc: Benson Leung <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: <[email protected]>
base-commit: 3123109284176b1532874591f7c81f3837bbdc17
--
https://chromeos.dev
Add a peripheral charger count API similar to the one implemented in the
ChromeOS PCHG driver so we can use it to decide whether or not to
register the PCHG device in the cros_ec MFD driver.
Cc: Lee Jones <[email protected]>
Cc: Daisuke Nojiri <[email protected]>
Cc: Benson Leung <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 31 +++++++++++++++++++++
include/linux/platform_data/cros_ec_proto.h | 1 +
2 files changed, 32 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index c4caf2e2de82..42269703ca6c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -832,6 +832,37 @@ bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
}
EXPORT_SYMBOL_GPL(cros_ec_check_features);
+/**
+ * cros_ec_pchg_port_count() - Return the number of peripheral charger ports.
+ * @ec: EC device.
+ *
+ * Return: Number of peripheral charger ports, or 0 in case of error.
+ */
+unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec)
+{
+ struct cros_ec_command *msg;
+ const struct ec_response_pchg_count *rsp;
+ struct cros_ec_device *ec_dev = ec->ec_dev;
+ int ret, count = 0;
+
+ msg = kzalloc(sizeof(*msg) + sizeof(*rsp), GFP_KERNEL);
+ if (!msg)
+ return 0;
+
+ msg->command = EC_CMD_PCHG_COUNT + ec->cmd_offset;
+ msg->insize = sizeof(*rsp);
+
+ ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ if (ret >= 0) {
+ rsp = (const struct ec_response_pchg_count *)msg->data;
+ count = rsp->port_count;
+ }
+ kfree(msg);
+
+ return count;
+}
+EXPORT_SYMBOL_GPL(cros_ec_pchg_port_count);
+
/**
* cros_ec_get_sensor_count() - Return the number of MEMS sensors supported.
*
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index df3c78c92ca2..8f5781bc2d7a 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -230,6 +230,7 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
+unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec);
int cros_ec_command(struct cros_ec_device *ec_dev, unsigned int version, int command, void *outdata,
int outsize, void *indata, int insize);
--
https://chromeos.dev
Use the common function cros_ec_pchg_port_count() now that we only
register this device when there are a non-zero number of peripheral
charger ports.
Cc: Lee Jones <[email protected]>
Cc: Daisuke Nojiri <[email protected]>
Cc: Benson Leung <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../power/supply/cros_peripheral_charger.c | 29 +------------------
1 file changed, 1 insertion(+), 28 deletions(-)
diff --git a/drivers/power/supply/cros_peripheral_charger.c b/drivers/power/supply/cros_peripheral_charger.c
index 9fe6d826148d..98cae6580eed 100644
--- a/drivers/power/supply/cros_peripheral_charger.c
+++ b/drivers/power/supply/cros_peripheral_charger.c
@@ -104,22 +104,6 @@ static bool cros_pchg_cmd_ver_check(const struct charger_data *charger)
return !!(rsp.version_mask & BIT(pchg_cmd_version));
}
-static int cros_pchg_port_count(const struct charger_data *charger)
-{
- struct ec_response_pchg_count rsp;
- int ret;
-
- ret = cros_pchg_ec_command(charger, 0, EC_CMD_PCHG_COUNT,
- NULL, 0, &rsp, sizeof(rsp));
- if (ret < 0) {
- dev_warn(charger->dev,
- "Unable to get number or ports (err:%d)\n", ret);
- return ret;
- }
-
- return rsp.port_count;
-}
-
static int cros_pchg_get_status(struct port_data *port)
{
struct charger_data *charger = port->charger;
@@ -281,24 +265,13 @@ static int cros_pchg_probe(struct platform_device *pdev)
charger->ec_dev = ec_dev;
charger->ec_device = ec_device;
- ret = cros_pchg_port_count(charger);
- if (ret <= 0) {
- /*
- * This feature is enabled by the EC and the kernel driver is
- * included by default for CrOS devices. Don't need to be loud
- * since this error can be normal.
- */
- dev_info(dev, "No peripheral charge ports (err:%d)\n", ret);
- return -ENODEV;
- }
-
if (!cros_pchg_cmd_ver_check(charger)) {
dev_err(dev, "EC_CMD_PCHG version %d isn't available.\n",
pchg_cmd_version);
return -EOPNOTSUPP;
}
- num_ports = ret;
+ num_ports = cros_ec_pchg_port_count(ec_dev);
if (num_ports > EC_PCHG_MAX_PORTS) {
dev_err(dev, "Too many peripheral charge ports (%d)\n",
num_ports);
--
https://chromeos.dev
On Thu, Apr 14, 2022 at 05:32:53PM -0700, Stephen Boyd wrote:
> Use the common function cros_ec_pchg_port_count() now that we only
> register this device when there are a non-zero number of peripheral
> charger ports.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daisuke Nojiri <[email protected]>
> Cc: Benson Leung <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
Share the same comment with the 1st patch in the series,
Reviewed-by: Tzung-Bi Shih <[email protected]>
On Thu, Apr 14, 2022 at 05:32:51PM -0700, Stephen Boyd wrote:
> Add a peripheral charger count API similar to the one implemented in the
> ChromeOS PCHG driver so we can use it to decide whether or not to
> register the PCHG device in the cros_ec MFD driver.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daisuke Nojiri <[email protected]>
> Cc: Benson Leung <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
With a minor comment about the naming,
Reviewed-by: Tzung-Bi Shih <[email protected]>
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index df3c78c92ca2..8f5781bc2d7a 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -230,6 +230,7 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
> bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
>
> int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
> +unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec);
I wonder if "cros_ec_get_pchg_port_count" would be a better name for the API.
On Apr 18 16:29, Stephen Boyd wrote:
> Quoting Prashant Malani (2022-04-18 16:21:52)
> > On Apr 18 16:16, Stephen Boyd wrote:
> > >
> > > Sure. I take it that I can drop this function entirely then?
> >
> > Yeah, if it's a simple response, should be fine.
> >
> > > BTW, why is
> > > that function name the same as a struct name? It confuses my ctags.
> >
> > Yeahhh, didn't think about ctags... :/
> > Topic for another series: probably can be renamed to cros_ec_cmd() (just to keep ctags happy) ?
> >
>
> But then there'll be two cros_ec_cmd() because there's a
> cros-ec-regulator. Fun! :)
Ugh :S
I think we can get rid of that one; it looks to be the same as this
one :)
I'll write up a cleanup series if it all looks OK.
On Apr 18 16:16, Stephen Boyd wrote:
> Quoting Prashant Malani (2022-04-18 16:08:39)
> > On Apr 14 17:32, Stephen Boyd wrote:
> > > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > > index c4caf2e2de82..42269703ca6c 100644
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -832,6 +832,37 @@ bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> > > }
> > > EXPORT_SYMBOL_GPL(cros_ec_check_features);
> > >
> > > +/**
> > > + * cros_ec_pchg_port_count() - Return the number of peripheral charger ports.
> > > + * @ec: EC device.
> > > + *
> > > + * Return: Number of peripheral charger ports, or 0 in case of error.
> > > + */
> > > +unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec)
> > > +{
> > > + struct cros_ec_command *msg;
> > > + const struct ec_response_pchg_count *rsp;
> > > + struct cros_ec_device *ec_dev = ec->ec_dev;
> > > + int ret, count = 0;
> > > +
> > > + msg = kzalloc(sizeof(*msg) + sizeof(*rsp), GFP_KERNEL);
> > > + if (!msg)
> > > + return 0;
> > > +
> > > + msg->command = EC_CMD_PCHG_COUNT + ec->cmd_offset;
> > > + msg->insize = sizeof(*rsp);
> > > +
> > > + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > > + if (ret >= 0) {
> > > + rsp = (const struct ec_response_pchg_count *)msg->data;
> > > + count = rsp->port_count;
> > > + }
> > > + kfree(msg);
> >
> > Can we use the wrapper function cros_ec_command() [1] here, which does
> > the kzalloc and msg construction?
> >
>
> Sure. I take it that I can drop this function entirely then?
Yeah, if it's a simple response, should be fine.
> BTW, why is
> that function name the same as a struct name? It confuses my ctags.
Yeahhh, didn't think about ctags... :/
Topic for another series: probably can be renamed to cros_ec_cmd() (just to keep ctags happy) ?
Best regards,
-Prashant
Quoting Prashant Malani (2022-04-18 16:08:39)
> On Apr 14 17:32, Stephen Boyd wrote:
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index c4caf2e2de82..42269703ca6c 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -832,6 +832,37 @@ bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> > }
> > EXPORT_SYMBOL_GPL(cros_ec_check_features);
> >
> > +/**
> > + * cros_ec_pchg_port_count() - Return the number of peripheral charger ports.
> > + * @ec: EC device.
> > + *
> > + * Return: Number of peripheral charger ports, or 0 in case of error.
> > + */
> > +unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec)
> > +{
> > + struct cros_ec_command *msg;
> > + const struct ec_response_pchg_count *rsp;
> > + struct cros_ec_device *ec_dev = ec->ec_dev;
> > + int ret, count = 0;
> > +
> > + msg = kzalloc(sizeof(*msg) + sizeof(*rsp), GFP_KERNEL);
> > + if (!msg)
> > + return 0;
> > +
> > + msg->command = EC_CMD_PCHG_COUNT + ec->cmd_offset;
> > + msg->insize = sizeof(*rsp);
> > +
> > + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> > + if (ret >= 0) {
> > + rsp = (const struct ec_response_pchg_count *)msg->data;
> > + count = rsp->port_count;
> > + }
> > + kfree(msg);
>
> Can we use the wrapper function cros_ec_command() [1] here, which does
> the kzalloc and msg construction?
>
Sure. I take it that I can drop this function entirely then? BTW, why is
that function name the same as a struct name? It confuses my ctags.
Hey Stephen,
On Apr 14 17:32, Stephen Boyd wrote:
> Add a peripheral charger count API similar to the one implemented in the
> ChromeOS PCHG driver so we can use it to decide whether or not to
> register the PCHG device in the cros_ec MFD driver.
>
> Cc: Lee Jones <[email protected]>
> Cc: Daisuke Nojiri <[email protected]>
> Cc: Benson Leung <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 31 +++++++++++++++++++++
> include/linux/platform_data/cros_ec_proto.h | 1 +
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index c4caf2e2de82..42269703ca6c 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -832,6 +832,37 @@ bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> }
> EXPORT_SYMBOL_GPL(cros_ec_check_features);
>
> +/**
> + * cros_ec_pchg_port_count() - Return the number of peripheral charger ports.
> + * @ec: EC device.
> + *
> + * Return: Number of peripheral charger ports, or 0 in case of error.
> + */
> +unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec)
> +{
> + struct cros_ec_command *msg;
> + const struct ec_response_pchg_count *rsp;
> + struct cros_ec_device *ec_dev = ec->ec_dev;
> + int ret, count = 0;
> +
> + msg = kzalloc(sizeof(*msg) + sizeof(*rsp), GFP_KERNEL);
> + if (!msg)
> + return 0;
> +
> + msg->command = EC_CMD_PCHG_COUNT + ec->cmd_offset;
> + msg->insize = sizeof(*rsp);
> +
> + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> + if (ret >= 0) {
> + rsp = (const struct ec_response_pchg_count *)msg->data;
> + count = rsp->port_count;
> + }
> + kfree(msg);
Can we use the wrapper function cros_ec_command() [1] here, which does
the kzalloc and msg construction?
Best regards,
-Prashant
[1] https://elixir.bootlin.com/linux/latest/source/drivers/platform/chrome/cros_ec_proto.c#L914
Quoting Prashant Malani (2022-04-18 16:31:57)
> On Apr 18 16:29, Stephen Boyd wrote:
> > Quoting Prashant Malani (2022-04-18 16:21:52)
> > > On Apr 18 16:16, Stephen Boyd wrote:
> > > >
> > > > Sure. I take it that I can drop this function entirely then?
> > >
> > > Yeah, if it's a simple response, should be fine.
> > >
> > > > BTW, why is
> > > > that function name the same as a struct name? It confuses my ctags.
> > >
> > > Yeahhh, didn't think about ctags... :/
> > > Topic for another series: probably can be renamed to cros_ec_cmd() (just to keep ctags happy) ?
> > >
> >
> > But then there'll be two cros_ec_cmd() because there's a
> > cros-ec-regulator. Fun! :)
>
> Ugh :S
>
> I think we can get rid of that one; it looks to be the same as this
> one :)
>
>
> I'll write up a cleanup series if it all looks OK.
>
Cool thanks! Would be useful to change those insize and outsize sizes to
size_t as well. Please Cc me on the series. I'll resend this series with
cros_ec_command() shortly.
Quoting Prashant Malani (2022-04-18 16:21:52)
> On Apr 18 16:16, Stephen Boyd wrote:
> >
> > Sure. I take it that I can drop this function entirely then?
>
> Yeah, if it's a simple response, should be fine.
>
> > BTW, why is
> > that function name the same as a struct name? It confuses my ctags.
>
> Yeahhh, didn't think about ctags... :/
> Topic for another series: probably can be renamed to cros_ec_cmd() (just to keep ctags happy) ?
>
But then there'll be two cros_ec_cmd() because there's a
cros-ec-regulator. Fun! :)
Quoting Tzung-Bi Shih (2022-04-17 20:23:27)
> On Thu, Apr 14, 2022 at 05:32:51PM -0700, Stephen Boyd wrote:
> > Add a peripheral charger count API similar to the one implemented in the
> > ChromeOS PCHG driver so we can use it to decide whether or not to
> > register the PCHG device in the cros_ec MFD driver.
> >
> > Cc: Lee Jones <[email protected]>
> > Cc: Daisuke Nojiri <[email protected]>
> > Cc: Benson Leung <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
>
> With a minor comment about the naming,
> Reviewed-by: Tzung-Bi Shih <[email protected]>
Cool thanks.
>
> > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > index df3c78c92ca2..8f5781bc2d7a 100644
> > --- a/include/linux/platform_data/cros_ec_proto.h
> > +++ b/include/linux/platform_data/cros_ec_proto.h
> > @@ -230,6 +230,7 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
> > bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
> >
> > int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
> > +unsigned int cros_ec_pchg_port_count(struct cros_ec_dev *ec);
>
> I wonder if "cros_ec_get_pchg_port_count" would be a better name for the API.
Sure. I left out 'get' even though it's similar to
cros_ec_get_sensor_count() because it seemed redundant. We can't "set"
the port count. Anyway, I don't really care so I'll leave it up to cros
maintainers.