[PATCH 1/3] mfd: support 88pm80x in 80x driver
Arnd Bergmann
arnd at arndb.de
Thu Jun 14 08:27:48 EDT 2012
On Wednesday 13 June 2012, Qiao Zhou wrote:
> 88PM800 and 88PM805 are two discrete chips used for power management.
> Hardware designer can use them together or only one of them according to
> requirement.
>
> There's some logic tightly linked between these two chips. For example, USB
> charger driver needs to access both chips by I2C interface.
>
> Now share one driver to these two devices. Only one I2C client is identified
> in platform init data. If one chip is also used, user should mark it with
> related i2c_addr field of platform init data. Then driver could create another
> I2C client for the chip.
>
> All I2C operations are accessed by 80x-i2c driver. In order to support all
> I2C client address, the read/write API is changed in below.
>
> reg_read(client, offset)
> reg_write(client, offset, data)
>
> The benefit is that client drivers only need one kind of read/write API. I2C
> and MFD driver can be shared in both 800 and 805.
>
> Signed-off-by: Qiao Zhou <zhouqiao at marvell.com>
> drivers/mfd/88pm80x-core.c | 1002 +++++++++++++++++++++++++++++++++++++++++++
> drivers/mfd/88pm80x-i2c.c | 370 ++++++++++++++++
> drivers/mfd/Kconfig | 11 +
> drivers/mfd/Makefile | 2 +
> include/linux/mfd/88pm80x.h | 701 ++++++++++++++++++++++++++++++
> 5 files changed, 2086 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/88pm80x-core.c
> create mode 100644 drivers/mfd/88pm80x-i2c.c
> create mode 100644 include/linux/mfd/88pm80x.h
>
> +
> +static struct resource rtc_resources[] = {
> + {
> + .name = "88pm80x-rtc",
> + .start = PM800_IRQ_RTC,
> + .end = PM800_IRQ_RTC,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct mfd_cell rtc_devs[] = {
> + {
> + .name = "88pm80x-rtc",
> + .num_resources = ARRAY_SIZE(rtc_resources),
> + .resources = &rtc_resources[0],
> + .id = -1,
> + },
> +};
> +
> +static struct resource onkey_resources[] = {
> + {
> + .name = "88pm80x-onkey",
> + .start = PM800_IRQ_ONKEY,
> + .end = PM800_IRQ_ONKEY,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct mfd_cell onkey_devs[] = {
> + {
> + .name = "88pm80x-onkey",
> + .num_resources = 1,
> + .resources = &onkey_resources[0],
> + .id = -1,
> + },
> +};
I wonder if it really makes sense to use the mfd_cell abstraction here, when each
array only contains a single device. Why not just use
platform_device_register_simple()?
> +
> +static struct pm80x_irq_data pm805_irqs[] = {
> + [PM805_IRQ_LDO_OFF] = { /*0 */
> + .reg = PM805_INT_STATUS1,
> + .mask_reg = PM805_INT_MASK1,
> + .offs = 1 << 5,
> + },
> + [PM805_IRQ_SRC_DPLL_LOCK] = {
> + .reg = PM805_INT_STATUS1,
> + .mask_reg = PM805_INT_MASK1,
> + .offs = 1 << 4,
> + },
> + [PM805_IRQ_CLIP_FAULT] = {
> + .reg = PM805_INT_STATUS1,
> + .mask_reg = PM805_INT_MASK1,
> + .offs = 1 << 3,
> + },
> + [PM805_IRQ_MIC_CONFLICT] = {
> + .reg = PM805_INT_STATUS1,
> + .mask_reg = PM805_INT_MASK1,
> + .offs = 1 << 2,
> + },
[coding style] This uses a very unusual indentation. Just use a single tab to
indent the fields in each of the irq data descriptions.
> +static inline struct pm80x_irq_data *irq_to_pm805(struct pm80x_chip *chip,
> + int irq)
> +{
> + int offset = irq - chip->pm805_chip->irq_base;
> + if (!chip->pm805_chip || (offset < 0)
> + || (offset >= ARRAY_SIZE(pm805_irqs)))
> + return NULL;
> + return &pm805_irqs[offset];
> +}
> +
> +static irqreturn_t pm805_irq(int irq, void *data)
> +{
> + struct pm80x_chip *chip = data;
> + struct pm80x_subchip *pm805_chip = chip->pm805_chip;
> + struct pm80x_irq_data *irq_data;
> + struct i2c_client *i2c;
> + int i, read_reg = -1, value = 0;
The functions for pm800 and pm805 look almost identical. Have you tried
consolidating them so you can use the same irqchip and code with different
init data structures?
> +int pm80x_reg_read(struct i2c_client *i2c, int reg)
> +{
> + struct regmap *map = verify_regmap(i2c);
> + unsigned int data;
> + int ret;
> +
> + ret = regmap_read(map, reg, &data);
> + if (ret < 0)
> + return ret;
> + else
> + return (int)data;
> +}
> +EXPORT_SYMBOL(pm80x_reg_read);
> +
> +int pm80x_reg_write(struct i2c_client *i2c, int reg,
> + unsigned char data)
> +{
> + struct regmap *map = verify_regmap(i2c);
> + int ret;
> +
> + ret = regmap_write(map, reg, data);
> + return ret;
> +}
> +EXPORT_SYMBOL(pm80x_reg_write);
These functions should be EXPORT_SYMBOL_GPL if you really need them.
My impression however is that you'd be better off without them
and just let the client use the regmap directly after you export
the verify_regmap function under a more appropriate name.
> +static const struct i2c_device_id pm80x_id_table[] = {
> + {"88PM80x", 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pm80x_id_table);
> +
> +static int verify_addr(struct i2c_client *i2c)
> +{
> + unsigned short addr_800[] = { 0x30, 0x34 };
> + unsigned short addr_805[] = { 0x38, 0x39 };
> + int size, i;
> +
> + if (i2c == NULL)
> + return 0;
> + size = ARRAY_SIZE(addr_800);
> + for (i = 0; i < size; i++) {
> + if (i2c->addr == *(addr_800 + i))
> + return CHIP_PM800;
> + }
> + size = ARRAY_SIZE(addr_805);
> + for (i = 0; i < size; i++) {
> + if (i2c->addr == *(addr_805 + i))
> + return CHIP_PM805;
> + }
> + return 0;
> +}
I think you should just put the devices separately into the device
tree so they get probed independently. Hardcoding the address
in the source code does not seem appropriate. Also, I would expect
that the device name is already the specific one (88pm800 or
88pm805) in the device tree. Just put both in the pm80x_id_table
and use the driver_data field to distinguish them.
> +static int pm80x_pages_init(struct pm80x_chip *chip, struct i2c_client *client,
> + struct pm80x_platform_data *pdata)
> +{
> + /*
> + * Both pm800 and pm805 shares the same platform driver.
> + * check whether we have have pm805 driver.
> + */
> + if (pdata->pm805_addr) {
> + chip->pm805_addr = pdata->pm805_addr;
> + /* in case we only have pm805, it already registers
> + * i2c handle, then no need to register again.
> + */
> + if (chip->id != CHIP_PM805) {
> + chip->client_pm805 =
> + i2c_new_dummy(client->adapter, chip->pm805_addr);
> + chip->regmap_pm805 = devm_regmap_init_i2c(chip->client_pm805, &pm80x_regmap_config);
> + i2c_set_clientdata(chip->client_pm805, chip);
> +
> + device_init_wakeup(&chip->client_pm805->dev, 1);
> + }
No reason to put the address into platform_data, just use the regular
i2c device address.
> +struct pm80x_subchip {
> + struct device *dev;
> + struct pm80x_chip *chip;
> + struct pm80x_platform_data *pdata;
> + struct i2c_client *client;
> + int irq;
> + int irq_base;
> + int irq_mode;
> +};
What is a "subchip"?
> +struct pm80x_chip {
> + /*chip_version can only on the top of the struct*/
> + unsigned char chip800_version;
> + unsigned char chip805_version;
> + struct device *dev;
> + struct pm80x_subchip *pm800_chip;
> + struct pm80x_subchip *pm805_chip;
> + struct mutex io_lock;
> + struct mutex pm800_irq_lock;
> + struct mutex pm805_irq_lock;
> + struct i2c_client *client_pm800;
> + struct i2c_client *client_pm805;
> + struct i2c_client *base_page; /* chip client for base page */
> + struct i2c_client *power_page; /* chip client for power page */
> + struct i2c_client *gpadc_page; /* chip client for gpadc page */
> + struct regmap *regmap_pm800;
> + struct regmap *regmap_pm805;
> + struct regmap *regmap_power;
> + struct regmap *regmap_gpadc;
> +
> + unsigned short pm800_addr;
> + unsigned short pm805_addr;
> + unsigned short base_page_addr; /* base page I2C address */
> + unsigned short power_page_addr; /* power page I2C address */
> + unsigned short gpadc_page_addr; /* gpadc page I2C address */
> + int id;
> + int irq_pm800;
> + int irq_pm805;
> + int irq_pm800_base;
> + int irq_pm805_base;
> + unsigned int wu_flag_pm800;
> + unsigned int wu_flag_pm805;
> +};
You have duplicated almost every field in this structure, which appears
to be very suboptimal. Better put all common fields into one structure
and then make a derived data structure for the specific ones, like
struct pm80x_chip {
char version;
struct pm80x_subchip *subchip;
struct mutex irq_lock;
struct i2c_client i2c;
...
unsigned int wu_flag;
};
struct pm800_chip {
struct pm80x_chip chip;
struct i2c_client *base_page;
...
};
struct pm805_chip {
struct pm80x_chip chip;
struct i2c_client *power_page;
...
};
> +struct pm80x_rtc_pdata {
> + int (*sync)(unsigned int ticks);
> + int vrtc;
> + int rtc_wakeup;
> +};
> +
> +struct pm80x_platform_data {
> + struct pm80x_rtc_pdata *rtc;
> + unsigned short pm800_addr;
> + unsigned short pm805_addr;
> + unsigned short base_page_addr; /* base page I2C address */
> + unsigned short power_page_addr; /* power page I2C address */
> + unsigned short gpadc_page_addr; /* gpadc page I2C address */
> + unsigned short test_page_addr; /* test page regs I2C address */
> + int i2c_port; /* Controlled by GI2C or PI2C */
> + int irq_mode; /* Clear interrupt by read/write(0/1) */
> + int irq_pm800; /* IRQ of 88pm800 */
> + int irq_pm805; /* IRQ of 88pm805 */
> + int irq_pm800_base; /* IRQ base number of 88pm800 */
> + int irq_pm805_base; /* IRQ base number of 88pm805 */
> + int batt_det; /* enable/disable */
> +
> + int (*pm800_plat_config)(struct pm80x_chip *chip,
> + struct pm80x_platform_data *pdata);
> + int (*pm805_plat_config)(struct pm80x_chip *chip,
> + struct pm80x_platform_data *pdata);
> +};
The *_plat_config fields are completely unused here, better remove them.
The interrupt numbers should become irq resources.
Any field in the platform data that cannot be converted to a resource
or completely removed should have a respective property in the device
tree binding for this device.
Arnd
More information about the linux-arm-kernel
mailing list