Re: [PATCH v6 3/7] mfd: Add MFD driver for ATC260x PMICs

[Lee Jones]

On Wed, 20 Jan 2021, Cristian Ciocaltea wrote:

> Add initial support for the Actions Semi ATC260x PMICs which integrates
> Audio Codec, Power management, Clock generation and GPIO controller
> blocks.
> 
> For the moment this driver only supports Regulator, Poweroff and Onkey
> functionalities for the ATC2603C and ATC2609A chip variants.
> 
> Since the PMICs can be accessed using both I2C and SPI buses, the
> following driver structure has been adopted:
> 
>            -----> atc260x-core.c (Implements core functionalities)
>           /
> ATC260x --------> atc260x-i2c.c (Implements I2C interface)
>           \
>            -----> atc260x-spi.c (Implements SPI interface - TODO)
> 
> Co-developed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxx>
> ---
> Changes in v6 - per Lee's feedback:
> - Added ATC260X_CHIP_REV_MAX magic number
> - Fixed code formattting arround atc260x_i2c_driver initialization
> - Replaced dev_init() callback in struct atc260x with a pointer to a new struct
> atc260x_init_regs to hold hardware specific registry information
> - Added a generic atc260x_dev_init() function and instantiated atc2603c_init_regs
> and atc2609a_init_regs
> 
> Changes in v5:
>  - None
> 
> Changes in v4 - according to Lee's review:
>  - Replaced 'regmap_add_irq_chip()' with 'devm' counterpart and dropped
>    'atc260x_device_remove()' and 'atc260x_i2c_remove()' functions
>  - Moved kerneldoc sections from prototypes to real functions
>  - Placed single line entries on one line for mfd_cells[]
>  - Several other minor changes
> 
> Changes in v3:
>  - Fixed the issues reported by Lee's kernel test robot:
>    WARNING: modpost: missing MODULE_LICENSE() in drivers/mfd/atc260x-core.o
>    >> FATAL: modpost: drivers/mfd/atc260x-i2c: sizeof(struct i2c_device_id)=24 is
>       not a modulo of the size of section __mod_i2c__<identifier>_device_table=588.
>    >> Fix definition of struct i2c_device_id in mod_devicetable.h
>  - Dropped the usage of '.of_compatible' fields in {atc2603c,atc2609a}_mfd_cells[]
>  - Added 'Co-developed-by' tag in commit message and dropped [cristian: ...] line
> 
>  drivers/mfd/Kconfig                  |  18 ++
>  drivers/mfd/Makefile                 |   3 +
>  drivers/mfd/atc260x-core.c           | 310 +++++++++++++++++++++++++++
>  drivers/mfd/atc260x-i2c.c            |  64 ++++++
>  include/linux/mfd/atc260x/atc2603c.h | 281 ++++++++++++++++++++++++
>  include/linux/mfd/atc260x/atc2609a.h | 308 ++++++++++++++++++++++++++
>  include/linux/mfd/atc260x/core.h     |  58 +++++
>  7 files changed, 1042 insertions(+)
>  create mode 100644 drivers/mfd/atc260x-core.c
>  create mode 100644 drivers/mfd/atc260x-i2c.c
>  create mode 100644 include/linux/mfd/atc260x/atc2603c.h
>  create mode 100644 include/linux/mfd/atc260x/atc2609a.h
>  create mode 100644 include/linux/mfd/atc260x/core.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..a27ff2e83e7a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2064,6 +2064,24 @@ config MFD_WCD934X
>  	  This driver provides common support WCD934x audio codec and its
>  	  associated Pin Controller, Soundwire Controller and Audio codec.
>  
> +config MFD_ATC260X
> +	tristate
> +	select MFD_CORE
> +	select REGMAP
> +	select REGMAP_IRQ
> +
> +config MFD_ATC260X_I2C
> +	tristate "Actions Semi ATC260x PMICs with I2C"
> +	select MFD_ATC260X
> +	select REGMAP_I2C
> +	depends on I2C
> +	help
> +	  Support for the Actions Semi ATC260x PMICs controlled via I2C.
> +
> +	  This driver provides common support for accessing the ATC2603C
> +	  and ATC2609A chip variants, additional drivers must be enabled
> +	  in order to use the functionality of the device.
> +
>  config MFD_KHADAS_MCU
>  	tristate "Support for Khadas System control Microcontroller"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..1ea88d2c83b4 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -268,3 +268,6 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> +
> +obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
> +obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> diff --git a/drivers/mfd/atc260x-core.c b/drivers/mfd/atc260x-core.c
> new file mode 100644
> index 000000000000..7148ff5b05b1
> --- /dev/null
> +++ b/drivers/mfd/atc260x-core.c
> @@ -0,0 +1,310 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Core support for ATC260x PMICs
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + * Copyright (C) 2020 Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxx>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/mfd/atc260x/core.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#define ATC260X_CHIP_REV_MAX	31
> +
> +struct atc260x_init_regs {
> +	unsigned int cmu_devrst;
> +	unsigned int cmu_devrst_ints;
> +	unsigned int ints_msk;
> +	unsigned int pad_en;
> +	unsigned int pad_en_extirq;
> +};
> +
> +static void regmap_lock_mutex(void *__mutex)
> +{
> +	struct mutex *mutex = __mutex;
> +
> +	/*
> +	 * Using regmap within an atomic context (e.g. accessing a PMIC when
> +	 * powering system down) is normally allowed only if the regmap type
> +	 * is MMIO and the regcache type is either REGCACHE_NONE or
> +	 * REGCACHE_FLAT. For slow buses like I2C and SPI, the regmap is
> +	 * internally protected by a mutex which is acquired non-atomically.
> +	 *
> +	 * Let's improve this by using a customized locking scheme inspired
> +	 * from I2C atomic transfer. See i2c_in_atomic_xfer_mode() for a
> +	 * starting point.
> +	 */
> +	if (system_state > SYSTEM_RUNNING && irqs_disabled())
> +		mutex_trylock(mutex);
> +	else
> +		mutex_lock(mutex);
> +}

Would this be useful to anyone else?

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@xxxxxxxxxx>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog