From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 184D8C433EF for ; Wed, 11 May 2022 16:23:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344662AbiEKQXo (ORCPT ); Wed, 11 May 2022 12:23:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60530 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244060AbiEKQXj (ORCPT ); Wed, 11 May 2022 12:23:39 -0400 Received: from mail-ej1-x636.google.com (mail-ej1-x636.google.com [IPv6:2a00:1450:4864:20::636]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D88E60BA1 for ; Wed, 11 May 2022 09:23:38 -0700 (PDT) Received: by mail-ej1-x636.google.com with SMTP id m20so5078812ejj.10 for ; Wed, 11 May 2022 09:23:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=POdbBUuLUOeD8dnJi9C2So78rhobliTsIgeT1Jg08bk=; b=gbgLI2yp/3VCBc7DlaQPxNdeiz65WMkVuJXn1mPLH0mQQ1ZRV2z74XHm/flUlawuiI Vf1/8rHCM/p0dFcR/CxKOJPqxWMPYXUtZYqUZvVeTRrYUur/kAi0pk3APCvJZuSE1dIh mF5e4Xcl+DYTO6tg7veoz5qFwfthauZyE+Kn2wDQIcxkHbLInY8Qg2vj3/7AEfRF84Df S6FZXxkasSWS0s47QTiqqjptEEojp7c9ACoJP1KD7xp0QfvNQJTgbpMtFEjyU2/oQykk 9h1BNdmfXmpA2f/Z8q6G2hgoloXuy5lrshBqAgxpQBu/NeRfVzyf6Zy3v6IkntTsFzcL 5DSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=POdbBUuLUOeD8dnJi9C2So78rhobliTsIgeT1Jg08bk=; b=cA+SR1iWvzTNcOZ2TN0R3clnC2N2xUqtF3e759tbGW6TE4IH7TgQXCdB3tMZjYXHcY koPj25Hm9V92im3NULzcWoBH1Eu7Ys9fQWRgfj1ZpBXEZIw6wnduRUXowfQy0RYpkjQT aMOtl5XyxMMXs9GE6jfJBvfVBoLSqT7ab0oZf+8LMb1AVT1fQFe8WjzgW2HuvmW2v2bo XpI6RItRIKRT5+oqWllkhPuXiSoP8GIjVFqj3vVdTylp5obPNe7ZqMxVqHHIYlOOIO0z /vX05wrRTPByjVWjFvafWDWoSPMpPK7BH9gkgJba4c0+r2tX7GXJgz28CBmaHuBiQ5fZ eISQ== X-Gm-Message-State: AOAM530NggV6U8AY/dYFhGD6hSTd0p0wYjL3/dFDxkit2MNmJgTw2H60 Xk8rONjiwhSwFf/mEBc9Q4e+0lYzLGcvSnkRrF+PRg== X-Google-Smtp-Source: ABdhPJxVpcJI5J1XjN8yl3eLnyQ1rHrvKD4tRUFk0C2I3SD6mc/x46SCPzGa2lPgn91Y3rMniYGyUgadmrlhuQUO0HQ= X-Received: by 2002:a17:907:9711:b0:6f4:6989:6afb with SMTP id jg17-20020a170907971100b006f469896afbmr26603981ejc.618.1652286216547; Wed, 11 May 2022 09:23:36 -0700 (PDT) MIME-Version: 1.0 References: <8bd83f45-5278-e817-3f65-88fafd0ad3f4@collabora.com> In-Reply-To: <8bd83f45-5278-e817-3f65-88fafd0ad3f4@collabora.com> From: Guenter Roeck Date: Wed, 11 May 2022 09:23:24 -0700 Message-ID: Subject: Re: [PATCH RESEND v11] platform/chrome: Add ChromeOS ACPI device driver To: Muhammad Usama Anjum Cc: Andy Shevchenko , "Rafael J. Wysocki" , Len Brown , Hans de Goede , Mark Gross , Benson Leung , Enric Balletbo i Serra , Greg Kroah-Hartman , Collabora Kernel ML , Guenter Roeck , Dmitry Torokhov , Gwendal Grignou , vbendeb@chromium.org, Andy Shevchenko , Ayman Bagabas , Benjamin Tissoires , =?UTF-8?Q?Bla=C5=BE_Hrastnik?= , Darren Hart , Dmitry Torokhov , Jeremy Soller , Mattias Jacobsson <2pi@mok.nu>, Mauro Carvalho Chehab , Rajat Jain , Srinivas Pandruvada , Platform Driver , Linux Kernel Mailing List , ACPI Devel Maling List , "Rafael J . Wysocki" , chrome-platform@lists.linux.dev Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 11, 2022 at 8:59 AM Muhammad Usama Anjum wrote: > > Hi Andy, > > Thank you for reviewing. > > On 5/10/22 2:33 PM, Andy Shevchenko wrote: > > On Tue, May 10, 2022 at 8:44 AM Muhammad Usama Anjum > > wrote: > >> > >> From: Enric Balletbo i Serra > >> > >> The x86 Chromebooks have the ChromeOS ACPI device. This driver attaches > >> to the ChromeOS ACPI device and exports the values reported by ACPI in a > >> sysfs directory. This data isn't present in ACPI tables when read > >> through ACPI tools, hence a driver is needed to do it. The driver gets > >> data from firmware using the ACPI component of the kernel. The ACPI values > >> are presented in string form (numbers as decimal values) or binary > >> blobs, and can be accessed as the contents of the appropriate read only > >> files in the standard ACPI device's sysfs directory tree. This data is > >> consumed by the ChromeOS user space. > > > >> Cc: Rafael J. Wysocki > >> Cc: Dmitry Torokhov > >> Cc: Hans de Goede > > > > You can use --cc parameter to `git send-email` instead of putting > > these lines in the commit message. > > > > ... > > > >> +#define DEV_ATTR(_var, _name) \ > >> + static struct device_attribute dev_attr_##_var = \ > >> + __ATTR(_name, 0444, chromeos_first_level_attr_show, NULL); > >> + > > > > Why not ATTR_RO()? > It'll not work as attribute name has . in it. > > > > > ... > > > >> +#define GPIO_ATTR_GROUP(_group, _name, _num) \ > >> + static umode_t attr_is_visible_gpio_##_num(struct kobject *kobj, \ > >> + struct attribute *attr, int n) \ > >> + { \ > >> + if (_num < chromeos_acpi_gpio_groups) \ > >> + return attr->mode; \ > > > >> + else \ > > > > Redundant. > We are deciding on run time that how many GPIO attribute groups need to > be shown. chromeos_acpi_gpio_groups is set at run time. I don't see why > `else` can be redundant here. > else after return is _always_ unnecessary (and results in static analyzer messages). > > > >> + return 0; \ > >> + } \ > >> + static ssize_t chromeos_attr_show_gpio_##_num(struct device *dev, \ > >> + struct device_attribute *attr, \ > >> + char *buf) \ > >> + { \ > >> + char name[ACPI_ATTR_NAME_LEN + 1]; \ > >> + int ret, num; \ > >> + \ > >> + ret = parse_attr_name(attr->attr.name, name, &num); \ > >> + if (ret) \ > >> + return ret; \ > > > >> + ret = chromeos_acpi_evaluate_method(dev, _num, num, name, buf); \ > >> + if (ret < 0) \ > >> + ret = 0; \ > > > > Below I saw the same code, why is the error ignored? > > > I'll return the error in both places. > > >> + return ret; \ > >> + } \ > >> + static struct device_attribute dev_attr_0_##_group = \ > >> + __ATTR(GPIO.0, 0444, chromeos_attr_show_gpio_##_num, NULL); \ > >> + static struct device_attribute dev_attr_1_##_group = \ > >> + __ATTR(GPIO.1, 0444, chromeos_attr_show_gpio_##_num, NULL); \ > >> + static struct device_attribute dev_attr_2_##_group = \ > >> + __ATTR(GPIO.2, 0444, chromeos_attr_show_gpio_##_num, NULL); \ > >> + static struct device_attribute dev_attr_3_##_group = \ > >> + __ATTR(GPIO.3, 0444, chromeos_attr_show_gpio_##_num, NULL); \ > >> + \ > >> + static struct attribute *attrs_##_group[] = { \ > >> + &dev_attr_0_##_group.attr, \ > >> + &dev_attr_1_##_group.attr, \ > >> + &dev_attr_2_##_group.attr, \ > >> + &dev_attr_3_##_group.attr, \ > >> + NULL \ > >> + }; \ > >> + static const struct attribute_group attr_group_##_group = { \ > >> + .name = _name, \ > >> + .is_visible = attr_is_visible_gpio_##_num, \ > > > >> + .attrs = attrs_##_group \ > > > > Keep a comma here. > Is there any particular reason for it? If there is, I'll add commas to > all the structures. > ... > > > > ... > > > >> +static int parse_attr_name(const char *name, char *attr_name, int *attr_num) > >> +{ > >> + int ret = 0; > >> + > >> + strscpy(attr_name, name, ACPI_ATTR_NAME_LEN + 1); > >> + > >> + if (strlen(name) > ACPI_ATTR_NAME_LEN) > > > > This seems strange, esp. taking into account that strscpy() returns that. > > > > int ret; > > > > ret = strscpy(...); > > if (ret == -E2BIG) > > return kstrtoint(...); > > > > return 0; > This is very nice way to do it. I'll update. > ... > > -- > Muhammad Usama Anjum