Re: [PATCH] rtc: sd3078: add alarm function

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hello,

On 04/12/2020 22:23:04+0800, zoro wrote:
> Complete common functions of sd3078.

Please wirte a better commit message

> 
> Signed-off-by: zoro <long17.cool@163.com>
> ---
>  drivers/rtc/rtc-sd3078.c | 319 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 313 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sd3078.c b/drivers/rtc/rtc-sd3078.c
> index a7aa943..3626a43 100644
> --- a/drivers/rtc/rtc-sd3078.c
> +++ b/drivers/rtc/rtc-sd3078.c
> @@ -7,9 +7,11 @@
>  #include <linux/bcd.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> +#include <linux/of_gpio.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
>  #include <linux/slab.h>
> +#include <linux/version.h>
>  
>  #define SD3078_REG_SC			0x00
>  #define SD3078_REG_MN			0x01
> @@ -19,13 +21,38 @@
>  #define SD3078_REG_MO			0x05
>  #define SD3078_REG_YR			0x06
>  
> -#define SD3078_REG_CTRL1		0x0f

this is an unrelated change

> +#define SD3078_ALARM_SC			0x07
> +#define SD3078_ALARM_MN			0x08
> +#define SD3078_ALARM_HR			0x09
> +#define SD3078_ALARM_DW			0x0A
> +#define SD3078_ALARM_DM			0x0B
> +#define SD3078_ALARM_MO			0x0C
> +#define SD3078_ALARM_YR			0x0D
> +#define SD3078_ALARM_EN			0x0E
> +
> +#define SD3078_REG_CTRL1		0x0F
>  #define SD3078_REG_CTRL2		0x10
>  #define SD3078_REG_CTRL3		0x11
>  
> -#define KEY_WRITE1		0x80
> -#define KEY_WRITE2		0x04
> -#define KEY_WRITE3		0x80
> +#define KEY_WRITE1				0x80
> +#define KEY_WRITE2				0x04
> +#define KEY_WRITE3				0x80

Unrelated changes

