| Message ID | 20220415003253.1973106-2-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: Only register PCHG if present | expand |
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 <lee.jones@linaro.org> > Cc: Daisuke Nojiri <dnojiri@chromium.org> > Cc: Benson Leung <bleung@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: <chrome-platform@lists.linux.dev> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> With a minor comment about the naming, Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org> > 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.
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 <lee.jones@linaro.org> > > Cc: Daisuke Nojiri <dnojiri@chromium.org> > > Cc: Benson Leung <bleung@chromium.org> > > Cc: Guenter Roeck <groeck@chromium.org> > > Cc: <chrome-platform@lists.linux.dev> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > With a minor comment about the naming, > Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org> 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.
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 <lee.jones@linaro.org> > Cc: Daisuke Nojiri <dnojiri@chromium.org> > Cc: Benson Leung <bleung@chromium.org> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: <chrome-platform@lists.linux.dev> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > 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: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.
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: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! :)
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.
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.
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);
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 <lee.jones@linaro.org> Cc: Daisuke Nojiri <dnojiri@chromium.org> Cc: Benson Leung <bleung@chromium.org> Cc: Guenter Roeck <groeck@chromium.org> Cc: <chrome-platform@lists.linux.dev> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/platform/chrome/cros_ec_proto.c | 31 +++++++++++++++++++++ include/linux/platform_data/cros_ec_proto.h | 1 + 2 files changed, 32 insertions(+)