linux-kernel - RE: [PATCH V3 1/4] mfd: da9062: DA9062 MFD core driver

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:	Thu, 11 Jun 2015 08:27:57 +0000
From:	"Opensource [Steve Twiss]" <stwiss.opensource@...semi.com>
To:	'Lee Jones' <lee.jones@...aro.org>
CC:	LINUXKERNEL <linux-kernel@...r.kernel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	DEVICETREE <devicetree@...r.kernel.org>,
	David Dajun Chen <david.chen@...semi.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	LINUXINPUT <linux-input@...r.kernel.org>,
	LINUXWATCHDOG <linux-watchdog@...r.kernel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	"Mark Brown" <broonie@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	RTCLINUX <rtc-linux@...glegroups.com>,
	Rob Herring <robh+dt@...nel.org>,
	Support Opensource <Support.Opensource@...semi.com>,
	Wim Van Sebroeck <wim@...ana.be>
Subject: RE: [PATCH V3 1/4] mfd: da9062: DA9062 MFD core driver

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<<x)
> 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