lkml.org 
[lkml]   [2018]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 7/8] platform/chrome: cros_ec_lightbar: instantiate only if the EC has a lightbar.
    On Tue, Nov 27, 2018 at 4:19 AM Enric Balletbo i Serra
    <enric.balletbo@collabora.com> wrote:
    >
    > Due to the way attribute groups visibility work, the function
    > cros_ec_lightbar_attrs_are_visible is called multiple times, once per
    > attribute, and each of these calls makes an EC transaction. For what is
    > worth the EC log reports multiple errors on boot when the lightbar is
    > not available. Instead, check if the EC has a lightbar in the probe
    > function and only instantiate the device.
    >
    > Ideally we should have instantiate the driver only if the
    > EC_FEATURE_LIGHTBAR is defined, but that's not possible because that flag
    > is not in the very first Pixel Chromebook (Link), only on Samus. So, the
    > driver is instantiated by his parent always.
    >
    > This patch changes a bit the actual behaviour. Before the patch if an EC
    > doesn't have a lightbar an empty lightbar folder is created in
    > /sys/class/chromeos/<ec device>, after the patch the empty folder is not
    > created, so, the folder is only created if the lightbar exists.
    >
    > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

    Nitpicks, otherwise

    Reviewed-by: Guenter Roeck <groeck@chromium.org>

    > ---
    >
    > Changes in v3: None
    > Changes in v2:
    > - Removed ec_with_lightbar variable.
    >
    > drivers/platform/chrome/cros_ec_lightbar.c | 37 ++++++++--------------
    > 1 file changed, 13 insertions(+), 24 deletions(-)
    >
    > diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
    > index 80eed6317570..6f6a4cbfc3fc 100644
    > --- a/drivers/platform/chrome/cros_ec_lightbar.c
    > +++ b/drivers/platform/chrome/cros_ec_lightbar.c
    > @@ -43,7 +43,6 @@ static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
    > * If this is true, we won't do anything during suspend/resume.
    > */
    > static bool userspace_control;
    > -static struct cros_ec_dev *ec_with_lightbar;
    >
    > static ssize_t interval_msec_show(struct device *dev,
    > struct device_attribute *attr, char *buf)
    > @@ -381,9 +380,6 @@ static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
    > struct cros_ec_command *msg;
    > int ret;
    >
    > - if (ec != ec_with_lightbar)
    > - return 0;
    > -
    > msg = alloc_lightbar_cmd_msg(ec);
    > if (!msg)
    > return -ENOMEM;
    > @@ -567,37 +563,26 @@ static struct attribute *__lb_cmds_attrs[] = {
    > NULL,
    > };
    >
    > -static bool ec_has_lightbar(struct cros_ec_dev *ec)
    > +static bool cros_ec_has_lightbar(struct cros_ec_dev *ec_dev)
    > {
    > - return !!get_lightbar_version(ec, NULL, NULL);
    > -}
    > -
    > -static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
    > - struct attribute *a, int n)
    > -{
    > - struct device *dev = container_of(kobj, struct device, kobj);
    > - struct cros_ec_dev *ec = to_cros_ec_dev(dev);
    > - struct platform_device *pdev = to_platform_device(ec->dev);
    > + struct platform_device *pdev = to_platform_device(ec_dev->dev);
    > struct cros_ec_platform *pdata = pdev->dev.platform_data;
    > int is_cros_ec;
    >
    > is_cros_ec = strcmp(pdata->ec_name, CROS_EC_DEV_NAME);
    >
    > if (is_cros_ec != 0)
    > - return 0;
    > + return false;
    >
    FWIW, is_cros_ec is really unnecessary here, though I guess that might
    be a different patch.

    > - /* Only instantiate this stuff if the EC has a lightbar */
    > - if (ec_has_lightbar(ec)) {
    > - ec_with_lightbar = ec;
    > - return a->mode;
    > - }
    > - return 0;
    > + if (!!get_lightbar_version(ec_dev, NULL, NULL))
    > + return true;
    > +
    > + return false;

    Why not just the following ?
    return !!get_lightbar_version(ec_dev, NULL, NULL);
    or even
    return !strcmp(pdata->ec_name, CROS_EC_DEV_NAME) &&
    !!get_lightbar_version(ec_dev, NULL, NULL);

    > }
    >
    > struct attribute_group cros_ec_lightbar_attr_group = {
    > .name = "lightbar",
    > .attrs = __lb_cmds_attrs,
    > - .is_visible = cros_ec_lightbar_attrs_are_visible,
    > };
    >
    > static int cros_ec_lightbar_probe(struct platform_device *pd)
    > @@ -606,6 +591,10 @@ static int cros_ec_lightbar_probe(struct platform_device *pd)
    > struct device *dev = &pd->dev;
    > int ret;
    >
    > + /* Only instantiate this stuff if the EC has a lightbar */
    > + if (!cros_ec_has_lightbar(ec_dev))
    > + return -ENODEV;
    > +
    > /* Take control of the lightbar from the EC. */
    > lb_manual_suspend_ctrl(ec_dev, 1);
    >
    > @@ -635,7 +624,7 @@ static int __maybe_unused cros_ec_lightbar_resume(struct device *dev)
    > {
    > struct cros_ec_dev *ec_dev = dev_get_drvdata(dev);
    >
    > - if (userspace_control || ec_dev != ec_with_lightbar)
    > + if (userspace_control)
    > return 0;
    >
    > return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_RESUME);
    > @@ -645,7 +634,7 @@ static int __maybe_unused cros_ec_lightbar_suspend(struct device *dev)
    > {
    > struct cros_ec_dev *ec_dev = dev_get_drvdata(dev);
    >
    > - if (userspace_control || ec_dev != ec_with_lightbar)
    > + if (userspace_control)
    > return 0;
    >
    > return lb_send_empty_cmd(ec_dev, LIGHTBAR_CMD_SUSPEND);
    > --
    > 2.19.1
    >

    \
     
     \ /
      Last update: 2018-12-10 19:21    [W:3.586 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site