Re: [PATCH rtems v1 2/2] bsps/zynqmp: Added I2C support for ZynqMP
On 25/8/21 12:05 am, Stephen Clark wrote: > This approach was also used in bsps/arm/xilinx-zynq/include/bsp/i2c.h. I kept > it specifically for consistency; I assumed it was the standard approach, but > your response makes me think it's not. I am also not sure and why I asked :) > Is there a case to be made for breaking the register functions in both i2c.h > files out into their own c files? I do not know. There can be a tendency to place things into line functions that do not need to be. It saves work by not need to touch the build system. If the code is being moved should it be moved to a C file? This is the reason I raised this. I am happy to follow what ever Joel thinks. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
RE: [PATCH rtems v1 2/2] bsps/zynqmp: Added I2C support for ZynqMP
Chris,
This approach was also used in bsps/arm/xilinx-zynq/include/bsp/i2c.h. I kept
it specifically for consistency; I assumed it was the standard approach, but
your response makes me think it's not.
Is there a case to be made for breaking the register functions in both i2c.h
files out into their own c files?
Thanks,
Stephen
> -Original Message-
> From: Chris Johns
> Sent: Monday, August 23, 2021 9:38 PM
> To: Stephen Clark ; devel@rtems.org
> Subject: Re: [PATCH rtems v1 2/2] bsps/zynqmp: Added I2C support for ZynqMP
>
> On 24/8/21 8:24 am, Stephen Clark wrote:
> > Added I2C drivers for ZynqMP and updated build system accordingly.
> > ---
> > bsps/aarch64/xilinx-zynqmp/include/bsp.h | 4 ++
> > bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h | 63
> > +++ bsps/aarch64/xilinx-zynqmp/include/bsp/irq.h | 2 +
> > bsps/aarch64/xilinx-zynqmp/start/bspstart.c | 10 +++
> > spec/build/bsps/aarch64/xilinx-zynqmp/grp.yml | 4 ++
> > .../bsps/aarch64/xilinx-zynqmp/grp_zu3eg.yml | 2 +
> > .../aarch64/xilinx-zynqmp/objcadencei2c.yml | 19 ++
> > .../bsps/aarch64/xilinx-zynqmp/optclki2c0.yml | 19 ++
> > .../bsps/aarch64/xilinx-zynqmp/optclki2c1.yml | 19 ++
> > 9 files changed, 142 insertions(+)
> > create mode 100644 bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> > create mode 100644
> > spec/build/bsps/aarch64/xilinx-zynqmp/objcadencei2c.yml
> > create mode 100644
> > spec/build/bsps/aarch64/xilinx-zynqmp/optclki2c0.yml
> > create mode 100644
> > spec/build/bsps/aarch64/xilinx-zynqmp/optclki2c1.yml
> >
> > diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> > b/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> > index 83f2e2f4e4..6d49b9ad2a 100644
> > --- a/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> > +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> > @@ -70,6 +70,10 @@ BSP_START_TEXT_SECTION void
> > zynqmp_setup_mmu_and_cache(void);
> >
> > void zynqmp_debug_console_flush(void);
> >
> > +uint32_t zynqmp_clock_i2c0(void);
> > +
> > +uint32_t zynqmp_clock_i2c1(void);
> > +
> > #ifdef __cplusplus
> > }
> > #endif /* __cplusplus */
> > diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> > b/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> > new file mode 100644
> > index 00..e09747d414
> > --- /dev/null
> > +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> > @@ -0,0 +1,63 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (C) 2021 On-Line Applications Research (OAR)
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *notice, this list of conditions and the following disclaimer in the
> > + *documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> LIMITED
> > +TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR
> > +PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> > +CONTRIBUTORS BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY,
> > +OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> PROCUREMENT
> > +OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> > +BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> > +WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > +OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > +ADVISED OF THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#ifndef LIBBSP_ARM_XILINX_ZYNQ_I2C_H
> > +#define LIBBSP_ARM_XILINX_ZYNQ_I2C_H
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> > +static inline int zynqmp_register_i2c_0(void) {
> > + return i2c_bus_register_cadence(
> > +"/dev/i2c-0",
> > +0x00FF02,
> > +zynqmp_clock_i2c0(),
> > +ZYNQMP_IRQ_I2C_0
> > + );
> > +}
> > +
> > +static inline int zynqmp_register_i2c_1(void) {
> > + return i2c_bus_register_cadence(
> > +"/dev/i2c-1",
> > +0x00FF03,
> > +zynqmp_clock_i2c1(),
> > +ZYNQMP_IRQ_I2C_1
> > + );
>
> I know these are currently inlined but I do not know why they are. It is the
> only
> BSP that does this. Should they be moved to a .c file seem they are being
> touched?
>
> Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH rtems v1 2/2] bsps/zynqmp: Added I2C support for ZynqMP
On 24/8/21 8:24 am, Stephen Clark wrote:
> Added I2C drivers for ZynqMP and updated build system accordingly.
> ---
> bsps/aarch64/xilinx-zynqmp/include/bsp.h | 4 ++
> bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h | 63 +++
> bsps/aarch64/xilinx-zynqmp/include/bsp/irq.h | 2 +
> bsps/aarch64/xilinx-zynqmp/start/bspstart.c | 10 +++
> spec/build/bsps/aarch64/xilinx-zynqmp/grp.yml | 4 ++
> .../bsps/aarch64/xilinx-zynqmp/grp_zu3eg.yml | 2 +
> .../aarch64/xilinx-zynqmp/objcadencei2c.yml | 19 ++
> .../bsps/aarch64/xilinx-zynqmp/optclki2c0.yml | 19 ++
> .../bsps/aarch64/xilinx-zynqmp/optclki2c1.yml | 19 ++
> 9 files changed, 142 insertions(+)
> create mode 100644 bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> create mode 100644 spec/build/bsps/aarch64/xilinx-zynqmp/objcadencei2c.yml
> create mode 100644 spec/build/bsps/aarch64/xilinx-zynqmp/optclki2c0.yml
> create mode 100644 spec/build/bsps/aarch64/xilinx-zynqmp/optclki2c1.yml
>
> diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> b/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> index 83f2e2f4e4..6d49b9ad2a 100644
> --- a/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp.h
> @@ -70,6 +70,10 @@ BSP_START_TEXT_SECTION void
> zynqmp_setup_mmu_and_cache(void);
>
> void zynqmp_debug_console_flush(void);
>
> +uint32_t zynqmp_clock_i2c0(void);
> +
> +uint32_t zynqmp_clock_i2c1(void);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> diff --git a/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> b/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> new file mode 100644
> index 00..e09747d414
> --- /dev/null
> +++ b/bsps/aarch64/xilinx-zynqmp/include/bsp/i2c.h
> @@ -0,0 +1,63 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (C) 2021 On-Line Applications Research (OAR)
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *notice, this list of conditions and the following disclaimer in the
> + *documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef LIBBSP_ARM_XILINX_ZYNQ_I2C_H
> +#define LIBBSP_ARM_XILINX_ZYNQ_I2C_H
> +
> +#include
> +#include
> +#include
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +static inline int zynqmp_register_i2c_0(void)
> +{
> + return i2c_bus_register_cadence(
> +"/dev/i2c-0",
> +0x00FF02,
> +zynqmp_clock_i2c0(),
> +ZYNQMP_IRQ_I2C_0
> + );
> +}
> +
> +static inline int zynqmp_register_i2c_1(void)
> +{
> + return i2c_bus_register_cadence(
> +"/dev/i2c-1",
> +0x00FF03,
> +zynqmp_clock_i2c1(),
> +ZYNQMP_IRQ_I2C_1
> + );
I know these are currently inlined but I do not know why they are. It is the
only BSP that does this. Should they be moved to a .c file seem they are being
touched?
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
