Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
Quoting Stephen Boyd (2020-09-10 17:53:07)
> Quoting Enric Balletbo i Serra (2020-09-10 08:49:42)
> > On 10/9/20 16:52, Guenter Roeck wrote:
> > > On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
> > > wrote:
> > >> On 10/9/20 16:18, Guenter Roeck wrote:
> > >>> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
> > @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device
> > *pdev)
> > }
> > }
> >
> > + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> > + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
> > >>>
> > >>> Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> > >>> There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> > >>> be used. It would be much more convenient if that feature check could
> > >>> be used instead of moving the get_lightbar_version command and its
> > >>> helper function around.
> > >>>
> > >>
> > >> IIRC it was to support a very old device, the Pixel Chromebook (Link).
> > >> This flag
> > >> is not set in this device but has a lightbar, hence we had this 'weird'
> > >> way to
> > >> detect the lightbar.
> > >>
> > >
> > > If that is the only reason, wouldn't it be better to use something
> > > else (eg dmi_match) to determine if the system in question is a Pixel
> > > Chromebook (Link) ?
> > >
> > > if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> > > (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
> > > dmi_match(DMI_PRODUCT_NAME, "Link")) {
> > >
> >
> > That looks a better solution, indeed. And definetely I'd prefer use the
> > check
> > features way.
> >
> > Gwendal, can you confirm that the Pixel Chromebook (Link) is the _only_ one
> > affected? This one is the only that comes to my mind but I might miss
> > others.
> >
> > I think that Samus has this flag (I can double check) and this was discussed
> > with you (long, long time ago :-) )
> >
>
> Sounds fine by me. I'll wait for Gwendal to inform us.
Anything come of this? I haven't seen any updates.
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
Quoting Enric Balletbo i Serra (2020-09-10 08:49:42)
> On 10/9/20 16:52, Guenter Roeck wrote:
> > On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
> > wrote:
> >> On 10/9/20 16:18, Guenter Roeck wrote:
> >>> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device
> *pdev)
> }
> }
>
> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
> >>>
> >>> Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> >>> There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> >>> be used. It would be much more convenient if that feature check could
> >>> be used instead of moving the get_lightbar_version command and its
> >>> helper function around.
> >>>
> >>
> >> IIRC it was to support a very old device, the Pixel Chromebook (Link).
> >> This flag
> >> is not set in this device but has a lightbar, hence we had this 'weird'
> >> way to
> >> detect the lightbar.
> >>
> >
> > If that is the only reason, wouldn't it be better to use something
> > else (eg dmi_match) to determine if the system in question is a Pixel
> > Chromebook (Link) ?
> >
> > if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> > (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
> > dmi_match(DMI_PRODUCT_NAME, "Link")) {
> >
>
> That looks a better solution, indeed. And definetely I'd prefer use the check
> features way.
>
> Gwendal, can you confirm that the Pixel Chromebook (Link) is the _only_ one
> affected? This one is the only that comes to my mind but I might miss others.
>
> I think that Samus has this flag (I can double check) and this was discussed
> with you (long, long time ago :-) )
>
Sounds fine by me. I'll wait for Gwendal to inform us.
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
Hi,
On 10/9/20 16:52, Guenter Roeck wrote:
> On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
> wrote:
>>
>> Hi,
>>
>> On 10/9/20 16:18, Guenter Roeck wrote:
>>> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
The cros ec lightbar driver has a check in probe to fail early if the ec
device isn't the cros_ec one or if it can't read the lightbar version
from the EC. Let's move this check to the place where the lightbar
device is registered. This way we don't expose devices that aren't
actually there on devices that don't have a lightbar.
This should improve memory and possibly performance too because struct
devices don't exactly have a small memory footprint and they contribute
to suspend/resume regardless of driver attachment.
Cc: Gwendal Grignou
Cc: Guenter Roeck
Cc: Lee Jones
Signed-off-by: Stephen Boyd
---
Changes from v1:
* Rebased on linux-next patches to this file
drivers/mfd/cros_ec_dev.c | 16 ++-
drivers/platform/chrome/cros_ec_lightbar.c | 102 ++--
drivers/platform/chrome/cros_ec_proto.c | 84
include/linux/platform_data/cros_ec_proto.h | 4 +
4 files changed, 111 insertions(+), 95 deletions(-)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index d07b43d7c761..9e98b2ec5d92 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
{ .name = "cros-ec-cec", },
};
+static const struct mfd_cell cros_ec_lightbar_cells[] = {
+ { .name = "cros-ec-lightbar", },
+};
+
static const struct mfd_cell cros_ec_rtc_cells[] = {
{ .name = "cros-ec-rtc", },
};
@@ -112,7 +116,6 @@ static const struct cros_feature_to_cells
cros_subdevices[] = {
static const struct mfd_cell cros_ec_platform_cells[] = {
{ .name = "cros-ec-chardev", },
{ .name = "cros-ec-debugfs", },
- { .name = "cros-ec-lightbar", },
{ .name = "cros-ec-sysfs", },
};
@@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device
*pdev)
}
}
+ if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
+ !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
>>>
>>> Any idea why the lightbar code doesn't use cros_ec_check_features() ?
>>> There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
>>> be used. It would be much more convenient if that feature check could
>>> be used instead of moving the get_lightbar_version command and its
>>> helper function around.
>>>
>>
>> IIRC it was to support a very old device, the Pixel Chromebook (Link). This
>> flag
>> is not set in this device but has a lightbar, hence we had this 'weird' way
>> to
>> detect the lightbar.
>>
>
> If that is the only reason, wouldn't it be better to use something
> else (eg dmi_match) to determine if the system in question is a Pixel
> Chromebook (Link) ?
>
> if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
> dmi_match(DMI_PRODUCT_NAME, "Link")) {
>
That looks a better solution, indeed. And definetely I'd prefer use the check
features way.
Gwendal, can you confirm that the Pixel Chromebook (Link) is the _only_ one
affected? This one is the only that comes to my mind but I might miss others.
I think that Samus has this flag (I can double check) and this was discussed
with you (long, long time ago :-) )
Thanks,
Enric
> Thanks,
> Guenter
>
>>> Thanks,
>>> Guenter
>>>
+ retval = mfd_add_hotplug_devices(ec->dev,
+ cros_ec_lightbar_cells,
+ ARRAY_SIZE(cros_ec_lightbar_cells));
+ if (retval)
+ dev_err(ec->dev,
+ "failed to add lightbar device: %d\n",
+ retval);
+ }
+
/*
* The PD notifier driver cell is separate since it only needs to
be
* explicitly added on platforms that don't have the PD notifier
ACPI
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c
b/drivers/platform/chrome/cros_ec_lightbar.c
index de8dfb12e486..a7cfde90c8e5 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -82,77 +82,6 @@ static int lb_throttle(void)
return ret;
}
-static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev
*ec)
-{
- struct cros_ec_command *msg;
- int len;
-
- len
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
wrote:
>
> Hi,
>
> On 10/9/20 16:18, Guenter Roeck wrote:
> > On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
> >>
> >> The cros ec lightbar driver has a check in probe to fail early if the ec
> >> device isn't the cros_ec one or if it can't read the lightbar version
> >> from the EC. Let's move this check to the place where the lightbar
> >> device is registered. This way we don't expose devices that aren't
> >> actually there on devices that don't have a lightbar.
> >>
> >> This should improve memory and possibly performance too because struct
> >> devices don't exactly have a small memory footprint and they contribute
> >> to suspend/resume regardless of driver attachment.
> >>
> >> Cc: Gwendal Grignou
> >> Cc: Guenter Roeck
> >> Cc: Lee Jones
> >> Signed-off-by: Stephen Boyd
> >> ---
> >>
> >> Changes from v1:
> >> * Rebased on linux-next patches to this file
> >>
> >> drivers/mfd/cros_ec_dev.c | 16 ++-
> >> drivers/platform/chrome/cros_ec_lightbar.c | 102 ++--
> >> drivers/platform/chrome/cros_ec_proto.c | 84
> >> include/linux/platform_data/cros_ec_proto.h | 4 +
> >> 4 files changed, 111 insertions(+), 95 deletions(-)
> >>
> >> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> >> index d07b43d7c761..9e98b2ec5d92 100644
> >> --- a/drivers/mfd/cros_ec_dev.c
> >> +++ b/drivers/mfd/cros_ec_dev.c
> >> @@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
> >> { .name = "cros-ec-cec", },
> >> };
> >>
> >> +static const struct mfd_cell cros_ec_lightbar_cells[] = {
> >> + { .name = "cros-ec-lightbar", },
> >> +};
> >> +
> >> static const struct mfd_cell cros_ec_rtc_cells[] = {
> >> { .name = "cros-ec-rtc", },
> >> };
> >> @@ -112,7 +116,6 @@ static const struct cros_feature_to_cells
> >> cros_subdevices[] = {
> >> static const struct mfd_cell cros_ec_platform_cells[] = {
> >> { .name = "cros-ec-chardev", },
> >> { .name = "cros-ec-debugfs", },
> >> - { .name = "cros-ec-lightbar", },
> >> { .name = "cros-ec-sysfs", },
> >> };
> >>
> >> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device
> >> *pdev)
> >> }
> >> }
> >>
> >> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> >> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
> >
> > Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> > There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> > be used. It would be much more convenient if that feature check could
> > be used instead of moving the get_lightbar_version command and its
> > helper function around.
> >
>
> IIRC it was to support a very old device, the Pixel Chromebook (Link). This
> flag
> is not set in this device but has a lightbar, hence we had this 'weird' way to
> detect the lightbar.
>
If that is the only reason, wouldn't it be better to use something
else (eg dmi_match) to determine if the system in question is a Pixel
Chromebook (Link) ?
if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
(cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
dmi_match(DMI_PRODUCT_NAME, "Link")) {
Thanks,
Guenter
> > Thanks,
> > Guenter
> >
> >> + retval = mfd_add_hotplug_devices(ec->dev,
> >> + cros_ec_lightbar_cells,
> >> + ARRAY_SIZE(cros_ec_lightbar_cells));
> >> + if (retval)
> >> + dev_err(ec->dev,
> >> + "failed to add lightbar device: %d\n",
> >> + retval);
> >> + }
> >> +
> >> /*
> >> * The PD notifier driver cell is separate since it only needs to
> >> be
> >> * explicitly added on platforms that don't have the PD notifier
> >> ACPI
> >> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c
> >> b/drivers/platform/chrome/cros_ec_lightbar.c
> >> index de8dfb12e486..a7cfde90c8e5 100644
> >> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> >> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> >> @@ -82,77 +82,6 @@ static int lb_throttle(void)
> >> return ret;
> >> }
> >>
> >> -static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev
> >> *ec)
> >> -{
> >> - struct cros_ec_command *msg;
> >> - int len;
> >> -
> >> - len = max(sizeof(struct ec_params_lightbar),
> >> - sizeof(struct ec_response_lightbar));
> >> -
> >> - msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> >> - if (!msg)
> >> - return NULL;
> >> -
> >> - msg->version = 0;
> >> - msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
> >> - msg->outsize = sizeof(struct ec_params_lightbar);
> >> - msg->insize = sizeof(struct ec_response_lightbar);
> >> -
>
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
Hi,
On 10/9/20 16:18, Guenter Roeck wrote:
> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
>>
>> The cros ec lightbar driver has a check in probe to fail early if the ec
>> device isn't the cros_ec one or if it can't read the lightbar version
>> from the EC. Let's move this check to the place where the lightbar
>> device is registered. This way we don't expose devices that aren't
>> actually there on devices that don't have a lightbar.
>>
>> This should improve memory and possibly performance too because struct
>> devices don't exactly have a small memory footprint and they contribute
>> to suspend/resume regardless of driver attachment.
>>
>> Cc: Gwendal Grignou
>> Cc: Guenter Roeck
>> Cc: Lee Jones
>> Signed-off-by: Stephen Boyd
>> ---
>>
>> Changes from v1:
>> * Rebased on linux-next patches to this file
>>
>> drivers/mfd/cros_ec_dev.c | 16 ++-
>> drivers/platform/chrome/cros_ec_lightbar.c | 102 ++--
>> drivers/platform/chrome/cros_ec_proto.c | 84
>> include/linux/platform_data/cros_ec_proto.h | 4 +
>> 4 files changed, 111 insertions(+), 95 deletions(-)
>>
>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
>> index d07b43d7c761..9e98b2ec5d92 100644
>> --- a/drivers/mfd/cros_ec_dev.c
>> +++ b/drivers/mfd/cros_ec_dev.c
>> @@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
>> { .name = "cros-ec-cec", },
>> };
>>
>> +static const struct mfd_cell cros_ec_lightbar_cells[] = {
>> + { .name = "cros-ec-lightbar", },
>> +};
>> +
>> static const struct mfd_cell cros_ec_rtc_cells[] = {
>> { .name = "cros-ec-rtc", },
>> };
>> @@ -112,7 +116,6 @@ static const struct cros_feature_to_cells
>> cros_subdevices[] = {
>> static const struct mfd_cell cros_ec_platform_cells[] = {
>> { .name = "cros-ec-chardev", },
>> { .name = "cros-ec-debugfs", },
>> - { .name = "cros-ec-lightbar", },
>> { .name = "cros-ec-sysfs", },
>> };
>>
>> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device *pdev)
>> }
>> }
>>
>> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
>> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
>
> Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> be used. It would be much more convenient if that feature check could
> be used instead of moving the get_lightbar_version command and its
> helper function around.
>
IIRC it was to support a very old device, the Pixel Chromebook (Link). This flag
is not set in this device but has a lightbar, hence we had this 'weird' way to
detect the lightbar.
> Thanks,
> Guenter
>
>> + retval = mfd_add_hotplug_devices(ec->dev,
>> + cros_ec_lightbar_cells,
>> + ARRAY_SIZE(cros_ec_lightbar_cells));
>> + if (retval)
>> + dev_err(ec->dev,
>> + "failed to add lightbar device: %d\n",
>> + retval);
>> + }
>> +
>> /*
>> * The PD notifier driver cell is separate since it only needs to be
>> * explicitly added on platforms that don't have the PD notifier ACPI
>> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c
>> b/drivers/platform/chrome/cros_ec_lightbar.c
>> index de8dfb12e486..a7cfde90c8e5 100644
>> --- a/drivers/platform/chrome/cros_ec_lightbar.c
>> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
>> @@ -82,77 +82,6 @@ static int lb_throttle(void)
>> return ret;
>> }
>>
>> -static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev
>> *ec)
>> -{
>> - struct cros_ec_command *msg;
>> - int len;
>> -
>> - len = max(sizeof(struct ec_params_lightbar),
>> - sizeof(struct ec_response_lightbar));
>> -
>> - msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
>> - if (!msg)
>> - return NULL;
>> -
>> - msg->version = 0;
>> - msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
>> - msg->outsize = sizeof(struct ec_params_lightbar);
>> - msg->insize = sizeof(struct ec_response_lightbar);
>> -
>> - return msg;
>> -}
>> -
>> -static int get_lightbar_version(struct cros_ec_dev *ec,
>> - uint32_t *ver_ptr, uint32_t *flg_ptr)
>> -{
>> - struct ec_params_lightbar *param;
>> - struct ec_response_lightbar *resp;
>> - struct cros_ec_command *msg;
>> - int ret;
>> -
>> - msg = alloc_lightbar_cmd_msg(ec);
>> - if (!msg)
>> - return 0;
>> -
>> - param = (struct ec_params_lightbar *)msg->data;
>> - param->cmd = LIGHTBAR_CMD_VERSION;
>> - msg->outsize = sizeof(param->cmd);
>> - msg->result = sizeof(resp->version);
>> - ret = cros_ec_cmd_xfer_status(ec->ec_dev,
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
>
> The cros ec lightbar driver has a check in probe to fail early if the ec
> device isn't the cros_ec one or if it can't read the lightbar version
> from the EC. Let's move this check to the place where the lightbar
> device is registered. This way we don't expose devices that aren't
> actually there on devices that don't have a lightbar.
>
> This should improve memory and possibly performance too because struct
> devices don't exactly have a small memory footprint and they contribute
> to suspend/resume regardless of driver attachment.
>
> Cc: Gwendal Grignou
> Cc: Guenter Roeck
> Cc: Lee Jones
> Signed-off-by: Stephen Boyd
> ---
>
> Changes from v1:
> * Rebased on linux-next patches to this file
>
> drivers/mfd/cros_ec_dev.c | 16 ++-
> drivers/platform/chrome/cros_ec_lightbar.c | 102 ++--
> drivers/platform/chrome/cros_ec_proto.c | 84
> include/linux/platform_data/cros_ec_proto.h | 4 +
> 4 files changed, 111 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index d07b43d7c761..9e98b2ec5d92 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
> { .name = "cros-ec-cec", },
> };
>
> +static const struct mfd_cell cros_ec_lightbar_cells[] = {
> + { .name = "cros-ec-lightbar", },
> +};
> +
> static const struct mfd_cell cros_ec_rtc_cells[] = {
> { .name = "cros-ec-rtc", },
> };
> @@ -112,7 +116,6 @@ static const struct cros_feature_to_cells
> cros_subdevices[] = {
> static const struct mfd_cell cros_ec_platform_cells[] = {
> { .name = "cros-ec-chardev", },
> { .name = "cros-ec-debugfs", },
> - { .name = "cros-ec-lightbar", },
> { .name = "cros-ec-sysfs", },
> };
>
> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device *pdev)
> }
> }
>
> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
Any idea why the lightbar code doesn't use cros_ec_check_features() ?
There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
be used. It would be much more convenient if that feature check could
be used instead of moving the get_lightbar_version command and its
helper function around.
Thanks,
Guenter
> + retval = mfd_add_hotplug_devices(ec->dev,
> + cros_ec_lightbar_cells,
> + ARRAY_SIZE(cros_ec_lightbar_cells));
> + if (retval)
> + dev_err(ec->dev,
> + "failed to add lightbar device: %d\n",
> + retval);
> + }
> +
> /*
> * The PD notifier driver cell is separate since it only needs to be
> * explicitly added on platforms that don't have the PD notifier ACPI
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c
> b/drivers/platform/chrome/cros_ec_lightbar.c
> index de8dfb12e486..a7cfde90c8e5 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -82,77 +82,6 @@ static int lb_throttle(void)
> return ret;
> }
>
> -static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev *ec)
> -{
> - struct cros_ec_command *msg;
> - int len;
> -
> - len = max(sizeof(struct ec_params_lightbar),
> - sizeof(struct ec_response_lightbar));
> -
> - msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> - if (!msg)
> - return NULL;
> -
> - msg->version = 0;
> - msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
> - msg->outsize = sizeof(struct ec_params_lightbar);
> - msg->insize = sizeof(struct ec_response_lightbar);
> -
> - return msg;
> -}
> -
> -static int get_lightbar_version(struct cros_ec_dev *ec,
> - uint32_t *ver_ptr, uint32_t *flg_ptr)
> -{
> - struct ec_params_lightbar *param;
> - struct ec_response_lightbar *resp;
> - struct cros_ec_command *msg;
> - int ret;
> -
> - msg = alloc_lightbar_cmd_msg(ec);
> - if (!msg)
> - return 0;
> -
> - param = (struct ec_params_lightbar *)msg->data;
> - param->cmd = LIGHTBAR_CMD_VERSION;
> - msg->outsize = sizeof(param->cmd);
> - msg->result = sizeof(resp->version);
> - ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> - if (ret < 0 && ret != -EINVAL) {
> - ret = 0;
> - goto exit;
> - }
> -
> - switch (msg->result) {
> - case EC_RES_INVALID_PARAM:
> - /* Pixel had no version command. */
> - if (ver_ptr)
> - *ver_ptr = 0;
> - if (flg_ptr)
> -
[PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
The cros ec lightbar driver has a check in probe to fail early if the ec
device isn't the cros_ec one or if it can't read the lightbar version
from the EC. Let's move this check to the place where the lightbar
device is registered. This way we don't expose devices that aren't
actually there on devices that don't have a lightbar.
This should improve memory and possibly performance too because struct
devices don't exactly have a small memory footprint and they contribute
to suspend/resume regardless of driver attachment.
Cc: Gwendal Grignou
Cc: Guenter Roeck
Cc: Lee Jones
Signed-off-by: Stephen Boyd
---
Changes from v1:
* Rebased on linux-next patches to this file
drivers/mfd/cros_ec_dev.c | 16 ++-
drivers/platform/chrome/cros_ec_lightbar.c | 102 ++--
drivers/platform/chrome/cros_ec_proto.c | 84
include/linux/platform_data/cros_ec_proto.h | 4 +
4 files changed, 111 insertions(+), 95 deletions(-)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index d07b43d7c761..9e98b2ec5d92 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
{ .name = "cros-ec-cec", },
};
+static const struct mfd_cell cros_ec_lightbar_cells[] = {
+ { .name = "cros-ec-lightbar", },
+};
+
static const struct mfd_cell cros_ec_rtc_cells[] = {
{ .name = "cros-ec-rtc", },
};
@@ -112,7 +116,6 @@ static const struct cros_feature_to_cells cros_subdevices[]
= {
static const struct mfd_cell cros_ec_platform_cells[] = {
{ .name = "cros-ec-chardev", },
{ .name = "cros-ec-debugfs", },
- { .name = "cros-ec-lightbar", },
{ .name = "cros-ec-sysfs", },
};
@@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device *pdev)
}
}
+ if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
+ !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
+ retval = mfd_add_hotplug_devices(ec->dev,
+ cros_ec_lightbar_cells,
+ ARRAY_SIZE(cros_ec_lightbar_cells));
+ if (retval)
+ dev_err(ec->dev,
+ "failed to add lightbar device: %d\n",
+ retval);
+ }
+
/*
* The PD notifier driver cell is separate since it only needs to be
* explicitly added on platforms that don't have the PD notifier ACPI
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c
b/drivers/platform/chrome/cros_ec_lightbar.c
index de8dfb12e486..a7cfde90c8e5 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -82,77 +82,6 @@ static int lb_throttle(void)
return ret;
}
-static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev *ec)
-{
- struct cros_ec_command *msg;
- int len;
-
- len = max(sizeof(struct ec_params_lightbar),
- sizeof(struct ec_response_lightbar));
-
- msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
- if (!msg)
- return NULL;
-
- msg->version = 0;
- msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
- msg->outsize = sizeof(struct ec_params_lightbar);
- msg->insize = sizeof(struct ec_response_lightbar);
-
- return msg;
-}
-
-static int get_lightbar_version(struct cros_ec_dev *ec,
- uint32_t *ver_ptr, uint32_t *flg_ptr)
-{
- struct ec_params_lightbar *param;
- struct ec_response_lightbar *resp;
- struct cros_ec_command *msg;
- int ret;
-
- msg = alloc_lightbar_cmd_msg(ec);
- if (!msg)
- return 0;
-
- param = (struct ec_params_lightbar *)msg->data;
- param->cmd = LIGHTBAR_CMD_VERSION;
- msg->outsize = sizeof(param->cmd);
- msg->result = sizeof(resp->version);
- ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
- if (ret < 0 && ret != -EINVAL) {
- ret = 0;
- goto exit;
- }
-
- switch (msg->result) {
- case EC_RES_INVALID_PARAM:
- /* Pixel had no version command. */
- if (ver_ptr)
- *ver_ptr = 0;
- if (flg_ptr)
- *flg_ptr = 0;
- ret = 1;
- goto exit;
-
- case EC_RES_SUCCESS:
- resp = (struct ec_response_lightbar *)msg->data;
-
- /* Future devices w/lightbars should implement this command */
- if (ver_ptr)
- *ver_ptr = resp->version.num;
- if (flg_ptr)
- *flg_ptr = resp->version.flags;
- ret = 1;
- goto exit;
- }
-
- /* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
- ret = 0;
-exit:
- kfree(msg);
- return
