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

[Sander Speetjens <sander.speetjens@gmail.com>]

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?

>> +#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?

> 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.

Kind regards,

Sander Speetjens