Messages in this thread | | | Date | Wed, 17 Dec 2008 16:49:16 +0800 | From | "Eric Miao" <> | Subject | Re: [RFC PATCH 1/2] Add Dialog DA9030 battery charger driver |
| |
On Wed, Dec 17, 2008 at 3:56 PM, Mike Rapoport <mike@compulab.co.il> wrote: > This patch amends DA903x MFD driver with definitions and methods needed for > battery charger driver. >
Patch looks generally good, see some of my concerns below:
> Signed-off-by: Mike Rapoport <mike@compulab.co.il> > > drivers/mfd/da903x.c | 16 +++++++++++++++- > include/linux/mfd/da903x.h | 44 ++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 57 insertions(+), 3 deletions(-) > > diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c > index 0b5bd85..2e04b44 100644 > --- a/drivers/mfd/da903x.c > +++ b/drivers/mfd/da903x.c > @@ -151,12 +151,24 @@ int da903x_write(struct device *dev, int reg, uint8_t val) > } > EXPORT_SYMBOL_GPL(da903x_write); > > +int da903x_writes(struct device *dev, int reg, int len, uint8_t *val) > +{ > + return __da903x_writes(to_i2c_client(dev), reg, len, val); > +} > +EXPORT_SYMBOL_GPL(da903x_writes); > + > int da903x_read(struct device *dev, int reg, uint8_t *val) > { > return __da903x_read(to_i2c_client(dev), reg, val); > } > EXPORT_SYMBOL_GPL(da903x_read); > > +int da903x_reads(struct device *dev, int reg, int len, uint8_t *val) > +{ > + return __da903x_reads(to_i2c_client(dev), reg, len, val); > +} > +EXPORT_SYMBOL_GPL(da903x_reads); > + > int da903x_set_bits(struct device *dev, int reg, uint8_t bit_mask) > { > struct da903x_chip *chip = dev_get_drvdata(dev); > @@ -254,7 +266,7 @@ static int da9030_unmask_events(struct da903x_chip *chip, unsigned int events) > { > uint8_t v[3]; > > - chip->events_mask &= ~events; > + chip->events_mask |= events; >
I wonder if this is true, it may also impact the da9030_mask_events(), which is a pair.
> v[0] = (chip->events_mask & 0xff); > v[1] = (chip->events_mask >> 8) & 0xff; > @@ -391,6 +403,8 @@ static void da903x_irq_work(struct work_struct *work) > if (chip->ops->read_events(chip, &events)) > break; > > +/* pr_info("%s: events=%x events_mask=%x\n", */ > +/* __func__, events, chip->events_mask); */
I'd prefer to remove this if it is annoying, sorry for this initial garbage I sent.
> events &= ~chip->events_mask; > if (events == 0) > break;
| |