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

["Opensource [Steve Twiss]" <stwiss.opensource@diasemi.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4292 bytes --]

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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