| Message ID | ZsV6MNS_tUPPSffJ@google.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | x86/platform/geode: switch GPIO buttons and LEDs to software properties | expand |
Hi Dmitry, On 8/21/24 7:25 AM, Dmitry Torokhov wrote: > Convert GPIO-connected buttons and LEDs in Geode boards to software > nodes/properties, so that support for platform data can be removed from > gpio-keys driver (which will rely purely on generic device properties > for configuration). > > To avoid repeating the same data structures over and over and over > factor them out into a new geode-common.c file. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Question has this been tested on at least 1 affected device ? Regards, Hans > --- > arch/x86/Kconfig | 6 + > arch/x86/platform/geode/Makefile | 1 + > arch/x86/platform/geode/alix.c | 82 ++--------- > arch/x86/platform/geode/geode-common.c | 180 +++++++++++++++++++++++++ > arch/x86/platform/geode/geode-common.h | 21 +++ > arch/x86/platform/geode/geos.c | 80 +---------- > arch/x86/platform/geode/net5501.c | 69 +--------- > 7 files changed, 230 insertions(+), 209 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 09f8fbcfe000..96b02e813332 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -3073,9 +3073,13 @@ config OLPC_XO15_SCI > - AC adapter status updates > - Battery status updates > > +config GEODE_COMMON > + bool > + > config ALIX > bool "PCEngines ALIX System Support (LED setup)" > select GPIOLIB > + select GEODE_COMMON > help > This option enables system support for the PCEngines ALIX. > At present this just sets up LEDs for GPIO control on > @@ -3090,12 +3094,14 @@ config ALIX > config NET5501 > bool "Soekris Engineering net5501 System Support (LEDS, GPIO, etc)" > select GPIOLIB > + select GEODE_COMMON > help > This option enables system support for the Soekris Engineering net5501. > > config GEOS > bool "Traverse Technologies GEOS System Support (LEDS, GPIO, etc)" > select GPIOLIB > + select GEODE_COMMON > depends on DMI > help > This option enables system support for the Traverse Technologies GEOS. > diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile > index a8a6b1dedb01..34b53e97a0ad 100644 > --- a/arch/x86/platform/geode/Makefile > +++ b/arch/x86/platform/geode/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_GEODE_COMMON) += geode-common.o > obj-$(CONFIG_ALIX) += alix.o > obj-$(CONFIG_NET5501) += net5501.o > obj-$(CONFIG_GEOS) += geos.o > diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c > index b39bf3b5e108..be65cd704e21 100644 > --- a/arch/x86/platform/geode/alix.c > +++ b/arch/x86/platform/geode/alix.c > @@ -18,15 +18,12 @@ > #include <linux/io.h> > #include <linux/string.h> > #include <linux/moduleparam.h> > -#include <linux/leds.h> > -#include <linux/platform_device.h> > -#include <linux/input.h> > -#include <linux/gpio_keys.h> > -#include <linux/gpio/machine.h> > #include <linux/dmi.h> > > #include <asm/geode.h> > > +#include "geode-common.h" > + > #define BIOS_SIGNATURE_TINYBIOS 0xf0000 > #define BIOS_SIGNATURE_COREBOOT 0x500 > #define BIOS_REGION_SIZE 0x10000 > @@ -41,79 +38,16 @@ module_param(force, bool, 0444); > /* FIXME: Award bios is not automatically detected as Alix platform */ > MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); > > -static struct gpio_keys_button alix_gpio_buttons[] = { > - { > - .code = KEY_RESTART, > - .gpio = 24, > - .active_low = 1, > - .desc = "Reset button", > - .type = EV_KEY, > - .wakeup = 0, > - .debounce_interval = 100, > - .can_disable = 0, > - } > -}; > -static struct gpio_keys_platform_data alix_buttons_data = { > - .buttons = alix_gpio_buttons, > - .nbuttons = ARRAY_SIZE(alix_gpio_buttons), > - .poll_interval = 20, > -}; > - > -static struct platform_device alix_buttons_dev = { > - .name = "gpio-keys-polled", > - .id = 1, > - .dev = { > - .platform_data = &alix_buttons_data, > - } > -}; > - > -static struct gpio_led alix_leds[] = { > - { > - .name = "alix:1", > - .default_trigger = "default-on", > - }, > - { > - .name = "alix:2", > - .default_trigger = "default-off", > - }, > - { > - .name = "alix:3", > - .default_trigger = "default-off", > - }, > -}; > - > -static struct gpio_led_platform_data alix_leds_data = { > - .num_leds = ARRAY_SIZE(alix_leds), > - .leds = alix_leds, > -}; > - > -static struct gpiod_lookup_table alix_leds_gpio_table = { > - .dev_id = "leds-gpio", > - .table = { > - /* The Geode GPIOs should be on the CS5535 companion chip */ > - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), > - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), > - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), > - { } > - }, > -}; > - > -static struct platform_device alix_leds_dev = { > - .name = "leds-gpio", > - .id = -1, > - .dev.platform_data = &alix_leds_data, > -}; > - > -static struct platform_device *alix_devs[] __initdata = { > - &alix_buttons_dev, > - &alix_leds_dev, > +static const struct geode_led alix_leds[] __initconst = { > + { 6, true }, > + { 25, false }, > + { 27, false }, > }; > > static void __init register_alix(void) > { > - /* Setup LED control through leds-gpio driver */ > - gpiod_add_lookup_table(&alix_leds_gpio_table); > - platform_add_devices(alix_devs, ARRAY_SIZE(alix_devs)); > + geode_create_restart_key(24); > + geode_create_leds("alix", alix_leds, ARRAY_SIZE(alix_leds)); > } > > static bool __init alix_present(unsigned long bios_phys, > diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c > new file mode 100644 > index 000000000000..8f365388cfbb > --- /dev/null > +++ b/arch/x86/platform/geode/geode-common.c > @@ -0,0 +1,180 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Shared helpers to register GPIO-connected buttons and LEDs > + * on AMD Geode boards. > + */ > + > +#include <linux/err.h> > +#include <linux/gpio/machine.h> > +#include <linux/gpio/property.h> > +#include <linux/input.h> > +#include <linux/leds.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > + > +#include "geode-common.h" > + > +const struct software_node geode_gpiochip_node = { > + .name = "cs5535-gpio", > +}; > + > +static const struct property_entry geode_gpio_keys_props[] = { > + PROPERTY_ENTRY_U32("poll-interval", 20), > + { } > +}; > + > +static const struct software_node geode_gpio_keys_node = { > + .name = "geode-gpio-keys", > + .properties = geode_gpio_keys_props, > +}; > + > +static struct property_entry geode_restart_key_props[] = { > + { /* Placeholder for GPIO property */ }, > + PROPERTY_ENTRY_U32("linux,code", KEY_RESTART), > + PROPERTY_ENTRY_STRING("label", "Reset button"), > + PROPERTY_ENTRY_U32("debounce-interval", 100), > + { } > +}; > + > +static const struct software_node geode_restart_key_node = { > + .parent = &geode_gpio_keys_node, > + .properties = geode_restart_key_props, > +}; > + > +static const struct software_node *geode_gpio_keys_swnodes[] __initconst = { > + &geode_gpiochip_node, > + &geode_gpio_keys_node, > + &geode_restart_key_node, > + NULL > +}; > + > +/* > + * Creates gpio-keys-polled device for the restart key. > + * > + * Note that it needs to be called first, before geode_create_leds(), > + * because it registers gpiochip software node used by both gpio-keys and > + * leds-gpio devices. > + */ > +int __init geode_create_restart_key(unsigned int pin) > +{ > + struct platform_device_info keys_info = { > + .name = "gpio-keys-polled", > + .id = 1, > + }; > + struct platform_device *pd; > + int err; > + > + geode_restart_key_props[0] = PROPERTY_ENTRY_GPIO("gpios", > + &geode_gpiochip_node, > + pin, GPIO_ACTIVE_LOW); > + > + err = software_node_register_node_group(geode_gpio_keys_swnodes); > + if (err) { > + pr_err("failed to register gpio-keys software nodes: %d\n", err); > + return err; > + } > + > + keys_info.fwnode = software_node_fwnode(&geode_gpio_keys_node); > + > + pd = platform_device_register_full(&keys_info); > + err = PTR_ERR_OR_ZERO(pd); > + if (err) { > + pr_err("failed to create gpio-keys device: %d\n", err); > + software_node_unregister_node_group(geode_gpio_keys_swnodes); > + return err; > + } > + > + return 0; > +} > + > +static const struct software_node geode_gpio_leds_node = { > + .name = "geode-leds", > +}; > + > +#define MAX_LEDS 3 > + > +int __init geode_create_leds(const char *label, const struct geode_led *leds, > + unsigned int n_leds) > +{ > + const struct software_node *group[MAX_LEDS + 2] = { 0 }; > + struct software_node *swnodes; > + struct property_entry *props; > + struct platform_device_info led_info = { > + .name = "leds-gpio", > + .id = PLATFORM_DEVID_NONE, > + }; > + struct platform_device *led_dev; > + const char *node_name; > + int err; > + int i; > + > + if (n_leds > MAX_LEDS) { > + pr_err("%s: too many LEDs\n", __func__); > + return -EINVAL; > + } > + > + swnodes = kcalloc(n_leds, sizeof(*swnodes), GFP_KERNEL); > + if (!swnodes) > + return -ENOMEM; > + > + /* > + * Each LED is represented by 3 properties: "gpios", > + * "linux,default-trigger", and am empty terminator. > + */ > + props = kcalloc(n_leds * 3, sizeof(*props), GFP_KERNEL); > + if (!props) { > + err = -ENOMEM; > + goto err_free_swnodes; > + } > + > + group[0] = &geode_gpio_leds_node; > + for (i = 0; i < n_leds; i++) { > + node_name = kasprintf(GFP_KERNEL, "%s:%d", label, i); > + if (!node_name) { > + err = -ENOMEM; > + goto err_free_names; > + } > + > + props[i * 3 + 0] = > + PROPERTY_ENTRY_GPIO("gpios", &geode_gpiochip_node, > + leds[i].pin, GPIO_ACTIVE_LOW); > + props[i * 3 + 1] = > + PROPERTY_ENTRY_STRING("linux,default-trigger", > + leds[i].default_on ? > + "default-on" : "default-off"); > + /* props[i * 3 + 2] is an empty terminator */ > + > + swnodes[i] = SOFTWARE_NODE(node_name, &props[i * 3], > + &geode_gpio_leds_node); > + group[i + 1] = &swnodes[i]; > + } > + > + err = software_node_register_node_group(group); > + if (err) { > + pr_err("failed to register LED software nodes: %d\n", err); > + goto err_free_names; > + } > + > + led_info.fwnode = software_node_fwnode(&geode_gpio_leds_node); > + > + led_dev = platform_device_register_full(&led_info); > + err = PTR_ERR_OR_ZERO(led_dev); > + if (err) { > + pr_err("failed to create LED device: %d\n", err); > + goto err_unregister_group; > + } > + > + return 0; > + > +err_unregister_group: > + software_node_unregister_node_group(group); > +err_free_names: > + while (--i >= 0) > + kfree(swnodes[i].name); > + kfree(props); > +err_free_swnodes: > + kfree(swnodes); > + return err; > +} > + > + > diff --git a/arch/x86/platform/geode/geode-common.h b/arch/x86/platform/geode/geode-common.h > new file mode 100644 > index 000000000000..9e0afd34bfad > --- /dev/null > +++ b/arch/x86/platform/geode/geode-common.h > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Shared helpers to register GPIO-connected buttons and LEDs > + * on AMD Geode boards. > + */ > + > +#ifndef __PLATFORM_GEODE_COMMON_H > +#define __PLATFORM_GEODE_COMMON_H > + > +#include <linux/property.h> > + > +struct geode_led { > + unsigned int pin; > + bool default_on; > +}; > + > +int geode_create_restart_key(unsigned int pin); > +int geode_create_leds(const char *label, const struct geode_led *leds, > + unsigned int n_leds); > + > +#endif /* __PLATFORM_GEODE_COMMON_H */ > diff --git a/arch/x86/platform/geode/geos.c b/arch/x86/platform/geode/geos.c > index d263528c90bb..98027fb1ec32 100644 > --- a/arch/x86/platform/geode/geos.c > +++ b/arch/x86/platform/geode/geos.c > @@ -16,88 +16,22 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/string.h> > -#include <linux/leds.h> > -#include <linux/platform_device.h> > -#include <linux/input.h> > -#include <linux/gpio_keys.h> > -#include <linux/gpio/machine.h> > #include <linux/dmi.h> > > #include <asm/geode.h> > > -static struct gpio_keys_button geos_gpio_buttons[] = { > - { > - .code = KEY_RESTART, > - .gpio = 3, > - .active_low = 1, > - .desc = "Reset button", > - .type = EV_KEY, > - .wakeup = 0, > - .debounce_interval = 100, > - .can_disable = 0, > - } > -}; > -static struct gpio_keys_platform_data geos_buttons_data = { > - .buttons = geos_gpio_buttons, > - .nbuttons = ARRAY_SIZE(geos_gpio_buttons), > - .poll_interval = 20, > -}; > - > -static struct platform_device geos_buttons_dev = { > - .name = "gpio-keys-polled", > - .id = 1, > - .dev = { > - .platform_data = &geos_buttons_data, > - } > -}; > - > -static struct gpio_led geos_leds[] = { > - { > - .name = "geos:1", > - .default_trigger = "default-on", > - }, > - { > - .name = "geos:2", > - .default_trigger = "default-off", > - }, > - { > - .name = "geos:3", > - .default_trigger = "default-off", > - }, > -}; > - > -static struct gpio_led_platform_data geos_leds_data = { > - .num_leds = ARRAY_SIZE(geos_leds), > - .leds = geos_leds, > -}; > - > -static struct gpiod_lookup_table geos_leds_gpio_table = { > - .dev_id = "leds-gpio", > - .table = { > - /* The Geode GPIOs should be on the CS5535 companion chip */ > - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), > - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), > - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), > - { } > - }, > -}; > - > -static struct platform_device geos_leds_dev = { > - .name = "leds-gpio", > - .id = -1, > - .dev.platform_data = &geos_leds_data, > -}; > +#include "geode-common.h" > > -static struct platform_device *geos_devs[] __initdata = { > - &geos_buttons_dev, > - &geos_leds_dev, > +static const struct geode_led geos_leds[] __initconst = { > + { 6, true }, > + { 25, false }, > + { 27, false }, > }; > > static void __init register_geos(void) > { > - /* Setup LED control through leds-gpio driver */ > - gpiod_add_lookup_table(&geos_leds_gpio_table); > - platform_add_devices(geos_devs, ARRAY_SIZE(geos_devs)); > + geode_create_restart_key(3); > + geode_create_leds("geos", geos_leds, ARRAY_SIZE(geos_leds)); > } > > static int __init geos_init(void) > diff --git a/arch/x86/platform/geode/net5501.c b/arch/x86/platform/geode/net5501.c > index 558384acd777..c9cee7dea99b 100644 > --- a/arch/x86/platform/geode/net5501.c > +++ b/arch/x86/platform/geode/net5501.c > @@ -16,80 +16,25 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/string.h> > -#include <linux/leds.h> > -#include <linux/platform_device.h> > #include <linux/input.h> > -#include <linux/gpio_keys.h> > #include <linux/gpio/machine.h> > +#include <linux/gpio/property.h> > > #include <asm/geode.h> > > +#include "geode-common.h" > + > #define BIOS_REGION_BASE 0xffff0000 > #define BIOS_REGION_SIZE 0x00010000 > > -static struct gpio_keys_button net5501_gpio_buttons[] = { > - { > - .code = KEY_RESTART, > - .gpio = 24, > - .active_low = 1, > - .desc = "Reset button", > - .type = EV_KEY, > - .wakeup = 0, > - .debounce_interval = 100, > - .can_disable = 0, > - } > -}; > -static struct gpio_keys_platform_data net5501_buttons_data = { > - .buttons = net5501_gpio_buttons, > - .nbuttons = ARRAY_SIZE(net5501_gpio_buttons), > - .poll_interval = 20, > -}; > - > -static struct platform_device net5501_buttons_dev = { > - .name = "gpio-keys-polled", > - .id = 1, > - .dev = { > - .platform_data = &net5501_buttons_data, > - } > -}; > - > -static struct gpio_led net5501_leds[] = { > - { > - .name = "net5501:1", > - .default_trigger = "default-on", > - }, > -}; > - > -static struct gpio_led_platform_data net5501_leds_data = { > - .num_leds = ARRAY_SIZE(net5501_leds), > - .leds = net5501_leds, > -}; > - > -static struct gpiod_lookup_table net5501_leds_gpio_table = { > - .dev_id = "leds-gpio", > - .table = { > - /* The Geode GPIOs should be on the CS5535 companion chip */ > - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_HIGH), > - { } > - }, > -}; > - > -static struct platform_device net5501_leds_dev = { > - .name = "leds-gpio", > - .id = -1, > - .dev.platform_data = &net5501_leds_data, > -}; > - > -static struct platform_device *net5501_devs[] __initdata = { > - &net5501_buttons_dev, > - &net5501_leds_dev, > +static const struct geode_led net5501_leds[] __initconst = { > + { 6, true }, > }; > > static void __init register_net5501(void) > { > - /* Setup LED control through leds-gpio driver */ > - gpiod_add_lookup_table(&net5501_leds_gpio_table); > - platform_add_devices(net5501_devs, ARRAY_SIZE(net5501_devs)); > + geode_create_restart_key(24); > + geode_create_leds("net5501", net5501_leds, ARRAY_SIZE(net5501_leds)); > } > > struct net5501_board {
On Wed, Aug 21, 2024 at 12:15:51PM +0200, Hans de Goede wrote: > Hi Dmitry, > > On 8/21/24 7:25 AM, Dmitry Torokhov wrote: > > Convert GPIO-connected buttons and LEDs in Geode boards to software > > nodes/properties, so that support for platform data can be removed from > > gpio-keys driver (which will rely purely on generic device properties > > for configuration). > > > > To avoid repeating the same data structures over and over and over > > factor them out into a new geode-common.c file. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Thanks, patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Question has this been tested on at least 1 affected device ? No unfortunately it has not been as I do not have the hardware. I am hoping folks on geode list could give this patch a spin. Thanks.
Hi, On 8/21/24 8:15 PM, Dmitry Torokhov wrote: > On Wed, Aug 21, 2024 at 12:15:51PM +0200, Hans de Goede wrote: >> Hi Dmitry, >> >> On 8/21/24 7:25 AM, Dmitry Torokhov wrote: >>> Convert GPIO-connected buttons and LEDs in Geode boards to software >>> nodes/properties, so that support for platform data can be removed from >>> gpio-keys driver (which will rely purely on generic device properties >>> for configuration). >>> >>> To avoid repeating the same data structures over and over and over >>> factor them out into a new geode-common.c file. >>> >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> >> Thanks, patch looks good to me: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Question has this been tested on at least 1 affected device ? > > No unfortunately it has not been as I do not have the hardware. I am > hoping folks on geode list could give this patch a spin. Ok. I assume this is part of some bigger plan to remove platform_data support from either LEDs and/or the GPIO buttons ? I would rather not merge this untested, but if it is part of some bigger plan, then I'm ok with merging this if still no-one has tested this when the rest of the bits for the plan are ready. IOW lets wait a bit to see if someone steps up to test and of not then lets merge this before it becomes a blocker for further changes. Regards, Hans
On Thu, Aug 22, 2024 at 11:46:33AM +0200, Hans de Goede wrote: > Hi, > > On 8/21/24 8:15 PM, Dmitry Torokhov wrote: > > On Wed, Aug 21, 2024 at 12:15:51PM +0200, Hans de Goede wrote: > >> Hi Dmitry, > >> > >> On 8/21/24 7:25 AM, Dmitry Torokhov wrote: > >>> Convert GPIO-connected buttons and LEDs in Geode boards to software > >>> nodes/properties, so that support for platform data can be removed from > >>> gpio-keys driver (which will rely purely on generic device properties > >>> for configuration). > >>> > >>> To avoid repeating the same data structures over and over and over > >>> factor them out into a new geode-common.c file. > >>> > >>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > >> > >> Thanks, patch looks good to me: > >> > >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >> > >> Question has this been tested on at least 1 affected device ? > > > > No unfortunately it has not been as I do not have the hardware. I am > > hoping folks on geode list could give this patch a spin. > > Ok. I assume this is part of some bigger plan to remove platform_data > support from either LEDs and/or the GPIO buttons ? Can't say about LEDs but yes about GPIO buttons and input devices in general. I would like to move all of them to generic device properties. > > I would rather not merge this untested, but if it is part of some > bigger plan, then I'm ok with merging this if still no-one has tested > this when the rest of the bits for the plan are ready. > > IOW lets wait a bit to see if someone steps up to test and of not > then lets merge this before it becomes a blocker for further changes. OK, I have a few other boards for which I am trying to push similar changes through, once they are done I'll bug you again. Thanks.
Hi Dmitry, kernel test robot noticed the following build warnings: [auto build test WARNING on tip/x86/core] [also build test WARNING on linus/master v6.11-rc4 next-20240823] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Dmitry-Torokhov/x86-platform-geode-switch-GPIO-buttons-and-LEDs-to-software-properties/20240821-132705 base: tip/x86/core patch link: https://lore.kernel.org/r/ZsV6MNS_tUPPSffJ%40google.com patch subject: [PATCH] x86/platform/geode: switch GPIO buttons and LEDs to software properties config: i386-randconfig-062-20240824 (https://download.01.org/0day-ci/archive/20240824/202408241957.UNeWRRij-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240824/202408241957.UNeWRRij-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408241957.UNeWRRij-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> arch/x86/platform/geode/geode-common.c:17:28: sparse: sparse: symbol 'geode_gpiochip_node' was not declared. Should it be static? vim +/geode_gpiochip_node +17 arch/x86/platform/geode/geode-common.c 16 > 17 const struct software_node geode_gpiochip_node = { 18 .name = "cs5535-gpio", 19 }; 20
On Thu, Aug 22, 2024 at 11:46 AM Hans de Goede <hdegoede@redhat.com> wrote: > Ok. I assume this is part of some bigger plan to remove platform_data > support from either LEDs and/or the GPIO buttons ? We want to remove any GPIO numbers from ALL platform data, because we want them out of the kernel completely. See drivers/gpio/TODO for details. The large amount of platform data for LEDs and misc input devices is indeed the bulk of that problem. Yours, Linus Walleij
Hi, On 8/21/24 12:15 PM, Hans de Goede wrote: > Hi Dmitry, > > On 8/21/24 7:25 AM, Dmitry Torokhov wrote: >> Convert GPIO-connected buttons and LEDs in Geode boards to software >> nodes/properties, so that support for platform data can be removed from >> gpio-keys driver (which will rely purely on generic device properties >> for configuration). >> >> To avoid repeating the same data structures over and over and over >> factor them out into a new geode-common.c file. >> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Thanks, patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Question has this been tested on at least 1 affected device ? Since no one has stepped up to test this I was thinking I might just as well merge it. But I just noticed that these files are under arch/x86/platform rather then under drivers/platform/x86 ... So I guess this should be picked up by the x86/tip folks. Or I can merge it through platform-drivers-x86.git/for-next with an ack from one of the x86 maintainers. Regards, Hans >> --- >> arch/x86/Kconfig | 6 + >> arch/x86/platform/geode/Makefile | 1 + >> arch/x86/platform/geode/alix.c | 82 ++--------- >> arch/x86/platform/geode/geode-common.c | 180 +++++++++++++++++++++++++ >> arch/x86/platform/geode/geode-common.h | 21 +++ >> arch/x86/platform/geode/geos.c | 80 +---------- >> arch/x86/platform/geode/net5501.c | 69 +--------- >> 7 files changed, 230 insertions(+), 209 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index 09f8fbcfe000..96b02e813332 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -3073,9 +3073,13 @@ config OLPC_XO15_SCI >> - AC adapter status updates >> - Battery status updates >> >> +config GEODE_COMMON >> + bool >> + >> config ALIX >> bool "PCEngines ALIX System Support (LED setup)" >> select GPIOLIB >> + select GEODE_COMMON >> help >> This option enables system support for the PCEngines ALIX. >> At present this just sets up LEDs for GPIO control on >> @@ -3090,12 +3094,14 @@ config ALIX >> config NET5501 >> bool "Soekris Engineering net5501 System Support (LEDS, GPIO, etc)" >> select GPIOLIB >> + select GEODE_COMMON >> help >> This option enables system support for the Soekris Engineering net5501. >> >> config GEOS >> bool "Traverse Technologies GEOS System Support (LEDS, GPIO, etc)" >> select GPIOLIB >> + select GEODE_COMMON >> depends on DMI >> help >> This option enables system support for the Traverse Technologies GEOS. >> diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile >> index a8a6b1dedb01..34b53e97a0ad 100644 >> --- a/arch/x86/platform/geode/Makefile >> +++ b/arch/x86/platform/geode/Makefile >> @@ -1,4 +1,5 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> +obj-$(CONFIG_GEODE_COMMON) += geode-common.o >> obj-$(CONFIG_ALIX) += alix.o >> obj-$(CONFIG_NET5501) += net5501.o >> obj-$(CONFIG_GEOS) += geos.o >> diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c >> index b39bf3b5e108..be65cd704e21 100644 >> --- a/arch/x86/platform/geode/alix.c >> +++ b/arch/x86/platform/geode/alix.c >> @@ -18,15 +18,12 @@ >> #include <linux/io.h> >> #include <linux/string.h> >> #include <linux/moduleparam.h> >> -#include <linux/leds.h> >> -#include <linux/platform_device.h> >> -#include <linux/input.h> >> -#include <linux/gpio_keys.h> >> -#include <linux/gpio/machine.h> >> #include <linux/dmi.h> >> >> #include <asm/geode.h> >> >> +#include "geode-common.h" >> + >> #define BIOS_SIGNATURE_TINYBIOS 0xf0000 >> #define BIOS_SIGNATURE_COREBOOT 0x500 >> #define BIOS_REGION_SIZE 0x10000 >> @@ -41,79 +38,16 @@ module_param(force, bool, 0444); >> /* FIXME: Award bios is not automatically detected as Alix platform */ >> MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); >> >> -static struct gpio_keys_button alix_gpio_buttons[] = { >> - { >> - .code = KEY_RESTART, >> - .gpio = 24, >> - .active_low = 1, >> - .desc = "Reset button", >> - .type = EV_KEY, >> - .wakeup = 0, >> - .debounce_interval = 100, >> - .can_disable = 0, >> - } >> -}; >> -static struct gpio_keys_platform_data alix_buttons_data = { >> - .buttons = alix_gpio_buttons, >> - .nbuttons = ARRAY_SIZE(alix_gpio_buttons), >> - .poll_interval = 20, >> -}; >> - >> -static struct platform_device alix_buttons_dev = { >> - .name = "gpio-keys-polled", >> - .id = 1, >> - .dev = { >> - .platform_data = &alix_buttons_data, >> - } >> -}; >> - >> -static struct gpio_led alix_leds[] = { >> - { >> - .name = "alix:1", >> - .default_trigger = "default-on", >> - }, >> - { >> - .name = "alix:2", >> - .default_trigger = "default-off", >> - }, >> - { >> - .name = "alix:3", >> - .default_trigger = "default-off", >> - }, >> -}; >> - >> -static struct gpio_led_platform_data alix_leds_data = { >> - .num_leds = ARRAY_SIZE(alix_leds), >> - .leds = alix_leds, >> -}; >> - >> -static struct gpiod_lookup_table alix_leds_gpio_table = { >> - .dev_id = "leds-gpio", >> - .table = { >> - /* The Geode GPIOs should be on the CS5535 companion chip */ >> - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), >> - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), >> - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), >> - { } >> - }, >> -}; >> - >> -static struct platform_device alix_leds_dev = { >> - .name = "leds-gpio", >> - .id = -1, >> - .dev.platform_data = &alix_leds_data, >> -}; >> - >> -static struct platform_device *alix_devs[] __initdata = { >> - &alix_buttons_dev, >> - &alix_leds_dev, >> +static const struct geode_led alix_leds[] __initconst = { >> + { 6, true }, >> + { 25, false }, >> + { 27, false }, >> }; >> >> static void __init register_alix(void) >> { >> - /* Setup LED control through leds-gpio driver */ >> - gpiod_add_lookup_table(&alix_leds_gpio_table); >> - platform_add_devices(alix_devs, ARRAY_SIZE(alix_devs)); >> + geode_create_restart_key(24); >> + geode_create_leds("alix", alix_leds, ARRAY_SIZE(alix_leds)); >> } >> >> static bool __init alix_present(unsigned long bios_phys, >> diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c >> new file mode 100644 >> index 000000000000..8f365388cfbb >> --- /dev/null >> +++ b/arch/x86/platform/geode/geode-common.c >> @@ -0,0 +1,180 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Shared helpers to register GPIO-connected buttons and LEDs >> + * on AMD Geode boards. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/gpio/machine.h> >> +#include <linux/gpio/property.h> >> +#include <linux/input.h> >> +#include <linux/leds.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> + >> +#include "geode-common.h" >> + >> +const struct software_node geode_gpiochip_node = { >> + .name = "cs5535-gpio", >> +}; >> + >> +static const struct property_entry geode_gpio_keys_props[] = { >> + PROPERTY_ENTRY_U32("poll-interval", 20), >> + { } >> +}; >> + >> +static const struct software_node geode_gpio_keys_node = { >> + .name = "geode-gpio-keys", >> + .properties = geode_gpio_keys_props, >> +}; >> + >> +static struct property_entry geode_restart_key_props[] = { >> + { /* Placeholder for GPIO property */ }, >> + PROPERTY_ENTRY_U32("linux,code", KEY_RESTART), >> + PROPERTY_ENTRY_STRING("label", "Reset button"), >> + PROPERTY_ENTRY_U32("debounce-interval", 100), >> + { } >> +}; >> + >> +static const struct software_node geode_restart_key_node = { >> + .parent = &geode_gpio_keys_node, >> + .properties = geode_restart_key_props, >> +}; >> + >> +static const struct software_node *geode_gpio_keys_swnodes[] __initconst = { >> + &geode_gpiochip_node, >> + &geode_gpio_keys_node, >> + &geode_restart_key_node, >> + NULL >> +}; >> + >> +/* >> + * Creates gpio-keys-polled device for the restart key. >> + * >> + * Note that it needs to be called first, before geode_create_leds(), >> + * because it registers gpiochip software node used by both gpio-keys and >> + * leds-gpio devices. >> + */ >> +int __init geode_create_restart_key(unsigned int pin) >> +{ >> + struct platform_device_info keys_info = { >> + .name = "gpio-keys-polled", >> + .id = 1, >> + }; >> + struct platform_device *pd; >> + int err; >> + >> + geode_restart_key_props[0] = PROPERTY_ENTRY_GPIO("gpios", >> + &geode_gpiochip_node, >> + pin, GPIO_ACTIVE_LOW); >> + >> + err = software_node_register_node_group(geode_gpio_keys_swnodes); >> + if (err) { >> + pr_err("failed to register gpio-keys software nodes: %d\n", err); >> + return err; >> + } >> + >> + keys_info.fwnode = software_node_fwnode(&geode_gpio_keys_node); >> + >> + pd = platform_device_register_full(&keys_info); >> + err = PTR_ERR_OR_ZERO(pd); >> + if (err) { >> + pr_err("failed to create gpio-keys device: %d\n", err); >> + software_node_unregister_node_group(geode_gpio_keys_swnodes); >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +static const struct software_node geode_gpio_leds_node = { >> + .name = "geode-leds", >> +}; >> + >> +#define MAX_LEDS 3 >> + >> +int __init geode_create_leds(const char *label, const struct geode_led *leds, >> + unsigned int n_leds) >> +{ >> + const struct software_node *group[MAX_LEDS + 2] = { 0 }; >> + struct software_node *swnodes; >> + struct property_entry *props; >> + struct platform_device_info led_info = { >> + .name = "leds-gpio", >> + .id = PLATFORM_DEVID_NONE, >> + }; >> + struct platform_device *led_dev; >> + const char *node_name; >> + int err; >> + int i; >> + >> + if (n_leds > MAX_LEDS) { >> + pr_err("%s: too many LEDs\n", __func__); >> + return -EINVAL; >> + } >> + >> + swnodes = kcalloc(n_leds, sizeof(*swnodes), GFP_KERNEL); >> + if (!swnodes) >> + return -ENOMEM; >> + >> + /* >> + * Each LED is represented by 3 properties: "gpios", >> + * "linux,default-trigger", and am empty terminator. >> + */ >> + props = kcalloc(n_leds * 3, sizeof(*props), GFP_KERNEL); >> + if (!props) { >> + err = -ENOMEM; >> + goto err_free_swnodes; >> + } >> + >> + group[0] = &geode_gpio_leds_node; >> + for (i = 0; i < n_leds; i++) { >> + node_name = kasprintf(GFP_KERNEL, "%s:%d", label, i); >> + if (!node_name) { >> + err = -ENOMEM; >> + goto err_free_names; >> + } >> + >> + props[i * 3 + 0] = >> + PROPERTY_ENTRY_GPIO("gpios", &geode_gpiochip_node, >> + leds[i].pin, GPIO_ACTIVE_LOW); >> + props[i * 3 + 1] = >> + PROPERTY_ENTRY_STRING("linux,default-trigger", >> + leds[i].default_on ? >> + "default-on" : "default-off"); >> + /* props[i * 3 + 2] is an empty terminator */ >> + >> + swnodes[i] = SOFTWARE_NODE(node_name, &props[i * 3], >> + &geode_gpio_leds_node); >> + group[i + 1] = &swnodes[i]; >> + } >> + >> + err = software_node_register_node_group(group); >> + if (err) { >> + pr_err("failed to register LED software nodes: %d\n", err); >> + goto err_free_names; >> + } >> + >> + led_info.fwnode = software_node_fwnode(&geode_gpio_leds_node); >> + >> + led_dev = platform_device_register_full(&led_info); >> + err = PTR_ERR_OR_ZERO(led_dev); >> + if (err) { >> + pr_err("failed to create LED device: %d\n", err); >> + goto err_unregister_group; >> + } >> + >> + return 0; >> + >> +err_unregister_group: >> + software_node_unregister_node_group(group); >> +err_free_names: >> + while (--i >= 0) >> + kfree(swnodes[i].name); >> + kfree(props); >> +err_free_swnodes: >> + kfree(swnodes); >> + return err; >> +} >> + >> + >> diff --git a/arch/x86/platform/geode/geode-common.h b/arch/x86/platform/geode/geode-common.h >> new file mode 100644 >> index 000000000000..9e0afd34bfad >> --- /dev/null >> +++ b/arch/x86/platform/geode/geode-common.h >> @@ -0,0 +1,21 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Shared helpers to register GPIO-connected buttons and LEDs >> + * on AMD Geode boards. >> + */ >> + >> +#ifndef __PLATFORM_GEODE_COMMON_H >> +#define __PLATFORM_GEODE_COMMON_H >> + >> +#include <linux/property.h> >> + >> +struct geode_led { >> + unsigned int pin; >> + bool default_on; >> +}; >> + >> +int geode_create_restart_key(unsigned int pin); >> +int geode_create_leds(const char *label, const struct geode_led *leds, >> + unsigned int n_leds); >> + >> +#endif /* __PLATFORM_GEODE_COMMON_H */ >> diff --git a/arch/x86/platform/geode/geos.c b/arch/x86/platform/geode/geos.c >> index d263528c90bb..98027fb1ec32 100644 >> --- a/arch/x86/platform/geode/geos.c >> +++ b/arch/x86/platform/geode/geos.c >> @@ -16,88 +16,22 @@ >> #include <linux/init.h> >> #include <linux/io.h> >> #include <linux/string.h> >> -#include <linux/leds.h> >> -#include <linux/platform_device.h> >> -#include <linux/input.h> >> -#include <linux/gpio_keys.h> >> -#include <linux/gpio/machine.h> >> #include <linux/dmi.h> >> >> #include <asm/geode.h> >> >> -static struct gpio_keys_button geos_gpio_buttons[] = { >> - { >> - .code = KEY_RESTART, >> - .gpio = 3, >> - .active_low = 1, >> - .desc = "Reset button", >> - .type = EV_KEY, >> - .wakeup = 0, >> - .debounce_interval = 100, >> - .can_disable = 0, >> - } >> -}; >> -static struct gpio_keys_platform_data geos_buttons_data = { >> - .buttons = geos_gpio_buttons, >> - .nbuttons = ARRAY_SIZE(geos_gpio_buttons), >> - .poll_interval = 20, >> -}; >> - >> -static struct platform_device geos_buttons_dev = { >> - .name = "gpio-keys-polled", >> - .id = 1, >> - .dev = { >> - .platform_data = &geos_buttons_data, >> - } >> -}; >> - >> -static struct gpio_led geos_leds[] = { >> - { >> - .name = "geos:1", >> - .default_trigger = "default-on", >> - }, >> - { >> - .name = "geos:2", >> - .default_trigger = "default-off", >> - }, >> - { >> - .name = "geos:3", >> - .default_trigger = "default-off", >> - }, >> -}; >> - >> -static struct gpio_led_platform_data geos_leds_data = { >> - .num_leds = ARRAY_SIZE(geos_leds), >> - .leds = geos_leds, >> -}; >> - >> -static struct gpiod_lookup_table geos_leds_gpio_table = { >> - .dev_id = "leds-gpio", >> - .table = { >> - /* The Geode GPIOs should be on the CS5535 companion chip */ >> - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), >> - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), >> - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), >> - { } >> - }, >> -}; >> - >> -static struct platform_device geos_leds_dev = { >> - .name = "leds-gpio", >> - .id = -1, >> - .dev.platform_data = &geos_leds_data, >> -}; >> +#include "geode-common.h" >> >> -static struct platform_device *geos_devs[] __initdata = { >> - &geos_buttons_dev, >> - &geos_leds_dev, >> +static const struct geode_led geos_leds[] __initconst = { >> + { 6, true }, >> + { 25, false }, >> + { 27, false }, >> }; >> >> static void __init register_geos(void) >> { >> - /* Setup LED control through leds-gpio driver */ >> - gpiod_add_lookup_table(&geos_leds_gpio_table); >> - platform_add_devices(geos_devs, ARRAY_SIZE(geos_devs)); >> + geode_create_restart_key(3); >> + geode_create_leds("geos", geos_leds, ARRAY_SIZE(geos_leds)); >> } >> >> static int __init geos_init(void) >> diff --git a/arch/x86/platform/geode/net5501.c b/arch/x86/platform/geode/net5501.c >> index 558384acd777..c9cee7dea99b 100644 >> --- a/arch/x86/platform/geode/net5501.c >> +++ b/arch/x86/platform/geode/net5501.c >> @@ -16,80 +16,25 @@ >> #include <linux/init.h> >> #include <linux/io.h> >> #include <linux/string.h> >> -#include <linux/leds.h> >> -#include <linux/platform_device.h> >> #include <linux/input.h> >> -#include <linux/gpio_keys.h> >> #include <linux/gpio/machine.h> >> +#include <linux/gpio/property.h> >> >> #include <asm/geode.h> >> >> +#include "geode-common.h" >> + >> #define BIOS_REGION_BASE 0xffff0000 >> #define BIOS_REGION_SIZE 0x00010000 >> >> -static struct gpio_keys_button net5501_gpio_buttons[] = { >> - { >> - .code = KEY_RESTART, >> - .gpio = 24, >> - .active_low = 1, >> - .desc = "Reset button", >> - .type = EV_KEY, >> - .wakeup = 0, >> - .debounce_interval = 100, >> - .can_disable = 0, >> - } >> -}; >> -static struct gpio_keys_platform_data net5501_buttons_data = { >> - .buttons = net5501_gpio_buttons, >> - .nbuttons = ARRAY_SIZE(net5501_gpio_buttons), >> - .poll_interval = 20, >> -}; >> - >> -static struct platform_device net5501_buttons_dev = { >> - .name = "gpio-keys-polled", >> - .id = 1, >> - .dev = { >> - .platform_data = &net5501_buttons_data, >> - } >> -}; >> - >> -static struct gpio_led net5501_leds[] = { >> - { >> - .name = "net5501:1", >> - .default_trigger = "default-on", >> - }, >> -}; >> - >> -static struct gpio_led_platform_data net5501_leds_data = { >> - .num_leds = ARRAY_SIZE(net5501_leds), >> - .leds = net5501_leds, >> -}; >> - >> -static struct gpiod_lookup_table net5501_leds_gpio_table = { >> - .dev_id = "leds-gpio", >> - .table = { >> - /* The Geode GPIOs should be on the CS5535 companion chip */ >> - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_HIGH), >> - { } >> - }, >> -}; >> - >> -static struct platform_device net5501_leds_dev = { >> - .name = "leds-gpio", >> - .id = -1, >> - .dev.platform_data = &net5501_leds_data, >> -}; >> - >> -static struct platform_device *net5501_devs[] __initdata = { >> - &net5501_buttons_dev, >> - &net5501_leds_dev, >> +static const struct geode_led net5501_leds[] __initconst = { >> + { 6, true }, >> }; >> >> static void __init register_net5501(void) >> { >> - /* Setup LED control through leds-gpio driver */ >> - gpiod_add_lookup_table(&net5501_leds_gpio_table); >> - platform_add_devices(net5501_devs, ARRAY_SIZE(net5501_devs)); >> + geode_create_restart_key(24); >> + geode_create_leds("net5501", net5501_leds, ARRAY_SIZE(net5501_leds)); >> } >> >> struct net5501_board { >
On Wed, Sep 04, 2024 at 03:02:17PM +0200, Hans de Goede wrote: > Or I can merge it through platform-drivers-x86.git/for-next > with an ack from one of the x86 maintainers. Acked-by: Borislav Petkov (AMD) <bp@alien8.de>
Hi, On 9/4/24 3:21 PM, Borislav Petkov wrote: > On Wed, Sep 04, 2024 at 03:02:17PM +0200, Hans de Goede wrote: >> Or I can merge it through platform-drivers-x86.git/for-next >> with an ack from one of the x86 maintainers. > > Acked-by: Borislav Petkov (AMD) <bp@alien8.de> Thank you. I've applied this patch to my review-hans branch now: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans
On Wed, Sep 04, 2024 at 06:01:30PM +0200, Hans de Goede wrote: > Hi, > > On 9/4/24 3:21 PM, Borislav Petkov wrote: > > On Wed, Sep 04, 2024 at 03:02:17PM +0200, Hans de Goede wrote: > >> Or I can merge it through platform-drivers-x86.git/for-next > >> with an ack from one of the x86 maintainers. > > > > Acked-by: Borislav Petkov (AMD) <bp@alien8.de> > > > Thank you. > > I've applied this patch to my review-hans branch now: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Note it will show up in my review-hans branch once I've pushed my > local branch there, which might take a while. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. Hans, could you squash the following bit into the original patch please: diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c index 8f365388cfbb..d35aaf3e76a0 100644 --- a/arch/x86/platform/geode/geode-common.c +++ b/arch/x86/platform/geode/geode-common.c @@ -14,7 +14,7 @@ #include "geode-common.h" -const struct software_node geode_gpiochip_node = { +static const struct software_node geode_gpiochip_node = { .name = "cs5535-gpio", };
Hi Dmitry, On 9/4/24 8:15 PM, Dmitry Torokhov wrote: > On Wed, Sep 04, 2024 at 06:01:30PM +0200, Hans de Goede wrote: >> Hi, >> >> On 9/4/24 3:21 PM, Borislav Petkov wrote: >>> On Wed, Sep 04, 2024 at 03:02:17PM +0200, Hans de Goede wrote: >>>> Or I can merge it through platform-drivers-x86.git/for-next >>>> with an ack from one of the x86 maintainers. >>> >>> Acked-by: Borislav Petkov (AMD) <bp@alien8.de> >> >> >> Thank you. >> >> I've applied this patch to my review-hans branch now: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> Note it will show up in my review-hans branch once I've pushed my >> local branch there, which might take a while. >> >> Once I've run some tests on this branch the patches there will be >> added to the platform-drivers-x86/for-next branch and eventually >> will be included in the pdx86 pull-request to Linus for the next >> merge-window. > > Hans, could you squash the following bit into the original patch please: Ah right, I think I remember a lkp report about this, thank you. I've squashed this in now. Thanks & Regards, Hans > diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c > index 8f365388cfbb..d35aaf3e76a0 100644 > --- a/arch/x86/platform/geode/geode-common.c > +++ b/arch/x86/platform/geode/geode-common.c > @@ -14,7 +14,7 @@ > > #include "geode-common.h" > > -const struct software_node geode_gpiochip_node = { > +static const struct software_node geode_gpiochip_node = { > .name = "cs5535-gpio", > }; > >
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 09f8fbcfe000..96b02e813332 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -3073,9 +3073,13 @@ config OLPC_XO15_SCI - AC adapter status updates - Battery status updates +config GEODE_COMMON + bool + config ALIX bool "PCEngines ALIX System Support (LED setup)" select GPIOLIB + select GEODE_COMMON help This option enables system support for the PCEngines ALIX. At present this just sets up LEDs for GPIO control on @@ -3090,12 +3094,14 @@ config ALIX config NET5501 bool "Soekris Engineering net5501 System Support (LEDS, GPIO, etc)" select GPIOLIB + select GEODE_COMMON help This option enables system support for the Soekris Engineering net5501. config GEOS bool "Traverse Technologies GEOS System Support (LEDS, GPIO, etc)" select GPIOLIB + select GEODE_COMMON depends on DMI help This option enables system support for the Traverse Technologies GEOS. diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile index a8a6b1dedb01..34b53e97a0ad 100644 --- a/arch/x86/platform/geode/Makefile +++ b/arch/x86/platform/geode/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_GEODE_COMMON) += geode-common.o obj-$(CONFIG_ALIX) += alix.o obj-$(CONFIG_NET5501) += net5501.o obj-$(CONFIG_GEOS) += geos.o diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c index b39bf3b5e108..be65cd704e21 100644 --- a/arch/x86/platform/geode/alix.c +++ b/arch/x86/platform/geode/alix.c @@ -18,15 +18,12 @@ #include <linux/io.h> #include <linux/string.h> #include <linux/moduleparam.h> -#include <linux/leds.h> -#include <linux/platform_device.h> -#include <linux/input.h> -#include <linux/gpio_keys.h> -#include <linux/gpio/machine.h> #include <linux/dmi.h> #include <asm/geode.h> +#include "geode-common.h" + #define BIOS_SIGNATURE_TINYBIOS 0xf0000 #define BIOS_SIGNATURE_COREBOOT 0x500 #define BIOS_REGION_SIZE 0x10000 @@ -41,79 +38,16 @@ module_param(force, bool, 0444); /* FIXME: Award bios is not automatically detected as Alix platform */ MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); -static struct gpio_keys_button alix_gpio_buttons[] = { - { - .code = KEY_RESTART, - .gpio = 24, - .active_low = 1, - .desc = "Reset button", - .type = EV_KEY, - .wakeup = 0, - .debounce_interval = 100, - .can_disable = 0, - } -}; -static struct gpio_keys_platform_data alix_buttons_data = { - .buttons = alix_gpio_buttons, - .nbuttons = ARRAY_SIZE(alix_gpio_buttons), - .poll_interval = 20, -}; - -static struct platform_device alix_buttons_dev = { - .name = "gpio-keys-polled", - .id = 1, - .dev = { - .platform_data = &alix_buttons_data, - } -}; - -static struct gpio_led alix_leds[] = { - { - .name = "alix:1", - .default_trigger = "default-on", - }, - { - .name = "alix:2", - .default_trigger = "default-off", - }, - { - .name = "alix:3", - .default_trigger = "default-off", - }, -}; - -static struct gpio_led_platform_data alix_leds_data = { - .num_leds = ARRAY_SIZE(alix_leds), - .leds = alix_leds, -}; - -static struct gpiod_lookup_table alix_leds_gpio_table = { - .dev_id = "leds-gpio", - .table = { - /* The Geode GPIOs should be on the CS5535 companion chip */ - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), - { } - }, -}; - -static struct platform_device alix_leds_dev = { - .name = "leds-gpio", - .id = -1, - .dev.platform_data = &alix_leds_data, -}; - -static struct platform_device *alix_devs[] __initdata = { - &alix_buttons_dev, - &alix_leds_dev, +static const struct geode_led alix_leds[] __initconst = { + { 6, true }, + { 25, false }, + { 27, false }, }; static void __init register_alix(void) { - /* Setup LED control through leds-gpio driver */ - gpiod_add_lookup_table(&alix_leds_gpio_table); - platform_add_devices(alix_devs, ARRAY_SIZE(alix_devs)); + geode_create_restart_key(24); + geode_create_leds("alix", alix_leds, ARRAY_SIZE(alix_leds)); } static bool __init alix_present(unsigned long bios_phys, diff --git a/arch/x86/platform/geode/geode-common.c b/arch/x86/platform/geode/geode-common.c new file mode 100644 index 000000000000..8f365388cfbb --- /dev/null +++ b/arch/x86/platform/geode/geode-common.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Shared helpers to register GPIO-connected buttons and LEDs + * on AMD Geode boards. + */ + +#include <linux/err.h> +#include <linux/gpio/machine.h> +#include <linux/gpio/property.h> +#include <linux/input.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#include "geode-common.h" + +const struct software_node geode_gpiochip_node = { + .name = "cs5535-gpio", +}; + +static const struct property_entry geode_gpio_keys_props[] = { + PROPERTY_ENTRY_U32("poll-interval", 20), + { } +}; + +static const struct software_node geode_gpio_keys_node = { + .name = "geode-gpio-keys", + .properties = geode_gpio_keys_props, +}; + +static struct property_entry geode_restart_key_props[] = { + { /* Placeholder for GPIO property */ }, + PROPERTY_ENTRY_U32("linux,code", KEY_RESTART), + PROPERTY_ENTRY_STRING("label", "Reset button"), + PROPERTY_ENTRY_U32("debounce-interval", 100), + { } +}; + +static const struct software_node geode_restart_key_node = { + .parent = &geode_gpio_keys_node, + .properties = geode_restart_key_props, +}; + +static const struct software_node *geode_gpio_keys_swnodes[] __initconst = { + &geode_gpiochip_node, + &geode_gpio_keys_node, + &geode_restart_key_node, + NULL +}; + +/* + * Creates gpio-keys-polled device for the restart key. + * + * Note that it needs to be called first, before geode_create_leds(), + * because it registers gpiochip software node used by both gpio-keys and + * leds-gpio devices. + */ +int __init geode_create_restart_key(unsigned int pin) +{ + struct platform_device_info keys_info = { + .name = "gpio-keys-polled", + .id = 1, + }; + struct platform_device *pd; + int err; + + geode_restart_key_props[0] = PROPERTY_ENTRY_GPIO("gpios", + &geode_gpiochip_node, + pin, GPIO_ACTIVE_LOW); + + err = software_node_register_node_group(geode_gpio_keys_swnodes); + if (err) { + pr_err("failed to register gpio-keys software nodes: %d\n", err); + return err; + } + + keys_info.fwnode = software_node_fwnode(&geode_gpio_keys_node); + + pd = platform_device_register_full(&keys_info); + err = PTR_ERR_OR_ZERO(pd); + if (err) { + pr_err("failed to create gpio-keys device: %d\n", err); + software_node_unregister_node_group(geode_gpio_keys_swnodes); + return err; + } + + return 0; +} + +static const struct software_node geode_gpio_leds_node = { + .name = "geode-leds", +}; + +#define MAX_LEDS 3 + +int __init geode_create_leds(const char *label, const struct geode_led *leds, + unsigned int n_leds) +{ + const struct software_node *group[MAX_LEDS + 2] = { 0 }; + struct software_node *swnodes; + struct property_entry *props; + struct platform_device_info led_info = { + .name = "leds-gpio", + .id = PLATFORM_DEVID_NONE, + }; + struct platform_device *led_dev; + const char *node_name; + int err; + int i; + + if (n_leds > MAX_LEDS) { + pr_err("%s: too many LEDs\n", __func__); + return -EINVAL; + } + + swnodes = kcalloc(n_leds, sizeof(*swnodes), GFP_KERNEL); + if (!swnodes) + return -ENOMEM; + + /* + * Each LED is represented by 3 properties: "gpios", + * "linux,default-trigger", and am empty terminator. + */ + props = kcalloc(n_leds * 3, sizeof(*props), GFP_KERNEL); + if (!props) { + err = -ENOMEM; + goto err_free_swnodes; + } + + group[0] = &geode_gpio_leds_node; + for (i = 0; i < n_leds; i++) { + node_name = kasprintf(GFP_KERNEL, "%s:%d", label, i); + if (!node_name) { + err = -ENOMEM; + goto err_free_names; + } + + props[i * 3 + 0] = + PROPERTY_ENTRY_GPIO("gpios", &geode_gpiochip_node, + leds[i].pin, GPIO_ACTIVE_LOW); + props[i * 3 + 1] = + PROPERTY_ENTRY_STRING("linux,default-trigger", + leds[i].default_on ? + "default-on" : "default-off"); + /* props[i * 3 + 2] is an empty terminator */ + + swnodes[i] = SOFTWARE_NODE(node_name, &props[i * 3], + &geode_gpio_leds_node); + group[i + 1] = &swnodes[i]; + } + + err = software_node_register_node_group(group); + if (err) { + pr_err("failed to register LED software nodes: %d\n", err); + goto err_free_names; + } + + led_info.fwnode = software_node_fwnode(&geode_gpio_leds_node); + + led_dev = platform_device_register_full(&led_info); + err = PTR_ERR_OR_ZERO(led_dev); + if (err) { + pr_err("failed to create LED device: %d\n", err); + goto err_unregister_group; + } + + return 0; + +err_unregister_group: + software_node_unregister_node_group(group); +err_free_names: + while (--i >= 0) + kfree(swnodes[i].name); + kfree(props); +err_free_swnodes: + kfree(swnodes); + return err; +} + + diff --git a/arch/x86/platform/geode/geode-common.h b/arch/x86/platform/geode/geode-common.h new file mode 100644 index 000000000000..9e0afd34bfad --- /dev/null +++ b/arch/x86/platform/geode/geode-common.h @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Shared helpers to register GPIO-connected buttons and LEDs + * on AMD Geode boards. + */ + +#ifndef __PLATFORM_GEODE_COMMON_H +#define __PLATFORM_GEODE_COMMON_H + +#include <linux/property.h> + +struct geode_led { + unsigned int pin; + bool default_on; +}; + +int geode_create_restart_key(unsigned int pin); +int geode_create_leds(const char *label, const struct geode_led *leds, + unsigned int n_leds); + +#endif /* __PLATFORM_GEODE_COMMON_H */ diff --git a/arch/x86/platform/geode/geos.c b/arch/x86/platform/geode/geos.c index d263528c90bb..98027fb1ec32 100644 --- a/arch/x86/platform/geode/geos.c +++ b/arch/x86/platform/geode/geos.c @@ -16,88 +16,22 @@ #include <linux/init.h> #include <linux/io.h> #include <linux/string.h> -#include <linux/leds.h> -#include <linux/platform_device.h> -#include <linux/input.h> -#include <linux/gpio_keys.h> -#include <linux/gpio/machine.h> #include <linux/dmi.h> #include <asm/geode.h> -static struct gpio_keys_button geos_gpio_buttons[] = { - { - .code = KEY_RESTART, - .gpio = 3, - .active_low = 1, - .desc = "Reset button", - .type = EV_KEY, - .wakeup = 0, - .debounce_interval = 100, - .can_disable = 0, - } -}; -static struct gpio_keys_platform_data geos_buttons_data = { - .buttons = geos_gpio_buttons, - .nbuttons = ARRAY_SIZE(geos_gpio_buttons), - .poll_interval = 20, -}; - -static struct platform_device geos_buttons_dev = { - .name = "gpio-keys-polled", - .id = 1, - .dev = { - .platform_data = &geos_buttons_data, - } -}; - -static struct gpio_led geos_leds[] = { - { - .name = "geos:1", - .default_trigger = "default-on", - }, - { - .name = "geos:2", - .default_trigger = "default-off", - }, - { - .name = "geos:3", - .default_trigger = "default-off", - }, -}; - -static struct gpio_led_platform_data geos_leds_data = { - .num_leds = ARRAY_SIZE(geos_leds), - .leds = geos_leds, -}; - -static struct gpiod_lookup_table geos_leds_gpio_table = { - .dev_id = "leds-gpio", - .table = { - /* The Geode GPIOs should be on the CS5535 companion chip */ - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("cs5535-gpio", 25, NULL, 1, GPIO_ACTIVE_LOW), - GPIO_LOOKUP_IDX("cs5535-gpio", 27, NULL, 2, GPIO_ACTIVE_LOW), - { } - }, -}; - -static struct platform_device geos_leds_dev = { - .name = "leds-gpio", - .id = -1, - .dev.platform_data = &geos_leds_data, -}; +#include "geode-common.h" -static struct platform_device *geos_devs[] __initdata = { - &geos_buttons_dev, - &geos_leds_dev, +static const struct geode_led geos_leds[] __initconst = { + { 6, true }, + { 25, false }, + { 27, false }, }; static void __init register_geos(void) { - /* Setup LED control through leds-gpio driver */ - gpiod_add_lookup_table(&geos_leds_gpio_table); - platform_add_devices(geos_devs, ARRAY_SIZE(geos_devs)); + geode_create_restart_key(3); + geode_create_leds("geos", geos_leds, ARRAY_SIZE(geos_leds)); } static int __init geos_init(void) diff --git a/arch/x86/platform/geode/net5501.c b/arch/x86/platform/geode/net5501.c index 558384acd777..c9cee7dea99b 100644 --- a/arch/x86/platform/geode/net5501.c +++ b/arch/x86/platform/geode/net5501.c @@ -16,80 +16,25 @@ #include <linux/init.h> #include <linux/io.h> #include <linux/string.h> -#include <linux/leds.h> -#include <linux/platform_device.h> #include <linux/input.h> -#include <linux/gpio_keys.h> #include <linux/gpio/machine.h> +#include <linux/gpio/property.h> #include <asm/geode.h> +#include "geode-common.h" + #define BIOS_REGION_BASE 0xffff0000 #define BIOS_REGION_SIZE 0x00010000 -static struct gpio_keys_button net5501_gpio_buttons[] = { - { - .code = KEY_RESTART, - .gpio = 24, - .active_low = 1, - .desc = "Reset button", - .type = EV_KEY, - .wakeup = 0, - .debounce_interval = 100, - .can_disable = 0, - } -}; -static struct gpio_keys_platform_data net5501_buttons_data = { - .buttons = net5501_gpio_buttons, - .nbuttons = ARRAY_SIZE(net5501_gpio_buttons), - .poll_interval = 20, -}; - -static struct platform_device net5501_buttons_dev = { - .name = "gpio-keys-polled", - .id = 1, - .dev = { - .platform_data = &net5501_buttons_data, - } -}; - -static struct gpio_led net5501_leds[] = { - { - .name = "net5501:1", - .default_trigger = "default-on", - }, -}; - -static struct gpio_led_platform_data net5501_leds_data = { - .num_leds = ARRAY_SIZE(net5501_leds), - .leds = net5501_leds, -}; - -static struct gpiod_lookup_table net5501_leds_gpio_table = { - .dev_id = "leds-gpio", - .table = { - /* The Geode GPIOs should be on the CS5535 companion chip */ - GPIO_LOOKUP_IDX("cs5535-gpio", 6, NULL, 0, GPIO_ACTIVE_HIGH), - { } - }, -}; - -static struct platform_device net5501_leds_dev = { - .name = "leds-gpio", - .id = -1, - .dev.platform_data = &net5501_leds_data, -}; - -static struct platform_device *net5501_devs[] __initdata = { - &net5501_buttons_dev, - &net5501_leds_dev, +static const struct geode_led net5501_leds[] __initconst = { + { 6, true }, }; static void __init register_net5501(void) { - /* Setup LED control through leds-gpio driver */ - gpiod_add_lookup_table(&net5501_leds_gpio_table); - platform_add_devices(net5501_devs, ARRAY_SIZE(net5501_devs)); + geode_create_restart_key(24); + geode_create_leds("net5501", net5501_leds, ARRAY_SIZE(net5501_leds)); } struct net5501_board {
Convert GPIO-connected buttons and LEDs in Geode boards to software nodes/properties, so that support for platform data can be removed from gpio-keys driver (which will rely purely on generic device properties for configuration). To avoid repeating the same data structures over and over and over factor them out into a new geode-common.c file. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- arch/x86/Kconfig | 6 + arch/x86/platform/geode/Makefile | 1 + arch/x86/platform/geode/alix.c | 82 ++--------- arch/x86/platform/geode/geode-common.c | 180 +++++++++++++++++++++++++ arch/x86/platform/geode/geode-common.h | 21 +++ arch/x86/platform/geode/geos.c | 80 +---------- arch/x86/platform/geode/net5501.c | 69 +--------- 7 files changed, 230 insertions(+), 209 deletions(-)