Re: [PATCH RESEND v11] platform/chrome: Add ChromeOS ACPI device driver
From: Andy Shevchenko
Date: Tue May 10 2022 - 05:34:06 EST
On Tue, May 10, 2022 at 8:44 AM Muhammad Usama Anjum
<usama.anjum@xxxxxxxxxxxxx> wrote:
>
> From: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>
> 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 <rafael.j.wysocki@xxxxxxxxx>
> Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
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()?
...
> +#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.
> + 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?
> + 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.
> + };
> + // select sub element inside this package
Seems you have different comment styles over the same file. Please,
use /* ... */ here which seems what you wanted.
...
> +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;
> + ret = kstrtoint(&name[ACPI_ATTR_NAME_LEN + 1], 0, attr_num);
> +
> + return ret;
> +}
...
> +static ssize_t chromeos_first_level_attr_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + char attr_name[ACPI_ATTR_NAME_LEN + 1];
> + int ret, attr_num = 0;
> +
> + ret = parse_attr_name(attr->attr.name, attr_name, &attr_num);
> + if (ret)
> + return 0;
> + ret = chromeos_acpi_evaluate_method(dev, attr_num, 0, attr_name, buf);
> + if (ret < 0)
> + ret = 0;
Why is the error not reported?
> + return ret;
> +}
...
> +static unsigned int get_gpio_pkg_num(struct device *dev)
> +{
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + acpi_status status;
> + unsigned int count = 0;
> + char *name = "GPIO";
> +
> + status = acpi_evaluate_object(ACPI_HANDLE(dev), name, NULL, &output);
> + if (ACPI_FAILURE(status)) {
> + dev_err(dev, "failed to retrieve %s. %s\n", name, acpi_format_exception(status));
> + return count;
> + }
> +
> + if (((union acpi_object *)output.pointer)->type == ACPI_TYPE_PACKAGE)
> + count = ((union acpi_object *)output.pointer)->package.count;
Instead of doing ugly castings here, just use a temporary variable of
the correct type.
> + kfree(output.pointer);
> + return count;
> +}
...
> +/* Every platform can have different number of GPIO attribute groups.
a different
> + * Define upper limit groups. At run time, the platform decides to show
> + * the present number of groups only, others are hidden.
> + */
Comment style, see below.
...
> +static int chromeos_acpi_device_probe(struct platform_device *pdev)
> +{
> + chromeos_acpi_gpio_groups = get_gpio_pkg_num(&pdev->dev);
> + /* If platform has more GPIO attribute groups than the number of
If the platform
> + * groups this driver supports, give out a warning message.
> + */
/*
* This style is for network subsystem, we use
* this one.
*/
> + if (chromeos_acpi_gpio_groups > (ARRAY_SIZE(chromeos_acpi_all_groups) - 2))
> + dev_warn(&(pdev->dev), "Only %u GPIO attr groups supported by the driver out of total %u.\n",
In both lines too many parentheses.
> + (unsigned int)(ARRAY_SIZE(chromeos_acpi_all_groups) - 2),
Oh la la, instead of doing ugly castings, use proper specifiers, i.e. %zu.
> + chromeos_acpi_gpio_groups);
> + return 0;
> +}
...
> +static struct platform_driver chromeos_acpi_device_driver = {
> + .probe = chromeos_acpi_device_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .dev_groups = chromeos_acpi_all_groups,
> + .acpi_match_table = ACPI_PTR(chromeos_device_ids)
ACPI_PTR in most cases is not only useless, but might give a compiler warning.
> + }
> +};
--
With Best Regards,
Andy Shevchenko