Re: [PATCH v2 6/9] mfd: max77714: Add driver for Maxim MAX77714 PMIC

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Lee,

On 21/10/21 20:43, Lee Jones wrote:
> On Tue, 19 Oct 2021, Luca Ceresoli wrote:
[...]
>> 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.

>> +/*
>> + * MAX77714 initially uses the internal, low precision oscillator. Enable
>> + * the external oscillator by setting the XOSC_RETRY bit. If the external
>> + * oscillator is not OK (probably not installed) this has no effect.
>> + */
>> +static int max77714_setup_xosc(struct max77714 *chip)
> 
> May as well just pass 'dev' and 'regmap' to this function and do away
> with the superfluous struct along with all of it's memory management
> handling requirements.

Good idea!

>> +{
>> +	/* 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,
  };

besides adding lots of lines and lots of "MAX77714_LOAD_CAP_". Even
worse, there is potential for copy-paste errors -- can you spot it? ;)
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?

Apart from this coding style topic I'm OK with all the other
improvements you proposed to this patch, all of them will be in v3.

Thank you for the detailed review!
-- 
Luca