[PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

Update compatible string with runtime detected chip revision
information, for example qcom,pm8941 will become qcom,pm8941-v1.0.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 .../devicetree/bindings/mfd/qcom,spmi-pmic.txt     |  18 ++-
 drivers/mfd/qcom-spmi-pmic.c                       | 142 +++++++++++++++++++++
 2 files changed, 156 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
index 7182b88..bbe7db8 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
@@ -15,10 +15,20 @@ each. A function can consume one or more of these fixed-size register regions.
 
 Required properties:
 - compatible:      Should contain one of:
-                     "qcom,pm8941"
-                     "qcom,pm8841"
-                     "qcom,pma8084"
-                     or generalized "qcom,spmi-pmic".
+                   qcom,pm8941,
+                   qcom,pm8841,
+                   qcom,pm8019,
+                   qcom,pm8226,
+                   qcom,pm8110,
+                   qcom,pma8084,
+                   qcom,pmi8962,
+                   qcom,pmd9635,
+                   qcom,pm8994,
+                   qcom,pmi8994,
+                   qcom,pm8916,
+                   qcom,pm8004,
+                   qcom,pm8909,
+                   or generalized "qcom,spmi-pmic".
 - reg:             Specifies the SPMI USID slave address for this device.
                    For more information see:
                    Documentation/devicetree/bindings/spmi/spmi.txt
diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c
index 4b8beb2..67446a4 100644
--- a/drivers/mfd/qcom-spmi-pmic.c
+++ b/drivers/mfd/qcom-spmi-pmic.c
@@ -13,10 +13,126 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/spmi.h>
 #include <linux/regmap.h>
+#include <linux/of.h>
 #include <linux/of_platform.h>
 
+#define PMIC_REV2		0x101
+#define PMIC_REV3		0x102
+#define PMIC_REV4		0x103
+#define PMIC_TYPE		0x104
+#define PMIC_SUBTYPE		0x105
+
+#define PMIC_TYPE_VALUE		0x51
+
+#define PM8941_SUBTYPE		0x01
+#define PM8841_SUBTYPE		0x02
+#define PM8019_SUBTYPE		0x03
+#define PM8226_SUBTYPE		0x04
+#define PM8110_SUBTYPE		0x05
+#define PMA8084_SUBTYPE		0x06
+#define PMI8962_SUBTYPE		0x07
+#define PMD9635_SUBTYPE		0x08
+#define PM8994_SUBTYPE		0x09
+#define PMI8994_SUBTYPE		0x0a
+#define PM8916_SUBTYPE		0x0b
+#define PM8004_SUBTYPE		0x0c
+#define PM8909_SUBTYPE		0x0d
+
+static int pmic_spmi_read_revid(struct regmap *map, char **name,
+				int *major, int *minor)
+{
+	unsigned int rev2, rev3, rev4, type, subtype;
+	int ret;
+
+	ret = regmap_read(map, PMIC_TYPE, &type);
+	if (ret < 0)
+		return ret;
+
+	if (type != PMIC_TYPE_VALUE)
+		return -EINVAL;
+
+	ret = regmap_read(map, PMIC_SUBTYPE, &subtype);
+	if (ret < 0)
+		return ret;
+
+	rev2 = regmap_read(map, PMIC_REV2, &rev2);
+	if (ret < 0)
+		return ret;
+
+	rev3 = regmap_read(map, PMIC_REV3, &rev3);
+	if (ret < 0)
+		return ret;
+
+	rev4 = regmap_read(map, PMIC_REV4, &rev4);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * In early versions of PM8941 and PM8226, the major revision number
+	 * started incrementing from 0 (eg 0 = v1.0, 1 = v2.0).
+	 * Increment the major revision number here if the chip is an early
+	 * version of PM8941 or PM8226.
+	 */
+	if ((subtype == PM8941_SUBTYPE || subtype == PM8226_SUBTYPE) &&
+	    rev4 < 0x02)
+		rev4++;
+
+	*major = rev4;
+	if (subtype == PM8110_SUBTYPE)
+		*minor = rev2;
+	else
+		*minor = rev3;
+
+	switch (subtype) {
+	case PM8941_SUBTYPE:
+		*name = "pm8941";
+		break;
+	case PM8841_SUBTYPE:
+		*name = "pm8841";
+		break;
+	case PM8019_SUBTYPE:
+		*name = "pm8019";
+		break;
+	case PM8226_SUBTYPE:
+		*name = "pm8226";
+		break;
+	case PM8110_SUBTYPE:
+		*name = "pm8110";
+		break;
+	case PMA8084_SUBTYPE:
+		*name = "pma8084";
+		break;
+	case PMI8962_SUBTYPE:
+		*name = "pmi8962";
+		break;
+	case PMD9635_SUBTYPE:
+		*name = "pmd8635";
+		break;
+	case PM8994_SUBTYPE:
+		*name = "pm8994";
+		break;
+	case PMI8994_SUBTYPE:
+		*name = "pmi8994";
+		break;
+	case PM8916_SUBTYPE:
+		*name = "pm8916";
+		break;
+	case PM8004_SUBTYPE:
+		*name = "pm8004";
+		break;
+	case PM8909_SUBTYPE:
+		*name = "pm8909";
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct regmap_config spmi_regmap_config = {
 	.reg_bits	= 16,
 	.val_bits	= 8,
@@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
 {
 	struct device_node *root = sdev->dev.of_node;
 	struct regmap *regmap;
+	struct property *prop;
+	int major, minor, ret;
+	char *name, compatible[32];
 
 	regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
+	ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
+	if (!ret) {
+		snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
+			 name, major, minor);
+		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+		if (prop) {
+			prop->name = kstrdup("compatible", GFP_KERNEL);
+			prop->value = kstrdup(compatible, GFP_KERNEL);
+			prop->length = strlen(prop->value);
+			of_update_property(root, prop);
+		}
+	}
+
 	return of_platform_populate(root, NULL, NULL, &sdev->dev);
 }
 
@@ -45,7 +177,17 @@ static const struct of_device_id pmic_spmi_id_table[] = {
 	{ .compatible = "qcom,spmi-pmic" },
 	{ .compatible = "qcom,pm8941" },
 	{ .compatible = "qcom,pm8841" },
+	{ .compatible = "qcom,pm8019" },
+	{ .compatible = "qcom,pm8226" },
+	{ .compatible = "qcom,pm8110" },
 	{ .compatible = "qcom,pma8084" },
+	{ .compatible = "qcom,pmi8962" },
+	{ .compatible = "qcom,pmd9635" },
+	{ .compatible = "qcom,pm8994" },
+	{ .compatible = "qcom,pmi8994" },
+	{ .compatible = "qcom,pm8916" },
+	{ .compatible = "qcom,pm8004" },
+	{ .compatible = "qcom,pm8909" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pmic_spmi_id_table);
-- 
1.9.1

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Fabio Estevam]

On Tue, Nov 4, 2014 at 11:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:

> +       ret = regmap_read(map, PMIC_SUBTYPE, &subtype);
> +       if (ret < 0)
> +               return ret;
> +
> +       rev2 = regmap_read(map, PMIC_REV2, &rev2);
> +       if (ret < 0)
> +               return ret;

I think you meant:
ret = regmap_read(map, PMIC_REV2, &rev2);

> +
> +       rev3 = regmap_read(map, PMIC_REV3, &rev3);

Likewise.

> +       if (ret < 0)
> +               return ret;
> +
> +       rev4 = regmap_read(map, PMIC_REV4, &rev4);

Likewise.

> +       if (ret < 0)
> +               return ret;

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Tue, 2014-11-04 at 12:56 -0200, Fabio Estevam wrote:
> On Tue, Nov 4, 2014 at 11:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> 
> > +       ret = regmap_read(map, PMIC_SUBTYPE, &subtype);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       rev2 = regmap_read(map, PMIC_REV2, &rev2);
> > +       if (ret < 0)
> > +               return ret;
> 
> I think you meant:
> ret = regmap_read(map, PMIC_REV2, &rev2);
> 
> > +
> > +       rev3 = regmap_read(map, PMIC_REV3, &rev3);
> 
> Likewise.
> 
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       rev4 = regmap_read(map, PMIC_REV4, &rev4);
> 
> Likewise.
> 
> > 


True. Will fix.

Thank you.
Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Stanimir Varbanov]

Hi Ivan,

Thanks for the patch!

On 11/04/2014 03:33 PM, Ivan T. Ivanov wrote:
> Update compatible string with runtime detected chip revision
> information, for example qcom,pm8941 will become qcom,pm8941-v1.0.
> 
> Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> ---
>  .../devicetree/bindings/mfd/qcom,spmi-pmic.txt     |  18 ++-
>  drivers/mfd/qcom-spmi-pmic.c                       | 142 +++++++++++++++++++++
>  2 files changed, 156 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> index 7182b88..bbe7db8 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> @@ -15,10 +15,20 @@ each. A function can consume one or more of these fixed-size register regions.
>  
>  Required properties:
>  - compatible:      Should contain one of:
> -                     "qcom,pm8941"
> -                     "qcom,pm8841"
> -                     "qcom,pma8084"
> -                     or generalized "qcom,spmi-pmic".
> +                   qcom,pm8941,
> +                   qcom,pm8841,
> +                   qcom,pm8019,
> +                   qcom,pm8226,
> +                   qcom,pm8110,
> +                   qcom,pma8084,
> +                   qcom,pmi8962,
> +                   qcom,pmd9635,
> +                   qcom,pm8994,
> +                   qcom,pmi8994,
> +                   qcom,pm8916,
> +                   qcom,pm8004,
> +                   qcom,pm8909,
> +                   or generalized "qcom,spmi-pmic".
>  - reg:             Specifies the SPMI USID slave address for this device.
>                     For more information see:
>                     Documentation/devicetree/bindings/spmi/spmi.txt
> diff --git a/drivers/mfd/qcom-spmi-pmic.c b/drivers/mfd/qcom-spmi-pmic.c
> index 4b8beb2..67446a4 100644
> --- a/drivers/mfd/qcom-spmi-pmic.c
> +++ b/drivers/mfd/qcom-spmi-pmic.c
> @@ -13,10 +13,126 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
>  #include <linux/spmi.h>
>  #include <linux/regmap.h>
> +#include <linux/of.h>
>  #include <linux/of_platform.h>
>  
> +#define PMIC_REV2		0x101
> +#define PMIC_REV3		0x102
> +#define PMIC_REV4		0x103
> +#define PMIC_TYPE		0x104
> +#define PMIC_SUBTYPE		0x105
> +
> +#define PMIC_TYPE_VALUE		0x51
> +
> +#define PM8941_SUBTYPE		0x01
> +#define PM8841_SUBTYPE		0x02
> +#define PM8019_SUBTYPE		0x03
> +#define PM8226_SUBTYPE		0x04
> +#define PM8110_SUBTYPE		0x05
> +#define PMA8084_SUBTYPE		0x06
> +#define PMI8962_SUBTYPE		0x07
> +#define PMD9635_SUBTYPE		0x08
> +#define PM8994_SUBTYPE		0x09
> +#define PMI8994_SUBTYPE		0x0a
> +#define PM8916_SUBTYPE		0x0b
> +#define PM8004_SUBTYPE		0x0c
> +#define PM8909_SUBTYPE		0x0d
> +
> +static int pmic_spmi_read_revid(struct regmap *map, char **name,
> +				int *major, int *minor)
> +{
> +	unsigned int rev2, rev3, rev4, type, subtype;
> +	int ret;
> +
> +	ret = regmap_read(map, PMIC_TYPE, &type);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (type != PMIC_TYPE_VALUE)
> +		return -EINVAL;
> +
> +	ret = regmap_read(map, PMIC_SUBTYPE, &subtype);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev2 = regmap_read(map, PMIC_REV2, &rev2);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev3 = regmap_read(map, PMIC_REV3, &rev3);
> +	if (ret < 0)
> +		return ret;
> +
> +	rev4 = regmap_read(map, PMIC_REV4, &rev4);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * In early versions of PM8941 and PM8226, the major revision number
> +	 * started incrementing from 0 (eg 0 = v1.0, 1 = v2.0).
> +	 * Increment the major revision number here if the chip is an early
> +	 * version of PM8941 or PM8226.
> +	 */
> +	if ((subtype == PM8941_SUBTYPE || subtype == PM8226_SUBTYPE) &&
> +	    rev4 < 0x02)
> +		rev4++;
> +
> +	*major = rev4;
> +	if (subtype == PM8110_SUBTYPE)
> +		*minor = rev2;
> +	else
> +		*minor = rev3;
> +
> +	switch (subtype) {
> +	case PM8941_SUBTYPE:
> +		*name = "pm8941";
> +		break;

The XXX_SUBTYPE seems are continuous why not make it an const array and
get the name by index in this array?

> +	case PM8841_SUBTYPE:
> +		*name = "pm8841";
> +		break;
> +	case PM8019_SUBTYPE:
> +		*name = "pm8019";
> +		break;
> +	case PM8226_SUBTYPE:
> +		*name = "pm8226";
> +		break;
> +	case PM8110_SUBTYPE:
> +		*name = "pm8110";
> +		break;
> +	case PMA8084_SUBTYPE:
> +		*name = "pma8084";
> +		break;
> +	case PMI8962_SUBTYPE:
> +		*name = "pmi8962";
> +		break;
> +	case PMD9635_SUBTYPE:
> +		*name = "pmd8635";
> +		break;
> +	case PM8994_SUBTYPE:
> +		*name = "pm8994";
> +		break;
> +	case PMI8994_SUBTYPE:
> +		*name = "pmi8994";
> +		break;
> +	case PM8916_SUBTYPE:
> +		*name = "pm8916";
> +		break;
> +	case PM8004_SUBTYPE:
> +		*name = "pm8004";
> +		break;
> +	case PM8909_SUBTYPE:
> +		*name = "pm8909";
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct regmap_config spmi_regmap_config = {
>  	.reg_bits	= 16,
>  	.val_bits	= 8,
> @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
>  {
>  	struct device_node *root = sdev->dev.of_node;
>  	struct regmap *regmap;
> +	struct property *prop;
> +	int major, minor, ret;
> +	char *name, compatible[32];
>  
>  	regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
>  	if (IS_ERR(regmap))
>  		return PTR_ERR(regmap);
>  
> +	ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
> +	if (!ret) {

Are you sure that we want to continue if we can't read the revision id
and therefore will not be able to construct properly the compatible
property?

> +		snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
> +			 name, major, minor);
> +		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +		if (prop) {
> +			prop->name = kstrdup("compatible", GFP_KERNEL);
> +			prop->value = kstrdup(compatible, GFP_KERNEL);
> +			prop->length = strlen(prop->value);
> +			of_update_property(root, prop);

of_update_property can fail, check the returned value.

<snip>

-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Tue, 2014-11-04 at 17:06 +0200, Stanimir Varbanov wrote:
> Hi Ivan,
> 
> 
+
> > +       switch (subtype) {
> > +       case PM8941_SUBTYPE:
> > +       *name = "pm8941";
> > +       break;
> 
> The XXX_SUBTYPE seems are continuous why not make it an const array and
> get the name by index in this array?
> 

Yep, it _seems_ to be continuous. But, yes. probably using array will
more compact way to represent this.

<snip>

> > @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
> >  {
> >         struct device_node *root = sdev->dev.of_node;
> >         struct regmap *regmap;
> > +       struct property *prop;
> > +       int major, minor, ret;
> > +       char *name, compatible[32];
> > 
> >         regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
> >         if (IS_ERR(regmap))
> >         return PTR_ERR(regmap);
> > 
> > +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
> > +       if (!ret) {
> 
> Are you sure that we want to continue if we can't read the revision id
> and therefore will not be able to construct properly the compatible
> property?
> 

Yes. Driver is working fine even without exact chip version
appended to compatible string.

> > +       snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
> > +       name, major, minor);
> > +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +       if (prop) {
> > +       prop->name = kstrdup("compatible", GFP_KERNEL);
> > +       prop->value = kstrdup(compatible, GFP_KERNEL);
> > +       prop->length = strlen(prop->value);
> > +       of_update_property(root, prop);
> 
> of_update_property can fail, check the returned value.

Same thing as above, but probably allocated memory at least can be freed.

Thanks Ivan.

> 
> <snip>
> 
> 

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Stanimir Varbanov]

On 11/04/2014 05:22 PM, Ivan T. Ivanov wrote:
> 
> On Tue, 2014-11-04 at 17:06 +0200, Stanimir Varbanov wrote:
>> Hi Ivan,
>>
>>
> +
>>> +       switch (subtype) {
>>> +       case PM8941_SUBTYPE:
>>> +       *name = "pm8941";
>>> +       break;
>>
>> The XXX_SUBTYPE seems are continuous why not make it an const array and
>> get the name by index in this array?
>>
> 
> Yep, it _seems_ to be continuous. But, yes. probably using array will
> more compact way to represent this.
> 
> <snip>
> 
>>> @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
>>>  {
>>>         struct device_node *root = sdev->dev.of_node;
>>>         struct regmap *regmap;
>>> +       struct property *prop;
>>> +       int major, minor, ret;
>>> +       char *name, compatible[32];
>>>
>>>         regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
>>>         if (IS_ERR(regmap))
>>>         return PTR_ERR(regmap);
>>>
>>> +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
>>> +       if (!ret) {
>>
>> Are you sure that we want to continue if we can't read the revision id
>> and therefore will not be able to construct properly the compatible
>> property?
>>
> 
> Yes. Driver is working fine even without exact chip version
> appended to compatible string.
> 
>>> +       snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
>>> +       name, major, minor);
>>> +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>>> +       if (prop) {
>>> +       prop->name = kstrdup("compatible", GFP_KERNEL);
>>> +       prop->value = kstrdup(compatible, GFP_KERNEL);
>>> +       prop->length = strlen(prop->value);
>>> +       of_update_property(root, prop);
>>
>> of_update_property can fail, check the returned value.
> 
> Same thing as above, but probably allocated memory at least can be freed.

might be better idea to use devm_kzalloc and devm_kstrdup?

-- 
regards,
Stan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Tue, 2014-11-04 at 17:26 +0200, Stanimir Varbanov wrote:
> On 11/04/2014 05:22 PM, Ivan T. Ivanov wrote:
> > On Tue, 2014-11-04 at 17:06 +0200, Stanimir Varbanov wrote:

<snip>

> > > > +       snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
> > > > +       name, major, minor);
> > > > +       prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > > > +       if (prop) {
> > > > +       prop->name = kstrdup("compatible", GFP_KERNEL);
> > > > +       prop->value = kstrdup(compatible, GFP_KERNEL);
> > > > +       prop->length = strlen(prop->value);
> > > > +       of_update_property(root, prop);
> > > 
> > > of_update_property can fail, check the returned value.
> > 
> > Same thing as above, but probably allocated memory at least can be freed.
> 
> might be better idea to use devm_kzalloc and devm_kstrdup?
> 

compatible property is attached to device not to driver, so
memory should be there even after driver is unloaded, I think.

Regards,
Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Andreas Färber]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Hi,

Am 04.11.2014 um 14:33 schrieb Ivan T. Ivanov:
> Update compatible string with runtime detected chip revision
> information, for example qcom,pm8941 will become qcom,pm8941-v1.0.

That's not what the patch does though?

> 
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  .../devicetree/bindings/mfd/qcom,spmi-pmic.txt     |  18 ++-
>  drivers/mfd/qcom-spmi-pmic.c                       | 142 +++++++++++++++++++++
>  2 files changed, 156 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> index 7182b88..bbe7db8 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> @@ -15,10 +15,20 @@ each. A function can consume one or more of these fixed-size register regions.
>  
>  Required properties:
>  - compatible:      Should contain one of:
> -                     "qcom,pm8941"
> -                     "qcom,pm8841"
> -                     "qcom,pma8084"
> -                     or generalized "qcom,spmi-pmic".
> +                   qcom,pm8941,
> +                   qcom,pm8841,
> +                   qcom,pm8019,
> +                   qcom,pm8226,
> +                   qcom,pm8110,
> +                   qcom,pma8084,
> +                   qcom,pmi8962,
> +                   qcom,pmd9635,
> +                   qcom,pm8994,
> +                   qcom,pmi8994,
> +                   qcom,pm8916,
> +                   qcom,pm8004,
> +                   qcom,pm8909,

Please either keep the strings consistently quoted, or drop the trailing
comma to avoid any confusion of where it terminates.

> +                   or generalized "qcom,spmi-pmic".
>  - reg:             Specifies the SPMI USID slave address for this device.
>                     For more information see:
>                     Documentation/devicetree/bindings/spmi/spmi.txt

Regards,
Andreas

[...]
> @@ -45,7 +177,17 @@ static const struct of_device_id pmic_spmi_id_table[] = {
>  	{ .compatible = "qcom,spmi-pmic" },
>  	{ .compatible = "qcom,pm8941" },
>  	{ .compatible = "qcom,pm8841" },
> +	{ .compatible = "qcom,pm8019" },
> +	{ .compatible = "qcom,pm8226" },
> +	{ .compatible = "qcom,pm8110" },
>  	{ .compatible = "qcom,pma8084" },
> +	{ .compatible = "qcom,pmi8962" },
> +	{ .compatible = "qcom,pmd9635" },
> +	{ .compatible = "qcom,pm8994" },
> +	{ .compatible = "qcom,pmi8994" },
> +	{ .compatible = "qcom,pm8916" },
> +	{ .compatible = "qcom,pm8004" },
> +	{ .compatible = "qcom,pm8909" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pmic_spmi_id_table);

-- 
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Wed, 2014-11-05 at 13:49 +0100, Andreas Färber wrote:
> Hi,
> 
> Am 04.11.2014 um 14:33 schrieb Ivan T. Ivanov:
> > Update compatible string with runtime detected chip revision
> > information, for example qcom,pm8941 will become qcom,pm8941-v1.0.
> 
> That's not what the patch does though?

Patch add support for more chips and make compatible property more specific.
Will correct description.

> 
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> >  .../devicetree/bindings/mfd/qcom,spmi-pmic.txt     |  18 ++-
> >  drivers/mfd/qcom-spmi-pmic.c                       | 142 +++++++++++++++++++++
> >  2 files changed, 156 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt 
> > b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> > index 7182b88..bbe7db8 100644
> > --- a/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> > +++ b/Documentation/devicetree/bindings/mfd/qcom,spmi-pmic.txt
> > @@ -15,10 +15,20 @@ each. A function can consume one or more of these fixed-size register 
> > regions.
> > 
> >  Required properties:
> >  - compatible:      Should contain one of:
> > -                     "qcom,pm8941"
> > -                     "qcom,pm8841"
> > -                     "qcom,pma8084"
> > -                     or generalized "qcom,spmi-pmic".
> > +                   qcom,pm8941,
> > +                   qcom,pm8841,
> > +                   qcom,pm8019,
> > +                   qcom,pm8226,
> > +                   qcom,pm8110,
> > +                   qcom,pma8084,
> > +                   qcom,pmi8962,
> > +                   qcom,pmd9635,
> > +                   qcom,pm8994,
> > +                   qcom,pmi8994,
> > +                   qcom,pm8916,
> > +                   qcom,pm8004,
> > +                   qcom,pm8909,
> 
> Please either keep the strings consistently quoted, or drop the trailing
> comma to avoid any confusion of where it terminates.

Sure, will fix it.

Thank you,
Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Bjorn Andersson]

On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
[..]
> @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device *sdev)
>  {
>         struct device_node *root = sdev->dev.of_node;
>         struct regmap *regmap;
> +       struct property *prop;
> +       int major, minor, ret;
> +       char *name, compatible[32];
>
>         regmap = devm_regmap_init_spmi_ext(sdev, &spmi_regmap_config);
>         if (IS_ERR(regmap))
>                 return PTR_ERR(regmap);
>
> +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
> +       if (!ret) {
> +               snprintf(compatible, ARRAY_SIZE(compatible), "qcom,%s-v%d.%d",
> +                        name, major, minor);
> +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +               if (prop) {
> +                       prop->name = kstrdup("compatible", GFP_KERNEL);
> +                       prop->value = kstrdup(compatible, GFP_KERNEL);
> +                       prop->length = strlen(prop->value);
> +                       of_update_property(root, prop);
> +               }
> +       }
> +

Why would you do this?
What benefit does it give to patch the of_node to have a more specific
compatible?

It is no longer matching any compatible defined in the kernel and is
anyone actually looking at this?

Reading out the revid information and providing that in some way to
the children could be beneficial, except for qpnp already giving you
this version information per block.

[..]
> @@ -45,7 +177,17 @@ static const struct of_device_id pmic_spmi_id_table[] = {
>         { .compatible = "qcom,spmi-pmic" },
>         { .compatible = "qcom,pm8941" },
>         { .compatible = "qcom,pm8841" },
> +       { .compatible = "qcom,pm8019" },
> +       { .compatible = "qcom,pm8226" },
> +       { .compatible = "qcom,pm8110" },
>         { .compatible = "qcom,pma8084" },
> +       { .compatible = "qcom,pmi8962" },
> +       { .compatible = "qcom,pmd9635" },
> +       { .compatible = "qcom,pm8994" },
> +       { .compatible = "qcom,pmi8994" },
> +       { .compatible = "qcom,pm8916" },
> +       { .compatible = "qcom,pm8004" },
> +       { .compatible = "qcom,pm8909" },
>         { }

This part is good, please send this out on it's own.

Regards,
Bjorn

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Wed, 2014-11-05 at 10:11 -0800, Bjorn Andersson wrote:
> On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com> 
> wrote:
> [..]
> > @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device 
> > *sdev)
> >  {
> >         struct device_node *root = sdev->dev.of_node;
> >         struct regmap *regmap;
> > +       struct property *prop;
> > +       int major, minor, ret;
> > +       char *name, compatible[32];
> > 
> >         regmap = devm_regmap_init_spmi_ext(sdev, 
> > &spmi_regmap_config);
> >         if (IS_ERR(regmap))
> >                 return PTR_ERR(regmap);
> > 
> > +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
> > +       if (!ret) {
> > +               snprintf(compatible, ARRAY_SIZE(compatible), 
> > "qcom,%s-v%d.%d",
> > +                        name, major, minor);
> > +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +               if (prop) {
> > +                       prop->name = kstrdup("compatible", 
> > GFP_KERNEL);
> > +                       prop->value = kstrdup(compatible, 
> > GFP_KERNEL);
> > +                       prop->length = strlen(prop->value);
> > +                       of_update_property(root, prop);
> > +               }
> > +       }
> > +
> 
> Why would you do this?
> What benefit does it give to patch the of_node to have a more 
> specific
> compatible?

Some of the child device drivers have to know PMIC chip revision.

Regards,
Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Bjorn Andersson]

On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> wrote:
>
> On Wed, 2014-11-05 at 10:11 -0800, Bjorn Andersson wrote:
>> On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>> wrote:
>> [..]
>> > @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device
>> > *sdev)
>> >  {
>> >         struct device_node *root = sdev->dev.of_node;
>> >         struct regmap *regmap;
>> > +       struct property *prop;
>> > +       int major, minor, ret;
>> > +       char *name, compatible[32];
>> >
>> >         regmap = devm_regmap_init_spmi_ext(sdev,
>> > &spmi_regmap_config);
>> >         if (IS_ERR(regmap))
>> >                 return PTR_ERR(regmap);
>> >
>> > +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
>> > +       if (!ret) {
>> > +               snprintf(compatible, ARRAY_SIZE(compatible),
>> > "qcom,%s-v%d.%d",
>> > +                        name, major, minor);
>> > +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> > +               if (prop) {
>> > +                       prop->name = kstrdup("compatible",
>> > GFP_KERNEL);
>> > +                       prop->value = kstrdup(compatible,
>> > GFP_KERNEL);
>> > +                       prop->length = strlen(prop->value);
>> > +                       of_update_property(root, prop);
>> > +               }
>> > +       }
>> > +
>>
>> Why would you do this?
>> What benefit does it give to patch the of_node to have a more
>> specific
>> compatible?
>
> Some of the child device drivers have to know PMIC chip revision.
>

So your plan is to have a strstr(parent->compatible, "-v2") there?

Could you be a little bit more elaborate on what you're trying to do
and which child devices that might be?

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
> On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> wrote:
> > On Wed, 2014-11-05 at 10:11 -0800, Bjorn Andersson wrote:
> > > On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> > > wrote:
> > > [..]
> > > > @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device
> > > > *sdev)
> > > >  {
> > > >         struct device_node *root = sdev->dev.of_node;
> > > >         struct regmap *regmap;
> > > > +       struct property *prop;
> > > > +       int major, minor, ret;
> > > > +       char *name, compatible[32];
> > > > 
> > > >         regmap = devm_regmap_init_spmi_ext(sdev,
> > > > &spmi_regmap_config);
> > > >         if (IS_ERR(regmap))
> > > >                 return PTR_ERR(regmap);
> > > > 
> > > > +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
> > > > +       if (!ret) {
> > > > +               snprintf(compatible, ARRAY_SIZE(compatible),
> > > > "qcom,%s-v%d.%d",
> > > > +                        name, major, minor);
> > > > +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > > > +               if (prop) {
> > > > +                       prop->name = kstrdup("compatible",
> > > > GFP_KERNEL);
> > > > +                       prop->value = kstrdup(compatible,
> > > > GFP_KERNEL);
> > > > +                       prop->length = strlen(prop->value);
> > > > +                       of_update_property(root, prop);
> > > > +               }
> > > > +       }
> > > > +
> > > 
> > > Why would you do this?
> > > What benefit does it give to patch the of_node to have a more
> > > specific
> > > compatible?
> > 
> > Some of the child device drivers have to know PMIC chip revision.
> > 
> 
> So your plan is to have a strstr(parent->compatible, "-v2") there?

Actually also PMIC subtype (pm8841, pm8226...) is also required, so 
the plan is to have something like this:

{
        static const struct of_device_id pmic_match_table[] = {
                { .compatible = "qcom,pm8941-v1.0" },
                { .compatible = "qcom,pm8841-v0.0" },
                { }

        };

        const struct of_device_id *match;

        match = of_match_device(pmic_match_table, pdev->dev.parent);
        if (match) {
                dev_info(&pdev->dev, "%s chip detected\n", match->compatible);
        }
}

> 
> Could you be a little bit more elaborate on what you're trying to do
> and which child devices that might be?

For example ADC drivers are required temperature compensation based
on PMIC variant and chip manufacturer.

This patch have one issue, at least :-). Using of_update_property will prevent
driver to be build as module. which, I think, is coming from the fact the
on first load it will modify device compatible property and will be impossible
driver to match device id again. Still thinking how to overcome this.

Regards,
Ivan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Bjorn Andersson]

On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>
> On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
>> On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
[..]
>> > Some of the child device drivers have to know PMIC chip revision.
>> >
>>
>> So your plan is to have a strstr(parent->compatible, "-v2") there?
>
> Actually also PMIC subtype (pm8841, pm8226...) is also required, so
> the plan is to have something like this:
>
> {
>         static const struct of_device_id pmic_match_table[] = {
>                 { .compatible = "qcom,pm8941-v1.0" },
>                 { .compatible = "qcom,pm8841-v0.0" },
>                 { }
>
>         };
>
>         const struct of_device_id *match;
>
>         match = of_match_device(pmic_match_table, pdev->dev.parent);
>         if (match) {
>                 dev_info(&pdev->dev, "%s chip detected\n", match->compatible);
>         }
> }
>

To me this is a hack, you should not alter the devicetree to make it
"better express the hardware". Either you know these things from boot
and they go in device tree, or you can probe them and they should not
go in device tree.

If you really need these values you should expose them through some api.

>>
>> Could you be a little bit more elaborate on what you're trying to do
>> and which child devices that might be?
>
> For example ADC drivers are required temperature compensation based
> on PMIC variant and chip manufacturer.
>

I see, is that compensation of any practical value? Or is the
compensation of academic proportions?

Regards,
Bjorn

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]


On Thu, 2014-11-06 at 08:55 -0800, Bjorn Andersson wrote:
> On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
> > > On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> [..]
> > > > Some of the child device drivers have to know PMIC chip revision.
> > > > 
> > > 
> > > So your plan is to have a strstr(parent->compatible, "-v2") there?
> > 
> > Actually also PMIC subtype (pm8841, pm8226...) is also required, so
> > the plan is to have something like this:
> > 
> > {
> >         static const struct of_device_id pmic_match_table[] = {
> >                 { .compatible = "qcom,pm8941-v1.0" },
> >                 { .compatible = "qcom,pm8841-v0.0" },
> >                 { }
> > 
> >         };
> > 
> >         const struct of_device_id *match;
> > 
> >         match = of_match_device(pmic_match_table, pdev->dev.parent);
> >         if (match) {
> >                 dev_info(&pdev->dev, "%s chip detected\n", match->compatible);
> >         }
> > }
> > 
> 
> To me this is a hack, you should not alter the devicetree to make it
> "better express the hardware". Either you know these things from boot
> and they go in device tree, or you can probe them and they should not
> go in device tree.
> 
> If you really need these values you should expose them through some api.

I would like to avoid compile time dependency between these drivers.
There are several precedents of using of_update_property() for enhancing
compatible property already.  

> 
> > > Could you be a little bit more elaborate on what you're trying to do
> > > and which child devices that might be?
> > 
> > For example ADC drivers are required temperature compensation based
> > on PMIC variant and chip manufacturer.
> > 
> 
> I see, is that compensation of any practical value? Or is the
> compensation of academic proportions?

It depends of what you mean by academic :-). Attached file have test
application which dump difference between non compensated and compensated
values for different temperature, manufacture and input value.

Output format of the program is:
Column 1: manufacturer GF=0, SMIC=1, TSMC=2
Column 2: chip revision
Column 3: die temperature in mili deg Celsius 
Column 4: input for compensation in micro Volts
Column 5: compensated result in micro Volts
Column 6: difference in micro Volts


Regards,
Ivan

[-- Attachment #2: temperature-compensation-check.c --]
[-- Type: text/x-csrc, Size: 14107 bytes --]


#include <stdio.h>

/* SW index's for PMIC type and version used by QPNP VADC and IADC */
#define REV_ID_8941_3_1	1
#define REV_ID_8026_1_0	2
#define REV_ID_8026_2_0	3
#define REV_ID_8110_1_0	4
#define REV_ID_8026_2_1	5
#define REV_ID_8110_2_0	6
#define REV_ID_8026_2_2	7
#define REV_ID_8941_3_0	8
#define REV_ID_8941_2_0	9
#define REV_ID_8916_1_0	10
#define REV_ID_8916_1_1	11
#define REV_ID_8916_2_0	12

#define FACTORY_GF 		0
#define FACTORY_SMIC		1
#define FACTORY_TSMC		2

#define VBAT_COEFF_1	3000
#define VBAT_COEFF_2	45810000
#define VBAT_COEFF_3	100000
#define VBAT_COEFF_4	3500
#define VBAT_COEFF_5	80000000
#define VBAT_COEFF_6	4400
#define VBAT_COEFF_7	32200000
#define VBAT_COEFF_8	3880
#define VBAT_COEFF_9	5770
#define VBAT_COEFF_10	3660
#define VBAT_COEFF_11	5320
#define VBAT_COEFF_12	8060000
#define VBAT_COEFF_13	102640000
#define VBAT_COEFF_14	22220000
#define VBAT_COEFF_15	83060000
#define VBAT_COEFF_16	2810
#define VBAT_COEFF_17	5260
#define VBAT_COEFF_18	8027
#define VBAT_COEFF_19	2347
#define VBAT_COEFF_20	6043
#define VBAT_COEFF_21	1914
#define VBAT_OFFSET_SMIC	9446
#define VBAT_OFFSET_GF	9441
#define OCV_OFFSET_SMIC	4596
#define OCV_OFFSET_GF	5896
#define VBAT_COEFF_22	6800
#define VBAT_COEFF_23	3500
#define VBAT_COEFF_24	4360
#define VBAT_COEFF_25	8060
#define VBAT_COEFF_26	7895
#define VBAT_COEFF_27	5658
#define VBAT_COEFF_28	5760
#define VBAT_COEFF_29	7900
#define VBAT_COEFF_30	5660
#define VBAT_COEFF_31	3620
#define VBAT_COEFF_32	1230
#define VBAT_COEFF_33	5760
#define VBAT_COEFF_34	4080
#define VBAT_COEFF_35	7000
#define VBAT_COEFF_36	3040
#define VBAT_COEFF_37	3850
#define VBAT_COEFF_38	5000
#define VBAT_COEFF_39	2610
#define VBAT_COEFF_40	4190
#define VBAT_COEFF_41	5800
#define VBAT_COEFF_42	2620
#define VBAT_COEFF_43	4030
#define VBAT_COEFF_44	3230
#define VBAT_COEFF_45	3450
#define VBAT_COEFF_46	2120
#define VBAT_COEFF_47	3560
#define VBAT_COEFF_48	2190

static long
qpnp_ocv_comp(int version, int factory, long result, long die_temp)
{
    long temp_var = 0, offset = 0;

    if (version == REV_ID_8026_2_2) {
        if (die_temp > 25000)
            return result;
    }

    switch (version) {
        case REV_ID_8941_3_1:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_4));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_1));
                    break;
            }
            break;
        case REV_ID_8026_1_0:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = (((die_temp * (-VBAT_COEFF_10))
                                 - VBAT_COEFF_14));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = (((die_temp * (-VBAT_COEFF_8))
                                 + VBAT_COEFF_12));
                    break;
            }
            break;
        case REV_ID_8026_2_0:
        case REV_ID_8026_2_1:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_10));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_8));
                    break;
            }
            break;
        case REV_ID_8026_2_2:
            switch (factory) {
                case FACTORY_TSMC:
                    result -= VBAT_COEFF_22;
                    temp_var = (die_temp - 25000) * VBAT_COEFF_24;
                    break;
                default:
                case FACTORY_GF:
                    result -= VBAT_COEFF_22;
                    temp_var = (die_temp - 25000) * VBAT_COEFF_25;
                    break;
            }
            break;
        case REV_ID_8110_2_0:
            switch (factory) {
                case FACTORY_SMIC:
                    result -= OCV_OFFSET_SMIC;
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_18;
                    else
                        temp_var = VBAT_COEFF_19;
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
                default:
                case FACTORY_GF:
                    result -= OCV_OFFSET_GF;
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_20;
                    else
                        temp_var = VBAT_COEFF_21;
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
            }
            break;
        case REV_ID_8916_1_0:
            switch (factory) {
                case FACTORY_SMIC:
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_26;
                    else
                        temp_var = VBAT_COEFF_27;
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
                default:
                case FACTORY_GF:
                    offset = OCV_OFFSET_GF;
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_26;
                    else
                        temp_var = VBAT_COEFF_27;
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
            }
            break;
        case REV_ID_8916_1_1:
            switch (factory) {
                    /* FAB_ID is zero */
                case FACTORY_GF:
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_29;
                    else
                        temp_var = VBAT_COEFF_30;
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
                    /* FAB_ID is non-zero */
                default:
                    if (die_temp < 25000)
                        temp_var = VBAT_COEFF_31;
                    else
                        temp_var = (-VBAT_COEFF_32);
                    temp_var = (die_temp - 25000) * temp_var;
                    break;
            }
            break;
        case REV_ID_8916_2_0:
            switch (factory) {
                case FACTORY_SMIC:
                    offset = (-VBAT_COEFF_38);
                    if (die_temp < 0)
                        temp_var = die_temp * VBAT_COEFF_36;
                    else if (die_temp > 40000)
                        temp_var = ((die_temp - 40000) * (-VBAT_COEFF_37));
                    break;
                case FACTORY_TSMC:
                    if (die_temp < 10000)
                        temp_var = ((die_temp - 10000) * VBAT_COEFF_41);
                    else if (die_temp > 50000)
                        temp_var = ((die_temp - 50000) * (-VBAT_COEFF_42));
                    break;
                default:
                case FACTORY_GF:
                    if (die_temp < 20000)
                        temp_var = ((die_temp - 20000) * VBAT_COEFF_45);
                    else if (die_temp > 40000)
                        temp_var = ((die_temp - 40000) * (-VBAT_COEFF_46));
                    break;
            }
            break;
        default:
            temp_var = 0;
            break;
    }

    temp_var = temp_var / VBAT_COEFF_3;

    temp_var = 1000000 + temp_var;

    result = result * temp_var;

    if (offset)
        result -= offset;

    result = result / 1000000;

    return result;
}

static long
qpnp_vbat_sns_comp(int version, int factory, long result, long die_temp)
{
    long temp_var = 0, offset = 0;

    if (version != REV_ID_8941_3_1) {
        /* min(die_temp_c, 60_degC) */
        if (die_temp > 60000)
            die_temp = 60000;
    }

    switch (version) {
        case REV_ID_8941_3_1:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_1));
                    break;
                default:
                case FACTORY_GF:
                    /* min(die_temp_c, 60_degC) */
                    if (die_temp > 60000)
                        die_temp = 60000;
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_1));
                    break;
            }
            break;
        case REV_ID_8026_1_0:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = (((die_temp * (-VBAT_COEFF_11))
                                 + VBAT_COEFF_15));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = (((die_temp * (-VBAT_COEFF_9))
                                 + VBAT_COEFF_13));
                    break;
            }
            break;
        case REV_ID_8026_2_0:
        case REV_ID_8026_2_1:
            switch (factory) {
                case FACTORY_TSMC:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_11));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = ((die_temp - 25000) * (-VBAT_COEFF_9));
                    break;
            }
            break;
        case REV_ID_8026_2_2:
            switch (factory) {
                case FACTORY_TSMC:
                    result -= VBAT_COEFF_23;
                    temp_var = 0;
                    break;
                default:
                case FACTORY_GF:
                    result -= VBAT_COEFF_23;
                    temp_var = 0;
                    break;
            }
            break;
        case REV_ID_8110_2_0:
            switch (factory) {
                case FACTORY_SMIC:
                    result -= VBAT_OFFSET_SMIC;
                    temp_var = ((die_temp - 25000) * (VBAT_COEFF_17));
                    break;
                default:
                case FACTORY_GF:
                    result -= VBAT_OFFSET_GF;
                    temp_var = ((die_temp - 25000) * (VBAT_COEFF_16));
                    break;
            }
            break;
        case REV_ID_8916_1_0:
            switch (factory) {
                case FACTORY_SMIC:
                    temp_var = ((die_temp - 25000) * (VBAT_COEFF_28));
                    break;
                default:
                case FACTORY_GF:
                    temp_var = ((die_temp - 25000) * (VBAT_COEFF_28));
                    break;
            }
            break;
        case REV_ID_8916_1_1:
            switch (factory) {
                    /* FAB_ID is zero */
                case FACTORY_GF:
                    temp_var = ((die_temp - 25000) * (VBAT_COEFF_33));
                    break;
                    /* FAB_ID is non-zero */
                default:
                    offset = VBAT_COEFF_35;
                    if (die_temp > 50000) {
                        temp_var = ((die_temp - 25000) * (VBAT_COEFF_34));
                    }
                    break;
            }
            break;
        case REV_ID_8916_2_0:
            switch (factory) {
                case FACTORY_SMIC:
                    if (die_temp < 0) {
                        temp_var = (die_temp * VBAT_COEFF_39);
                    } else if (die_temp > 40000) {
                        temp_var = ((die_temp - 40000) * (-VBAT_COEFF_40));
                    }
                    break;
                case FACTORY_TSMC:
                    if (die_temp < 10000)
                        temp_var = ((die_temp - 10000) * VBAT_COEFF_43);
                    else if (die_temp > 50000)
                        temp_var = ((die_temp - 50000) * (-VBAT_COEFF_44));
                    break;
                default:
                case FACTORY_GF:
                    if (die_temp < 20000)
                        temp_var = ((die_temp - 20000) * VBAT_COEFF_47);
                    else if (die_temp > 40000)
                        temp_var = ((die_temp - 40000) * (-VBAT_COEFF_48));
                    break;
            }
            break;
        default:
            temp_var = 0;
            break;
    }

    temp_var = temp_var / VBAT_COEFF_3;

    temp_var = 1000000 + temp_var;

    result = result * temp_var;

    if (offset)
        result -= offset;

    result = result / 1000000;

    return result;
}

