Re: [U-Boot] [PATCH v4] mx6sx: Add initial support for Samtec VIN|ING 2000 board

On 11/23/2016 11:26 PM, Christoph Fritz wrote:

This patch adds initial support for Samtec VIN|ING 2000 board.

Signed-off-by: Christoph Fritz chf.fritz@googlemail.com

Changes since v1:

• add more comments (enet phy init)
• fix layout style e.g. multi-line-comments
• use pinmux macros
• use helper funcs clrsetbits_le32() and wait_for_bit()
• make some functions static
• drop board_ehci_hcd_init() and board_phy_config()
• simplify environment

Changes since v2:

• adapt error handling in board_eth_init() and read_adc()
• purge unused macros
• use config_distro_bootcmd as environment
• fix CONFIG_PWM_IMX undef handling

Changes since v3:

• fix comment spelling
• use u32
• rearrange if condition

[...]

diff --git a/board/samtec/vining_2000/Kconfig b/board/samtec/vining_2000/Kconfig new file mode 100644 index 0000000..f40b3b4 --- /dev/null +++ b/board/samtec/vining_2000/Kconfig @@ -0,0 +1,12 @@ +if TARGET_VINING_2000

Darn, minor nit, should be TARGET_SAMTEC_VINING_2000 (for consistency with VINING_FPGA).

+config SYS_BOARD

• default "vining_2000"

+config SYS_VENDOR

• default "samtec"

+config SYS_CONFIG_NAME

• default "vining_2000"

+endif

[...]

+#ifdef CONFIG_PWM_IMX +static int set_pwm_leds(void) +{

• imx_iomux_v3_setup_multiple_pads(pwm_led_pads,

• 			 ARRAY_SIZE(pwm_led_pads));
  

• /* enable backlight PWM 2, green LED */
• if (pwm_init(1, 0, 0))

• goto error;
  

Looking at this, doesn't pwm_init() / pwm_config() return a valid error code ? If so, propagate it out of this function instead of always returning -EINVAL .

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(1, 200, 8000))

• goto error;
  

• if (pwm_enable(1))

• goto error;
  

• /* enable backlight PWM 1, blue LED */
• if (pwm_init(0, 0, 0))

• goto error;
  

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(0, 200, 8000))

• goto error;
  

• if (pwm_enable(0))

• goto error;
  

• /* enable backlight PWM 6, red LED */
• if (pwm_init(5, 0, 0))

• goto error;
  

• /* duty cycle 200ns, period: 8000ns */
• if (pwm_config(5, 200, 8000))

• goto error;
  

• if (pwm_enable(5))

• goto error;
  

• return 0;

+error:

• return -EINVAL;

+} +#else +static int set_pwm_leds(void) +{

• return 0;

+} +#endif

[...]

Looks great otherwise, thanks.