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

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

Hi!

> +static int sd2405al_setup(struct sd2405al *sd2405al)
> +{
> +	unsigned int val;
> +	int ret;

I would still remove `val` and inline everything.

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

> +static int sd2405al_read_time(struct device *dev, struct rtc_time *time)
> +{
> +	u8 hour;

This variable could also be inlined.

Other than that, LGTM. Though I don't have this chip, I can only go by 
the linked datasheet.

On 6/20/24 11:55, Tóth János via B4 Relay wrote:
 > From: Tóth János <gomba007@gmail.com>
 >
 > Add support for the DFRobot SD2405AL I2C RTC Module.
 >
 > Datasheet:
 > 
https://image.dfrobot.com/image/data/TOY0021/SD2405AL%20datasheet%20(Angelo%20v0.1).pdf
 >
 > Product:
 > 	https://www.dfrobot.com/product-1600.html
 >
 > To instantiate (assuming device is connected to I2C-1)
 > as root:
 > 	echo sd2405al 0x32 > /sys/bus/i2c/devices/i2c-1/new_device
 > as user:
 > 	echo 'sd2405al 0x32' | sudo tee /sys/class/i2c-adapter/i2c-1/new_device
 >
 > The driver is tested with:
 > 	+ hwclock
 > 	+ tools/testing/selftests/rtc/setdate
 > 	+ tools/testing/selftests/rtc/rtctest
 >
 > Signed-off-by: Tóth János <gomba007@gmail.com>

Reviewed-by: Csókás, Bence <csokas.bence@prolan.hu>