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:

quoted

quoted

+	/* 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

quoted

quoted

diff --git a/include/linux/mfd/da9062/registers.h

b/include/linux/mfd/da9062/registers.h

quoted

new file mode 100644
index 0000000..d07c2bc

--- /dev/null
+++ b/include/linux/mfd/da9062/registers.h

[...]

quoted

quoted

+/*
+ * Registers
+ */

Really? ;)

quoted

+#define DA9062AA_PAGE_CON		0x000
+#define DA9062AA_STATUS_A		0x001
+#define DA9062AA_STATUS_B		0x002

[...]

quoted

quoted

+
+/*
+ * 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