Re: [PATCH v2] drivers: rtc: Add driver for SD2405AL.

["Csókás Bence" <csokas.bence@prolan.hu>]

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