Hi!
> > + /* 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.
Sure, I'll change it.
> > + /*
> > + * 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:
I did not want to define to many things I do not use
(like alarm registers in v1).
> + regmap_write(sd2405al->regmap, SD2405AL_REG_CTR2,
> + SD2405AL_BIT_WRTC1);
Yes, if I go back to the v1 registers, these calls will be simplified.
> > + 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`.
They are there to keep the lines short.
> > +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()?
This is just a check to verify if the device is functioning correctly.
I tried to use CTR1.RTCF for this, as you suggested, but it only works if
the device is battery-powered. Since the device is configured to be writable
in `setup()`, this check must pass.
> > +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()?
I use the writable state as an indicator to see if the device is working
as expected.
> Also, you could rename sd2405al_setup() to sd2405al_set_writable() so it is
> clear that that's what it does.
It is setting the initial state of the registers as well.
János
