Messages in this thread | | | Date | Fri, 26 Nov 2010 16:25:19 +0100 | From | Samuel Ortiz <> | Subject | Re: [PATCH] MFD: ab8500 chip version 2 |
| |
Hi Mattias,
On Fri, Nov 26, 2010 at 01:04:03PM +0100, Mattias Wallin wrote: > This patch adds support for chip version 2.0. > One new event latch register is added to v2 - Latch12. > Due to hardware problems reading this address on earlier > versions it is treated seperately. Fine.
> This patch also adds platform IORESOURCE irq to various > subdrivers to make way for their mainlining. That deserves a separate patch.
> Removing nested interrupts to allow them to be shared > is also added. Why ? I don't see how that's related to v2 support. If that's the case, please prepare an addtional patch as well.
Some more comments:
> #include <linux/kernel.h> > #include <linux/slab.h> > #include <linux/init.h> > #include <linux/irq.h> > -#include <linux/delay.h> This is unrelated as well.
> @@ -103,7 +94,10 @@ static const int ab8500_irq_regoffset[AB8500_NUM_IRQ_REGS] = { > > static int ab8500_get_chip_id(struct device *dev) > { > - struct ab8500 *ab8500 = dev_get_drvdata(dev->parent); > + struct ab8500 *ab8500; > + if (!dev) > + return -EINVAL; > + ab8500 = dev_get_drvdata(dev->parent); This is a potential bug fix, that should be separated aas well.
> @@ -303,13 +308,33 @@ static irqreturn_t ab8500_irq(int irq, void *dev) > continue; > > do { > - int bit = __ffs(status); > + int bit = __ffs(value); Another bug fix squeezed into that patch, it seems.
> diff --git a/include/linux/mfd/ab8500.h b/include/linux/mfd/ab8500.h > index d63b605..fc92391 100644 > --- a/include/linux/mfd/ab8500.h > +++ b/include/linux/mfd/ab8500.h > @@ -91,23 +91,29 @@ > #define AB8500_INT_ID_DET_R4F 93 > #define AB8500_INT_USB_CHG_DET_DONE 94 > #define AB8500_INT_USB_CH_TH_PROT_F 96 > -#define AB8500_INT_USB_CH_TH_PROP_R 97 > -#define AB8500_INT_MAIN_CH_TH_PROP_F 98 > +#define AB8500_INT_USB_CH_TH_PROT_R 97 > +#define AB8500_INT_MAIN_CH_TH_PROT_F 98 This is cleanup, not relevant to this patch.
> #define AB8500_INT_MAIN_CH_TH_PROT_R 99 > #define AB8500_INT_USB_CHARGER_NOT_OKF 103 > +#define AB8500_INT_ADP_SOURCE_ERROR 104 > +#define AB8500_INT_ADP_SINK_ERROR 105 > +#define AB8500_INT_ADP_PROBE_PLUG 106 > +#define AB8500_INT_ADP_PROBE_UNPLUG 107 > +#define AB8500_INT_ADP_SENSE_OFF 108 > +#define AB8500_INT_USB_LINK_STATUS 111 > > -#define AB8500_NR_IRQS 104 > +#define AB8500_NR_IRQS 112 > #define AB8500_NUM_IRQ_REGS 13 > > -#define AB8500_NUM_REGULATORS 15 > +#define AB8500_NUM_REGULATORS 15 Ditto.
> /** > * struct ab8500 - ab8500 internal structure > * @dev: parent device > * @lock: read/write operations lock > * @irq_lock: genirq bus lock > - * @revision: chip revision > * @irq: irq line > + * @chip_id: chip revision id > * @write: register write > * @read: register read > * @rx_buf: rx buf for SPI > @@ -119,7 +125,6 @@ struct ab8500 { > struct device *dev; > struct mutex lock; > struct mutex irq_lock; > - int revision; > int irq_base; > int irq; > u8 chip_id; This also is irrelevant to this patch.
Bottom line: Please send properly separated patches. It helps me review them, and it will help you debug your code when bisecting your git trees.
Cheers, Samuel.
-- Intel Open Source Technology Centre http://oss.intel.com/
| |