| Message ID | 1398429171-8566-1-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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", ®); + 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");