int
main(void)
{
    long result, original, die_temp, diff;
    int version, factory;

    printf("==================================================================\n");
    printf("                         OCV Compensation                         \n");
    printf("==================================================================\n");

    for (factory = FACTORY_GF; factory <= FACTORY_TSMC; factory++) {

        for (version = REV_ID_8941_3_1; version <= REV_ID_8916_2_0; version++) {

            for (die_temp = 0; die_temp <= 100000; die_temp += 10000) {

                for (original = 0; original <= 1800000; original += 100000) {

                    result = qpnp_ocv_comp(version, factory, original, die_temp);

                    diff = result - original;

                    printf("%d\t%d\t%ld\t%ld\t%ld\t%ld\n", factory, version, die_temp, original, result, diff);
                }
            }
        }
    }

    printf("==================================================================\n");
    printf("                         VBAT Compensation                        \n");
    printf("==================================================================\n");

    for (factory = FACTORY_GF; factory <= FACTORY_TSMC; factory++) {

        for (version = REV_ID_8941_3_1; version <= REV_ID_8916_2_0; version++) {

            for (die_temp = 0; die_temp <= 100000; die_temp += 10000) {

                for (original = 0; original <= 1800000; original += 100000) {

                    result = qpnp_vbat_sns_comp(version, factory, original, die_temp);

                    diff = result - original;

                    printf("%d\t%d\t%ld\t%ld\t%ld\t%ld\n", factory, version, die_temp, original, result, diff);
                }
            }
        }
    }

    return 0;
}

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Fri, 2014-11-07 at 17:33 +0200, Ivan T. Ivanov wrote:
> On Thu, 2014-11-06 at 08:55 -0800, Bjorn Andersson wrote:
> > On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > > On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
> > > > On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > [..]
> > > > > Some of the child device drivers have to know PMIC chip revision.
> > > > > 
> > > > 
> > > > So your plan is to have a strstr(parent->compatible, "-v2") there?
> > > 
> > > Actually also PMIC subtype (pm8841, pm8226...) is also required, so
> > > the plan is to have something like this:
> > > 
> > > {
> > >         static const struct of_device_id pmic_match_table[] = {
> > >                 { .compatible = "qcom,pm8941-v1.0" },
> > >                 { .compatible = "qcom,pm8841-v0.0" },
> > >                 { }
> > > 
> > >         };
> > > 
> > >         const struct of_device_id *match;
> > > 
> > >         match = of_match_device(pmic_match_table, pdev->dev.parent);
> > >         if (match) {
> > >                 dev_info(&pdev->dev, "%s chip detected\n", match->compatible);
> > >         }
> > > }
> > > 
> > 
> > To me this is a hack, you should not alter the devicetree to make it
> > "better express the hardware". Either you know these things from boot
> > and they go in device tree, or you can probe them and they should not
> > go in device tree.
> > 
> > If you really need these values you should expose them through some api.
> 
> I would like to avoid compile time dependency between these drivers.
> There are several precedents of using of_update_property() for enhancing
> compatible property already.
> 
> > > > Could you be a little bit more elaborate on what you're trying to do
> > > > and which child devices that might be?
> > > 
> > > For example ADC drivers are required temperature compensation based
> > > on PMIC variant and chip manufacturer.
> > > 
> > 
> > I see, is that compensation of any practical value? Or is the
> > compensation of academic proportions?
> 
> It depends of what you mean by academic :-). Attached file have test
> application which dump difference between non compensated and compensated
> values for different temperature, manufacture and input value.
> 
> Output format of the program is:
> Column 1: manufacturer GF=0, SMIC=1, TSMC=2
> Column 2: chip revision
> Column 3: die temperature in mili deg Celsius
> Column 4: input for compensation in micro Volts
> Column 5: compensated result in micro Volts
> Column 6: difference in micro Volts

Forgot to add. PMIC subtype and version are used also in charger and BMS
drivers to workaround hardware issues.

Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Courtney Cavin]

On Fri, Nov 07, 2014 at 04:40:52PM +0100, Ivan T. Ivanov wrote:
> On Fri, 2014-11-07 at 17:33 +0200, Ivan T. Ivanov wrote:
> > On Thu, 2014-11-06 at 08:55 -0800, Bjorn Andersson wrote:
> > > On Wed, Nov 5, 2014 at 11:54 PM, Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> wrote:
> > > > On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
[...]
> > > > > Could you be a little bit more elaborate on what you're trying to do
> > > > > and which child devices that might be?
> > > > 
> > > > For example ADC drivers are required temperature compensation based
> > > > on PMIC variant and chip manufacturer.
> > > > 
> > > 
> > > I see, is that compensation of any practical value? Or is the
> > > compensation of academic proportions?
> > 
> > It depends of what you mean by academic :-). Attached file have test
> > application which dump difference between non compensated and compensated
> > values for different temperature, manufacture and input value.
> > 
[...]
> Forgot to add. PMIC subtype and version are used also in charger and BMS
> drivers to workaround hardware issues.

All of the blocks on the PM8x41 series have their own version numbers.
There's no need to look at the chip revision.

In fact, the SMBB (charger) documentation (80-NA555-12) specifically
refers to the SMBB_MISC block revision registers as the method for
determining the hardware version.  The "qpnp-charger" SMBB driver in the
CAF 3.4 & 3.10 trees utilizes block specific revision numbers, as do the
"qpnp-bms" BMS driver, the "qpnp-adc-current" IADC driver, and the
"qpnp-adc-voltage" VADC driver.

The revision of the PMIC itself should be completely irrelevant to any
of the software interfacing with it.

-Courtney
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

On Tue, 2014-11-11 at 12:27 -0800, Courtney Cavin wrote:
> On Fri, Nov 07, 2014 at 04:40:52PM +0100, Ivan T. Ivanov wrote:
> > 
> > Forgot to add. PMIC subtype and version are used also in charger and BMS
> > drivers to workaround hardware issues.
> 
> All of the blocks on the PM8x41 series have their own version numbers.
> There's no need to look at the chip revision.

I am suspecting that, for whatever the reason is, after updates inside blocks,
the PMIC chip revisions update was made instead of blocks own version update.

> 
> In fact, the SMBB (charger) documentation (80-NA555-12) specifically

It would be nice if I had this document :-).

> refers to the SMBB_MISC block revision registers as the method for
> determining the hardware version.  The "qpnp-charger" SMBB driver in the
> CAF 3.4 & 3.10 trees utilizes block specific revision numbers, as do the
> "qpnp-bms" BMS driver, the "qpnp-adc-current" IADC driver, and the
> "qpnp-adc-voltage" VADC driver.

Hm, they read its own block revision, but they are using PMIC chip revision
for workaround decisions. What could be the reason for this?

> 
> The revision of the PMIC itself should be completely irrelevant to any
> of the software interfacing with it.
> 

