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

[Tony Dinh]

On Sun, Dec 19, 2021 at 11:23 PM Stefan Roese <sr at denx.de> wrote:
>
> 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.

Concur with you and Pali! Will merge the constants to .c file.

Thanks,
Tony

>
> Thanks,
> Stefan