linux-kernel - Re: [PATCH] MFD: ab8500 chip version 2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 26 Nov 2010 16:25:19 +0100
From:	Samuel Ortiz <sameo@...ux.intel.com>
To:	Mattias Wallin <mattias.wallin@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org,
	Linus Walleij <linus.walleij@...ricsson.com>
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/