RTEMS | bsp/aarch64/raspberrypi4: Add PWM peripheral support (!509)

[Christian Mauderer (@c-mauderer)]

Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509 was reviewed by Christian Mauderer

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-pwm.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124670

 > +#include "bsp/utility.h"
 > +
 > +typedef enum { raspberrypi_pwm0, raspberrypi_pwm1 } raspberrypi_pwm_channel;

To be consistent in one file, I would add line breaks in the enum. So

```
typedef enum {
  raspberrypi_pwm0,
  raspberrypi_pwm1,
} raspberrypi_pwm_channel;
```

Of course if the formatter told you something different, just ignore that suggestion.

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-pwm.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124671

 > +  uint32_t                range;   /**< Range for the PWM signal */
 > +  uint32_t                data;    /**< Data to be written to the PWM */
 > +} raspberrypi_pwm_t;

I don't see where you use that type. Do I miss something?

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-pwm.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124672

 > +#define S_WERR1 BSP_BIT32( 2 )
 > +#define S_EMPT1 BSP_BIT32( 1 )
 > +#define S_FULL1 BSP_BIT32( 0 )

Do you want that the user of the API can use these registers directly? If yes, please add some Doxygen documentation (like a group for all register offsets). If no, I would suggest moving them either into a header that is not installed or into the C file.

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/include/bsp/raspberrypi-pwm.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124673

 > +  uint32_t                range,
 > +  uint32_t                data
 > +);

Can you please add a doxygen description for these?

Please don't just tell me in the description that `master` is a `PWM master` like you did in the (unused) structure earlier. That part is already obvious from the parameter and function. A better description for the `master` parameter could be for example `@a master selects the hardware instance to be used.`

For the enums, the possible values are clear. But for the other parameters, maybe also add the possible range. For example, for `divisor`, tell me how I get a value. Which clock is divided? If I have to understand the PWM module to use the parameter, it's also OK if you tell me `Please check the reference manual and hardware data sheet for the valid range`. Or maybe `Use @ref pwm_divisor_from_Hz function to calculate the value` if you have a helper function for it.

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/pwm/raspberrypi-pwm.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124674

 > +    }
 > +    BCM2835_REG( BCM2711_PWM1_BASE + BCM2711_PWM_CONTROL ) = control;
 > +  }

You have a bit of repetition in your code here. I would suggest removing that:
```
uint32_t control_addr;
uint32_t control;

if (master == raspberrypi_pwm_master0) {
  control_addr = BCM2711_PWM0_BASE + BCM2711_PWM_CONTROL;
} else {
  control_addr = BCM2711_PWM1_BASE + BCM2711_PWM_CONTROL;
}

control = BCM2835_REG( control_addr );
if ( channel == raspberrypi_pwm0 ) {
  ...
```

You have that pattern a few more times in the following code. You might think about simplifying these parts by not passing a `raspberrypi_pwm_master master` to your functions but instead just pass the base address in a `uintptr_t pwm_base`. That would also work if the next generation of the chip has three or four PWMs instead of only two.

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/pwm/raspberrypi-pwm.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124675

 > +  if ( channel != raspberrypi_pwm0 && channel != raspberrypi_pwm1 ) {
 > +    return RTEMS_INVALID_NUMBER;
 > +  }

You repeat that block quite a few times in your code. Maybe add a inline function or a define for it instead. Something like

```
static inline bool pwm_channel_valid(raspberrypi_pwm_channel channel) {
    return ( channel != raspberrypi_pwm0 && channel != raspberrypi_pwm1 );
}
```

or

```
#define PWM_CHANNEL_VALID(channel) ( (channel) != raspberrypi_pwm0 && (channel) != raspberrypi_pwm1 )
```

If you need to change it later to support (for example) a third channel on another controller, that is simpler to change.

--
  
Christian Mauderer started a new discussion on bsps/aarch64/raspberrypi/pwm/raspberrypi-pwm.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509#note_124676

 > +  rtems_status_code sc;
 > +
 > +  sc = rpi_pwm_set_clock( divisor );

The clock does not depend on the master, right? In that case you should warn your user in the decription of the function that calling the `rpi_pwm_init` will change the clock for all pwm modules in the controller.


-- 
View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/509
You're receiving this email because of your account on gitlab.rtems.org.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/bugs/attachments/20250615/cb756af0/attachment-0001.htm>