RE: [PATCH v2] mfd: ADP5520 Multifunction LCD Backlight and Keypad Input Device Driver

From: Hennerich, Michael
Date: Fri Oct 02 2009 - 05:38:26 EST


Hi Samuel,

>From: Samuel Ortiz [mailto:sameo@xxxxxxxxxxxxxxx]
>Hi Mike,
>
>Some comments on this patch:
>
>On Wed, Sep 23, 2009 at 01:11:04AM -0400, Mike Frysinger wrote:
>> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>>
>> Base driver for Analog Devices ADP5520/ADP5501 MFD PMICs
>>
>> Subdevs:
>> LCD Backlight : drivers/video/backlight/adp5520_bl
>> LEDs : drivers/led/leds-adp5520
>> GPIO : drivers/gpio/adp5520-gpio (ADP5520 only)
>> Keys : drivers/input/keyboard/adp5520-keys (ADP5520 only)
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>> Signed-off-by: Bryan Wu <cooloney@xxxxxxxxxx>
>> Signed-off-by: Mike Frysinger <vapier@xxxxxxxxxx>
>> ---
>> v2
>> - fix return type of irq handler
>> - fix unbalanced paren in BL_CTRL_VAL() macro
>>
>> drivers/mfd/Kconfig | 10 ++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/adp5520.c | 371
+++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mfd/adp5520.h | 303
+++++++++++++++++++++++++++++++++++
>> 4 files changed, 685 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/mfd/adp5520.c
>> create mode 100644 include/linux/mfd/adp5520.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 570be13..34e8595 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -160,6 +160,16 @@ config PMIC_DA903X
>> individual components like LCD backlight, voltage regulators,
>> LEDs and battery-charger under the corresponding menus.
>>
>> +config PMIC_ADP5520
>> + bool "Analog Devices ADP5520/01 MFD PMIC Core Support"
>> + depends on I2C=y
>> + help
>> + Say yes here to add support for Analog Devices AD5520 and
ADP5501,
>> + Multifunction Power Management IC. This includes
>> + the I2C driver and the core APIs _only_, you have to select
>> + individual components like LCD backlight, LEDs, GPIOs and
Kepad
>> + under the corresponding menus.
>> +
>> config MFD_WM8400
>> tristate "Support Wolfson Microelectronics WM8400"
>> select MFD_CORE
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index f3b277b..a42248e 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -50,3 +50,4 @@ obj-$(CONFIG_PCF50633_ADC) += pcf50633-adc.o
>> obj-$(CONFIG_PCF50633_GPIO) += pcf50633-gpio.o
>> obj-$(CONFIG_AB3100_CORE) += ab3100-core.o
>> obj-$(CONFIG_AB3100_OTP) += ab3100-otp.o
>> +obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
>> diff --git a/drivers/mfd/adp5520.c b/drivers/mfd/adp5520.c
>> new file mode 100644
>> index 0000000..1083626
>> --- /dev/null
>> +++ b/drivers/mfd/adp5520.c
>> @@ -0,0 +1,371 @@
>> +/*
>> + * Base driver for Analog Devices ADP5520/ADP5501 MFD PMICs
>> + * LCD Backlight: drivers/video/backlight/adp5520_bl
>> + * LEDs : drivers/led/leds-adp5520
>> + * GPIO : drivers/gpio/adp5520-gpio (ADP5520 only)
>> + * Keys : drivers/input/keyboard/adp5520-keys (ADP5520
only)
>> + *
>> + * Copyright 2009 Analog Devices Inc.
>> + *
>> + * Derived from da903x:
>> + * Copyright (C) 2008 Compulab, Ltd.
>> + * Mike Rapoport <mike@xxxxxxxxxxxxxx>
>> + *
>> + * Copyright (C) 2006-2008 Marvell International Ltd.
>> + * Eric Miao <eric.miao@xxxxxxxxxxx>
>> + *
>> + * Licensed under the GPL-2 or later.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/i2c.h>
>> +
>> +#include <linux/mfd/adp5520.h>
>> +
>> +struct adp5520_chip {
>> + struct i2c_client *client;
>> + struct device *dev;
>> + struct mutex lock;
>> + struct work_struct irq_work;
>> + struct blocking_notifier_head notifier_list;
>> + int irq;
>> +};
>> +
>> +static int __adp5520_read(struct i2c_client *client,
>> + int reg, uint8_t *val)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_read_byte_data(client, reg);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "failed reading at 0x%02x\n",
reg);
>> + return ret;
>> + }
>> +
>> + *val = (uint8_t)ret;
>> + return 0;
>> +}
>> +
>> +static int __adp5520_write(struct i2c_client *client,
>> + int reg, uint8_t val)
>> +{
>> + int ret;
>> +
>> + ret = i2c_smbus_write_byte_data(client, reg, val);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "failed writing 0x%02x to
0x%02x\n",
>> + val, reg);
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> +int __adp5520_ack_bits(struct i2c_client *client, int reg, uint8_t
bit_mask)
>> +{
>> + struct adp5520_chip *chip = i2c_get_clientdata(client);
>> + uint8_t reg_val;
>> + int ret;
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = __adp5520_read(client, reg, &reg_val);
>> +
>> + if (!ret) {
>> + reg_val |= bit_mask;
>> + ret = __adp5520_write(client, reg, reg_val);
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> +}
>> +
>> +int adp5520_write(struct device *dev, int reg, uint8_t val)
>> +{
>> + return __adp5520_write(to_i2c_client(dev), reg, val);
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_write);
>> +
>> +int adp5520_read(struct device *dev, int reg, uint8_t *val)
>> +{
>> + return __adp5520_read(to_i2c_client(dev), reg, val);
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_read);
>> +
>> +int adp5520_set_bits(struct device *dev, int reg, uint8_t bit_mask)
>> +{
>> + struct adp5520_chip *chip = dev_get_drvdata(dev);
>> + uint8_t reg_val;
>> + int ret;
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = __adp5520_read(chip->client, reg, &reg_val);
>> +
>> + if (!ret && ((reg_val & bit_mask) == 0)) {
>> + reg_val |= bit_mask;
>> + ret = __adp5520_write(chip->client, reg, reg_val);
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_set_bits);
>> +
>> +int adp5520_clr_bits(struct device *dev, int reg, uint8_t bit_mask)
>> +{
>> + struct adp5520_chip *chip = dev_get_drvdata(dev);
>> + uint8_t reg_val;
>> + int ret;
>> +
>> + mutex_lock(&chip->lock);
>> +
>> + ret = __adp5520_read(chip->client, reg, &reg_val);
>> +
>> + if (!ret && (reg_val & bit_mask)) {
>> + reg_val &= ~bit_mask;
>> + ret = __adp5520_write(chip->client, reg, reg_val);
>> + }
>> +
>> + mutex_unlock(&chip->lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_clr_bits);
>> +
>> +int adp5520_register_notifier(struct device *dev, struct
notifier_block *nb,
>> + unsigned int events)
>> +{
>> + struct adp5520_chip *chip = dev_get_drvdata(dev);
>> +
>> + if (chip->irq) {
>> + adp5520_set_bits(chip->dev, INTERRUPT_ENABLE,
>> + events & (KP_IEN | KR_IEN | OVP_IEN |
CMPR_IEN));
>> +
>> + return
blocking_notifier_chain_register(&chip->notifier_list,
>> + nb);
>> + }
>> +
>> + return -ENODEV;
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_register_notifier);
>> +
>> +int adp5520_unregister_notifier(struct device *dev, struct
notifier_block *nb,
>> + unsigned int events)
>> +{
>> + struct adp5520_chip *chip = dev_get_drvdata(dev);
>> +
>> + adp5520_clr_bits(chip->dev, INTERRUPT_ENABLE,
>> + events & (KP_IEN | KR_IEN | OVP_IEN | CMPR_IEN));
>> +
>> + return blocking_notifier_chain_unregister(&chip->notifier_list,
nb);
>> +}
>> +EXPORT_SYMBOL_GPL(adp5520_unregister_notifier);
>> +
>> +static void adp5520_irq_work(struct work_struct *work)
>> +{
>> + struct adp5520_chip *chip =
>> + container_of(work, struct adp5520_chip, irq_work);
>> + unsigned int events;
>> + uint8_t reg_val;
>> + int ret;
>> +
>> + ret = __adp5520_read(chip->client, MODE_STATUS, &reg_val);
>> + if (ret)
>> + goto out;
>> +
>> + events = reg_val & (OVP_INT | CMPR_INT | GPI_INT | KR_INT |
KP_INT);
>> +
>> + blocking_notifier_call_chain(&chip->notifier_list, events,
NULL);
>> + /* ACK, Sticky bits are W1C */
>> + __adp5520_ack_bits(chip->client, MODE_STATUS, events);
>> +
>> +out:
>> + enable_irq(chip->client->irq);
>> +}
>> +
>> +static irqreturn_t adp5520_irq_handler(int irq, void *data)
>> +{
>> + struct adp5520_chip *chip = data;
>> +
>> + disable_irq_nosync(irq);
>> + schedule_work(&chip->irq_work);
>Have you considered using a threaded irq handler here ?

The Linux version I developed this driver on didn't feature threaded irq
handlers.
But thanks I'm going to take a look here.

>
>
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int __remove_subdev(struct device *dev, void *unused)
>> +{
>> + platform_device_unregister(to_platform_device(dev));
>> + return 0;
>> +}
>> +
>> +static int adp5520_remove_subdevs(struct adp5520_chip *chip)
>> +{
>> + return device_for_each_child(chip->dev, NULL, __remove_subdev);
>> +}
>> +
>> +static int __devinit adp5520_add_subdevs(struct adp5520_chip *chip,
>> + struct adp5520_platform_data
*pdata)
>> +{
>> + struct adp5520_subdev_info *subdev;
>> + struct platform_device *pdev;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i < pdata->num_subdevs; i++) {
>> + subdev = &pdata->subdevs[i];
>> +
>> + pdev = platform_device_alloc(subdev->name, subdev->id);
>> +
>> + pdev->dev.parent = chip->dev;
>> + pdev->dev.platform_data = subdev->platform_data;
>> +
>> + ret = platform_device_add(pdev);
>> + if (ret)
>> + goto failed;
>> + }
>> + return 0;
>Here I would expect the MFD core driver to know about all the potential
>subdevices and add them in that routine, and not take the subdevices
list from
>the platform definition.
>I realize da903x has the same issue btw.

The ADP5520 is an I2C device and gets registered via struct
i2c_board_info.
How about having multiple ADP5520 in a system with different I2C salve
addresses?
Each ADP5520 having different Keypad, Backlight and GPIO configurations
passed in platform_data?
How will they map? The MFD core is struct resource centric, which is not
going to help here.

I could be doing something like this:

Index: drivers/mfd/adp5520.c

===================================================================

--- drivers/mfd/adp5520.c (revision 7535)

+++ drivers/mfd/adp5520.c (working copy)

@@ -25,6 +25,7 @@

#include <linux/irq.h>

#include <linux/workqueue.h>

#include <linux/i2c.h>

+#include <linux/mfd/core.h>



#include <linux/mfd/adp5520.h>



@@ -235,6 +236,21 @@

return ret;

}



