Message ID | 20160304195611.48d5177b@wiggum |
---|---|
State | Superseded |
Headers | show |
On 04/03/2016 at 19:56:11 +0100, Michael Büsch wrote : > This simplifies mask/set operations on device I2C registers. > > Signed-off-by: Michael Buesch <m@bues.ch> > --- > drivers/rtc/rtc-rv3029c2.c | 54 ++++++++++++++++++++++++---------------------- > 1 file changed, 28 insertions(+), 26 deletions(-) > > diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c > index 29cc871..769f73a 100644 > --- a/drivers/rtc/rtc-rv3029c2.c > +++ b/drivers/rtc/rtc-rv3029c2.c > @@ -2,6 +2,7 @@ > * Micro Crystal RV-3029 rtc class driver > * > * Author: Gregory Hermant <gregory.hermant@calao-systems.com> > + * Michael Buesch <m@bues.ch> > * > * based on previously existing rtc class drivers > * > @@ -143,6 +144,24 @@ rv3029_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[], > } > > static int > +rv3029_i2c_update_bits(struct i2c_client *client, u8 reg, u8 mask, u8 set) > +{ > + u8 buf; > + int ret; > + > + ret = rv3029_i2c_read_regs(client, reg, &buf, 1); > + if (ret < 0) > + return ret; > + buf &= mask; > + buf |= set; Well, this is not exactly what is expected from an update_bits function, it should be: buf &= ~mask; buf |= set & mask; What you pass to the function is the mask of bits you want to set. > + ret = rv3029_i2c_write_regs(client, reg, &buf, 1); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int > rv3029_i2c_get_sr(struct i2c_client *client, u8 *buf) > { > int ret = rv3029_i2c_read_regs(client, RV3029_STATUS, buf, 1); > @@ -260,22 +279,13 @@ static int rv3029_rtc_i2c_alarm_set_irq(struct i2c_client *client, > int enable) > { > int ret; > - u8 buf[1]; > - > - /* enable AIE irq */ > - ret = rv3029_i2c_read_regs(client, RV3029_IRQ_CTRL, buf, 1); > - if (ret < 0) { > - dev_err(&client->dev, "can't read INT reg\n"); > - return ret; > - } > - if (enable) > - buf[0] |= RV3029_IRQ_CTRL_AIE; > - else > - buf[0] &= ~RV3029_IRQ_CTRL_AIE; > > - ret = rv3029_i2c_write_regs(client, RV3029_IRQ_CTRL, buf, 1); > + /* enable/disable AIE irq */ > + ret = rv3029_i2c_update_bits(client, RV3029_IRQ_CTRL, > + (u8)~RV3029_IRQ_CTRL_AIE, This allows to remove the bitwise not here > + (enable ? RV3029_IRQ_CTRL_AIE : 0)); > if (ret < 0) { > - dev_err(&client->dev, "can't set INT reg\n"); > + dev_err(&client->dev, "can't update INT reg\n"); > return ret; > } > > @@ -316,20 +326,11 @@ static int rv3029_rtc_i2c_set_alarm(struct i2c_client *client, > return ret; > > if (alarm->enabled) { > - u8 buf[1]; > - > /* clear AF flag */ > - ret = rv3029_i2c_read_regs(client, RV3029_IRQ_FLAGS, > - buf, 1); > - if (ret < 0) { > - dev_err(&client->dev, "can't read alarm flag\n"); > - return ret; > - } > - buf[0] &= ~RV3029_IRQ_FLAGS_AF; > - ret = rv3029_i2c_write_regs(client, RV3029_IRQ_FLAGS, > - buf, 1); > + ret = rv3029_i2c_update_bits(client, RV3029_IRQ_FLAGS, > + (u8)~RV3029_IRQ_FLAGS_AF, 0); and here. > if (ret < 0) { > - dev_err(&client->dev, "can't set alarm flag\n"); > + dev_err(&client->dev, "can't clear alarm flag\n"); > return ret; > } > /* enable AIE irq */ > @@ -454,5 +455,6 @@ static struct i2c_driver rv3029_driver = { > module_i2c_driver(rv3029_driver); > > MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>"); > +MODULE_AUTHOR("Michael Buesch <m@bues.ch>"); > MODULE_DESCRIPTION("Micro Crystal RV3029 RTC driver"); > MODULE_LICENSE("GPL"); > -- > 2.7.0 >
On Fri, 4 Mar 2016 20:42:23 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > + buf &= mask; > > + buf |= set; > > Well, this is not exactly what is expected from an update_bits function, > it should be: > > buf &= ~mask; > buf |= set & mask; > > What you pass to the function is the mask of bits you want to set. Meh, ok. Everybody does seem to feel differently on that issue. I did it exactly like this with ~mask in the b43 driver and received a lot of bashing for it. :) But well, to me either way is fine. So I'll change this to do it with ~mask too.
diff --git a/drivers/rtc/rtc-rv3029c2.c b/drivers/rtc/rtc-rv3029c2.c index 29cc871..769f73a 100644 --- a/drivers/rtc/rtc-rv3029c2.c +++ b/drivers/rtc/rtc-rv3029c2.c @@ -2,6 +2,7 @@ * Micro Crystal RV-3029 rtc class driver * * Author: Gregory Hermant <gregory.hermant@calao-systems.com> + * Michael Buesch <m@bues.ch> * * based on previously existing rtc class drivers * @@ -143,6 +144,24 @@ rv3029_i2c_write_regs(struct i2c_client *client, u8 reg, u8 const buf[], } static int +rv3029_i2c_update_bits(struct i2c_client *client, u8 reg, u8 mask, u8 set) +{ + u8 buf; + int ret; + + ret = rv3029_i2c_read_regs(client, reg, &buf, 1); + if (ret < 0) + return ret; + buf &= mask; + buf |= set; + ret = rv3029_i2c_write_regs(client, reg, &buf, 1); + if (ret < 0) + return ret; + + return 0; +} + +static int rv3029_i2c_get_sr(struct i2c_client *client, u8 *buf) { int ret = rv3029_i2c_read_regs(client, RV3029_STATUS, buf, 1); @@ -260,22 +279,13 @@ static int rv3029_rtc_i2c_alarm_set_irq(struct i2c_client *client, int enable) { int ret; - u8 buf[1]; - - /* enable AIE irq */ - ret = rv3029_i2c_read_regs(client, RV3029_IRQ_CTRL, buf, 1); - if (ret < 0) { - dev_err(&client->dev, "can't read INT reg\n"); - return ret; - } - if (enable) - buf[0] |= RV3029_IRQ_CTRL_AIE; - else - buf[0] &= ~RV3029_IRQ_CTRL_AIE; - ret = rv3029_i2c_write_regs(client, RV3029_IRQ_CTRL, buf, 1); + /* enable/disable AIE irq */ + ret = rv3029_i2c_update_bits(client, RV3029_IRQ_CTRL, + (u8)~RV3029_IRQ_CTRL_AIE, + (enable ? RV3029_IRQ_CTRL_AIE : 0)); if (ret < 0) { - dev_err(&client->dev, "can't set INT reg\n"); + dev_err(&client->dev, "can't update INT reg\n"); return ret; } @@ -316,20 +326,11 @@ static int rv3029_rtc_i2c_set_alarm(struct i2c_client *client, return ret; if (alarm->enabled) { - u8 buf[1]; - /* clear AF flag */ - ret = rv3029_i2c_read_regs(client, RV3029_IRQ_FLAGS, - buf, 1); - if (ret < 0) { - dev_err(&client->dev, "can't read alarm flag\n"); - return ret; - } - buf[0] &= ~RV3029_IRQ_FLAGS_AF; - ret = rv3029_i2c_write_regs(client, RV3029_IRQ_FLAGS, - buf, 1); + ret = rv3029_i2c_update_bits(client, RV3029_IRQ_FLAGS, + (u8)~RV3029_IRQ_FLAGS_AF, 0); if (ret < 0) { - dev_err(&client->dev, "can't set alarm flag\n"); + dev_err(&client->dev, "can't clear alarm flag\n"); return ret; } /* enable AIE irq */ @@ -454,5 +455,6 @@ static struct i2c_driver rv3029_driver = { module_i2c_driver(rv3029_driver); MODULE_AUTHOR("Gregory Hermant <gregory.hermant@calao-systems.com>"); +MODULE_AUTHOR("Michael Buesch <m@bues.ch>"); MODULE_DESCRIPTION("Micro Crystal RV3029 RTC driver"); MODULE_LICENSE("GPL");
This simplifies mask/set operations on device I2C registers. Signed-off-by: Michael Buesch <m@bues.ch> --- drivers/rtc/rtc-rv3029c2.c | 54 ++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 26 deletions(-)