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

[Lars Pedersen <lapeddk@gmail.com>]

Hello Ahmad.

Thanks for the feedback.

On Sat, 10 Apr 2021 at 12:43, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> 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?

I can't find anything in the DT binding for the SPI/TPM driver to use
a reset line. Can a DT gpio-hog toggle the pin? Don't you need a
driver for this? I see the following options:

1) gpio-hog with a label and use new gpiolib in board.c.
2) gpio-hog and control the pin in a boot script.

>
> > +}
> > +
> > +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?
>

I have applied the patches on a master branch (Last patch failed to
apply but was only watchdog) and the gpio-restart works perfectly.

> > +
> > +     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.
>

I'll move this into the device tree or delete it entirely.

> 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