* Re: [PATCH] rtc: sd3078: add alarm support
[not found] <20201230130615.6417-1-long17.cool@aliyun.com>
@ 2021-04-23 20:34 ` Alexandre Belloni
0 siblings, 0 replies; only message in thread
From: Alexandre Belloni @ 2021-04-23 20:34 UTC (permalink / raw)
To: Dianlong Li; +Cc: a.zummo, linux-rtc
Hello,
Sorry for the late review, I was note sure how it was related to the
previous one as it was not labelled v2.
On 30/12/2020 13:06:15+0000, Dianlong Li wrote:
> Add alarm support for the sd3078.
>
> Signed-off-by: Dianlong Li <long17.cool@aliyun.com>
> ---
> drivers/rtc/rtc-sd3078.c | 235 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 235 insertions(+)
>
> diff --git a/drivers/rtc/rtc-sd3078.c b/drivers/rtc/rtc-sd3078.c
> index a7aa943c1183..d80e7033e005 100644
> --- a/drivers/rtc/rtc-sd3078.c
> +++ b/drivers/rtc/rtc-sd3078.c
> @@ -27,6 +27,31 @@
> #define KEY_WRITE2 0x04
> #define KEY_WRITE3 0x80
>
> +#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 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
> +
Please use BIT() for all of those defines
> +#define INT_BITS (BIT_IM | BIT_INTS1 | BIT_INTS0 | \
> + BIT_INTDE | BIT_INTAE | BIT_INTFE)
> +
You can use GENMASK and the name should probably be INT_MSK.
> #define NUM_TIME_REGS (SD3078_REG_YR - SD3078_REG_SC + 1)
>
> /*
> @@ -39,6 +64,8 @@
> struct sd3078 {
> struct rtc_device *rtc;
> struct regmap *regmap;
> + bool suspended;
Is this variable really useful?
> + struct i2c_client *client;
> };
>
> /*
> @@ -152,9 +179,156 @@ static int sd3078_rtc_set_time(struct device *dev, struct rtc_time *tm)
> 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;
> +
This check is unnecessary
> + 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);
You should not have to enable and disable irqs.
> + } else {
> + regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL2,
> + INT_BITS, 0);
> + disable_irq(client->irq);
> + }
> +
> + 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)
> + 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)
> + 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)
> + 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;
> +
> + 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)
> + 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)
> + return ret;
> +
> + ret = sd3078_alarm_irq_enable(dev, alarm->enabled);
> + if (ret < 0)
> + return ret;
> +
> +#if WRITE_PROTECT_EN
> + sd3078_disable_reg_write(sd3078);
> +#endif
> +
> + 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,6 +337,19 @@ static const struct regmap_config regmap_config = {
> .max_register = 0x11,
> };
>
> +static irqreturn_t sd3078_threaded_irq(int irq, void *dev_id)
> +{
> + struct i2c_client *client = dev_id;
> + struct sd3078 *sd3078 = i2c_get_clientdata(client);
> +
> + /* clean interrupt flag */
> + regmap_update_bits(sd3078->regmap, SD3078_REG_CTRL1,
> + BIT_INTAF, 0);
You should probably check whether the interrupt really happened, that
would allow to share the interrupt line.
> +
> + rtc_update_irq(sd3078->rtc, 1, RTC_AF | RTC_IRQF);
> + return IRQ_HANDLED;
> +}
> +
> static int sd3078_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -176,6 +363,8 @@ static int sd3078_probe(struct i2c_client *client,
> if (!sd3078)
> return -ENOMEM;
>
> + sd3078->client = client;
> +
> sd3078->regmap = devm_regmap_init_i2c(client, ®map_config);
> if (IS_ERR(sd3078->regmap)) {
> dev_err(&client->dev, "regmap allocation failed\n");
> @@ -183,6 +372,7 @@ 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))
> @@ -196,11 +386,55 @@ static int sd3078_probe(struct i2c_client *client,
> if (ret)
> return ret;
>
> + if (client->irq) {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, sd3078_threaded_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
I don't have the datasheet but are you sure this shouldn't be
IRQF_TRIGGER_LOW.
> + client->name, client);
> + if (ret)
> + dev_err(&client->dev, "unable to request IRQ\n");
> + }
> +
> + if (!device_can_wakeup(&client->dev))
> + device_init_wakeup(&client->dev, 1);
You should call that only if the irq is present and has been requested
You should also
clear_bit(RTC_FEATURE_ALARM, sd3078->rtc->features) when there is no
alarm.
> +
> sd3078_enable_reg_write(sd3078);
>
> return 0;
> }
>
> +#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 +451,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.17.1
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] only message in thread