Re: [PATCH] ARM: i.MX: Kamstrup mx7 concentrator board support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Lars,

On 09.04.21 15:20, Lars Pedersen wrote:
> diff --git a/arch/arm/boards/kamstrup-mx7-concentrator/board.c b/arch/arm/boards/kamstrup-mx7-concentrator/board.c
> new file mode 100644
> index 000000000..ac32e9b05
> --- /dev/null
> +++ b/arch/arm/boards/kamstrup-mx7-concentrator/board.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// SPDX-FileCopyrightText: 2021 Kamstrup A/S
> +
> +/* Author: Lars Pedersen <lapeddk@gmail.com> */
> +
> +#include <common.h>
> +#include <init.h>
> +#include <io.h>
> +#include <gpio.h>
> +#include <restart.h>
> +#include <mach/imx7-regs.h>
> +#include <mach/iomux-mx7.h>
> +#include <mach/generic.h>
> +#include <mfd/imx7-iomuxc-gpr.h>
> +
> +#define BOARD_RESTART_GPIO IMX_GPIO_NR(7, 12)
> +#define TPM_RESET_GPIO IMX_GPIO_NR(3, 8)
> +
> +static void kamstrup_mx7_tpm_reset(void)
> +{
> +	imx7_setup_pad(MX7D_PAD_LCD_DATA03__GPIO3_IO8);
> +
> +	gpio_request(TPM_RESET_GPIO, "tpm-reset");
> +	gpio_direction_output(TPM_RESET_GPIO, 1);
> +	mdelay(100);
> +	gpio_set_value(TPM_RESET_GPIO, 0);
> +	mdelay(100);
> +	gpio_set_value(TPM_RESET_GPIO, 1);
> +	gpio_free(TPM_RESET_GPIO);

We are trying to cut down on code that doesn't use the driver model.
Couldn't this be represented as a gpio-hog in the device tree or
a reset line for the SPI device?

> +}
> +
> +static void __noreturn kamstrup_mx7_board_restart_gpio(struct restart_handler *rst)
> +{
> +	imx7_setup_pad(MX7D_PAD_ENET1_TX_CLK__GPIO7_IO12);
> +
> +	gpio_direction_output(BOARD_RESTART_GPIO, 0);
> +	mdelay(1);
> +	gpio_set_value(BOARD_RESTART_GPIO, 0);

I just sent out a patch[1] with a driver implementing the "gpio-restart" device
tree binding. Could you test that one and use it here instead?

> +
> +	hang();
> +}
> +
> +static int kamstrup_mx7_concentrator_coredevices_init(void)
> +{
> +	kamstrup_mx7_tpm_reset();
> +	restart_handler_register_fn("board", kamstrup_mx7_board_restart_gpio);
> +	barebox_set_model("Kamstrup OMNIA Concentrator");

The default model name is "Kamstrup OMNIA Flex Concentrator".
If that's too long, you could override /model in the barebox device tree.
With the changes suggested above, you could drop board.c then altogether.

The rest of the patch looks good to me. With the feedback addressed:
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

[1]: https://lore.pengutronix.de/barebox/20210410103511.2073504-1-ahmad@a3f.at/T/#mc4dbda46e6bd868fc9ecfcfa42f3dbfe943bef47
 
Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox