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

From: Luca Ceresoli <luca@lucaceresoli.net>
To: Lee Jones <lee.jones@linaro.org>
Cc: linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	devicetree@vger.kernel.org, linux-rtc@vger.kernel.org,
	linux-watchdog@vger.kernel.org,
	Chiwoong Byun <woong.byun@samsung.com>,
	Laxman Dewangan <ldewangan@nvidia.com>,
	Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v2 6/9] mfd: max77714: Add driver for Maxim MAX77714 PMIC
Date: Wed, 27 Oct 2021 16:23:57 +0200	[thread overview]
Message-ID: <d5674e8e-34c9-86ca-94cc-dca79ed82496@lucaceresoli.net> (raw)
In-Reply-To: <YXlXvovUsvOIPYyV@google.com>

Hi Lee,

On 27/10/21 15:44, Lee Jones wrote:
> On Wed, 27 Oct 2021, Luca Ceresoli wrote:
> 
>> 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.
> 
> Then you don't need to store it in a struct.
> 
> [...]
> 
>>>> +	/* 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
> 
>> 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.
> 
>> 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