| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 1 Oct 2009 16:09:49 +0200
From: Samuel Ortiz <sameo@...ux.intel.com>
To: Mike Frysinger <vapier@...too.org>
Cc: linux-kernel@...r.kernel.org,
uclinux-dist-devel@...ckfin.uclinux.org,
Michael Hennerich <michael.hennerich@...log.com>,
Bryan Wu <cooloney@...nel.org>
Subject: Re: [PATCH v2] mfd: ADP5520 Multifunction LCD Backlight and Keypad
Input Device Driver
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@...log.com>
>
> 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@...log.com>
> Signed-off-by: Bryan Wu <cooloney@...nel.org>
> Signed-off-by: Mike Frysinger <vapier@...too.org>
> ---
> 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@...pulab.co.il>
> + *
> + * Copyright (C) 2006-2008 Marvell International Ltd.
> + * Eric Miao <eric.miao@...vell.com>
> + *
> + * 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, ®_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, ®_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, ®_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, ®_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 ?
> + 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.
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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists