Re: [PATCH 2/2] bsps: Remove uses of BSP-specific interrupt API
On 16/6/2023 3:36 pm, Sebastian Huber wrote:
> On 16.06.23 07:04, Chris Johns wrote:
>> On 16/6/2023 2:51 pm, Sebastian Huber wrote:
>>> On 16.06.23 03:49, Chris Johns wrote:
> diff --git a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> index 96b77907a6..bc211e37b6 100644
> --- a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> +++ b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> @@ -41,7 +41,7 @@
> #include
> #include
> -#include
> +#include
> #include
> @@ -227,7 +227,7 @@ static int ambapp_grlib_int_clear
> struct drvmgr_dev *dev,
> int irq)
> {
> - BSP_shared_interrupt_clear(irq);
> + (void) rtems_interrupt_clear(irq);
> return DRVMGR_OK;
Why ignore the return code of the clear and assume the result is OK?
This pattern is repeated in other places.
>>>
>>> This is how it is. I didn't want to introduce functional or API changes with
>>> this patch set.
>>
>> I do not see the API change.
>>
>> If the code here is looking through the interface provided to the
>> implementation
>> so it knows no error will be returned then it is functionally equivalent to
>> what
>> exists if the error is checked. Nothing changes. The problem with the change
>> is
>> using the interface implies the error is being checked and a change in the
>> implementation to return an error would not be handled here. Leaving a latent
>> issue like that does not seem right?
>
> The BSP_shared_interrupt*() functions have no return value and they didn't
> check
> anything. The patch was supposed to be a simple change from one API to
> another.
> I added some error checks to v2 of the patch set.
I appreciate this. The interfaces that return void have no where to go and that
is fine. The ones discussed and updated are different. I was not explicit about
this. The v2 means it is one less thing to deal with later.
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 2/2] bsps: Remove uses of BSP-specific interrupt API
On 16.06.23 07:04, Chris Johns wrote:
On 16/6/2023 2:51 pm, Sebastian Huber wrote:
On 16.06.23 03:49, Chris Johns wrote:
diff --git a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
index 96b77907a6..bc211e37b6 100644
--- a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
+++ b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
@@ -41,7 +41,7 @@
#include
#include
-#include
+#include
#include
@@ -227,7 +227,7 @@ static int ambapp_grlib_int_clear
struct drvmgr_dev *dev,
int irq)
{
- BSP_shared_interrupt_clear(irq);
+ (void) rtems_interrupt_clear(irq);
return DRVMGR_OK;
Why ignore the return code of the clear and assume the result is OK?
This pattern is repeated in other places.
This is how it is. I didn't want to introduce functional or API changes with
this patch set.
I do not see the API change.
If the code here is looking through the interface provided to the implementation
so it knows no error will be returned then it is functionally equivalent to what
exists if the error is checked. Nothing changes. The problem with the change is
using the interface implies the error is being checked and a change in the
implementation to return an error would not be handled here. Leaving a latent
issue like that does not seem right?
The BSP_shared_interrupt*() functions have no return value and they
didn't check anything. The patch was supposed to be a simple change from
one API to another. I added some error checks to v2 of the patch set.
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 2/2] bsps: Remove uses of BSP-specific interrupt API
On 16/6/2023 2:51 pm, Sebastian Huber wrote:
> On 16.06.23 03:49, Chris Johns wrote:
>>> diff --git a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
>>> b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
>>> index 96b77907a6..bc211e37b6 100644
>>> --- a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
>>> +++ b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
>>> @@ -41,7 +41,7 @@
>>> #include
>>> #include
>>> -#include
>>> +#include
>>> #include
>>> @@ -227,7 +227,7 @@ static int ambapp_grlib_int_clear
>>> struct drvmgr_dev *dev,
>>> int irq)
>>> {
>>> - BSP_shared_interrupt_clear(irq);
>>> + (void) rtems_interrupt_clear(irq);
>>> return DRVMGR_OK;
>> Why ignore the return code of the clear and assume the result is OK?
>>
>> This pattern is repeated in other places.
>
> This is how it is. I didn't want to introduce functional or API changes with
> this patch set.
I do not see the API change.
If the code here is looking through the interface provided to the implementation
so it knows no error will be returned then it is functionally equivalent to what
exists if the error is checked. Nothing changes. The problem with the change is
using the interface implies the error is being checked and a change in the
implementation to return an error would not be handled here. Leaving a latent
issue like that does not seem right?
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 2/2] bsps: Remove uses of BSP-specific interrupt API
On 16.06.23 03:49, Chris Johns wrote:
diff --git a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
index 96b77907a6..bc211e37b6 100644
--- a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
+++ b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
@@ -41,7 +41,7 @@
#include
#include
-#include
+#include
#include
@@ -227,7 +227,7 @@ static int ambapp_grlib_int_clear
struct drvmgr_dev *dev,
int irq)
{
- BSP_shared_interrupt_clear(irq);
+ (void) rtems_interrupt_clear(irq);
return DRVMGR_OK;
Why ignore the return code of the clear and assume the result is OK?
This pattern is repeated in other places.
This is how it is. I didn't want to introduce functional or API changes
with this patch set.
--
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 2/2] bsps: Remove uses of BSP-specific interrupt API
On 15/6/2023 10:46 pm, Sebastian Huber wrote:
> Update #3269.
> ---
> bsps/riscv/griscv/include/bsp.h | 4 ---
> bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c | 8 +++---
> bsps/sparc/erc32/include/bsp.h | 15 ---
> bsps/sparc/leon2/include/bsp.h | 15 ---
> bsps/sparc/leon3/include/bsp.h | 15 ---
> bsps/sparc/shared/drvmgr/leon2_amba_bus.c | 5 ++--
> bsps/sparc/shared/irq/irq-shared.c | 29 -
> spec/build/bsps/sparc/leon3/obj.yml | 1 -
> 8 files changed, 43 insertions(+), 49 deletions(-)
>
> diff --git a/bsps/riscv/griscv/include/bsp.h b/bsps/riscv/griscv/include/bsp.h
> index 9d6fb2a16f..c7d24839ca 100644
> --- a/bsps/riscv/griscv/include/bsp.h
> +++ b/bsps/riscv/griscv/include/bsp.h
> @@ -71,10 +71,6 @@ extern "C" {
>
> /* GRLIB driver functions */
>
> -extern void BSP_shared_interrupt_mask(int irq);
> -extern void BSP_shared_interrupt_clear(int irq);
> -extern void BSP_shared_interrupt_unmask(int irq);
> -
> /*
> * Network driver configuration for greth
> */
> diff --git a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> index 96b77907a6..bc211e37b6 100644
> --- a/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> +++ b/bsps/shared/grlib/drvmgr/ambapp_bus_grlib.c
> @@ -41,7 +41,7 @@
> #include
>
> #include
> -#include
> +#include
>
> #include
>
> @@ -227,7 +227,7 @@ static int ambapp_grlib_int_clear
> struct drvmgr_dev *dev,
> int irq)
> {
> - BSP_shared_interrupt_clear(irq);
> + (void) rtems_interrupt_clear(irq);
> return DRVMGR_OK;
Why ignore the return code of the clear and assume the result is OK?
This pattern is repeated in other places.
Chris
> }
>
> @@ -237,7 +237,7 @@ static int ambapp_grlib_int_mask
> int irq
> )
> {
> - BSP_shared_interrupt_mask(irq);
> + bsp_interrupt_vector_disable(irq);
> return DRVMGR_OK;
> }
>
> @@ -247,7 +247,7 @@ static int ambapp_grlib_int_unmask
> int irq
> )
> {
> - BSP_shared_interrupt_unmask(irq);
> + bsp_interrupt_vector_enable(irq);
> return DRVMGR_OK;
> }
>
> diff --git a/bsps/sparc/erc32/include/bsp.h b/bsps/sparc/erc32/include/bsp.h
> index fd453fb6c2..cb62661aa1 100644
> --- a/bsps/sparc/erc32/include/bsp.h
> +++ b/bsps/sparc/erc32/include/bsp.h
> @@ -142,7 +142,10 @@ static __inline__ int BSP_shared_interrupt_unregister
> * Arguments
> * irq System IRQ number
> */
> -extern void BSP_shared_interrupt_clear(int irq);
> +static inline void BSP_shared_interrupt_clear( int irq )
> +{
> + (void) rtems_interrupt_clear( (rtems_vector_number) irq );
> +}
>
> /* Enable Interrupt. This function will unmask the IRQ at the interrupt
> * controller. This is normally done by _register(). Note that this will
> @@ -151,7 +154,10 @@ extern void BSP_shared_interrupt_clear(int irq);
> * Arguments
> * irq System IRQ number
> */
> -extern void BSP_shared_interrupt_unmask(int irq);
> +static inline void BSP_shared_interrupt_unmask( int irq )
> +{
> + (void) rtems_interrupt_vector_enable( (rtems_vector_number) irq );
> +}
>
> /* Disable Interrupt. This function will mask one IRQ at the interrupt
> * controller. This is normally done by _unregister(). Note that this will
> @@ -160,7 +166,10 @@ extern void BSP_shared_interrupt_unmask(int irq);
> * Arguments
> * irq System IRQ number
> */
> -extern void BSP_shared_interrupt_mask(int irq);
> +static inline void BSP_shared_interrupt_mask( int irq )
> +{
> + (void) rtems_interrupt_vector_disable( (rtems_vector_number) irq );
> +}
>
> /*
> * Delay for the specified number of microseconds.
> diff --git a/bsps/sparc/leon2/include/bsp.h b/bsps/sparc/leon2/include/bsp.h
> index 510262206b..4a2f5967ef 100644
> --- a/bsps/sparc/leon2/include/bsp.h
> +++ b/bsps/sparc/leon2/include/bsp.h
> @@ -166,7 +166,10 @@ static __inline__ int BSP_shared_interrupt_unregister
> * Arguments
> * irq System IRQ number
> */
> -extern void BSP_shared_interrupt_clear(int irq);
> +static inline void BSP_shared_interrupt_clear( int irq )
> +{
> + (void) rtems_interrupt_clear( (rtems_vector_number) irq );
> +}
>
> /* Enable Interrupt. This function will unmask the IRQ at the interrupt
> * controller. This is normally done by _register(). Note that this will
> @@ -175,7 +178,10 @@ extern void BSP_shared_interrupt_clear(int irq);
> * Arguments
> * irq System IRQ number
> */
> -extern void BSP_shared_interrupt_unmask(int irq);
> +static inline void BSP_shared_interrupt_unmask( int irq )
> +{
> + (void) rtems_interrupt_vector_enable( (rtems_vector_number) irq );
> +}
>
> /* Disable Interrupt. This function will mask one IRQ at the interrupt
> * controller. This is normally done by _unregister(). Note that this will
> @@ -184,7 +190,10 @@ extern void BSP_shared_interrupt_unmask(int irq);
> *
