[PATCH 6/8] arm: kirkwood: Pogoplug-V4 : Add board implementation header

[Stefan Roese]

Hi Tony,

On 12/18/21 22:47, Tony Dinh wrote:
> Hi Pali,
> 
> On Sat, Dec 18, 2021 at 5:09 AM Pali Rohár <pali at kernel.org> wrote:
>>
>> On Friday 17 December 2021 20:23:32 Tony Dinh wrote:
>>> Add board implementation header and Makefile for Pogoplug V4
>>>
>>> Signed-off-by: Tony Dinh <mibodhi at gmail.com>
>>> ---
>>>
>>>   board/cloudengines/pogo_v4/Makefile  | 10 ++++++++
>>>   board/cloudengines/pogo_v4/pogo_v4.h | 36 ++++++++++++++++++++++++++++
>>>   2 files changed, 46 insertions(+)
>>>   create mode 100644 board/cloudengines/pogo_v4/Makefile
>>>   create mode 100644 board/cloudengines/pogo_v4/pogo_v4.h
>>>
>>> diff --git a/board/cloudengines/pogo_v4/Makefile b/board/cloudengines/pogo_v4/Makefile
>>> new file mode 100644
>>> index 0000000000..511bf5ff7e
>>> --- /dev/null
>>> +++ b/board/cloudengines/pogo_v4/Makefile
>>> @@ -0,0 +1,10 @@
>>> +# SPDX-License-Identifier: GPL-2.0+
>>> +#
>>> +# (C) Copyright 2014-2021 Tony Dinh <mibodhi at gmail.com>
>>> +#
>>> +# Based on
>>> +# Marvell Semiconductor <www.marvell.com>
>>> +# Written-by: Prafulla Wadaskar <prafulla at marvell.com>
>>> +#
>>> +
>>> +obj-y        := pogo_v4.o
>>> diff --git a/board/cloudengines/pogo_v4/pogo_v4.h b/board/cloudengines/pogo_v4/pogo_v4.h
>>> new file mode 100644
>>> index 0000000000..bf3060de60
>>> --- /dev/null
>>> +++ b/board/cloudengines/pogo_v4/pogo_v4.h
>>> @@ -0,0 +1,36 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Copyright (C) 2014-2021 Tony Dinh <mibodhi at gmail.com>
>>> + *
>>> + * Based on
>>> + * Copyright (C) 2012 David Purdy <david.c.purdy at gmail.com>
>>> + *
>>> + * Based on Kirkwood support:
>>> + * (C) Copyright 2009
>>> + * Marvell Semiconductor <www.marvell.com>
>>> + * Written-by: Prafulla Wadaskar <prafulla at marvell.com>
>>> + */
>>> +
>>> +#ifndef __POGO_V4_H
>>> +#define __POGO_V4_H
>>> +
>>> +#include <linux/bitops.h>
>>> +
>>> +/* GPIO configuration */
>>> +#define POGO_V4_OE_LOW                               (~(0))
>>> +#define POGO_V4_OE_HIGH                              (~(0))
>>> +#define POGO_V4_OE_VAL_LOW                   BIT(29)
>>> +#define POGO_V4_OE_VAL_HIGH                  0
>>> +
>>> +/* PHY related */
>>> +#define MV88E1116_LED_FCTRL_REG                      10
>>> +#define MV88E1116_CPRSP_CR3_REG                      21
>>> +#define MV88E1116_MAC_CTRL_REG                       21
>>> +#define MV88E1116_PGADR_REG                  22
>>> +#define MV88E1116_RGMII_TXTM_CTRL            BIT(4)
>>> +#define MV88E1116_RGMII_RXTM_CTRL            BIT(5)
>>> +
>>> +/* button */
>>> +#define BTN_EJECT                            29
>>> +
>>> +#endif /* __POGO_V4_H */
>>
>> Hello! As this pogo_v4.h include file is used only in pogo_v4.c source
>> file and contains only few defines, you can move all these defines
>> directly into pogo_v4.c source file. There is no need to export these
>> constants if they are not used by other files or modules.
> 
> Sure, but that'll make the .c file harder to read? We've been using
> the .h file since the old days, I think mostly for readability. This
> is a small header file, but for some other boards, the header file is
> quite large.

I fail to see why moving these few lines from the header to the .c file
makes the code harder to read. Especially if the macros / defines are
only used in one specific file, it makes sense to place them in this
file IMHO.

Thanks,
Stefan