Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755041AbbHXQrU (ORCPT ); Mon, 24 Aug 2015 12:47:20 -0400 Received: from mail-pd0-f173.google.com ([209.85.192.173]:34730 "EHLO mail-pd0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753944AbbHXQrT (ORCPT ); Mon, 24 Aug 2015 12:47:19 -0400 Message-ID: <55DB4A91.9000702@linaro.org> Date: Mon, 24 Aug 2015 22:17:13 +0530 From: Vaibhav Hiremath User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Lee Jones CC: linux-arm-kernel@lists.infradead.org, Zhao Ye , Samuel Ortiz , open list Subject: Re: [PATCH-v6 5/6] mfd: 88pm800: Set default interrupt clear method References: <1436358392-15449-1-git-send-email-vaibhav.hiremath@linaro.org> <1436358392-15449-6-git-send-email-vaibhav.hiremath@linaro.org> <20150824135422.GF3237@x1> <55DB3627.2030503@linaro.org> <20150824155129.GA19409@x1> In-Reply-To: <20150824155129.GA19409@x1> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4482 Lines: 143 On Monday 24 August 2015 09:21 PM, Lee Jones wrote: > On Mon, 24 Aug 2015, Vaibhav Hiremath wrote: > >> >> >> On Monday 24 August 2015 07:24 PM, Lee Jones wrote: >>> On Wed, 08 Jul 2015, Vaibhav Hiremath wrote: >>> >>>> As per the spec, bit 1 (INT_CLEAR_MODE) of reg addr 0xe >>>> (page 0) controls the method of clearing interrupt >>>> status of 88pm800 family of devices; >>>> >>>> 0: clear on read >>>> 1: clear on write >>>> >>>> If pdata is not coming from board file, then set the >>>> default irq clear method to "irq clear on write" >>>> >>>> Also, as suggested by "Lee Jones" renaming variable field >>>> to appropriate name and removed unnecessary field >>>> pm80x_chip.irq_mode, using platform_data.irq_clr_method. >>>> >>>> Signed-off-by: Zhao Ye >>>> Signed-off-by: Vaibhav Hiremath >>>> Reviewed-by: Krzysztof Kozlowski >>>> --- >>>> drivers/mfd/88pm800.c | 15 ++++++++++----- >>>> include/linux/mfd/88pm80x.h | 9 +++++++-- >>>> 2 files changed, 17 insertions(+), 7 deletions(-) >>> >>> [...] >>> >>>> +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1) >>>> +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1) >>> >>> Use BIT(). >>> >>>> +/* Used by irq_clr_method */ >>>> +#define PM800_IRQ_CLR_ON_READ 0 >>>> +#define PM800_IRQ_CLR_ON_WRITE 1 >>> >>>> - int irq_mode; /* Clear interrupt by read/write(0/1) */ >>>> + bool irq_clr_method; /* Clear interrupt by read/write(0/1) */ >>> >>>> + irq_clr_mode = pdata->irq_clr_method == PM800_IRQ_CLR_ON_WRITE ? >>>> + PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR; >>>> + ret = regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode); >>> >>> This is pretty convoluted. >>> >>> For starters you're abusing the 'bool' type here. Bool is either >>> 'true' or 'false', so at the very least you should rename >>> 'irq_clr_method' to 'irq_clr_on_write'. >>> >>> Then you can do: >>> >>> irq_clr_mode = pdata->irq_clr_on_write ? >>> PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR; >>> >> >> We have discussed on this, and went back-n-forth. >> I think if I remember correctly, one of the version was using >> true/false then we decided to rename it to relevant macro. >> >> If I am not wrong V4 version of this series is exactly same as what you >> are referring to. > > Right. I made a few suggestions which vary in usefulness depending on > how you plan to implement all of this. Unfortunately this is a bit of > a bastardised version where some of it make sense and other parts > could do with some improvement. > This so called "basterdised version could have been avoided :) V2 version itself was clean and ready. It just got dragged into multiple iterations. >>> However, what I suggest you really do is share >>> PM800_WAKEUP2_INT_{READ,WRITE}_CLEAR with platform data and just pass >>> the value through directly. >>> >> >> I think we discussed about this also, and the reason I recall here is, >> >> we may need to control this from DT in the future so we decided to keep >> it boolean in platform_data and have simple check before writing to >> register. >> >> And I think that was also another reason we introduced >> >> /* Used by irq_clr_method */ >> #define PM800_IRQ_CLR_ON_READ 0 >> #define PM800_IRQ_CLR_ON_WRITE 1 > > I think these are still required. So it would look like this: > NO. I think you are confused here, We have two different macros playing around here, +/* Used by irq_clr_method */ +#define PM800_IRQ_CLR_ON_READ 0 +#define PM800_IRQ_CLR_ON_WRITE 1 /* Used to write to register */ +#define PM800_WAKEUP2_INT_READ_CLEAR (0 << 1) +#define PM800_WAKEUP2_INT_WRITE_CLEAR (1 << 1) > == Platform data == > > struct pdata { > bool clear_irq_on_write; > }; > > pdata->clear_irq_on_write = PM800_IRQ_CLR_ON_{READ,WRITE}; > > == Driver == > > irq_clr_mode = pdata->clear_irq_on_write ? > PM800_WAKEUP2_INT_WRITE_CLEAR : PM800_WAKEUP2_INT_READ_CLEAR; > regmap_update_bits(map, PM800_WAKEUP2, mask, irq_clr_mode); > Please check V2, which is exactly same as above. https://patchwork.kernel.org/patch/6627781/ If you are OK with it, I will spin another version and submit it. Thanks, Vaibhav -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/