It will be really nice if this is the case, but I am afraid it is not.

Regards,
Ivan

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Gilad Avidov]

On 11/6/2014 12:54 AM, Ivan T. Ivanov wrote:
> On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
>> On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>>> On Wed, 2014-11-05 at 10:11 -0800, Bjorn Andersson wrote:
>>>> On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com>
>>>> wrote:
>>>> [..]
>>>>> @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device
>>>>> *sdev)
>>>>>   {
>>>>>          struct device_node *root = sdev->dev.of_node;
>>>>>          struct regmap *regmap;
>>>>> +       struct property *prop;
>>>>> +       int major, minor, ret;
>>>>> +       char *name, compatible[32];
>>>>>
>>>>>          regmap = devm_regmap_init_spmi_ext(sdev,
>>>>> &spmi_regmap_config);
Hi Ivan, I have a general question about this driver/layer.
Since the driver is using regmap, why does it need to be 
qcom-*spmi*-pmic ? could we drop the spmi part?
regmap's point is abstraction of  the bus technology, and indeed some 
PMICs use i2c.

>>>>>          if (IS_ERR(regmap))
>>>>>                  return PTR_ERR(regmap);
>>>>>
>>>>> +       ret = pmic_spmi_read_revid(regmap, &name, &major, &minor);
>>>>> +       if (!ret) {
>>>>> +               snprintf(compatible, ARRAY_SIZE(compatible),
>>>>> "qcom,%s-v%d.%d",
>>>>> +                        name, major, minor);
>>>>> +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>>>>> +               if (prop) {
>>>>> +                       prop->name = kstrdup("compatible",
>>>>> GFP_KERNEL);
>>>>> +                       prop->value = kstrdup(compatible,
>>>>> GFP_KERNEL);
>>>>> +                       prop->length = strlen(prop->value);
>>>>> +                       of_update_property(root, prop);
>>>>> +               }
>>>>> +       }
>>>>> +
>>>> Why would you do this?
>>>> What benefit does it give to patch the of_node to have a more
>>>> specific
>>>> compatible?
>>> Some of the child device drivers have to know PMIC chip revision.
>>>
>> So your plan is to have a strstr(parent->compatible, "-v2") there?
> Actually also PMIC subtype (pm8841, pm8226...) is also required, so
> the plan is to have something like this:
>
> {
>          static const struct of_device_id pmic_match_table[] = {
>                  { .compatible = "qcom,pm8941-v1.0" },
>                  { .compatible = "qcom,pm8841-v0.0" },
>                  { }
>
>          };
>
>          const struct of_device_id *match;
>
>          match = of_match_device(pmic_match_table, pdev->dev.parent);
>          if (match) {
>                  dev_info(&pdev->dev, "%s chip detected\n", match->compatible);
>          }
> }
>
>> Could you be a little bit more elaborate on what you're trying to do
>> and which child devices that might be?
> For example ADC drivers are required temperature compensation based
> on PMIC variant and chip manufacturer.
>
> This patch have one issue, at least :-). Using of_update_property will prevent
> driver to be build as module. which, I think, is coming from the fact the
> on first load it will modify device compatible property and will be impossible
> driver to match device id again. Still thinking how to overcome this.
>
> 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


