| 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: Wed, 17 Sep 2014 22:39:26 -0700
From: Lee Jones <lee.jones@...aro.org>
To: Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc: IIO <linux-iio@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
DEVICE TREE <devicetree@...r.kernel.org>,
Carlo Caione <carlo@...one.org>,
Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
Aaron Lu <aaron.lu@...el.com>, Alan Cox <alan@...ux.intel.com>,
Jean Delvare <khali@...ux-fr.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Grant Likely <grant.likely@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Hartmut Knaack <knaack.h@....de>,
Fugang Duan <B38611@...escale.com>,
Arnd Bergmann <arnd@...db.de>,
Zubair Lutfullah <zubair.lutfullah@...il.com>,
Sebastian Reichel <sre@...ian.org>,
Johannes Thumshirn <johannes.thumshirn@....de>,
Philippe Reynes <tremyfr@...oo.fr>,
Angelo Compagnucci <angelo.compagnucci@...il.com>,
Doug Anderson <dianders@...omium.org>,
Ramakrishna Pallala <ramakrishna.pallala@...el.com>,
Peter Meerwald <pmeerw@...erw.net>,
Maxime Ripard <maxime.ripard@...e-electrons.com>,
Rafael Wysocki <rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v4 1/3] mfd/axp20x: extend axp20x to support axp288 pmic
On Tue, 16 Sep 2014, Jacob Pan wrote:
> X-Powers AXP288 is a customized PMIC for Intel Baytrail-CR platforms. Similar
> to AXP202/209, AXP288 comes with USB charger, more LDO and BUCK channels, and
> AD converters. It also provides extended status and interrupt reporting
> capabilities than the devices currently supported in axp20x.c.
>
> In addition to feature extension, this patch also adds ACPI binding for
> enumeration.
>
> This consolidated driver should support more X-Powers' PMICs in both device
> tree and ACPI enumerated platforms.
>
> Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> ---
> drivers/mfd/Kconfig | 3 +-
> drivers/mfd/axp20x.c | 353 +++++++++++++++++++++++++++++++++++++--------
> include/linux/mfd/axp20x.h | 58 ++++++++
> 3 files changed, 354 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index de5abf2..c183edb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -74,7 +74,8 @@ config MFD_AXP20X
> select REGMAP_IRQ
> depends on I2C=y
> help
> - If you say Y here you get support for the X-Powers AXP202 and AXP209.
> + If you say Y here you get support for the X-Powers AXP202, AXP209 and
> + AXP288 power management IC (PMIC).
> This driver include only the core APIs. You have to select individual
> components like regulators or the PEK (Power Enable Key) under the
> corresponding menus.
[...]
> -static const struct regmap_config axp20x_regmap_config = {
[...]
> +static struct regmap_config axp20x_regmap_config = {
Why have you removed const status?
> .reg_bits = 8,
> .val_bits = 8,
> .wr_table = &axp20x_writeable_table,
> @@ -70,47 +129,96 @@ static const struct regmap_config axp20x_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> -#define AXP20X_IRQ(_irq, _off, _mask) \
> - [AXP20X_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
> +static struct regmap_config axp288_regmap_config = {
const?
> + .reg_bits = 8,
> + .val_bits = 8,
> + .wr_table = &axp288_writeable_table,
> + .volatile_table = &axp288_volatile_table,
> + .max_register = AXP288_FG_TUNE5,
> + .cache_type = REGCACHE_RBTREE,
> +};
[...]
> -static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> +static struct acpi_device_id axp20x_acpi_match[] = {
> + {
> + .id = "INT33F4",
> + .driver_data = (kernel_ulong_t)AXP288_ID,
Why do you need to cast this?
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(acpi, axp20x_acpi_match);
> +
> +/* common irq chip attributes only */
> +static struct regmap_irq_chip axp20x_regmap_irq_chip = {
> .name = "axp20x_irq_chip",
> .status_base = AXP20X_IRQ1_STATE,
> .ack_base = AXP20X_IRQ1_STATE,
> .mask_base = AXP20X_IRQ1_EN,
> - .num_regs = 5,
> - .irqs = axp20x_regmap_irqs,
> - .num_irqs = ARRAY_SIZE(axp20x_regmap_irqs),
> .mask_invert = true,
> .init_ack_masked = true,
> };
> @@ -161,36 +276,155 @@ static struct mfd_cell axp20x_cells[] = {
> },
> };
[...]
> +static int axp20x_match_device(struct axp20x_dev *axp20x, struct device *dev)
> +{
> + const struct acpi_device_id *acpi_id;
> + const struct of_device_id *of_id;
> +
> + of_id = of_match_device(axp20x_of_match, dev);
> + if (of_id)
> + axp20x->variant = (long) of_id->data;
> + else {
> + acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
> + if (!acpi_id || !acpi_id->driver_data) {
> + dev_err(dev, "Unable to determine ID\n");
> + return -ENODEV;
> + }
> + axp20x->variant = (long) acpi_id->driver_data;
> + }
We can do better error handling here and give the user a better sense
of what happened if anything were to go wrong. Do:
if (dev->of_node)
of_id = of_match_device()
if (!of_id)
error()
variant = of_id->data;
else
acpi_id = acpi_match_device()
if (!acpi_id)
error()
variant = acpi_id->driver_data;
> + switch (axp20x->variant) {
> + case AXP202_ID:
> + case AXP209_ID:
> + axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> + axp20x->cells = axp20x_cells;
> + axp20x->regmap_cfg = &axp20x_regmap_config;
> + axp20x_regmap_irq_chip.num_regs = 5;
> + axp20x_regmap_irq_chip.irqs = axp20x_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp20x_regmap_irqs);
> + break;
> + case AXP288_ID:
> + axp20x->cells = axp288_cells;
> + axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
> + axp20x->regmap_cfg = &axp288_regmap_config;
> + axp20x_regmap_irq_chip.irqs = axp288_regmap_irqs;
> + axp20x_regmap_irq_chip.num_irqs =
> + ARRAY_SIZE(axp288_regmap_irqs);
> + axp20x_regmap_irq_chip.num_regs = 6;
> + break;
> + default:
> + dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> + return -ENODEV;
-EINVAL might be better here.
> + }
> + dev_info(dev, "AXP20x variant %s found\n",
> + axp20x_model_names[axp20x->variant]);
> +
> + return 0;
> +}
> +
> static int axp20x_i2c_probe(struct i2c_client *i2c,
> - const struct i2c_device_id *id)
> + const struct i2c_device_id *id)
Sneaky. ;)
> {
> struct axp20x_dev *axp20x;
> - const struct of_device_id *of_id;
> int ret;
>
> axp20x = devm_kzalloc(&i2c->dev, sizeof(*axp20x), GFP_KERNEL);
> if (!axp20x)
> return -ENOMEM;
>
> - of_id = of_match_device(axp20x_of_match, &i2c->dev);
> - if (!of_id) {
> - dev_err(&i2c->dev, "Unable to setup AXP20X data\n");
> - return -ENODEV;
> - }
> - axp20x->variant = (long) of_id->data;
> + ret = axp20x_match_device(axp20x, &i2c->dev);
> + if (ret)
> + return ret;
>
> axp20x->i2c_client = i2c;
> axp20x->dev = &i2c->dev;
> dev_set_drvdata(axp20x->dev, axp20x);
>
> - axp20x->regmap = devm_regmap_init_i2c(i2c, &axp20x_regmap_config);
> + axp20x->regmap = devm_regmap_init_i2c(i2c, axp20x->regmap_cfg);
> if (IS_ERR(axp20x->regmap)) {
> ret = PTR_ERR(axp20x->regmap);
> dev_err(&i2c->dev, "regmap init failed: %d\n", ret);
> @@ -206,8 +440,8 @@ static int axp20x_i2c_probe(struct i2c_client *i2c,
> return ret;
> }
>
> - ret = mfd_add_devices(axp20x->dev, -1, axp20x_cells,
> - ARRAY_SIZE(axp20x_cells), NULL, 0, NULL);
> + ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> + axp20x->nr_cells, NULL, 0, NULL);
>
> if (ret) {
> dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret);
[...]
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index d0e31a2..ffc69e2 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
[...]
> struct axp20x_dev {
> struct device *dev;
> struct i2c_client *i2c_client;
> struct regmap *regmap;
> struct regmap_irq_chip_data *regmap_irqc;
> long variant;
> + int nr_cells;
> + struct mfd_cell *cells;
> + struct regmap_config *regmap_cfg;
const?
> };
>
> #endif /* __LINUX_MFD_AXP20X_H */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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