| 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
| ||
|
Message-ID: <20140710083654.GQ2635@lee--X1>
Date: Thu, 10 Jul 2014 09:36:54 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Stanimir Varbanov <svarbanov@...sol.com>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Rob Herring <rob.herring@...xeda.com>,
Kumar Gala <galak@...eaurora.org>,
Grant Likely <grant.likely@...aro.org>,
Courtney Cavin <courtney.cavin@...ymobile.com>,
Josh Cartwright <joshc@...eaurora.org>
Subject: Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs
On Wed, 09 Jul 2014, Stanimir Varbanov wrote:
> Hi Lee,
>
> Thanks for the comments.
>
> On 07/09/2014 05:34 PM, Lee Jones wrote:
> > On Thu, 03 Jul 2014, Stanimir Varbanov wrote:
> >> From: Josh Cartwright <joshc@...eaurora.org>
> >>
> >> The Qualcomm QPNP PMIC chips are components used with the
> >> Snapdragon 800 series SoC family. This driver exists
> >> largely as a glue mfd component, it exists to be an owner
> >> of an SPMI regmap for children devices described in
> >> device tree.
> >>
> >> Signed-off-by: Josh Cartwright <joshc@...eaurora.org>
> >> Signed-off-by: Stanimir Varbanov <svarbanov@...sol.com>
> >> ---
> >> drivers/mfd/Kconfig | 15 ++++++
> >> drivers/mfd/Makefile | 1 +
> >> drivers/mfd/qpnp-spmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 145 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/mfd/qpnp-spmi.c
> >>
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index ee8204c..258b733 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -524,6 +524,21 @@ config MFD_PM8921_CORE
> >> Say M here if you want to include support for PM8921 chip as a module.
> >> This will build a module called "pm8921-core".
> >>
> >> +config MFD_QPNP_SPMI
> >> + tristate "Qualcomm QPNP SPMI PMIC"
> >> + depends on ARCH_QCOM || COMPILE_TEST
> >> + depends on OF
> >> + select MFD_CORE
> >> + select REGMAP_SPMI
> >> + help
> >> + This enables support for the Qualcomm QPNP SPMI PMICs.
> >> + These PMICs are currently used with the Snapdragon 800 series of
> >> + SoCs. Note, that this will only be useful paired with descriptions
> >> + of the independent functions as children nodes in the device tree.
> >
> > It would be helpful if the expected types of child devices were
> > mentioned here in the help section.
>
> They are many, but I could list the major ones.
>
> >
> >> + Say M here if you want to include support for the QPNP SPMI PMIC
> >> + series as a module. The module will be called "qpnp-spmi".
> >> +
> >> config MFD_RDC321X
> >> tristate "RDC R-321x southbridge"
> >> select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index 8afedba..31833d7 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -153,6 +153,7 @@ obj-$(CONFIG_MFD_SI476X_CORE) += si476x-core.o
> >> obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
> >> obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
> >> obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
> >> +obj-$(CONFIG_MFD_QPNP_SPMI) += qpnp-spmi.o
> >> obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
> >> obj-$(CONFIG_MFD_TPS65090) += tps65090.o
> >> obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
> >> diff --git a/drivers/mfd/qpnp-spmi.c b/drivers/mfd/qpnp-spmi.c
> >> new file mode 100644
> >> index 0000000..efd7d3e
> >> --- /dev/null
> >> +++ b/drivers/mfd/qpnp-spmi.c
> >> @@ -0,0 +1,129 @@
> >> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >> + *
> >
> > Non-standard multi-line comment. First line should be blank.
>
> sure, will fix.
>
> >
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 and
> >> + * only version 2 as published by the Free Software Foundation.
> >
> > No author?
> >
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/spmi.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/mfd/core.h>
> >> +
> >> +#define QPNP_RESOURCE_SIZE 256
> >> +
> >> +static const struct regmap_config qpnp_regmap_config = {
> >> + .reg_bits = 16,
> >> + .val_bits = 8,
> >> + .max_register = 0xffff,
> >> +};
> >> +
> >> +static int qpnp_index_to_resource(struct device_node *np, int index,
> >> + struct resource *res)
> >> +{
> >> + const char *name = NULL;
> >> + const __be32 *addrp;
> >> + u64 addr;
> >> +
> >> + addrp = of_get_address(np, index, NULL, NULL);
> >> + if (!addrp)
> >> + return -EINVAL;
> >> +
> >> + addr = of_read_number(addrp, 1);
> >> + if (addr == OF_BAD_ADDR)
> >> + return -EINVAL;
> >> +
> >> + of_property_read_string_index(np, "reg-names", index, &name);
> >> +
> >> + res->start = addr;
> >> + res->end = addr + QPNP_RESOURCE_SIZE - 1;
> >> + res->flags = IORESOURCE_REG;
> >> + res->name = name ? name : np->name;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int qpnp_add_device(struct spmi_device *root, struct device_node *child)
> >> +{
> >> + struct mfd_cell cell = {};
> >> + struct resource *res, *r;
> >> + int num_resources = 0;
> >> + const char *compat;
> >> + int ret, i;
> >> +
> >> + compat = of_get_property(child, "compatible", NULL);
> >> + if (!compat)
> >> + return -ENODEV;
> >> +
> >> + while (of_get_address(child, num_resources, NULL, NULL))
> >> + num_resources++;
> >> +
> >> + if (!num_resources)
> >> + return -ENODEV;
> >> +
> >> + res = kcalloc(num_resources, sizeof(*res), GFP_KERNEL);
> >> + if (!res)
> >> + return -ENOMEM;
> >> +
> >> + r = res;
> >> + for (i = 0; i < num_resources; i++, r++)
> >> + qpnp_index_to_resource(child, i, r);
> >> +
> >> + cell.name = kasprintf(GFP_KERNEL, "%x.%04x.%s", root->usid,
> >> + (u16)res[0].start, child->name);
> >> + cell.of_compatible = compat;
> >> + cell.num_resources = num_resources;
> >> + cell.resources = res;
> >> +
> >> + ret = mfd_add_devices(&root->dev, PLATFORM_DEVID_NONE, &cell, 1,
> >> + NULL, 0, NULL);
> >> +
> >> + kfree(res);
> >> + kfree(cell.name);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int qpnp_probe(struct spmi_device *sdev)
> >> +{
> >> + struct device_node *root = sdev->dev.of_node;
> >> + struct device_node *child;
> >> + struct regmap *regmap;
> >> +
> >> + regmap = devm_regmap_init_spmi_ext(sdev, &qpnp_regmap_config);
> >> + if (IS_ERR(regmap))
> >> + return PTR_ERR(regmap);
> >> +
> >> + for_each_available_child_of_node(root, child)
> >> + qpnp_add_device(sdev, child);
> >
> > This entire driver looks like a re-write of of_platform_populate().
> >
> > Why?
>
> of_platform_populate is not used because the PMIC function resources are
> non-translatable. You can see that the resources are of type
> IORESOURCE_REG (qpnp_index_to_resource()) not IORESOURCE_MEM or _IO.
>
> The whole point of this mfd driver is to parse devicetree to prepare
> resources for every child and create platform device for it through
> mfd_add_devices(). Then the PMIC function driver got its resources and
> use them as register addresses passed to regmap. These register accesses
> hits the SPMI controller which is the physical interface between PMIC's
> and SoC.
I can't help but think that if this is required, it should be part of
the core OF code, rather than doing your own thing which looks
frighteningly like existing framework functionality.
Either way, I'm going to need a DT Ack before I accept this.
--
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