Hi Philip,
One more remark.
On 1/14/23 00:11, Philip Prindeville wrote:
> From: Philip Prindeville <philipp@xxxxxxxxxxxxxxxxxxxxx>
I believe part of this code (the APU5 or APU6 bits?) are from
Ed Wildgoose right?
Then Ed Wildgoose should be set as author for that part,
you can use:
git commit --amend --author="Ed Wildgoose <lists@xxxxxxxxxxxxxx>"
to set the author (of your last commit).
>
> PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
> It also has support for 3x LTE modems with 6x SIM slots (pairs with a
> SIM switch device). Each mpcie slot for modems has a reset GPIO
>
> To ensure that the naming is sane between APU2-6 the GPIOS are
> renamed to be modem1-reset, modem2-reset, etc. This is significant
> because the slots that can be reset change between APU2 and APU3/4
>
> GPIO for simswap is moved to the end of the list as it could be dropped
> for APU2 boards (but causes no harm to leave it in, hardware could be
> added to a future rev of the board).
>
> Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
> many lines are moved around and there are simply more
> modems/resets/sim-swap lines to breakout.
>
> Also added APU6, which is essentially APU4 with a different ethernet
> interface and SFP cage on eth0.
>
> Signed-off-by: Ed Wildgoose <lists@xxxxxxxxxxxxxx>
> Reviewed-by: Philip Prindeville <philipp@xxxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Andreas Eberlein <foodeas@xxxxxxxxxxxx>
> Reviewed-by: Paul Spooren <paul@xxxxxxxxxx>
This is missing your Signed-off-by, since this code has passed through
your hands (and might even have been modified by you) this needs both
Ed's S-o-b as well as yours.
And if you have made significant changes, you can / may want to also
add a Co-authored-by. So in essence that would mean adding these
2 tags:
Co-authored-by: Philip Prindeville <philipp@xxxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Philip Prindeville <philipp@xxxxxxxxxxxxxxxxxxxxx>
I see that you have also added a Reviewed-by: Philip Prindeville,
that is fine if you are just forwarding code upstream (with maybe
some small changes but nothing significant). If you have made
significant changes however then adding a Reviewed-by to your own
work is a bit strange.
Regards,
Hans
> ---
> drivers/platform/x86/pcengines-apuv2.c | 113 ++++++++++++++++++++++---
> 1 file changed, 99 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index d063d91db9bcbe5ceb2ac641d3105df37651ac4d..8731564bab62c1e47e99adb6ec23b3de81b09069 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -1,10 +1,11 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> /*
> - * PC-Engines APUv2/APUv3 board platform driver
> + * PC-Engines APUv2-6 board platform driver
> * for GPIO buttons and LEDs
> *
> * Copyright (C) 2018 metux IT consult
> + * Copyright (C) 2022 Ed Wildgoose <lists@xxxxxxxxxxxxxx>
> * Author: Enrico Weigelt <info@xxxxxxxxx>
> */
>
> @@ -22,38 +23,70 @@
> #include <linux/platform_data/gpio/gpio-amd-fch.h>
>
> /*
> - * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
> + * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
> * has completely different register layouts.
> */
>
> +/*
> + * There are a number of APU variants, with differing features
> + * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
> + * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
> + * However, most APU3/4 have a SIM switch which we default on to reverse
> + * the order and keep physical SIM order matching physical modem order
> + * APU6 is approximately the same as APU4 with different ethernet layout
> + *
> + * APU5 has 3x SIM sockets, all with a SIM switch
> + * several GPIOs are shuffled (see schematic), including MODESW
> + */
> +
> /* Register mappings */
> #define APU2_GPIO_REG_LED1 AMD_FCH_GPIO_REG_GPIO57
> #define APU2_GPIO_REG_LED2 AMD_FCH_GPIO_REG_GPIO58
> #define APU2_GPIO_REG_LED3 AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
> #define APU2_GPIO_REG_MODESW AMD_FCH_GPIO_REG_GPIO32_GE1
> #define APU2_GPIO_REG_SIMSWAP AMD_FCH_GPIO_REG_GPIO33_GE2
> -#define APU2_GPIO_REG_MPCIE2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> -#define APU2_GPIO_REG_MPCIE3 AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
> +#define APU2_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +
> +#define APU5_GPIO_REG_MODESW AMT_FCH_GPIO_REG_GEVT22
> +#define APU5_GPIO_REG_SIMSWAP1 AMD_FCH_GPIO_REG_GPIO68
> +#define APU5_GPIO_REG_SIMSWAP2 AMD_FCH_GPIO_REG_GPIO32_GE1
> +#define APU5_GPIO_REG_SIMSWAP3 AMD_FCH_GPIO_REG_GPIO33_GE2
> +#define APU5_GPIO_REG_RESETM1 AMD_FCH_GPIO_REG_GPIO51
> +#define APU5_GPIO_REG_RESETM2 AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
> +#define APU5_GPIO_REG_RESETM3 AMD_FCH_GPIO_REG_GPIO64
>
> /* Order in which the GPIO lines are defined in the register list */
> #define APU2_GPIO_LINE_LED1 0
> #define APU2_GPIO_LINE_LED2 1
> #define APU2_GPIO_LINE_LED3 2
> #define APU2_GPIO_LINE_MODESW 3
> -#define APU2_GPIO_LINE_SIMSWAP 4
> -#define APU2_GPIO_LINE_MPCIE2 5
> -#define APU2_GPIO_LINE_MPCIE3 6
> +#define APU2_GPIO_LINE_RESETM1 4
> +#define APU2_GPIO_LINE_RESETM2 5
> +#define APU2_GPIO_LINE_SIMSWAP 6
> +
> +#define APU5_GPIO_LINE_LED1 0
> +#define APU5_GPIO_LINE_LED2 1
> +#define APU5_GPIO_LINE_LED3 2
> +#define APU5_GPIO_LINE_MODESW 3
> +#define APU5_GPIO_LINE_RESETM1 4
> +#define APU5_GPIO_LINE_RESETM2 5
> +#define APU5_GPIO_LINE_RESETM3 6
> +#define APU5_GPIO_LINE_SIMSWAP1 7
> +#define APU5_GPIO_LINE_SIMSWAP2 8
> +#define APU5_GPIO_LINE_SIMSWAP3 9
>
> -/* GPIO device */
> +
> +/* GPIO device - APU2/3/4/6 */
>
> static int apu2_gpio_regs[] = {
> [APU2_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
> [APU2_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
> [APU2_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
> [APU2_GPIO_LINE_MODESW] = APU2_GPIO_REG_MODESW,
> + [APU2_GPIO_LINE_RESETM1] = APU2_GPIO_REG_RESETM1,
> + [APU2_GPIO_LINE_RESETM2] = APU2_GPIO_REG_RESETM2,
> [APU2_GPIO_LINE_SIMSWAP] = APU2_GPIO_REG_SIMSWAP,
> - [APU2_GPIO_LINE_MPCIE2] = APU2_GPIO_REG_MPCIE2,
> - [APU2_GPIO_LINE_MPCIE3] = APU2_GPIO_REG_MPCIE3,
> };
>
> static const char * const apu2_gpio_names[] = {
> @@ -61,9 +94,9 @@ static const char * const apu2_gpio_names[] = {
> [APU2_GPIO_LINE_LED2] = "front-led2",
> [APU2_GPIO_LINE_LED3] = "front-led3",
> [APU2_GPIO_LINE_MODESW] = "front-button",
> + [APU2_GPIO_LINE_RESETM1] = "modem1-reset",
> + [APU2_GPIO_LINE_RESETM2] = "modem2-reset",
> [APU2_GPIO_LINE_SIMSWAP] = "simswap",
> - [APU2_GPIO_LINE_MPCIE2] = "mpcie2_reset",
> - [APU2_GPIO_LINE_MPCIE3] = "mpcie3_reset",
> };
>
> static const struct amd_fch_gpio_pdata board_apu2 = {
> @@ -72,6 +105,40 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
> .gpio_names = apu2_gpio_names,
> };
>
> +/* GPIO device - APU5 */
> +
> +static int apu5_gpio_regs[] = {
> + [APU5_GPIO_LINE_LED1] = APU2_GPIO_REG_LED1,
> + [APU5_GPIO_LINE_LED2] = APU2_GPIO_REG_LED2,
> + [APU5_GPIO_LINE_LED3] = APU2_GPIO_REG_LED3,
> + [APU5_GPIO_LINE_MODESW] = APU5_GPIO_REG_MODESW,
> + [APU5_GPIO_LINE_RESETM1] = APU5_GPIO_REG_RESETM1,
> + [APU5_GPIO_LINE_RESETM2] = APU5_GPIO_REG_RESETM2,
> + [APU5_GPIO_LINE_RESETM3] = APU5_GPIO_REG_RESETM3,
> + [APU5_GPIO_LINE_SIMSWAP1] = APU5_GPIO_REG_SIMSWAP1,
> + [APU5_GPIO_LINE_SIMSWAP2] = APU5_GPIO_REG_SIMSWAP2,
> + [APU5_GPIO_LINE_SIMSWAP3] = APU5_GPIO_REG_SIMSWAP3,
> +};
> +
> +static const char * const apu5_gpio_names[] = {
> + [APU5_GPIO_LINE_LED1] = "front-led1",
> + [APU5_GPIO_LINE_LED2] = "front-led2",
> + [APU5_GPIO_LINE_LED3] = "front-led3",
> + [APU5_GPIO_LINE_MODESW] = "front-button",
> + [APU5_GPIO_LINE_RESETM1] = "modem1-reset",
> + [APU5_GPIO_LINE_RESETM2] = "modem2-reset",
> + [APU5_GPIO_LINE_RESETM3] = "modem3-reset",
> + [APU5_GPIO_LINE_SIMSWAP1] = "simswap1",
> + [APU5_GPIO_LINE_SIMSWAP2] = "simswap2",
> + [APU5_GPIO_LINE_SIMSWAP3] = "simswap3",
> +};
> +
> +static const struct amd_fch_gpio_pdata board_apu5 = {
> + .gpio_num = ARRAY_SIZE(apu5_gpio_regs),
> + .gpio_reg = apu5_gpio_regs,
> + .gpio_names = apu5_gpio_names,
> +};
> +
> /* GPIO LEDs device */
>
> static const struct gpio_led apu2_leds[] = {
> @@ -215,6 +282,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
> },
> .driver_data = (void *)&board_apu2,
> },
> + /* APU5 w/ mainline BIOS */
> + {
> + .ident = "apu5",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> + DMI_MATCH(DMI_BOARD_NAME, "apu5")
> + },
> + .driver_data = (void *)&board_apu5,
> + },
> + /* APU6 w/ mainline BIOS */
> + {
> + .ident = "apu6",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> + DMI_MATCH(DMI_BOARD_NAME, "apu6")
> + },
> + .driver_data = (void *)&board_apu2,
> + },
> {}
> };
>
> @@ -249,7 +334,7 @@ static int __init apu_board_init(void)
>
> id = dmi_first_match(apu_gpio_dmi_table);
> if (!id) {
> - pr_err("failed to detect APU board via DMI\n");
> + pr_err("No APU board detected via DMI\n");
> return -ENODEV;
> }
>
> @@ -288,7 +373,7 @@ module_init(apu_board_init);
> module_exit(apu_board_exit);
>
> MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@xxxxxxxxx>");
> -MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
> +MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
> MODULE_LICENSE("GPL");
> MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
> MODULE_ALIAS("platform:pcengines-apuv2");
