Re: [PATCH 2/4] mfd: Add initial MFD driver for ATC260x PMICs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 17 Jun 2019, Manivannan Sadhasivam wrote:

> Add initial MFD driver for Actions Semi ATC260x PMICs. ATC260x series
> PMICs integrates Audio Codec, Power management, Clock generation, and GPIO
> controller blocks. This driver only supports Regulator functionality on
> ATC2609A PMIC variant for now.

Until you supply other functionality, this is not an MFD.

Please add additional support for more child devices.

> Since the PMICs can be accessed using both I2C and SPI busses, following
> driver structure has been adapted:
> 
>            ----->atc260x-core.c (Implements core funtionalities)
>           /
> ATC260x--------->atc260x-i2c.c (Implements I2C interface)
>           \
>            ----->atc2609a-helpers.c (Implements ATC2609A specific helpers)
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
>  drivers/mfd/Kconfig                       |  22 +++
>  drivers/mfd/Makefile                      |   7 +
>  drivers/mfd/atc2609a-helpers.c            |  91 +++++++++
>  drivers/mfd/atc260x-core.c                |  85 ++++++++
>  drivers/mfd/atc260x-i2c.c                 |  98 ++++++++++

Taking this set on it's own merits alone, I don't see a good reason to
split these up.  Please either supply the SPI interface within this
patch-set or amalgamate them into a single file.

>  drivers/mfd/atc260x.h                     |  22 +++
>  include/linux/mfd/atc260x/atc2609a_regs.h | 228 ++++++++++++++++++++++
>  include/linux/mfd/atc260x/core.h          |  64 ++++++
>  8 files changed, 617 insertions(+)
>  create mode 100644 drivers/mfd/atc2609a-helpers.c
>  create mode 100644 drivers/mfd/atc260x-core.c
>  create mode 100644 drivers/mfd/atc260x-i2c.c
>  create mode 100644 drivers/mfd/atc260x.h
>  create mode 100644 include/linux/mfd/atc260x/atc2609a_regs.h
>  create mode 100644 include/linux/mfd/atc260x/core.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index a17d275bf1d4..eb388505357b 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1945,6 +1945,28 @@ config MFD_STMFX
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_ATC260X
> +	tristate "Actions Semi ATC260x PMICs"
> +	select MFD_CORE
> +	select REGMAP
> +	select REGMAP_IRQ
> +	help
> +	  Support for the Actions Semi ATC260x PMICs.
> +
> +config MFD_ATC260X_I2C
> +	tristate "Actions Semi ATC260x PMICs with I2C"
> +	depends on MFD_ATC260X
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Support for the Actions Semi ATC260x PMICs controlled via I2C.
> +
> +config MFD_ATC2609A
> +	bool "Actions Semi ATC2609A PMIC"
> +	depends on MFD_ATC260X
> +	help
> +	  Support for Actions Semi ATC2609A PMIC
> +
>  menu "Multimedia Capabilities Port drivers"
>  	depends on ARCH_SA1100
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..a87e7ed55a02 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -249,3 +249,10 @@ obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
>  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> +
> +atc260x-objs			:= atc260x-core.o
> +ifeq ($(CONFIG_MFD_ATC2609A),y)
> +atc260x-objs			+= atc2609a-helpers.o
> +endif
> +obj-$(CONFIG_MFD_ATC260X)	+= atc260x.o
> +obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> diff --git a/drivers/mfd/atc2609a-helpers.c b/drivers/mfd/atc2609a-helpers.c
> new file mode 100644
> index 000000000000..6d304ea61552
> --- /dev/null
> +++ b/drivers/mfd/atc2609a-helpers.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Helper functions for ATC2609A PMIC
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#include <linux/mfd/atc260x/core.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#include "atc260x.h"
> +
> +const struct regmap_config atc2609a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = ATC2609A_SADDR,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +const struct regmap_irq atc2609a_irqs[] = {
> +	[ATC2609A_IRQ_AUDIO] = {
> +		.reg_offset = 0,
> +		.mask = BIT(0),
> +	},
> +	[ATC2609A_IRQ_OV] = {
> +		.reg_offset = 0,
> +		.mask = BIT(1),
> +	},
> +	[ATC2609A_IRQ_OC] = {
> +		.reg_offset = 0,
> +		.mask = BIT(2),
> +	},
> +	[ATC2609A_IRQ_OT] = {
> +		.reg_offset = 0,
> +		.mask = BIT(3),
> +	},
> +	[ATC2609A_IRQ_UV] = {
> +		.reg_offset = 0,
> +		.mask = BIT(4),
> +	},
> +	[ATC2609A_IRQ_ALARM] = {
> +		.reg_offset = 0,
> +		.mask = BIT(5),
> +	},
> +	[ATC2609A_IRQ_ONOFF] = {
> +		.reg_offset = 0,
> +		.mask = BIT(6),
> +	},
> +	[ATC2609A_IRQ_WKUP] = {
> +		.reg_offset = 0,
> +		.mask = BIT(7),
> +	},
> +	[ATC2609A_IRQ_IR] = {
> +		.reg_offset = 0,
> +		.mask = BIT(8),
> +	},
> +	[ATC2609A_IRQ_REMCON] = {
> +		.reg_offset = 0,
> +		.mask = BIT(9),
> +	},
> +	[ATC2609A_IRQ_POWER_IN] = {
> +		.reg_offset = 0,
> +		.mask = BIT(10),
> +	},
> +};

Please use REGMAP_IRQ_REG()

> +const struct regmap_irq_chip atc2609a_regmap_irq_chip = {
> +	.name = "atc2609a",
> +	.irqs = atc2609a_irqs,
> +	.num_irqs = ARRAY_SIZE(atc2609a_irqs),
> +	.num_regs = 1,
> +	.status_base = ATC2609A_INTS_PD,
> +	.mask_base = ATC2609A_INTS_MSK,
> +	.mask_invert = true,
> +};
> +
> +int atc2609a_dev_init(struct atc260x *atc260x)
> +{
> +	/* Initialize interrupt block */
> +	atc260x_cmu_reset(atc260x, ATC2609A_CMU_DEVRST, ATC260X_CMU_INTS,
> +			  ATC260X_CMU_INTS);
> +
> +	/* Disable all interrupt sources */
> +	regmap_write(atc260x->regmap, ATC2609A_INTS_MSK, 0);
> +
> +	/* Enable EXTIRQ pad */
> +	return regmap_update_bits(atc260x->regmap, ATC2609A_PAD_EN,
> +				  BIT(0), BIT(0));
> +}

No need for this to be in a separate file.  We can support multiple
chips from a single source file.  Only split them out when the level
of complexity makes it difficult to read/maintain.

> diff --git a/drivers/mfd/atc260x-core.c b/drivers/mfd/atc260x-core.c
> new file mode 100644
> index 000000000000..e65f1cb2648b
> --- /dev/null
> +++ b/drivers/mfd/atc260x-core.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Core MFD support for ATC260x PMICs
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/mfd/atc260x/core.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#include "atc260x.h"
> +
> +void atc260x_cmu_reset(struct atc260x *atc260x, u32 reg, u8 mask, u32 bit)
> +{
> +	/* Assert reset */
> +	regmap_update_bits(atc260x->regmap, reg, mask, ~bit);
> +
> +	/* De-assert reset */
> +	regmap_update_bits(atc260x->regmap, reg, mask, bit);
> +}

I only see one call-site.  Are you planning on reusing this?

> +int atc260x_core_init(struct atc260x *atc260x)
> +{
> +	struct device *dev = atc260x->dev;
> +	unsigned int chip_rev;
> +	int ret;
> +
> +	if (!atc260x->irq) {
> +		dev_err(dev, "No interrupt support\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Initialize the hardware */
> +	atc260x->dev_init(atc260x);

I don't think we need to mess around with pointers to functions in
this simple driver.

> +	ret = regmap_read(atc260x->regmap, atc260x->rev_reg, &chip_rev);
> +	if (ret) {
> +		dev_err(dev, "Failed to read revision register\n");

End users don't care about registers.

"Failed to obtain chip revision"

> +		return ret;
> +	}
> +
> +	if (chip_rev < 0 || chip_rev > 31) {

Do you really support 32 revisions?

> +		dev_err(dev, "Unknown chip revision: %d\n", ret);
> +		return -EINVAL;
> +	}
> +
> +	chip_rev = __ffs(chip_rev + 1U);

1 bit per revision?  That is highly inefficient.

> +	dev_info(dev, "%s chip revision: %d\n", atc260x->type_name, chip_rev);
> +
> +	ret = regmap_add_irq_chip(atc260x->regmap, atc260x->irq,
> +				  IRQF_ONESHOT, -1,
> +				  atc260x->regmap_irq_chip, &atc260x->irq_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to add irq_chip %d\n", ret);

"Failed to add IRQ Chip"

> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				   atc260x->cells, atc260x->nr_cells, NULL, 0,
> +				   regmap_irq_get_domain(atc260x->irq_data));
> +	if (ret) {
> +		dev_err(dev, "Failed to add MFD devices %d\n", ret);

"Failed to add child devices"

> +		goto err_irq;
> +	}
> +
> +	return 0;
> +
> +err_irq:
> +	regmap_del_irq_chip(atc260x->irq, atc260x->irq_data);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(atc260x_core_init);
> +
> +int atc260x_core_exit(struct atc260x *atc260x)
> +{
> +	regmap_del_irq_chip(atc260x->irq, atc260x->irq_data);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(atc260x_core_exit);
> diff --git a/drivers/mfd/atc260x-i2c.c b/drivers/mfd/atc260x-i2c.c
> new file mode 100644
> index 000000000000..3b7e8c1f5ac5
> --- /dev/null
> +++ b/drivers/mfd/atc260x-i2c.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * I2C bus interface for ATC260x PMICs
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#include <linux/i2c.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>
> +
> +#include "atc260x.h"
> +
> +const struct mfd_cell atc2609a_mfd_cells[] = {
> +	{ .name = "atc260x-regulator", },
> +};

What other child devices are there?

Please add more, or this is not an MFD.

> +static int atc260x_i2c_probe(struct i2c_client *client,
> +			     const struct i2c_device_id *id)
> +{
> +	struct atc260x *atc260x;
> +	const void *of_data;
> +	unsigned long atc260x_type;
> +
> +	atc260x = devm_kzalloc(&client->dev, sizeof(*atc260x), GFP_KERNEL);
> +	if (!atc260x)
> +		return -ENOMEM;
> +
> +	of_data = of_device_get_match_data(&client->dev);
> +	if (!of_data)
> +		return -ENODEV;
> +
> +	atc260x_type = (unsigned long)of_data;
> +
> +	switch (atc260x_type) {
> +	case ATC2609A:

How many more models are you expecting to support?

> +		atc260x->regmap_cfg = &atc2609a_regmap_config;
> +		atc260x->regmap_irq_chip = &atc2609a_regmap_irq_chip;
> +		atc260x->cells = atc2609a_mfd_cells;
> +		atc260x->nr_cells = ARRAY_SIZE(atc2609a_mfd_cells);
> +		atc260x->type_name = "atc2609a";
> +		atc260x->rev_reg = ATC2609A_CHIP_VER;
> +		atc260x->dev_init = atc2609a_dev_init;
> +		break;
> +	default:
> +		dev_err(&client->dev,
> +			"Unsupported ATC260x I2C device type %ld\n",
> +			atc260x_type);
> +		return -EINVAL;
> +	}

I'd assume you'd have to replicate all of this for SPI too.  That does
not sound like a good idea.  Please find a better, more succinct way
to handle this i.e. in the core driver.

> +	atc260x->regmap = devm_regmap_init_i2c(client, atc260x->regmap_cfg);
> +	if (IS_ERR(atc260x->regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed\n");
> +		return PTR_ERR(atc260x->regmap);
> +	}
> +
> +	i2c_set_clientdata(client, atc260x);
> +	atc260x->type = atc260x_type;

> +	atc260x->dev = &client->dev;
> +	atc260x->irq = client->irq;

You already have 'dev' and 'irq' stored in 'client', which you need in
order retrieve them back anyway.  So why store them again?

> +	return atc260x_core_init(atc260x);
> +}
> +
> +static int atc260x_i2c_remove(struct i2c_client *client)
> +{
> +	struct atc260x *atc260x = dev_get_drvdata(&client->dev);
> +
> +	atc260x_core_exit(atc260x);
> +
> +	return 0;
> +}
> +
> +const struct of_device_id atc260x_of_match[] = {
> +	{ .compatible = "actions,atc2609a", .data = (void *)ATC2609A },

Is there no way to dynamically request chip ID from the H/W?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, atc260x_of_match);
> +
> +static struct i2c_driver atc260x_i2c_driver = {
> +	.driver = {
> +		.name	= "atc260x",
> +		.of_match_table	= of_match_ptr(atc260x_of_match),
> +	},
> +	.probe		= atc260x_i2c_probe,
> +	.remove		= atc260x_i2c_remove,
> +};
> +
> +module_i2c_driver(atc260x_i2c_driver);
> +
> +MODULE_DESCRIPTION("ATC260x PMICs I2C bus interface");
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/atc260x.h b/drivers/mfd/atc260x.h
> new file mode 100644
> index 000000000000..30fc66dfba04
> --- /dev/null
> +++ b/drivers/mfd/atc260x.h
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MFD internals for ATC260x PMICs
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#ifndef ATC260X_MFD_H
> +#define ATC260X_MFD_H
> +
> +extern const struct of_device_id atc260x_of_match[];
> +int atc260x_core_init(struct atc260x *atc260x);
> +int atc260x_core_exit(struct atc260x *atc260x);
> +void atc260x_cmu_reset(struct atc260x *atc260x, u32 reg, u8 mask, u32 bit);
> +
> +extern const struct regmap_config atc2609a_regmap_config;
> +extern const struct mfd_cell atc2609a_mfd_cells[];
> +extern const struct regmap_irq_chip atc2609a_regmap_irq_chip;
> +extern const struct regmap_irq atc2609a_irqs[];

Yuck!  Please don't do this.  Please put this stuff in one file.

> +int atc2609a_dev_init(struct atc260x *atc260x);
> +
> +#endif /* ATC260X_MFD_H */
> diff --git a/include/linux/mfd/atc260x/atc2609a_regs.h b/include/linux/mfd/atc260x/atc2609a_regs.h
> new file mode 100644
> index 000000000000..851fb3dadd4f
> --- /dev/null
> +++ b/include/linux/mfd/atc260x/atc2609a_regs.h
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * ATC2609A PMIC register definitions
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#ifndef __ATC2609A_REGS_H__
> +#define __ATC2609A_REGS_H__
> +
> +enum atc2609a_irq_def {
> +	ATC2609A_IRQ_AUDIO = 0,
> +	ATC2609A_IRQ_OV = 1,
> +	ATC2609A_IRQ_OC = 2,
> +	ATC2609A_IRQ_OT = 3,
> +	ATC2609A_IRQ_UV = 4,
> +	ATC2609A_IRQ_ALARM = 5,
> +	ATC2609A_IRQ_ONOFF = 6,
> +	ATC2609A_IRQ_WKUP = 7,
> +	ATC2609A_IRQ_IR = 8,
> +	ATC2609A_IRQ_REMCON = 9,
> +	ATC2609A_IRQ_POWER_IN = 10,
> +};
> +
> +/* PMU Register */
> +#define ATC2609A_PMU_SYS_CTL0			0x00
> +#define ATC2609A_PMU_SYS_CTL1			0x01
> +#define ATC2609A_PMU_SYS_CTL2			0x02
> +#define ATC2609A_PMU_SYS_CTL3			0x03
> +#define ATC2609A_PMU_SYS_CTL4			0x04
> +#define ATC2609A_PMU_SYS_CTL5			0x05
> +#define ATC2609A_PMU_SYS_CTL6			0x06
> +#define ATC2609A_PMU_SYS_CTL7			0x07
> +#define ATC2609A_PMU_SYS_CTL8			0x08
> +#define ATC2609A_PMU_SYS_CTL9			0x09
> +#define ATC2609A_PMU_BAT_CTL0			0x0A
> +#define ATC2609A_PMU_BAT_CTL1			0x0B
> +#define ATC2609A_PMU_VBUS_CTL0			0x0C
> +#define ATC2609A_PMU_VBUS_CTL1			0x0D
> +#define ATC2609A_PMU_WALL_CTL0			0x0E
> +#define ATC2609A_PMU_WALL_CTL1			0x0F
> +#define ATC2609A_PMU_SYS_PENDING		0x10
> +#define ATC2609A_PMU_APDS_CTL0			0x11
> +#define ATC2609A_PMU_APDS_CTL1			0x12
> +#define ATC2609A_PMU_APDS_CTL2			0x13
> +#define ATC2609A_PMU_CHARGER_CTL		0x14
> +#define ATC2609A_PMU_BAKCHARGER_CTL		0x15
> +#define ATC2609A_PMU_SWCHG_CTL0			0x16
> +#define ATC2609A_PMU_SWCHG_CTL1			0x17
> +#define ATC2609A_PMU_SWCHG_CTL2			0x18
> +#define ATC2609A_PMU_SWCHG_CTL3			0x19
> +#define ATC2609A_PMU_SWCHG_CTL4			0x1A
> +#define ATC2609A_PMU_DC_OSC			0x1B
> +#define ATC2609A_PMU_DC0_CTL0			0x1C
> +#define ATC2609A_PMU_DC0_CTL1			0x1D
> +#define ATC2609A_PMU_DC0_CTL2			0x1E
> +#define ATC2609A_PMU_DC0_CTL3			0x1F
> +#define ATC2609A_PMU_DC0_CTL4			0x20
> +#define ATC2609A_PMU_DC0_CTL5			0x21
> +#define ATC2609A_PMU_DC0_CTL6			0x22
> +#define ATC2609A_PMU_DC1_CTL0			0x23
> +#define ATC2609A_PMU_DC1_CTL1			0x24
> +#define ATC2609A_PMU_DC1_CTL2			0x25
> +#define ATC2609A_PMU_DC1_CTL3			0x26
> +#define ATC2609A_PMU_DC1_CTL4			0x27
> +#define ATC2609A_PMU_DC1_CTL5			0x28
> +#define ATC2609A_PMU_DC1_CTL6			0x29
> +#define ATC2609A_PMU_DC2_CTL0			0x2A
> +#define ATC2609A_PMU_DC2_CTL1			0x2B
> +#define ATC2609A_PMU_DC2_CTL2			0x2C
> +#define ATC2609A_PMU_DC2_CTL3			0x2D
> +#define ATC2609A_PMU_DC2_CTL4			0x2E
> +#define ATC2609A_PMU_DC2_CTL5			0x2F
> +#define ATC2609A_PMU_DC2_CTL6			0x30
> +#define ATC2609A_PMU_DC3_CTL0			0x31
> +#define ATC2609A_PMU_DC3_CTL1			0x32
> +#define ATC2609A_PMU_DC3_CTL2			0x33
> +#define ATC2609A_PMU_DC3_CTL3			0x34
> +#define ATC2609A_PMU_DC3_CTL4			0x35
> +#define ATC2609A_PMU_DC3_CTL5			0x36
> +#define ATC2609A_PMU_DC3_CTL6			0x37
> +#define ATC2609A_PMU_DC_ZR			0x38
> +#define ATC2609A_PMU_LDO0_CTL0			0x39
> +#define ATC2609A_PMU_LDO0_CTL1			0x3A
> +#define ATC2609A_PMU_LDO1_CTL0			0x3B
> +#define ATC2609A_PMU_LDO1_CTL1			0x3C
> +#define ATC2609A_PMU_LDO2_CTL0			0x3D
> +#define ATC2609A_PMU_LDO2_CTL1			0x3E
> +#define ATC2609A_PMU_LDO3_CTL0			0x3F
> +#define ATC2609A_PMU_LDO3_CTL1			0x40
> +#define ATC2609A_PMU_LDO4_CTL0			0x41
> +#define ATC2609A_PMU_LDO4_CTL1			0x42
> +#define ATC2609A_PMU_LDO5_CTL0			0x43
> +#define ATC2609A_PMU_LDO5_CTL1			0x44
> +#define ATC2609A_PMU_LDO6_CTL0			0x45
> +#define ATC2609A_PMU_LDO6_CTL1			0x46
> +#define ATC2609A_PMU_LDO7_CTL0			0x47
> +#define ATC2609A_PMU_LDO7_CTL1			0x48
> +#define ATC2609A_PMU_LDO8_CTL0			0x49
> +#define ATC2609A_PMU_LDO8_CTL1			0x4A
> +#define ATC2609A_PMU_LDO9_CTL			0x4B
> +#define ATC2609A_PMU_OV_INT_EN			0x4C
> +#define ATC2609A_PMU_OV_STATUS			0x4D
> +#define ATC2609A_PMU_UV_INT_EN			0x4E
> +#define ATC2609A_PMU_UV_STATUS			0x4F
> +#define ATC2609A_PMU_OC_INT_EN			0x50
> +#define ATC2609A_PMU_OC_STATUS			0x51
> +#define ATC2609A_PMU_OT_CTL			0x52
> +#define ATC2609A_PMU_CM_CTL0			0x53
> +#define ATC2609A_PMU_FW_USE0			0x54
> +#define ATC2609A_PMU_FW_USE1			0x55
> +#define ATC2609A_PMU_ADC12B_I			0x56
> +#define ATC2609A_PMU_ADC12B_V			0x57
> +#define ATC2609A_PMU_ADC12B_DUMMY		0x58
> +#define ATC2609A_PMU_AUXADC_CTL0		0x59
> +#define ATC2609A_PMU_AUXADC_CTL1		0x5A
> +#define ATC2609A_PMU_BATVADC			0x5B
> +#define ATC2609A_PMU_BATIADC			0x5C
> +#define ATC2609A_PMU_WALLVADC			0x5D
> +#define ATC2609A_PMU_WALLIADC			0x5E
> +#define ATC2609A_PMU_VBUSVADC			0x5F
> +#define ATC2609A_PMU_VBUSIADC			0x60
> +#define ATC2609A_PMU_SYSPWRADC			0x61
> +#define ATC2609A_PMU_REMCONADC			0x62
> +#define ATC2609A_PMU_SVCCADC			0x63
> +#define ATC2609A_PMU_CHGIADC			0x64
> +#define ATC2609A_PMU_IREFADC			0x65
> +#define ATC2609A_PMU_BAKBATADC			0x66
> +#define ATC2609A_PMU_ICTEMPADC			0x67
> +#define ATC2609A_PMU_AUXADC0			0x68
> +#define ATC2609A_PMU_AUXADC1			0x69
> +#define ATC2609A_PMU_AUXADC2			0x6A
> +#define ATC2609A_PMU_AUXADC3			0x6B
> +#define ATC2609A_PMU_ICTEMPADC_ADJ		0x6C
> +#define ATC2609A_PMU_BDG_CTL			0x6D
> +#define ATC2609A_RTC_CTL			0x6E
> +#define ATC2609A_RTC_MSALM			0x6F
> +#define ATC2609A_RTC_HALM			0x70
> +#define ATC2609A_RTC_YMDALM			0x71
> +#define ATC2609A_RTC_MS				0x72
> +#define ATC2609A_RTC_H				0x73
> +#define ATC2609A_RTC_DC				0x74
> +#define ATC2609A_RTC_YMD			0x75
> +#define ATC2609A_EFUSE_DAT			0x76
> +#define ATC2609A_EFUSECRTL1			0x77
> +#define ATC2609A_EFUSECRTL2			0x78
> +#define ATC2609A_PMU_DC4_CTL0			0x79
> +#define ATC2609A_PMU_DC4_CTL1			0x7A
> +#define ATC2609A_PMU_DC4_CTL2			0x7B
> +#define ATC2609A_PMU_DC4_CTL3			0x7C
> +#define ATC2609A_PMU_DC4_CTL4			0x7D
> +#define ATC2609A_PMU_DC4_CTL5			0x7E
> +#define ATC2609A_PMU_DC4_CTL6			0x7F
> +#define ATC2609A_PMU_PWR_STATUS			0x80
> +#define ATC2609A_PMU_S2_PWR			0x81
> +#define ATC2609A_CLMT_CTL0			0x82
> +#define ATC2609A_CLMT_DATA0			0x83
> +#define ATC2609A_CLMT_DATA1			0x84
> +#define ATC2609A_CLMT_DATA2			0x85
> +#define ATC2609A_CLMT_DATA3			0x86
> +#define ATC2609A_CLMT_ADD0			0x87
> +#define ATC2609A_CLMT_ADD1			0x88
> +#define ATC2609A_CLMT_OCV_TABLE			0x89
> +#define ATC2609A_CLMT_R_TABLE			0x8A
> +#define ATC2609A_PMU_PWRON_CTL0			0x8D
> +#define ATC2609A_PMU_PWRON_CTL1			0x8E
> +#define ATC2609A_PMU_PWRON_CTL2			0x8F
> +#define ATC2609A_IRC_CTL			0x90
> +#define ATC2609A_IRC_STAT			0x91
> +#define ATC2609A_IRC_CC				0x92
> +#define ATC2609A_IRC_KDC			0x93
> +#define ATC2609A_IRC_WK				0x94
> +#define ATC2609A_IRC_RCC			0x95
> +
> +/* AUDIO_OUT Register */
> +#define ATC2609A_AUDIOINOUT_CTL			0xA0
> +#define ATC2609A_AUDIO_DEBUGOUTCTL		0xA1
> +#define ATC2609A_DAC_DIGITALCTL			0xA2
> +#define ATC2609A_DAC_VOLUMECTL0			0xA3
> +#define ATC2609A_DAC_ANALOG0			0xA4
> +#define ATC2609A_DAC_ANALOG1			0xA5
> +#define ATC2609A_DAC_ANALOG2			0xA6
> +#define ATC2609A_DAC_ANALOG3			0xA7
> +
> +/* AUDIO_IN Register */
> +#define ATC2609A_ADC_DIGITALCTL			0xA8
> +#define ATC2609A_ADC_HPFCTL			0xA9
> +#define ATC2609A_ADC_CTL			0xAA
> +#define ATC2609A_AGC_CTL0			0xAB
> +#define ATC2609A_AGC_CTL1			0xAC
> +#define ATC2609A_AGC_CTL2			0xAD
> +#define ATC2609A_ADC_ANALOG0			0xAE
> +#define ATC2609A_ADC_ANALOG1			0xAF
> +
> +/* PCM_IF Register */
> +#define ATC2609A_PCM0_CTL			0xB0
> +#define ATC2609A_PCM1_CTL			0xB1
> +#define ATC2609A_PCM2_CTL			0xB2
> +#define ATC2609A_PCMIF_CTL			0xB3
> +
> +/* CMU_CONTROL Register */
> +#define ATC2609A_CMU_DEVRST			0xC1
> +
> +/* INTS Register */
> +#define ATC2609A_INTS_PD			0xC8
> +#define ATC2609A_INTS_MSK			0xC9
> +
> +/* MFP Register */
> +#define ATC2609A_MFP_CTL			0xD0
> +#define ATC2609A_PAD_VSEL			0xD1
> +#define ATC2609A_GPIO_OUTEN			0xD2
> +#define ATC2609A_GPIO_INEN			0xD3
> +#define ATC2609A_GPIO_DAT			0xD4
> +#define ATC2609A_PAD_DRV			0xD5
> +#define ATC2609A_PAD_EN				0xD6
> +#define ATC2609A_DEBUG_SEL			0xD7
> +#define ATC2609A_DEBUG_IE			0xD8
> +#define ATC2609A_DEBUG_OE			0xD9
> +#define ATC2609A_CHIP_VER			0xDC
> +
> +/* PWSI Register */
> +#define ATC2609A_PWSI_CTL			0xF0
> +#define ATC2609A_PWSI_STATUS			0xF1
> +
> +/* TWSI Register */
> +#define ATC2609A_SADDR				0xFF
> +
> +#endif /* __ATC2609A_REGS_H__ */
> diff --git a/include/linux/mfd/atc260x/core.h b/include/linux/mfd/atc260x/core.h
> new file mode 100644
> index 000000000000..9d75eca731d4
> --- /dev/null
> +++ b/include/linux/mfd/atc260x/core.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Core MFD defines for ATC260x PMICs
> + *
> + * Copyright (C) 2019 Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#ifndef __ATC260X_CORE_H__
> +#define __ATC260X_CORE_H__
> +
> +#include <linux/mfd/atc260x/atc2609a_regs.h>
> +
> +enum atc260x_type {
> +	ATC2603A = 0,
> +	ATC2603C = 1,
> +	ATC2609A = 2,

Why don't you let the enum enumerate these numbers for you?

> +};
> +
> +enum atc260x_reg {
> +	ATC2609A_ID_DCDC0,
> +	ATC2609A_ID_DCDC1,
> +	ATC2609A_ID_DCDC2,
> +	ATC2609A_ID_DCDC3,
> +	ATC2609A_ID_DCDC4,
> +	ATC2609A_ID_LDO0,
> +	ATC2609A_ID_LDO1,
> +	ATC2609A_ID_LDO2,
> +	ATC2609A_ID_LDO3,
> +	ATC2609A_ID_LDO4,
> +	ATC2609A_ID_LDO5,
> +	ATC2609A_ID_LDO6,
> +	ATC2609A_ID_LDO7,
> +	ATC2609A_ID_LDO8,
> +	ATC2609A_ID_LDO9,
> +	ATC2609A_ID_MAX,
> +};
> +
> +enum atc260x_cmu_bits {
> +	ATC260X_CMU_TP = 0,
> +	ATC260X_CMU_MFP = 1,
> +	ATC260X_CMU_INTS = 2,
> +	ATC260X_CMU_ETHPHY = 3,
> +	ATC260X_CMU_AUDIO = 4,
> +	ATC260X_CMU_PWSI = 5,
> +};

Why don't you let the enum enumerate these numbers for you?

> +struct atc260x {
> +	struct device *dev;
> +	struct regmap_irq_chip_data *irq_data;
> +	struct regmap *regmap;
> +	const struct regmap_config *regmap_cfg;
> +	const struct regmap_irq_chip *regmap_irq_chip;
> +	const struct mfd_cell *cells;
> +	int nr_cells;
> +	int irq;
> +
> +	enum atc260x_type type;
> +	const char *type_name;
> +	unsigned int rev_reg;
> +
> +	int (*dev_init)(struct atc260x *atc260x);
> +};

If you do this right, you could probably remove this struct
completely.

> +#endif /* __ATC260X_CORE_H__ */

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

_______________________________________________
linux-actions mailing list
linux-actions@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-actions




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux