Hi Stephen,

thanks for your patch!

Overall it looks good I have a few nitpicks

On Sat, Feb 10, 2024 at 8:09 AM Stephen Boyd [off-list ref] wrote:

The ChromeOS embedded controller (EC) supports setting the state of
GPIOs when the system is unlocked, and getting the state of GPIOs in all
cases. The GPIOs are on the EC itself, so the EC acts similar to a GPIO
expander. Add a driver to get and set the GPIOs on the EC through the
host command interface.

Cc: Linus Walleij <redacted>
Cc: Bartosz Golaszewski <redacted>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: <redacted>
Cc: <redacted>
Cc: Pin-yen Lin <redacted>
Signed-off-by: Stephen Boyd <redacted>

(...)

+config GPIO_CROS_EC
+       tristate "ChromeOS EC GPIO support"
+       depends on CROS_EC
+       help
+         GPIO driver for exposing GPIOs on the ChromeOS Embedded
+         Controller.
+
+         This driver can also be built as a module. If so, the module
+         will be called gpio-cros-ec.

Classified as "MFD Expander" but I honestly don't know anything better.

+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>

Do you need init.h?
I guess maybe... I thought module would be enough for this.

+static int cros_ec_gpio_request(struct gpio_chip *chip, unsigned gpio_pin)
+{
+       if (gpio_pin < chip->ngpio)
+               return 0;
+
+       return -EINVAL;
+}

If this check is needed, it should be in gpiolib I think?

+#ifdef CONFIG_OF

This ifdef should not be needed these days.

+static const struct of_device_id cros_ec_gpio_of_match[] = {
+       { .compatible = "google,cros-ec-gpio" },
+       {}
+};
+MODULE_DEVICE_TABLE(of, cros_ec_gpio_of_match);
+#endif
+
+static struct platform_driver cros_ec_gpio_driver = {
+       .probe = cros_ec_gpio_probe,
+       .driver = {
+               .name = "cros-ec-gpio",
+               .of_match_table = of_match_ptr(cros_ec_gpio_of_match),

Nor the of_match_ptr() business.

With these fixed/addressed:
Reviewed-by: Linus Walleij <redacted>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel