Hi!
On 6/19/24 13:48, Tóth János via B4 Relay wrote:
Changes in v2:
- Refactored based on reviewer's suggestions.
- I couldn't get the I2C IRQ to work on Raspberry Pi 4, so alarm is
skipped.
That's sad to see, but I guess better not to include untested code into
the kernel.
+/* Control registers */
+#define SD2405AL_REG_CTR_BASE 0x0F
+#define SD2405AL_REG_CTR1 0x00
+ /* enable write, order is important */
+ reg = SD2405AL_REG_CTR_BASE + SD2405AL_REG_CTR2;
+ val = SD2405AL_BIT_WRTC1;
Here I wouldn't do CTR_BASE + REG_CTRx, just leave it as in v1. I only
suggested the base+offset for the alarm registers, because their layout
is the same as the time registers, i.e. REG_ALARM_SEC = ALARM_BASE +
REG_SEC.
+ /*
+ * CTR2.IM = 0, single event alarm
+ * CTR2.S{1,0} = 0, disable INT pin
+ * CTR2.FOBAT = 0, interrupt enabled during battery backup mode
+ * CTR2.INTDE = 0, countdown interrupt disabled
+ * CTR2.INTAE = 0, alarm interrupt disabled
+ * CTR2.INTFE = 0, frequency interrupt disabled
+ */
+ ret = regmap_write(sd2405al->regmap, reg, val);
Maybe you could #define all these? Just a suggestion though. And even
`reg` and `val` are not really needed, you could just as easily use:
+ regmap_write(sd2405al->regmap, SD2405AL_REG_CTR2,
+ SD2405AL_BIT_WRTC1);
+ w32 = data[SD2405AL_REG_CTR1];
+ w32 &= (SD2405AL_BIT_WRTC2 | SD2405AL_BIT_WRTC3);
+
+ w1 = data[SD2405AL_REG_CTR2];
+ w1 &= SD2405AL_BIT_WRTC1;
+
+ return (w32 != 0) && (w1 != 0);
Here you could also do away with `w1` and `w32`.
> +static int sd2405al_read_time(struct device *dev, struct rtc_time *time)
> +{
> + u8 hour;
> + u8 data[SD2405AL_NUM_T_REGS] = { 0 };
> + struct sd2405al *sd2405al = dev_get_drvdata(dev);
> + int ret;
> +
> + /* check if the device is powered on/working */
> + ret = sd2405al_check_writable(sd2405al);
> + if (ret == 0) {
> + dev_err(sd2405al->dev, "invalid device status\n");
Do you really need the RTC to be writable for read_time()?
> +static int sd2405al_set_time(struct device *dev, struct rtc_time *time)
> +{
> + u8 data[SD2405AL_NUM_T_REGS];
> + struct sd2405al *sd2405al = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = sd2405al_check_writable(sd2405al);
> + if (ret == 0) {
> + dev_err(sd2405al->dev, "device is not writable\n");
Couldn't you set it writable here? Instead of doing it in probe()?
Also, you could rename sd2405al_setup() to sd2405al_set_writable() so it
is clear that that's what it does.
Bence