-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] mfd: qcom-spmi-pmic: Add support for more chips versions

[Ivan T. Ivanov]

Hi Gilad,

On Fri, 2014-11-07 at 17:08 -0700, Gilad Avidov wrote:
> On 11/6/2014 12:54 AM, Ivan T. Ivanov wrote:
> > On Wed, 2014-11-05 at 17:36 -0800, Bjorn Andersson wrote:
> > > On Wed, Nov 5, 2014 at 10:31 AM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
> > > > On Wed, 2014-11-05 at 10:11 -0800, Bjorn Andersson wrote:
> > > > > On Tue, Nov 4, 2014 at 5:33 AM, Ivan T. Ivanov <iivanov@mm-sol.com>
> > > > > wrote:
> > > > > [..]
> > > > > > @@ -28,11 +144,27 @@ static int pmic_spmi_probe(struct spmi_device
> > > > > > *sdev)
> > > > > >   {
> > > > > >          struct device_node *root = sdev->dev.of_node;
> > > > > >          struct regmap *regmap;
> > > > > > +       struct property *prop;
> > > > > > +       int major, minor, ret;
> > > > > > +       char *name, compatible[32];
> > > > > > 
> > > > > >          regmap = devm_regmap_init_spmi_ext(sdev,
> > > > > > &spmi_regmap_config);
> Hi Ivan, I have a general question about this driver/layer.
> Since the driver is using regmap, why does it need to be
> qcom-*spmi*-pmic ? could we drop the spmi part?
> regmap's point is abstraction of  the bus technology, and indeed some
> PMICs use i2c.

This is driver for SPMI device, so no. The child device/drivers are
different question, but I don't want to start that discussion again.

Regards,
Ivan