linux-kernel - Re: [PATCH 2/4] mfd: 88pm800: use irq_mode to configure interrupt status reg clear method

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]

Message-ID: <20150601083159.GD3329@x1>
Date:	Mon, 1 Jun 2015 09:31:59 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Vaibhav Hiremath <vaibhav.hiremath@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, robh+dt@...nel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	rtc-linux@...glegroups.com, sameo@...ux.intel.com,
	a.zummo@...ertech.it, alexandre.belloni@...e-electrons.com,
	zhaoy <zhaoy@...vell.com>
Subject: Re: [PATCH 2/4] mfd: 88pm800: use irq_mode to configure interrupt
 status reg clear method

On Sat, 30 May 2015, Vaibhav Hiremath wrote:

> From the spec, bit 1 of reg 0xe (page 0): IN_CLEAR_MODE controls the
> method of clearing interrupt status register of 88pm800;
> 
>   0: clear on read
>   1: clear on write
> 
> Signed-off-by: zhaoy <zhaoy@...vell.com>

This signed-off is not acceptable.

No nicknames.  Full names only.

> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@...aro.org>
> ---
>  drivers/mfd/88pm800.c       | 4 +++-
>  include/linux/mfd/88pm80x.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index 06ee058..8ea4467 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -391,7 +391,8 @@ static int device_irq_init_800(struct pm80x_chip *chip)
>  	    PM800_WAKEUP2_INV_INT | PM800_WAKEUP2_INT_CLEAR |
>  	    PM800_WAKEUP2_INT_MASK;
>  
> -	data = PM800_WAKEUP2_INT_CLEAR;
> +	data = (chip->irq_mode) ?
> +		PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR;

These variable names are terrible.  'irq_mode' as a bool tells me
nothing.

What does; irq_mode = 'yes' and irq_mode = 'no' mean?  If I didn't
read the remainder of the code, I would assume if it was 'yes' then
the device was in IRQ Mode and if not, it would be in PIO or Polling
mode, but that's not what it means at all is it?

As for 'data', well, isn't everything data?

>  	ret = regmap_update_bits(map, PM800_WAKEUP2, mask, data);
>  
>  	if (ret < 0)
> @@ -514,6 +515,7 @@ static int device_800_init(struct pm80x_chip *chip,
>  	}
>  
>  	chip->regmap_irq_chip = &pm800_irq_chip;
> +	chip->irq_mode = pdata->irq_mode;
>  
>  	ret = device_irq_init_800(chip);
>  	if (ret < 0) {
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..6ed6c16 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -77,6 +77,8 @@ enum {
>  #define PM800_WAKEUP2		(0x0E)
>  #define PM800_WAKEUP2_INV_INT		(1 << 0)
>  #define PM800_WAKEUP2_INT_CLEAR		(1 << 1)
> +#define PM800_WAKEUP2_INT_READ_CLEAR		(0 << 1)
> +#define PM800_WAKEUP2_INT_WRITE_CLEAR		(1 << 1)
>  #define PM800_WAKEUP2_INT_MASK		(1 << 2)
>  
>  #define PM800_POWER_UP_LOG	(0x10)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/