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

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 13/10/2021 23:49, Luca Ceresoli wrote:
> Hi,
> 
> On 12/10/21 10:09, Krzysztof Kozlowski wrote:
>> On 11/10/2021 17:56, Luca Ceresoli wrote:
>>> Add a simple driver for the Maxim MAX77714 PMIC, supporting RTC and
>>> watchdog only.
>>>
>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>> ---
>>>  MAINTAINERS                  |   2 +
>>>  drivers/mfd/Kconfig          |  14 ++++
>>>  drivers/mfd/Makefile         |   1 +
>>>  drivers/mfd/max77714.c       | 151 +++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/max77714.h |  68 ++++++++++++++++
>>>  5 files changed, 236 insertions(+)
>>>  create mode 100644 drivers/mfd/max77714.c
>>>  create mode 100644 include/linux/mfd/max77714.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 4d0134752537..df394192f14e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -11389,6 +11389,8 @@ MAXIM MAX77714 PMIC MFD DRIVER
>>>  M:	Luca Ceresoli <luca@lucaceresoli.net>
>>>  S:	Maintained
>>>  F:	Documentation/devicetree/bindings/mfd/maxim,max77714.yaml
>>> +F:	drivers/mfd/max77714.c
>>> +F:	include/linux/mfd/max77714.h
>>>  
>>>  MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>>>  M:	Javier Martinez Canillas <javier@dowhile0.org>
>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>> index ca0edab91aeb..b5f6e6174508 100644
>>> --- a/drivers/mfd/Kconfig
>>> +++ b/drivers/mfd/Kconfig
>>> @@ -853,6 +853,20 @@ config MFD_MAX77693
>>>  	  additional drivers must be enabled in order to use the functionality
>>>  	  of the device.
>>>  
>>> +config MFD_MAX77714
>>> +	bool "Maxim Semiconductor MAX77714 PMIC Support"
>>
>> Why it cannot be a tristate (module)?
> 
> Because it's not done in the driver I initially copied from, I guess. :)
> 
> And also because I thought it's appropriate for a PMIC driver since
> regulators tend to be always instantiated. But I understand there are
> valid use cases for that -- will do in v2 unless a good reason pops up
> for not doing it.

Main PMIC as a module sometimes requires additional effort (like initrd
with the PMIC driver) to make system booting. Still for non-SoC
components we choose to allow modules (e.g. max77686).

It seems in your case it can be used as module easily because you did
not implement regulators, which are needed early for storage devices.

> 
>>> diff --git a/include/linux/mfd/max77714.h b/include/linux/mfd/max77714.h
>>> new file mode 100644
>>> index 000000000000..ca6b747b73c2
>>> --- /dev/null
>>> +++ b/include/linux/mfd/max77714.h
>>> @@ -0,0 +1,68 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Maxim MAX77714 Register and data structures definition.
>>> + *
>>> + * Copyright (C) 2021 Luca Ceresoli
>>> + * Author: Luca Ceresoli <luca@lucaceresoli.net>
>>> + */
>>> +
>>> +#ifndef _MFD_MAX77714_H_
>>> +#define _MFD_MAX77714_H_
>>
>> Header guard:
>> __LINUX_MFD_MAX77714_H_
> 
> OK.
> 
>>> +
>>> +struct max77714 {
>>> +	struct device *dev;
>>> +	struct regmap *regmap;
>>> +	struct regmap_irq_chip_data *irq_data;
>>> +
>>> +	int irq;
>>> +};
>>
>> Do you have to make it a public structure? If not, please put it in the
>> max77714.c
> 
> Good point. Will fix.
> 


Best regards,
Krzysztof