Hello Ahmad. Thanks for the feedback. On Sat, 10 Apr 2021 at 12:43, Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> 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@xxxxxxxxx> */ > > + > > +#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@xxxxxxxxxxxxxx> > > [1]: https://lore.pengutronix.de/barebox/20210410103511.2073504-1-ahmad@xxxxxx/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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox