Message ID | 20210208222203.22335-13-info@metux.net |
---|---|
State | New |
Headers | show |
Series | [RFC,01/12] of: base: improve error message in of_phandle_iterator_next() | expand |
On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > > Add oftree based support for PC Engines APUv2/3/4 board family. > This is entirely separate from the existing pcengines-apuv2 driver. > > Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> > --- > drivers/platform/of/Kconfig | 15 ++++++++ > drivers/platform/of/Makefile | 2 + > drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++ > drivers/platform/of/init.c | 7 ++++ > 4 files changed, 110 insertions(+) > create mode 100644 drivers/platform/of/apu2x.dts > > diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig > index a0b5a641a7c6..e13b6ee8c316 100644 > --- a/drivers/platform/of/Kconfig > +++ b/drivers/platform/of/Kconfig > @@ -38,4 +38,19 @@ config PLATFORM_OF_DRV_SYSFS_DT > Say Y here to enable exposing device tree nodes at > /sys/firmware/devicetree. > > +config PLATFORM_OF_DRV_PCENGINES_APU2 > + bool "Support PC Engines APU2/3/4 mainboards" > + depends on INPUT > + depends on GPIOLIB > + depends on X86 > + select GPIO_AMD_FCH > + select KEYBOARD_GPIO_POLLED > + select LEDS_GPIO > + select INPUT_KEYBOARD > + help > + Say Y to enable devicetree based support for PC Engines APU2/3/4 > + mainboards. This supersedes the older pcengines-apu2 driver. > + > + Supports Gpios, front panel LEDs and front button. > + > endif # PLATFORM_OF_DRV > diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile > index 84cf3003c500..dd4a13c18f16 100644 > --- a/drivers/platform/of/Makefile > +++ b/drivers/platform/of/Makefile > @@ -2,4 +2,6 @@ > > ofboard-y := init.o drv.o > > +ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o > + > obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o > diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts > new file mode 100644 > index 000000000000..c16a59eb2a0e > --- /dev/null > +++ b/drivers/platform/of/apu2x.dts > @@ -0,0 +1,86 @@ > +/dts-v1/; > + > +#include <dt-bindings/leds/common.h> > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/input/input.h> > +#include <dt-bindings/gpio/amd-fch-gpio.h> > + > +/ { > + apu2x { > + compatible = "virtual,dmi-board"; > + dmi-sys-vendor = "PC engines"; > + dmi-board-name = > + "APU2", > + "apu2", > + "PC engines apu2", > + "APU3", > + "apu3", > + "PC engines apu3", > + "APU4", > + "apu4", > + "PC engines apu4"; I think these DMI properties just need to be the compatible string(s). We already have a way to do matching with DT and don't need a secondary way. If you can > + unbind { > + acpi = "PNP0076:00", "PNP0B00:00"; > + platform = "platform-framebuffer.0", "PNP0103:00"; This node really needs to go. It's clearly Linuxisms. It either has to go in the kernel or userspace. > + }; > + devices { > + gpio1: gpio1 { > + compatible = "amd,fch-gpio"; This of course will need to be documented. > + gpio-controller; > + status = "okay"; nit: That's the default. > + ngpios=<7>; > + #gpio-cells=<2>; > + gpio-regs = < > + AMD_FCH_GPIO_REG_GPIO57 // led1 > + AMD_FCH_GPIO_REG_GPIO58 // led2 > + AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3 > + AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw > + AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap > + AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2 > + AMD_FCH_GPIO_REG_GPIO51 // mpcie3 > + >; > + gpio-line-names = > + "front-led1", > + "front-led2", > + "front-led3", > + "front-button", > + "simswap", > + "mpcie2_reset", > + "mpcie3_reset"; > + }; > + front-leds { > + compatible = "gpio-leds"; > + led@0 { > + gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; > + color = <LED_COLOR_ID_GREEN>; > + default-state = "keep"; > + label = "apu:green:1"; > + }; > + led@1 { > + gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>; > + color = <LED_COLOR_ID_GREEN>; > + default-state = "keep"; > + label = "apu:green:2"; > + }; > + led@2 { > + gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>; > + color = <LED_COLOR_ID_GREEN>; > + default-state = "keep"; > + label = "apu:green:3"; > + }; > + }; > + front-keys { > + compatible = "gpio-keys-polled"; > + address-cells = <1>; > + size-cells = <0>; > + poll-interval = <100>; > + button@1 { > + linux,code = <KEY_RESTART>; > + label = "front button"; > + debounce-interval = <10>; > + gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>; > + }; > + }; > + }; > + }; > +}; > diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c > index 3b8373cda77a..195120dad26d 100644 > --- a/drivers/platform/of/init.c > +++ b/drivers/platform/of/init.c > @@ -47,7 +47,14 @@ static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj, > return count; > } > > +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2 > +DECLARE_FDT_EXTERN(apu2x); > +#endif > + > static struct fdt_image fdt[] = { > +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2 > + FDT_IMAGE_ENT(apu2x) > +#endif > }; > > static int __init ofdrv_init_sysfs(struct fdt_image *image) > -- > 2.11.0 >
On 09.02.21 01:06, Rob Herring wrote: Hi, >> +/ { >> + apu2x { >> + compatible = "virtual,dmi-board"; >> + dmi-sys-vendor = "PC engines"; >> + dmi-board-name = >> + "APU2", >> + "apu2", >> + "PC engines apu2", >> + "APU3", >> + "apu3", >> + "PC engines apu3", >> + "APU4", >> + "apu4", >> + "PC engines apu4"; > > I think these DMI properties just need to be the compatible string(s). > We already have a way to do matching with DT and don't need a > secondary way. If you can It's not easy fitting that into one string, because we've got lots of combinations that need to be matched. In this specific case, I haven't seen any board where the vendor name isn't an exact match of the given string (that's why it's only one entry), but in the past seen several boards where even this changes between bios versions. The board names, more varying. Something that's not reflected in this example yet: there're even more subtle differences between production series (eg. certain pins not wired, etc). Supporting such things would need adding more matching rules and possibly runtime DT manipulations. >> + unbind { >> + acpi = "PNP0076:00", "PNP0B00:00"; >> + platform = "platform-framebuffer.0", "PNP0103:00"; > > This node really needs to go. It's clearly Linuxisms. It either has to > go in the kernel or userspace. Note that the whole thing here *is* a Linuxism. This kind of DTs is built into the kernel, not in firmware or anywhere else. This stuff is only for cases where firmware is not giving, or giving broken information. And it's for replacing hand-written C code by a machine readable description. I had to put that in, since in some cases firmware (-versions) already enumerates some devices, but does it in a wrong or incomplete way. So, these devices need to be removed first, before the correct ones can be initialized. (note that this patch, for now, is just an hacking example - some details are still broken). If anybody has a better idea how to do that, let me know. In general, I'd like to have everything for one board (family) in one declarative file. >> + }; >> + devices { >> + gpio1: gpio1 { >> + compatible = "amd,fch-gpio"; > > This of course will need to be documented. Yes, but that's a different issue. It's still in RFC stage. The gpio-amd-fch changes are in this patch queue for a complete example, but probably will be upstreamed separately. >> + gpio-controller; >> + status = "okay"; > > nit: That's the default. Okay, dropping it. --mtx
On Mon, Feb 8, 2021 at 11:22 PM Enrico Weigelt, metux IT consult <info@metux.net> wrote: > + gpio-regs = < > + AMD_FCH_GPIO_REG_GPIO57 // led1 > + AMD_FCH_GPIO_REG_GPIO58 // led2 > + AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3 > + AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw > + AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap > + AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2 > + AMD_FCH_GPIO_REG_GPIO51 // mpcie3 > + >; Please don't define registers in the DTS files. Determine the set of registers from the compatible string and put them in the driver. If that is not possible, the compatible string is not precise enough and needs to indicate properly which hardware this is. Yours, Linus Walleij
diff --git a/drivers/platform/of/Kconfig b/drivers/platform/of/Kconfig index a0b5a641a7c6..e13b6ee8c316 100644 --- a/drivers/platform/of/Kconfig +++ b/drivers/platform/of/Kconfig @@ -38,4 +38,19 @@ config PLATFORM_OF_DRV_SYSFS_DT Say Y here to enable exposing device tree nodes at /sys/firmware/devicetree. +config PLATFORM_OF_DRV_PCENGINES_APU2 + bool "Support PC Engines APU2/3/4 mainboards" + depends on INPUT + depends on GPIOLIB + depends on X86 + select GPIO_AMD_FCH + select KEYBOARD_GPIO_POLLED + select LEDS_GPIO + select INPUT_KEYBOARD + help + Say Y to enable devicetree based support for PC Engines APU2/3/4 + mainboards. This supersedes the older pcengines-apu2 driver. + + Supports Gpios, front panel LEDs and front button. + endif # PLATFORM_OF_DRV diff --git a/drivers/platform/of/Makefile b/drivers/platform/of/Makefile index 84cf3003c500..dd4a13c18f16 100644 --- a/drivers/platform/of/Makefile +++ b/drivers/platform/of/Makefile @@ -2,4 +2,6 @@ ofboard-y := init.o drv.o +ofboard-$(CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2) += apu2x.dtb.o + obj-$(CONFIG_PLATFORM_OF_DRV) += ofboard.o diff --git a/drivers/platform/of/apu2x.dts b/drivers/platform/of/apu2x.dts new file mode 100644 index 000000000000..c16a59eb2a0e --- /dev/null +++ b/drivers/platform/of/apu2x.dts @@ -0,0 +1,86 @@ +/dts-v1/; + +#include <dt-bindings/leds/common.h> +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> +#include <dt-bindings/gpio/amd-fch-gpio.h> + +/ { + apu2x { + compatible = "virtual,dmi-board"; + dmi-sys-vendor = "PC engines"; + dmi-board-name = + "APU2", + "apu2", + "PC engines apu2", + "APU3", + "apu3", + "PC engines apu3", + "APU4", + "apu4", + "PC engines apu4"; + unbind { + acpi = "PNP0076:00", "PNP0B00:00"; + platform = "platform-framebuffer.0", "PNP0103:00"; + }; + devices { + gpio1: gpio1 { + compatible = "amd,fch-gpio"; + gpio-controller; + status = "okay"; + ngpios=<7>; + #gpio-cells=<2>; + gpio-regs = < + AMD_FCH_GPIO_REG_GPIO57 // led1 + AMD_FCH_GPIO_REG_GPIO58 // led2 + AMD_FCH_GPIO_REG_GPIO59_DEVSLP1 // led3 + AMD_FCH_GPIO_REG_GPIO32_GE1 // modesw + AMD_FCH_GPIO_REG_GPIO33_GE2 // simawap + AMD_FCH_GPIO_REG_GPIO55_DEVSLP0 // mpcie2 + AMD_FCH_GPIO_REG_GPIO51 // mpcie3 + >; + gpio-line-names = + "front-led1", + "front-led2", + "front-led3", + "front-button", + "simswap", + "mpcie2_reset", + "mpcie3_reset"; + }; + front-leds { + compatible = "gpio-leds"; + led@0 { + gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>; + color = <LED_COLOR_ID_GREEN>; + default-state = "keep"; + label = "apu:green:1"; + }; + led@1 { + gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>; + color = <LED_COLOR_ID_GREEN>; + default-state = "keep"; + label = "apu:green:2"; + }; + led@2 { + gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>; + color = <LED_COLOR_ID_GREEN>; + default-state = "keep"; + label = "apu:green:3"; + }; + }; + front-keys { + compatible = "gpio-keys-polled"; + address-cells = <1>; + size-cells = <0>; + poll-interval = <100>; + button@1 { + linux,code = <KEY_RESTART>; + label = "front button"; + debounce-interval = <10>; + gpios = <&gpio1 3 GPIO_ACTIVE_HIGH>; + }; + }; + }; + }; +}; diff --git a/drivers/platform/of/init.c b/drivers/platform/of/init.c index 3b8373cda77a..195120dad26d 100644 --- a/drivers/platform/of/init.c +++ b/drivers/platform/of/init.c @@ -47,7 +47,14 @@ static ssize_t fdt_image_raw_read(struct file *filep, struct kobject *kobj, return count; } +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2 +DECLARE_FDT_EXTERN(apu2x); +#endif + static struct fdt_image fdt[] = { +#ifdef CONFIG_PLATFORM_OF_DRV_PCENGINES_APU2 + FDT_IMAGE_ENT(apu2x) +#endif }; static int __init ofdrv_init_sysfs(struct fdt_image *image)
Add oftree based support for PC Engines APUv2/3/4 board family. This is entirely separate from the existing pcengines-apuv2 driver. Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> --- drivers/platform/of/Kconfig | 15 ++++++++ drivers/platform/of/Makefile | 2 + drivers/platform/of/apu2x.dts | 86 +++++++++++++++++++++++++++++++++++++++++++ drivers/platform/of/init.c | 7 ++++ 4 files changed, 110 insertions(+) create mode 100644 drivers/platform/of/apu2x.dts