| Message ID | 20211218042335.5865-7-mibodhi@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | arm: kirkwood: Add support for Pogoplug V4 | expand |
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@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@gmail.com> > +# > +# Based on > +# Marvell Semiconductor <www.marvell.com> > +# Written-by: Prafulla Wadaskar <prafulla@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@gmail.com> > + * > + * Based on > + * Copyright (C) 2012 David Purdy <david.c.purdy@gmail.com> > + * > + * Based on Kirkwood support: > + * (C) Copyright 2009 > + * Marvell Semiconductor <www.marvell.com> > + * Written-by: Prafulla Wadaskar <prafulla@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. > -- > 2.20.1 >
Hi Pali, On Sat, Dec 18, 2021 at 5:09 AM Pali Rohár <pali@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@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@gmail.com> > > +# > > +# Based on > > +# Marvell Semiconductor <www.marvell.com> > > +# Written-by: Prafulla Wadaskar <prafulla@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@gmail.com> > > + * > > + * Based on > > + * Copyright (C) 2012 David Purdy <david.c.purdy@gmail.com> > > + * > > + * Based on Kirkwood support: > > + * (C) Copyright 2009 > > + * Marvell Semiconductor <www.marvell.com> > > + * Written-by: Prafulla Wadaskar <prafulla@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. Thanks, Tony > > -- > > 2.20.1 > >
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@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@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@gmail.com> >>> +# >>> +# Based on >>> +# Marvell Semiconductor <www.marvell.com> >>> +# Written-by: Prafulla Wadaskar <prafulla@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@gmail.com> >>> + * >>> + * Based on >>> + * Copyright (C) 2012 David Purdy <david.c.purdy@gmail.com> >>> + * >>> + * Based on Kirkwood support: >>> + * (C) Copyright 2009 >>> + * Marvell Semiconductor <www.marvell.com> >>> + * Written-by: Prafulla Wadaskar <prafulla@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
On Sun, Dec 19, 2021 at 11:23 PM Stefan Roese <sr@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@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@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@gmail.com> > >>> +# > >>> +# Based on > >>> +# Marvell Semiconductor <www.marvell.com> > >>> +# Written-by: Prafulla Wadaskar <prafulla@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@gmail.com> > >>> + * > >>> + * Based on > >>> + * Copyright (C) 2012 David Purdy <david.c.purdy@gmail.com> > >>> + * > >>> + * Based on Kirkwood support: > >>> + * (C) Copyright 2009 > >>> + * Marvell Semiconductor <www.marvell.com> > >>> + * Written-by: Prafulla Wadaskar <prafulla@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
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@gmail.com> +# +# Based on +# Marvell Semiconductor <www.marvell.com> +# Written-by: Prafulla Wadaskar <prafulla@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@gmail.com> + * + * Based on + * Copyright (C) 2012 David Purdy <david.c.purdy@gmail.com> + * + * Based on Kirkwood support: + * (C) Copyright 2009 + * Marvell Semiconductor <www.marvell.com> + * Written-by: Prafulla Wadaskar <prafulla@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 */
Add board implementation header and Makefile for Pogoplug V4 Signed-off-by: Tony Dinh <mibodhi@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