+static struct mfd_cell __devinitdata adp5520_cells[] = {

+ {

+ .name = "adp5520-backlight",

+ },

+ {

+ .name = "adp5520-led",

+ },

+ {
+ .name = "adp5520-gpio",
+ },
+ {
+ .name = "adp5520-keys",
+ },
+};
+
static int __devinit adp5520_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -284,11 +300,20 @@
goto out_free_irq;
}

+#if 0
ret = adp5520_add_subdevs(chip, pdata);

if (!ret)
return ret;
+#endif

+ ret = mfd_add_devices(&chip->dev, id->driver_data,
+ adp5520_cells, ARRAY_SIZE(adp5520_cells),
+ NULL, client->irq);
+
+ if (!ret)
+ return ret;
+
out_free_irq:
if (chip->irq)
free_irq(chip->irq, chip);
@@ -337,7 +362,8 @@
#endif

static const struct i2c_device_id adp5520_id[] = {
- { "pmic-adp5520", 0 },
+ { "adp5520", ID_ADP5520 },
+ { "adp5501", ID_ADP5501 },
{ }
};
MODULE_DEVICE_TABLE(i2c, adp5520_id);


However this would just work for exactly one ADP5520 in a system.
A way out could be to append the I2C salve address to the cell .name?

Comments appreciated.

>
>Also, please note that you could use the mfd-core API for adding
devices, but
>that's just optional.
>
>Cheers,
>Samuel.
>
>--
>Intel Open Source Technology Centre
>http://oss.intel.com/

Best regards,
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/