From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9F0F7C0018C for ; Wed, 16 Dec 2020 21:17:13 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 63D52233E2 for ; Wed, 16 Dec 2020 21:17:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727000AbgLPVRN (ORCPT ); Wed, 16 Dec 2020 16:17:13 -0500 Received: from relay3-d.mail.gandi.net ([217.70.183.195]:46007 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726987AbgLPVRM (ORCPT ); Wed, 16 Dec 2020 16:17:12 -0500 X-Originating-IP: 86.202.109.140 Received: from localhost (lfbn-lyo-1-13-140.w86-202.abo.wanadoo.fr [86.202.109.140]) (Authenticated sender: alexandre.belloni@bootlin.com) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 01A2360004; Wed, 16 Dec 2020 21:16:29 +0000 (UTC) Date: Wed, 16 Dec 2020 22:16:29 +0100 From: Alexandre Belloni To: zoro Cc: a.zummo@towertech.it, linux-rtc@vger.kernel.org Subject: Re: [PATCH] rtc: sd3078: add alarm function Message-ID: <20201216211629.GO2814589@piout.net> References: <1607091784-22328-1-git-send-email-long17.cool@163.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1607091784-22328-1-git-send-email-long17.cool@163.com> Precedence: bulk List-ID: X-Mailing-List: linux-rtc@vger.kernel.org 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 > --- > 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 > #include > #include > +#include > #include > #include > #include > +#include > > #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, ®map_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