Re: [RFC PATCH v2 1/5] mfd: qpnp: add support for Qualcomm QPNP PMICs

[Stanimir Varbanov]

Hi Lee,

On 07/11/2014 12:07 PM, Lee Jones wrote:
> On Thu, 10 Jul 2014, Stanimir Varbanov wrote:
>>>>>> 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@xxxxxxxxxxxxxx>
>>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxxxxx>
>>>>>> ---
>>>>>>  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.
>>>>>
>>
>> <snip>
>>
>>>>>
>>>>>> + * 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.
>>
>> does it make sense to have a common of_mfd code for devicetree parsing?
>> Is that discussed already? I mean presently the mfd_cell resources and
>> number of resources are passed by the mfd_add_devices users. Is it
>> possible to have common code which parses devicetree sub-nodes of the
>> parent device_node and fill resources/create platform devices for them.
> 
> I'm not sure it does.  Normally users _either_ represent devices in
> MFD cells from within the driver _or_ populate using existing DT
> interfaces i.e. of_platform_populate().  This is the first time I've
> seen someone attempt to parse the entire MFD node structure from
> within a driver.

Our goal is to use common mfd driver for various Qualcomm PMIC from the
same PMIC generation (codenamed QPNP). Presently we have good
abstraction of the register manipulation functions through regmap-spmi
layer. Using the devicetree to represent different peripherals base
addresses (reg property) gives us a benefit by avoiding huge header
files in linux/mfd and make flexible transition to new PMIC's which just
change the base addresses but keep the IP block the same. This way we
should change the devicetree binding without touch the drivers.

> 
> After searching for some documentation to try and figure this out, I
> noticed that you've also missed a patch from your set:
> 
>   mfd: qpnp-spmi: document DT bindings for Qualcomm QPNP PMICs

I sent a new version without RFC tag which includes this binding
document [1].

> 
> ... which might help enlighten the DT guys.
> 

-- 
regards,
Stan

[1] https://lkml.org/lkml/2014/7/8/428
--
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/