Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Stefan Wahren <wahrenst@gmx.net>]

Am 30.09.26 um 19:29 schrieb Sander Speetjens:
> Hi Stefan,
>
>>> +    // Check if our model is a Raspberry Pi 5, as the RTC is only 
>>> present on that model.
>>> +    if (!of_machine_is_compatible("brcm,bcm2712"))
>>> +        return;
>> I don't like the comment, because it doesn't check for Raspberry Pi 
>> 5, the code checks for a BCM2712 SoC which could also be on a CM5 or 
>> a Raspberry Pi 500+ 
> I changed it to BCM2712, but isn't RPi 5 the generation/platform name 
> and RPi 5b the specific board?
The specific model name for the Raspberry Pi 5 board is "Raspberry Pi 5" 
and it's devicetree compatible is "raspberrypi,5-model-b". Both doesn't 
have anything to do with the generation. "brcm,bcm2712" is the used 
System on chip (SoC), which is common for all board of the 5th generation.

So you can write something like
// Check if our Raspberry Pi board is from the 5th gen ...
>
>>> +#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
>>> +#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
>> Was there a specific reason to not include these defines to 
>> include/soc/bcm2835/raspberry-pi-firmware.h as all the others 
>> firmware tags? 
> No specific reason, this is how Raspberry Pi originally did it, I 
> moved them to raspberrypi-firmware.h
>
>>> +
>>> +enum {
>>> +    RTC_TIME,
>>> +    RTC_ALARM,
>>> +    RTC_ALARM_PENDING,
>>> +    RTC_ALARM_ENABLE,
>>> +    RTC_BBAT_CHG_VOLTS,
>>> +    RTC_BBAT_CHG_VOLTS_MIN,
>>> +    RTC_BBAT_CHG_VOLTS_MAX,
>>> +    RTC_BBAT_VOLTS
>>> +};
>> Hm, an enum suggests that we simply can add / remove items, but 
>> that's not the case. The Raspberry Pi firmware defines the values.
> Should I also move those to raspberrypi-firmware.h or is this not what 
> you imply?
No this wasn't my implication. One way to make this ABI more explicit 
would be to use #define. I think there is no need to move to 
raspberrypi-firmware.
>
>> In case rpi_rtc_set_limits() fails, both limit would be initialized 
>> with 0 and this always fail. Maybe we should dev_warn to 
>> rpi_rtc_set_limits()?
>
> I added a warning for failing to set the min and max values.
>
>> Just to be sure, both calls are optional and not critical for the 
>> drivers function?
>> Why does rpi_rtc_set_charge_voltage have a return value at all?
> I removed the return value
>
>>> +MODULE_ALIAS("platform:raspberrypi-rtc");
>> Is this really necessary for module autoloading?
> I'm not sure, it was used in the previous versions when using the 
> platform device register on the register_clk driver before it had a 
> custom dt node.
You can test by removing the line and compile it as a module. In case 
the RTC driver is still automatically loaded, we can drop it.
>
>
> Kind regards,
>
> Sander Speetjens