> +
> +#define BIT_IM					0x40
> +#define BIT_INTS1				0x20
> +#define BIT_INTS0				0x10
> +#define BIT_INTDE				0x04
> +#define BIT_INTAE				0x02
> +#define BIT_INTFE				0x01
> +#define BIT_INTAF				0x20
> +
> +#define BIT_EN_EAD				0x10
> +#define BIT_EN_EAH				0x04
> +#define BIT_EN_EAMN				0x02
> +#define BIT_EN_EAS				0x01
> +
> +#define INT_BITS	(BIT_IM | BIT_INTS1 | BIT_INTS0 | \
> +			BIT_INTDE | BIT_INTAE | BIT_INTFE)
>  
>  #define NUM_TIME_REGS   (SD3078_REG_YR - SD3078_REG_SC + 1)
>  
> @@ -39,6 +66,10 @@
>  struct sd3078 {
>  	struct rtc_device	*rtc;
>  	struct regmap		*regmap;
> +	int			irq_gpio;

This member is not necessary

> +	bool			suspended;
> +	struct work_struct	work;
> +	struct i2c_client	*client;
>  };
>  
>  /*
> @@ -113,7 +144,7 @@ static int sd3078_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  
>  	tm->tm_mday = bcd2bin(rtc_data[SD3078_REG_DM] & 0x3F);
>  	tm->tm_wday = rtc_data[SD3078_REG_DW] & 0x07;
> -	tm->tm_mon	= bcd2bin(rtc_data[SD3078_REG_MO] & 0x1F) - 1;
> +	tm->tm_mon  = bcd2bin(rtc_data[SD3078_REG_MO] & 0x1F) - 1;

Unrelated change

>  	tm->tm_year = bcd2bin(rtc_data[SD3078_REG_YR]) + 100;
>  
>  	return 0;
> @@ -126,6 +157,11 @@ static int sd3078_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	struct sd3078 *sd3078 = i2c_get_clientdata(client);
>  	int ret;
>  
> +	if (tm->tm_year < 100) {
> +		dev_err(dev, "year should be equal to or greate than 2000!\n");
> +		return -EINVAL;
> +	}
> +

This is an unrelated change, it introduces a bug and is unnecessary
since range_max is set properly.

>  	rtc_data[SD3078_REG_SC] = bin2bcd(tm->tm_sec);
>  	rtc_data[SD3078_REG_MN] = bin2bcd(tm->tm_min);
>  	rtc_data[SD3078_REG_HR] = bin2bcd(tm->tm_hour) | 0x80;
> @@ -152,9 +188,179 @@ static int sd3078_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	return 0;
>  }
>  
> +static int sd3078_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	unsigned char hour;
> +	int value;
> +	unsigned char alarm_data[NUM_TIME_REGS] = {0};
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = regmap_bulk_read(sd3078->regmap, SD3078_ALARM_SC, alarm_data,
> +			       NUM_TIME_REGS);
> +	if (ret < 0) {
> +		dev_err(dev, "reading from RTC failed with err:%d\n", ret);

I don't think this message adds any value, please remove it.

> +		return ret;
> +	}
> +
> +	alarm->time.tm_sec = bcd2bin(alarm_data[SD3078_ALARM_SC - SD3078_ALARM_SC] & 0x7F);
> +	alarm->time.tm_min = bcd2bin(alarm_data[SD3078_ALARM_MN - SD3078_ALARM_SC] & 0x7F);
> +
> +	/*
> +	 * The sd3078 supports 12/24 hour mode.
> +	 * When getting time,
> +	 * we need to convert the 12 hour mode to the 24 hour mode.
> +	 */
> +	hour = alarm_data[SD3078_ALARM_HR - SD3078_ALARM_SC];
> +	if (hour & 0x80) /* 24H MODE */
> +		alarm->time.tm_hour = bcd2bin(hour & 0x3F);
> +	else if (hour & 0x20) /* 12H MODE PM */
> +		alarm->time.tm_hour = bcd2bin(hour & 0x1F) + 12;
> +	else /* 12H MODE AM */
> +		alarm->time.tm_hour = bcd2bin(hour & 0x1F);
> +
> +	alarm->time.tm_mday = bcd2bin(alarm_data[SD3078_ALARM_DM - SD3078_ALARM_SC] & 0x3F);
> +	alarm->time.tm_wday = alarm_data[SD3078_ALARM_DW - SD3078_ALARM_SC] & 0x07;
> +	alarm->time.tm_mon  = bcd2bin(alarm_data[SD3078_ALARM_MO - SD3078_ALARM_SC] & 0x1F) - 1;
> +	alarm->time.tm_year = bcd2bin(alarm_data[SD3078_ALARM_YR - SD3078_ALARM_SC]) + 100;
> +
> +	ret = regmap_read(sd3078->regmap, SD3078_REG_CTRL2, &value);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC read ctrl2 failed with err:%d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	if ((value & (INT_BITS)) == (BIT_INTS0 | BIT_INTAE))
> +		alarm->enabled = 1;
> +	else
> +		alarm->enabled = 0;
> +
> +	ret = regmap_read(sd3078->regmap, SD3078_REG_CTRL1, &value);
> +	if (ret < 0) {
> +		dev_err(dev, "RTC read ctrl1 failed with err:%d\n", ret);

Ditto

> +		return ret;
> +	}
> +
> +	if ((value & (BIT_INTAF)) == BIT_INTAF)
> +		alarm->pending = 1;
> +	else
> +		alarm->pending = 0;
> +
> +	return 0;
> +}
> +
> +static int sd3078_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	unsigned char alarm_data[NUM_TIME_REGS];
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (alarm->time.tm_year < 100) {
> +		dev_err(dev, "year should be equal to or greate than 2000!\n");
> +		return -EINVAL;
> +	}
> +

This check is unnecessary

