On 01/20/2011 10:25 AM, Wolfgang Denk wrote:
> Dear Stefano Babic,
>
> In message <1295513194-16158-2-git-send-email-sba...@denx.de> you wrote:
>> The patch adds basic support for the Freescale's i.MX35
>> (arm1136 based) processor.
>>
>> Signed-off-by: Stefano Babic <sba...@denx.de>
>
> checkpatch says:
>
> [U-Boot] [PATCH V2 01/11] Add support for MX35 processor
> total: 7 errors, 8 warnings, 2109 lines checked
Mmmhh...checkpatch says:
total: 0 errors, 8 warnings, 2109 lines checked
0001-Add-support-for-MX35-processor.patch has style problems, please
review. If any of these errors
are false positives report them to the maintainer, see
Warnings are only related to typedef ("do not add typedef"), so I do not
know where the errors are coming from. Do you use the --no-tree option
as I usually set?
>
>> +u32 imx_get_uartclk(void)
>> +{
>> + u32 freq;
>> + struct ccm_regs *ccm =
>> + (struct ccm_regs *)IMX_CCM_BASE;
>> + u32 pdr4 = readl(&ccm->pdr4);
>> +
>> + if (readl(&ccm->pdr3) & MXC_CCM_PDR3_UART_M_U)
>> + freq = get_mcu_main_clk();
>> + else
>> + freq = decode_pll(readl(&ccm->ppctl),
>> + CONFIG_MX35_HCLK_FREQ);
>
> Braces needed
checkpatch (and generally accepted in linux, as I can see) requires that
single statements must not be surrounded by braces. checkpatch returns a
warning, explaining that braces are not needed.
I see in the past some comments requiring to remove braces, but it you
prefer to add them. IMHO it is better to follow the same codestyle as
linux, using the same tools as checkpatch. I do not know why we have two
different results from checkpatch, I try to investigate. I had prefer to
use always braces in if statement, to avoid possible errors if some code
is added, but since checkpatch complains about it I was convinced to
remove them.
>
>> + case USB_CLK:
>> + usb_prdf = (reg4 >> 25) & 0x7;
>> + usb_podf = (reg4 >> 22) & 0x7;
>> + if (reg4 & 0x200)
>> + pll = get_mcu_main_clk();
>> + else
>> + pll = decode_pll(readl(&ccm->ppctl),
>> + CONFIG_MX35_HCLK_FREQ);
>
> Ditto. Please fix globally.
See my previous comment. I would prefer to not have a different rule in
u-boot, and not go against some provided tool (we use both checkpatch)
if not strictly required.
>
> Indeed they should. Why don't you autogenerate these, then?
>
> We have all the tools in place, use them.
I will see how to use them.
>
>
>> +#define NFC_BUF_SIZE 0x1000
>> +#define NFC_BUFSIZE_REG_OFF (0 + 0x00)
>> +#define RAM_BUFFER_ADDRESS_REG_OFF (0 + 0x04)
>> +#define NAND_FLASH_ADD_REG_OFF (0 + 0x06)
>> +#define NAND_FLASH_CMD_REG_OFF (0 + 0x08)
>> +#define NFC_CONFIGURATION_REG_OFF (0 + 0x0A)
>> +#define ECC_STATUS_RESULT_REG_OFF (0 + 0x0C)
>> +#define ECC_RSLT_MAIN_AREA_REG_OFF (0 + 0x0E)
>> +#define ECC_RSLT_SPARE_AREA_REG_OFF (0 + 0x10)
>> +#define NF_WR_PROT_REG_OFF (0 + 0x12)
>> +#define NAND_FLASH_WR_PR_ST_REG_OFF (0 + 0x18)
>> +#define NAND_FLASH_CONFIG1_REG_OFF (0 + 0x1A)
>> +#define NAND_FLASH_CONFIG2_REG_OFF (0 + 0x1C)
>> +#define UNLOCK_START_BLK_ADD_REG_OFF (0 + 0x20)
>> +#define UNLOCK_END_BLK_ADD_REG_OFF (0 + 0x22)
>
> Why has this not been converted into a C struct?
I will check, I think I do not require anymore. NAND driver is in
mainline and does not require them. Probably I can remove them. In the
code I post now I do not use them at all, so probably I can remove them
without problems.
>
> ...
>> +typedef enum iomux_pin_config {
>> + MUX_CONFIG_FUNC = 0, /*!< used as function */
>> + MUX_CONFIG_ALT1, /*!< used as alternate function 1 */
>> + MUX_CONFIG_ALT2, /*!< used as alternate function 2 */
>> + MUX_CONFIG_ALT3, /*!< used as alternate function 3 */
>> + MUX_CONFIG_ALT4, /*!< used as alternate function 4 */
>> + MUX_CONFIG_ALT5, /*!< used as alternate function 5 */
>> + MUX_CONFIG_ALT6, /*!< used as alternate function 6 */
>> + MUX_CONFIG_ALT7, /*!< used as alternate function 7 */
>> + MUX_CONFIG_SION = 0x1 << 4, /*!< used as LOOPBACK:MUX SION bit */
>> + MUX_CONFIG_GPIO = MUX_CONFIG_ALT5, /*!< used as GPIO */
>> +} iomux_pin_cfg_t;
>
> /*!< ???
I forget to remove them, thanks.
> ...
>> +/*!
>> + * Request ownership for an IO pin. This function has to be the first one
>> + * being called before that pin is used. The caller has to check the
>> + * return value to make sure it returns 0.
>> + *
>> + * @param pin a name defined by \b iomux_pin_name_t
>> + * @param cfg an input function as defined in \b
>> #iomux_pin_cfg_t
>
> \b ???
>
>> + * @param pin a name defined by \b iomux_pin_name_t
>> + * @param cfg an input function as defined in \b
>> #iomux_pin_cfg_t
>
> "iomux_pin_name_t", but "#iomux_pin_cfg_t" ???
I'll fix them
>> +typedef enum iomux_pins {
> ...
>> + MX35_PIN_A0 = _MXC_BUILD_NON_GPIO_PIN(0x28, 0x368),
>> + MX35_PIN_A1 = _MXC_BUILD_NON_GPIO_PIN(0x2C, 0x36C),
>> + MX35_PIN_A2 = _MXC_BUILD_NON_GPIO_PIN(0x30, 0x370),
>> + MX35_PIN_A3 = _MXC_BUILD_NON_GPIO_PIN(0x34, 0x374),
>> + MX35_PIN_A4 = _MXC_BUILD_NON_GPIO_PIN(0x38, 0x378),
>> + MX35_PIN_A5 = _MXC_BUILD_NON_GPIO_PIN(0x3C, 0x37C),
>> + MX35_PIN_A6 = _MXC_BUILD_NON_GPIO_PIN(0x40, 0x380),
>> + MX35_PIN_A7 = _MXC_BUILD_NON_GPIO_PIN(0x44, 0x384),
>> + MX35_PIN_A8 = _MXC_BUILD_NON_GPIO_PIN(0x48, 0x388),
>> + MX35_PIN_A9 = _MXC_BUILD_NON_GPIO_PIN(0x4C, 0x38C),
>> + MX35_PIN_A10 = _MXC_BUILD_NON_GPIO_PIN(0x50, 0x390),
>> + MX35_PIN_MA10 = _MXC_BUILD_NON_GPIO_PIN(0x54, 0x394),
>> + MX35_PIN_A11 = _MXC_BUILD_NON_GPIO_PIN(0x58, 0x398),
>> + MX35_PIN_A12 = _MXC_BUILD_NON_GPIO_PIN(0x5C, 0x39C),
>> + MX35_PIN_A13 = _MXC_BUILD_NON_GPIO_PIN(0x60, 0x3A0),
>> + MX35_PIN_A14 = _MXC_BUILD_NON_GPIO_PIN(0x64, 0x3A4),
>> + MX35_PIN_A15 = _MXC_BUILD_NON_GPIO_PIN(0x68, 0x3A8),
>> + MX35_PIN_A16 = _MXC_BUILD_NON_GPIO_PIN(0x6C, 0x3AC),
>> + MX35_PIN_A17 = _MXC_BUILD_NON_GPIO_PIN(0x70, 0x3B0),
>> + MX35_PIN_A18 = _MXC_BUILD_NON_GPIO_PIN(0x74, 0x3B4),
>> + MX35_PIN_A19 = _MXC_BUILD_NON_GPIO_PIN(0x78, 0x3B8),
>> + MX35_PIN_A20 = _MXC_BUILD_NON_GPIO_PIN(0x7C, 0x3BC),
>> + MX35_PIN_A21 = _MXC_BUILD_NON_GPIO_PIN(0x80, 0x3C0),
>> + MX35_PIN_A22 = _MXC_BUILD_NON_GPIO_PIN(0x84, 0x3C4),
>> + MX35_PIN_A23 = _MXC_BUILD_NON_GPIO_PIN(0x88, 0x3C8),
>> + MX35_PIN_A24 = _MXC_BUILD_NON_GPIO_PIN(0x8C, 0x3CC),
>> + MX35_PIN_A25 = _MXC_BUILD_NON_GPIO_PIN(0x90, 0x3D0),
>
> Note: the following remark is a question, NOT a change request:
>
> Would it not be possible to reduce all these terrible lists? As far
> as I can tell, the list is built sequentially, with both arguments to
> _MXC_BUILD_NON_GPIO_PIN() being incremented by 4 for the next
> register. This begs for automatic code generation, doesn't it?
I do not know if it helps. The list follows exactly the description in
user manual, and, if you can see a rule for MX35_PIN_A*, it is not so
simply to find one for other pins, specially for the MXC_BUILD_GPIO_PIN.
At least, the list is at the moment coherent for all i.MX processors
(ok, ugly for all). The name of the pin cannot be generated, and it is
the name found in manual. Miore as generated, the list is sorted....
Best regards,
Stefano Babic
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
