[RFC PATCH] mfd: pm8x41: Naive function devices registration

[RFC PATCH] mfd: pm8x41: Naive function devices registration

[Ivan T. Ivanov]

From: "Ivan T. Ivanov" <iivanov@mm-sol.com>

Currently functions that exist in both the controller at the
same address offset can not be specified with the same names.

Adding Unique Slave ID device address to prefix function
device names fixes this.

Function devices are SPMI devices, so register them on
SPMI bus.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/mfd/pm8x41.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
index c85e0d6..29bc1e7 100644
--- a/drivers/mfd/pm8x41.c
+++ b/drivers/mfd/pm8x41.c
@@ -11,9 +11,10 @@
  */
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/spmi.h>
+#include <linux/of.h>
 #include <linux/regmap.h>
-#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/spmi.h>

 static const struct regmap_config pm8x41_regmap_config = {
 	.reg_bits	= 16,
@@ -23,7 +24,7 @@ static const struct regmap_config pm8x41_regmap_config = {

 static int pm8x41_remove_child(struct device *dev, void *unused)
 {
-	platform_device_unregister(to_platform_device(dev));
+	device_unregister(dev);
 	return 0;
 }

@@ -32,9 +33,40 @@ static void pm8x41_remove(struct spmi_device *sdev)
 	device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
 }

+static struct spmi_device *pm8x41_function_alloc(struct spmi_controller *ctrl,
+					       struct spmi_device *parent,
+					       struct device_node *node)
+{
+	struct spmi_device *function;
+	u32 reg;
+	int err;
+
+	err = of_property_read_u32(node, "reg", &reg);
+	if (err) {
+		dev_err(&parent->dev,
+			"node %s err (%d) does not have 'reg' property\n",
+			node->full_name, err);
+		return NULL;
+	}
+
+	function = spmi_device_alloc(ctrl);
+	if (!function)
+		return NULL;
+
+	function->dev.parent = &parent->dev;
+	function->dev.of_node = node;
+	function->usid = parent->usid;
+
+	dev_set_name(&function->dev, "%02x-%04x", function->usid, reg);
+
+	return function;
+}
+
 static int pm8x41_probe(struct spmi_device *sdev)
 {
+	struct device_node *node;
 	struct regmap *regmap;
+	int err = 0;

 	regmap = devm_regmap_init_spmi_ext(sdev, &pm8x41_regmap_config);
 	if (IS_ERR(regmap)) {
@@ -42,7 +74,24 @@ static int pm8x41_probe(struct spmi_device *sdev)
 		return PTR_ERR(regmap);
 	}

-	return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
+	for_each_available_child_of_node(sdev->dev.of_node, node) {
+		struct spmi_device *function;
+
+		dev_dbg(&sdev->dev, "adding child %s\n", node->full_name);
+
+		function = pm8x41_function_alloc(sdev->ctrl, sdev, node);
+		if (!function)
+			continue;
+
+		err = device_add(&function->dev);
+		if (err < 0) {
+			dev_err(&function->dev, "Can't add %s, status %d\n",
+				dev_name(&function->dev), err);
+			break;
+		}
+	}
+
+	return err;
 }

 static const struct of_device_id pm8x41_id_table[] = {
@@ -61,3 +110,7 @@ static struct spmi_driver pm8x41_driver = {
 	},
 };
 module_spmi_driver(pm8x41_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm SPMI PMIC core driver");
+MODULE_ALIAS("spmi:pm8x41");
--
1.8.3.2

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Rob Herring]

On Fri, Apr 25, 2014 at 7:32 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>
> Currently functions that exist in both the controller at the
> same address offset can not be specified with the same names.
>
> Adding Unique Slave ID device address to prefix function
> device names fixes this.
>
> Function devices are SPMI devices, so register them on
> SPMI bus.
>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/mfd/pm8x41.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----

No, this should be fixed in the core, not the driver.

Rob

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Ivan T. Ivanov]

On Fri, 2014-04-25 at 08:00 -0500, Rob Herring wrote:
> On Fri, Apr 25, 2014 at 7:32 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >
> > Currently functions that exist in both the controller at the
> > same address offset can not be specified with the same names.
> >
> > Adding Unique Slave ID device address to prefix function
> > device names fixes this.
> >
> > Function devices are SPMI devices, so register them on
> > SPMI bus.
> >
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> >  drivers/mfd/pm8x41.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----
> 
> No, this should be fixed in the core, not the driver.

I think that at core level they are no issues. 
There is no name clashes with "top level" devices.

spmi@...{
	...
	child@0 {
	        compatible = "qcom,pm8941";
	        reg = <0x0 SPMI_USID>;

	        #address-cells = <1>;
	        #size-cells = <0>;

	        revid@100 {
	                compatible = "qcom,qpnp-revid";
	                reg = <0x100>;
	        };
	};

	child@4 {
	        compatible = "qcom,pm8841";
	        reg = <0x4 SPMI_USID>;

	        #address-cells = <1>;
	        #size-cells = <0>;

	        revid@100 {
	                compatible = "qcom,qpnp-revid";
	                reg = <0x100>;
	        };
	};
};

I don't have experience with SPMI devices, but it looks
like address partitioning is specific to this "PMIC"
controllers.