> +	if (client->irq <= 0)
> +		return -EINVAL;
> +
> +	alarm_data[SD3078_ALARM_SC - SD3078_ALARM_SC] = bin2bcd(alarm->time.tm_sec);
> +	alarm_data[SD3078_ALARM_MN - SD3078_ALARM_SC] = bin2bcd(alarm->time.tm_min);
> +	alarm_data[SD3078_ALARM_HR - SD3078_ALARM_SC] = bin2bcd(alarm->time.tm_hour) | 0x80;
> +	alarm_data[SD3078_ALARM_DM - SD3078_ALARM_SC] = bin2bcd(alarm->time.tm_mday);
> +
> +#if WRITE_PROTECT_EN
> +	sd3078_enable_reg_write(sd3078);
> +#endif
> +
> +	ret = regmap_bulk_write(sd3078->regmap, SD3078_ALARM_SC, alarm_data,
> +				SD3078_ALARM_DM - SD3078_ALARM_SC + 1);
> +	if (ret < 0) {
> +		dev_err(dev, "writing to RTC alarm failed with err:%d\n", ret);

I really want to cut down the number of error strungs in this driver,
they are not useful.

> +		return ret;
> +	}
> +
> +	/*
> +	 * set the day, hour, minute and second alarm enable
> +	 * this operation will clear the alarm interrupt flag
> +	 */
> +	ret = regmap_update_bits(sd3078->regmap, SD3078_ALARM_EN,
> +				 BIT_EN_EAD | BIT_EN_EAH | BIT_EN_EAMN | BIT_EN_EAS,
> +				 BIT_EN_EAD | BIT_EN_EAH | BIT_EN_EAMN | BIT_EN_EAS);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "RTC set alarm enable failed with err:%d\n", ret);
> +		return ret;
> +	}
> +
> +	if (alarm->enabled) {
> +		regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL2,
> +				   INT_BITS, BIT_INTS0 | BIT_INTAE);
> +	} else {
> +		regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL2,
> +				   INT_BITS, 0);
> +	}
> +

You should probably call sd3078_alarm_irq_enable instead.

