Hi! Thank you four your review! > > +static int sd2405al_setup(struct sd2405al *sd2405al) > > +{ > > + unsigned int val; > > + int ret; > > I would still remove `val` and inline everything. Okay, I'll do that. > > +static int sd2405al_check_state(struct sd2405al *sd2405al) > > +{ > > + u8 data[2] = { 0 }; > > + int state; > > + int ret; > > + > > + ret = regmap_bulk_read(sd2405al->regmap, SD2405AL_REG_CTR1, data, 2); > > + if (ret < 0) { > > + dev_err(sd2405al->dev, "read failed: %d\n", ret); > > + return ret; > > + } > > + > > + /* CRT1 */ > > + state = (data[0] & (SD2405AL_BIT_WRTC2 | SD2405AL_BIT_WRTC3)) != 0; > > + > > + /* CTR2 */ > > + state &= (data[1] & SD2405AL_BIT_WRTC1) != 0; > > + > > + return state; > > Same here. I'll give it a try and see how it would look. > > +static int sd2405al_read_time(struct device *dev, struct rtc_time *time) > > +{ > > + u8 hour; > > This variable could also be inlined. Sure, I'll inline it. János