Hi Lee,

On 27/10/21 15:44, Lee Jones wrote:

On Wed, 27 Oct 2021, Luca Ceresoli wrote:

quoted

Hi Lee,

On 21/10/21 20:43, Lee Jones wrote:

quoted

On Tue, 19 Oct 2021, Luca Ceresoli wrote:

[...]

quoted

quoted

diff --git a/drivers/mfd/max77714.c b/drivers/mfd/max77714.c
new file mode 100644
index 000000000000..4b49d16fe199
--- /dev/null
+++ b/drivers/mfd/max77714.c

@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Maxim MAX77714 MFD Driver
+ *
+ * Copyright (C) 2021 Luca Ceresoli
+ * Author: Luca Ceresoli <luca@lucaceresoli.net>
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/max77714.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+struct max77714 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *irq_data;

Is this used outside of .probe()?

No.

Then you don't need to store it in a struct.

[...]

quoted

quoted

quoted

+	/* Internal Crystal Load Capacitance, indexed by value of 32KLOAD bits */
+	static const unsigned int load_cap[4] = {0, 10, 12, 22};

Probably best to define these magic numbers.

Since these numbers do not appear anywhere else I don't find added value in:

  #define MAX77714_LOAD_CAP_0   0
  #define MAX77714_LOAD_CAP_10  10
  #define MAX77714_LOAD_CAP_12  12
  #define MAX77714_LOAD_CAP_22  22
  [...]
  static const unsigned int load_cap[4] = {
      MAX77714_LOAD_CAP_0,
      MAX77714_LOAD_CAP_10,
      MAX77714_LOAD_CAP_12,
      MAX77714_LOAD_CAP_12,
  };

I don't find value in that nomenclature either! :)

I was suggesting that you used better, more forthcoming names.

 LOAD_CAPACITANCE_00_pF
 LOAD_CAPACITANCE_10_pF
 LOAD_CAPACITANCE_12_pF
 LOAD_CAPACITANCE_22_pF

quoted

besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even
worse, there is potential for copy-paste errors -- can you spot it? ;)

Yes.  Straight away.

quoted

Finally, consider this is not even global but local to a small function.

But I'd rather add the unit ("pF") to either the comment line of the
array name (load_cap -> load_cap_pf) for clarity. Would that be OK for you?

I did have to read the code again to get a handle on things (probably
not a good sign).  To keep things simple, just add "/* pF */" onto the
end of the load_cap line for now.  That should clear things up at
first glance.

Ok, let's see how it works. I'm sending v3 in the next few days.

-- 
Luca