> +#if WRITE_PROTECT_EN
> +	sd3078_disable_reg_write(sd3078);
> +#endif
> +
> +	return 0;
> +}
> +
> +static int sd3078_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +
> +	if (client->irq <= 0)
> +		return -EINVAL;
> +
> +	if (enabled) {
> +		/*
> +		 * CTRL2
> +		 * :IM(bit6)
> +		 *    0 : single event alarm
> +		 *        Output low level until INTAF bit is cleared.
> +		 *    1 : periodic alarm
> +		 *        Output low-level active pulses with 250ms width
> +		 *        until the interrupt enable bit is cleared
> +		 *
> +		 * :INTS1(bit5) :INTS0(bit4)
> +		 *       0          0     : battery alarm
> +		 *       0          1     : alarm interrupt output
> +		 *       1          0     : frequency interrupt output
> +		 *       1          1     : countdown interrupt output
> +		 *
> +		 * :INTDE(bit2)
> +		 *       0/1 : enable/disable countdown interrupt
> +		 *
> +		 * :INTFE(bit1)
> +		 *       0/1 : enable/disable frequency interrupt
> +		 *
> +		 * :INTAE(bit0)
> +		 *       0/1 : enable/disable alarm interrupt
> +		 */
> +
> +		regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL2,
> +				   INT_BITS, BIT_INTS0 | BIT_INTAE);
> +		enable_irq(client->irq);
> +	} else {
> +		regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL2,
> +				   INT_BITS, 0);
> +		disable_irq(client->irq);
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct rtc_class_ops sd3078_rtc_ops = {
>  	.read_time	= sd3078_rtc_read_time,
>  	.set_time	= sd3078_rtc_set_time,
> +	.read_alarm	= sd3078_rtc_read_alarm,
> +	.set_alarm	= sd3078_rtc_set_alarm,
> +	.alarm_irq_enable = sd3078_alarm_irq_enable,
>  };
>  
>  static const struct regmap_config regmap_config = {
> @@ -163,10 +369,57 @@ static const struct regmap_config regmap_config = {
>  	.max_register = 0x11,
>  };
>  
> +static void sd3078_work(struct work_struct *work)
> +{
> +	struct sd3078 *sd3078 = container_of(work, struct sd3078, work);
> +
> +	/* clean interrupt flag */
> +	regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL1,
> +			   BIT_INTAF, 0);
> +
> +	rtc_update_irq(sd3078->rtc, 1, RTC_AF | RTC_IRQF);
> +
> +	enable_irq(sd3078->client->irq);
> +}
> +
> +int sd3078_get_irq(struct i2c_client *client)
> +{
> +	int ret;
> +	struct device_node *node = client->dev.of_node;
> +	struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +
> +	sd3078->irq_gpio = of_get_named_gpio(node, "irq_gpio", 0);
> +	if (!gpio_is_valid(sd3078->irq_gpio)) {
> +		dev_err(&client->dev, "invalid gpio : %d\n", sd3078->irq_gpio);
> +		return 0;
> +	}
> +
> +	ret = devm_gpio_request_one(&client->dev, sd3078->irq_gpio, GPIOF_DIR_IN, "irq_gpio");
> +		if (ret) {
> +			dev_err(&client->dev, "can't get irq_gpio\n");
> +		return 0;
> +	}
> +
> +	return gpio_to_irq(sd3078->irq_gpio);
> +}
> +
> +static irqreturn_t sd3078_irq(int irq, void *dev_id)
> +{
> +	struct i2c_client *client = dev_id;
> +	struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +
> +	disable_irq_nosync(irq);
> +
> +	if (!sd3078->suspended)
> +		schedule_work(&sd3078->work);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int sd3078_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	int ret;
> +	int ret = 0;
>  	struct sd3078 *sd3078;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> @@ -176,6 +429,10 @@ static int sd3078_probe(struct i2c_client *client,
>  	if (!sd3078)
>  		return -ENOMEM;
>  
> +	INIT_WORK(&sd3078->work, sd3078_work);
> +
> +	sd3078->client = client;
> +
>  	sd3078->regmap = devm_regmap_init_i2c(client, &regmap_config);
>  	if (IS_ERR(sd3078->regmap)) {
>  		dev_err(&client->dev, "regmap allocation failed\n");
> @@ -183,12 +440,14 @@ static int sd3078_probe(struct i2c_client *client,
>  	}
>  
>  	i2c_set_clientdata(client, sd3078);
> +	dev_set_drvdata(&client->dev, sd3078);
>  
>  	sd3078->rtc = devm_rtc_allocate_device(&client->dev);
>  	if (IS_ERR(sd3078->rtc))
>  		return PTR_ERR(sd3078->rtc);
>  
>  	sd3078->rtc->ops = &sd3078_rtc_ops;
> +

Unrelated change

>  	sd3078->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>  	sd3078->rtc->range_max = RTC_TIMESTAMP_END_2099;
>  
> @@ -196,10 +455,57 @@ static int sd3078_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	if (client->irq <= 0) {
> +		/* get the interrupt number according to gpio in dts */
> +		client->irq = sd3078_get_irq(client);

I don't get why you would want to use irq_gpio instead of the correct
interrupts property, can you elaborate?

> +	}
> +
> +	if (client->irq) {
> +		ret = devm_request_irq(&client->dev, client->irq, sd3078_irq,
> +				       IRQF_TRIGGER_FALLING, client->name, client);

Please use a threaded interrupt, this removes the need for the
enable/diasble_irq dance and the work.

> +		if (ret)
> +			dev_err(&client->dev, "unable to request IRQ\n");
> +	}
> +
> +	if (!device_can_wakeup(&client->dev))
> +		device_init_wakeup(&client->dev, 1);
> +
>  	sd3078_enable_reg_write(sd3078);
>  
> +	return ret;
> +}
> +
> +#if CONFIG_PM_SLEEP
> +static int sd3078_suspend(struct device *dev)
> +{
> +	struct sd3078 *sd3078 = dev_get_drvdata(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (client->irq >= 0 && device_may_wakeup(dev)) {
> +		sd3078->suspended = true;
> +		enable_irq_wake(client->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int sd3078_resume(struct device *dev)
> +{
> +	struct sd3078 *sd3078 = dev_get_drvdata(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	if (client->irq >= 0 && device_may_wakeup(dev)) {
> +		sd3078->suspended = false;
> +		disable_irq_wake(client->irq);
> +	}
> +
>  	return 0;
>  }
> +#endif
> +
> +static const struct dev_pm_ops sd3078_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(sd3078_suspend, sd3078_resume)
> +};
>  
>  static const struct i2c_device_id sd3078_id[] = {
>  	{"sd3078", 0},
> @@ -217,6 +523,7 @@ static struct i2c_driver sd3078_driver = {
>  	.driver     = {
>  		.name   = "sd3078",
>  		.of_match_table = of_match_ptr(rtc_dt_match),
> +		.pm = &sd3078_pm_ops,
>  	},
>  	.probe      = sd3078_probe,
>  	.id_table   = sd3078_id,
> -- 
> 2.7.4
> 
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com