| Message ID | 20200505201310.255145-5-alexandre.belloni@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] rtc: add new VL flag for backup switchover | expand |
On 05/05/2020 22.13, Alexandre Belloni wrote: > Add support for the RTC_VL_BACKUP_SWITCH flag to report battery switch over > events. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/rtc/rtc-pcf2127.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c > index 039078029bd4..967de68e1b03 100644 > --- a/drivers/rtc/rtc-pcf2127.c > +++ b/drivers/rtc/rtc-pcf2127.c > @@ -188,18 +188,27 @@ static int pcf2127_rtc_ioctl(struct device *dev, > unsigned int cmd, unsigned long arg) > { > struct pcf2127 *pcf2127 = dev_get_drvdata(dev); > - int touser; > + int val, touser = 0; > int ret; > > switch (cmd) { > case RTC_VL_READ: > - ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &touser); > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &val); > if (ret) > return ret; > > - touser = touser & PCF2127_BIT_CTRL3_BLF ? RTC_VL_BACKUP_LOW : 0; > + if (val & PCF2127_BIT_CTRL3_BLF) > + touser = RTC_VL_BACKUP_LOW; > + > + if (val & PCF2127_BIT_CTRL3_BF) > + touser |= RTC_VL_BACKUP_SWITCH; I think it's a bit easier to read if you use |= in both cases. Re patch 3, one saves a little .text by eliding the ioctl function when, as you say, it cannot be called anyway. No strong opinion either way, I don't think anybody actually builds without CONFIG_RTC_INTF_DEV, but those that do are probably the ones that care about having a tiny vmlinux. Other than that, the series looks good to me. Thanks, Rasmus
On 05/05/2020 23:30:18+0200, Rasmus Villemoes wrote: > On 05/05/2020 22.13, Alexandre Belloni wrote: > > Add support for the RTC_VL_BACKUP_SWITCH flag to report battery switch over > > events. > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > drivers/rtc/rtc-pcf2127.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c > > index 039078029bd4..967de68e1b03 100644 > > --- a/drivers/rtc/rtc-pcf2127.c > > +++ b/drivers/rtc/rtc-pcf2127.c > > @@ -188,18 +188,27 @@ static int pcf2127_rtc_ioctl(struct device *dev, > > unsigned int cmd, unsigned long arg) > > { > > struct pcf2127 *pcf2127 = dev_get_drvdata(dev); > > - int touser; > > + int val, touser = 0; > > int ret; > > > > switch (cmd) { > > case RTC_VL_READ: > > - ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &touser); > > + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &val); > > if (ret) > > return ret; > > > > - touser = touser & PCF2127_BIT_CTRL3_BLF ? RTC_VL_BACKUP_LOW : 0; > > + if (val & PCF2127_BIT_CTRL3_BLF) > > + touser = RTC_VL_BACKUP_LOW; > > + > > + if (val & PCF2127_BIT_CTRL3_BF) > > + touser |= RTC_VL_BACKUP_SWITCH; > > I think it's a bit easier to read if you use |= in both cases. > > Re patch 3, one saves a little .text by eliding the ioctl function when, > as you say, it cannot be called anyway. No strong opinion either way, I > don't think anybody actually builds without CONFIG_RTC_INTF_DEV, but > those that do are probably the ones that care about having a tiny vmlinux. > Honestly, I don't think it is worth doing that. On armv7, this only removes 248 bytes. Also, compiling without CONFIG_RTC_INTF_DEV simply makes the RTC unusable. There are no tools actually using the sysfs interface instead of the char device interface. I prefer keeping CONFIG_RTC_INTF_DEV private to the core.
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c index 039078029bd4..967de68e1b03 100644 --- a/drivers/rtc/rtc-pcf2127.c +++ b/drivers/rtc/rtc-pcf2127.c @@ -188,18 +188,27 @@ static int pcf2127_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) { struct pcf2127 *pcf2127 = dev_get_drvdata(dev); - int touser; + int val, touser = 0; int ret; switch (cmd) { case RTC_VL_READ: - ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &touser); + ret = regmap_read(pcf2127->regmap, PCF2127_REG_CTRL3, &val); if (ret) return ret; - touser = touser & PCF2127_BIT_CTRL3_BLF ? RTC_VL_BACKUP_LOW : 0; + if (val & PCF2127_BIT_CTRL3_BLF) + touser = RTC_VL_BACKUP_LOW; + + if (val & PCF2127_BIT_CTRL3_BF) + touser |= RTC_VL_BACKUP_SWITCH; return put_user(touser, (unsigned int __user *)arg); + + case RTC_VL_CLR: + return regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL3, + PCF2127_BIT_CTRL3_BF, 0); + default: return -ENOIOCTLCMD; } @@ -493,7 +502,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap, */ ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL3, PCF2127_BIT_CTRL3_BTSE | - PCF2127_BIT_CTRL3_BF | PCF2127_BIT_CTRL3_BIE | PCF2127_BIT_CTRL3_BLIE, 0); if (ret) {
Add support for the RTC_VL_BACKUP_SWITCH flag to report battery switch over events. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/rtc/rtc-pcf2127.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)