Regards,
Ivan

> 
> Rob

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Rob Herring]

On Fri, Apr 25, 2014 at 8:29 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> On Fri, 2014-04-25 at 08:00 -0500, Rob Herring wrote:
>> On Fri, Apr 25, 2014 at 7:32 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
>> >
>> > Currently functions that exist in both the controller at the
>> > same address offset can not be specified with the same names.
>> >
>> > Adding Unique Slave ID device address to prefix function
>> > device names fixes this.
>> >
>> > Function devices are SPMI devices, so register them on
>> > SPMI bus.
>> >
>> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
>> > ---
>> >  drivers/mfd/pm8x41.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----
>>
>> No, this should be fixed in the core, not the driver.
>
> I think that at core level they are no issues.

By core, I mean the device naming conventions used by the DT platform
device code. There is a problem and it should be handled.

As I mentioned in the other thread, either we should not use the
address on non-translatable addresses like this or append the parent
address.

> There is no name clashes with "top level" devices.
>
> spmi@...{
>         ...
>         child@0 {
>                 compatible = "qcom,pm8941";
>                 reg = <0x0 SPMI_USID>;
>
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 revid@100 {
>                         compatible = "qcom,qpnp-revid";
>                         reg = <0x100>;
>                 };
>         };
>
>         child@4 {
>                 compatible = "qcom,pm8841";
>                 reg = <0x4 SPMI_USID>;
>
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 revid@100 {
>                         compatible = "qcom,qpnp-revid";
>                         reg = <0x100>;
>                 };
>         };
> };
>
> I don't have experience with SPMI devices, but it looks
> like address partitioning is specific to this "PMIC"
> controllers.
>
> Regards,
> Ivan
>
>>
>> Rob
>
>

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Ivan T. Ivanov]

On Fri, 2014-04-25 at 08:43 -0500, Rob Herring wrote:
> On Fri, Apr 25, 2014 at 8:29 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > On Fri, 2014-04-25 at 08:00 -0500, Rob Herring wrote:
> >> On Fri, Apr 25, 2014 at 7:32 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> >> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> >> >
> >> > Currently functions that exist in both the controller at the
> >> > same address offset can not be specified with the same names.
> >> >
> >> > Adding Unique Slave ID device address to prefix function
> >> > device names fixes this.
> >> >
> >> > Function devices are SPMI devices, so register them on
> >> > SPMI bus.
> >> >
> >> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> >> > ---
> >> >  drivers/mfd/pm8x41.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >>
> >> No, this should be fixed in the core, not the driver.
> >
> > I think that at core level they are no issues.
> 
> By core, I mean the device naming conventions used by the DT platform
> device code. There is a problem and it should be handled.
> 
> As I mentioned in the other thread, either we should not use the
> address on non-translatable addresses like this or append the parent
> address.

Ok, I see.

Regards,
Ivan

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Josh Cartwright]

On Fri, Apr 25, 2014 at 03:32:51PM +0300, Ivan T. Ivanov wrote:
> From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> 
> Currently functions that exist in both the controller at the
> same address offset can not be specified with the same names.

The terminology here is a bit confusing.  When I read "controller", I
hear "SPMI controller", but this is really not a limitation of the SPMI
core, but rather a limitation of of_platform_populate() used by this
particular SPMI slave MFD driver.

> Adding Unique Slave ID device address to prefix function
> device names fixes this.
> 
> Function devices are SPMI devices, so register them on
> SPMI bus.

This is a step backwards.  The PMIC functions are not individually
addressable SPMI slaves, and as such should not be represented as
independent devices to the SPMI core.

They really are subfunctions of a particular SPMI slave, and should be
modeled as children of that slave device.  With this driver, we've
chosen to model the child devices as platform devices, but it could
also be a separate bus type.

  Josh

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC PATCH] mfd: pm8x41: Naive function devices registration

[Ivan T. Ivanov]

On Fri, 2014-04-25 at 09:15 -0500, Josh Cartwright wrote:
> On Fri, Apr 25, 2014 at 03:32:51PM +0300, Ivan T. Ivanov wrote:
> > From: "Ivan T. Ivanov" <iivanov@mm-sol.com>
> > 
> > Currently functions that exist in both the controller at the
> > same address offset can not be specified with the same names.
> 
> The terminology here is a bit confusing.  When I read "controller", I
> hear "SPMI controller", 

Yes, it is badly worded.

> but this is really not a limitation of the SPMI
> core, but rather a limitation of of_platform_populate() used by this
> particular SPMI slave MFD driver.
> 
> > Adding Unique Slave ID device address to prefix function
> > device names fixes this.
> > 
> > Function devices are SPMI devices, so register them on
> > SPMI bus.
> 
> This is a step backwards.  The PMIC functions are not individually
> addressable SPMI slaves, and as such should not be represented as
> independent devices to the SPMI core.
> 
> They really are subfunctions of a particular SPMI slave, and should be
> modeled as children of that slave device.  With this driver, we've
> chosen to model the child devices as platform devices, but it could
> also be a separate bus type.

I tend to agree. My reasoning was that they are part of the 
device which sits on the SPMI bus, so they should also be part
of this bus.

Regards,
Ivan

> 
>   Josh
>