Re: [PATCH] MFD: ab8500 chip version 2

[Mattias Wallin <mattias.wallin@stericsson.com>]

Hi Sam,
As you point out I fixed a lot of small things in our internal tree but I have not done
my mainlining homework for a while and I am now trying to catch up.

I will divide this patch into several small ones.

/Wallin


On 11/26/2010 04:25 PM, Samuel Ortiz wrote:
> 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.
>