Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752653AbbFKI2k (ORCPT ); Thu, 11 Jun 2015 04:28:40 -0400 Received: from mail1.bemta3.messagelabs.com ([195.245.230.166]:20678 "EHLO mail1.bemta3.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751090AbbFKI2f (ORCPT ); Thu, 11 Jun 2015 04:28:35 -0400 X-Env-Sender: stwiss.opensource@diasemi.com X-Msg-Ref: server-11.tower-38.messagelabs.com!1434011278!3226553!1 X-Originating-IP: [94.185.165.51] X-StarScan-Received: X-StarScan-Version: 6.13.16; banners=-,-,- X-VirusChecked: Checked From: "Opensource [Steve Twiss]" To: "'Lee Jones'" CC: LINUXKERNEL , Samuel Ortiz , Alessandro Zummo , DEVICETREE , David Dajun Chen , Dmitry Torokhov , Ian Campbell , Kumar Gala , LINUXINPUT , LINUXWATCHDOG , Liam Girdwood , "Mark Brown" , Mark Rutland , Pawel Moll , RTCLINUX , Rob Herring , Support Opensource , Wim Van Sebroeck Subject: RE: [PATCH V3 1/4] mfd: da9062: DA9062 MFD core driver Thread-Topic: [PATCH V3 1/4] mfd: da9062: DA9062 MFD core driver Thread-Index: AQHQkje+BbbztfyjIkOz27ag7HsO6Z2OZ74AgALzVjCAFbuuAA== Date: Thu, 11 Jun 2015 08:27:57 +0000 Message-ID: <6ED8E3B22081A4459DAC7699F3695FB7014B22F4C4@SW-EX-MBX02.diasemi.com> References: <20150526161024.GQ11677@x1> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.20.26.77] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id t5B8SjU4009447 Content-Length: 4292 Lines: 129 On 28 May 2015 13:53, Steve Twiss wrote: > To: 'Lee Jones' > Subject: RE: [PATCH V3 1/4] mfd: da9062: DA9062 MFD core driver > > Hi Lee, > > I will refactor a lot of the driver and implement your changes as requested. Hi Lee, I realise this is a busy kernel time for you, as ever, so this is just to see if I am missing anything with the reply I sent about the requested alterations a couple of weeks ago. It relates to the addition of support for the Dialog DA9062 Power Management IC - https://lkml.org/lkml/2015/5/28/358 The main changes requested (for the core MFD) can be found in here: - https://lkml.org/lkml/2015/5/28/359 I believe that the only two major differences with your previous comments were those relating to the interrupt handler da9062_vdd_warn_event() -- which has been erased, and the header file -- which I would prefer the to remain [mostly] untouched if possible. The reasons for these two differences are described below: > > > + /* VDD WARN event support */ > > > + irq_vdd_warn = regmap_irq_get_virq(chip->regmap_irq, > > > + DA9062_IRQ_VDD_WARN); > > > + if (irq_vdd_warn < 0) { > > > + dev_err(chip->dev, "Failed to get IRQ.\n"); > > > + return irq_vdd_warn; > > > + } > > > + chip->irq_vdd_warn = irq_vdd_warn; > > > + > > > + ret = devm_request_threaded_irq(chip->dev, irq_vdd_warn, > > > + NULL, da9062_vdd_warn_event, > > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > + "VDD_WARN", chip); > > > + if (ret) { > > > + dev_warn(chip->dev, > > > + "Failed to request VDD_WARN IRQ.\n"); > > > + chip->irq_vdd_warn = -ENXIO; > > > + } > > > > This looks like a lot of code, which doesn't really do anything. What > > is a VDD warning indicator anyway? > > > > I will remove this. > > The IRQ handler da9062_vdd_warn_event() -- see earlier above -- does > not currently do anything apart from handle the IRQ that was requested > here. It prints a statement to say the main PMIC voltage supply dropped > below a defined trigger point, but doesn't actually do anything to mitigate > this problem. > > Previously this VDD_WARN was in the regulator driver, however it should > be made available even if the regulator driver is not installed -- so I added it > to the core instead. > > In a previous driver submission I had a similar problem, a warning IRQ was > just printing to the console to say there was an error -- the handler and > IRQ code was put in by me so it could be used if the driver was taken and > integrated into a fully working system. > > I was asked to remove it in the other driver -- and I have done the same > here for now. I can always add it back later. > And > > > diff --git a/include/linux/mfd/da9062/registers.h > > b/include/linux/mfd/da9062/registers.h > > > new file mode 100644 > > > index 0000000..d07c2bc > > > --- /dev/null > > > +++ b/include/linux/mfd/da9062/registers.h > > [...] > > > > +/* > > > + * Registers > > > + */ > > > > Really? ;) > > > > > +#define DA9062AA_PAGE_CON 0x000 > > > +#define DA9062AA_STATUS_A 0x001 > > > +#define DA9062AA_STATUS_B 0x002 > > [...] > > > > + > > > +/* > > > + * Bit fields > > > + */ > > > + > > > +/* DA9062AA_PAGE_CON = 0x000 */ > > > +#define DA9062AA_PAGE_SHIFT 0 > > > +#define DA9062AA_PAGE_MASK (0x3f << 0) > > > +#define DA9062AA_WRITE_MODE_SHIFT 6 > > > +#define DA9062AA_WRITE_MODE_MASK (0x01 << 6) > > > > For 1 << X, you should use BIT(X). > > > > For the two comments above "Registers" and "Bit fields" and the (1< definitions ... > > The whole of this file is automatically generated by our hardware designers > I would prefer it if the register definitions and bit fields are not altered using > the #define BIT(nr) (1UL<<(nr)) macro and the comments removed because > we have scripts that can be used to check this file automatically. > > Also if the register map is ever updated, then it will be easier for me to diff > the new delivered register and bit field definitions with the old one. > > My preference would be not to change this header file. > > [...] If these last two things are a problem can you please let me know. regards, Steve ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?