diff mbox

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

Message ID 1415108003-16387-1-git-send-email-iivanov@mm-sol.com
State New, archived
Headers show

Commit Message

Ivan T. Ivanov Nov. 4, 2014, 1:33 p.m. UTC
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(-)

Comments

Fabio Estevam Nov. 4, 2014, 2:56 p.m. UTC | #1
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;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov Nov. 4, 2014, 3:06 p.m. UTC | #2
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@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;

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>
Ivan T. Ivanov Nov. 4, 2014, 3:17 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov Nov. 4, 2014, 3:22 p.m. UTC | #4
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>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanimir Varbanov Nov. 4, 2014, 3:26 p.m. UTC | #5
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?
Ivan T. Ivanov Nov. 4, 2014, 3:49 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Färber Nov. 5, 2014, 12:49 p.m. UTC | #7
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);
Ivan T. Ivanov Nov. 5, 2014, 1:50 p.m. UTC | #8
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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Nov. 5, 2014, 6:11 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov Nov. 5, 2014, 6:31 p.m. UTC | #10
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Nov. 6, 2014, 1:36 a.m. UTC | #11
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);
>> >         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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov Nov. 6, 2014, 7:54 a.m. UTC | #12
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);
> > > >         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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Nov. 6, 2014, 4:55 p.m. UTC | #13
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov Nov. 7, 2014, 3:33 p.m. UTC | #14
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
Ivan T. Ivanov Nov. 7, 2014, 3:40 p.m. UTC | #15
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gilad Avidov Nov. 8, 2014, 12:08 a.m. UTC | #16
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
Ivan T. Ivanov Nov. 10, 2014, 7:46 a.m. UTC | #17
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Courtney Cavin Nov. 11, 2014, 8:27 p.m. UTC | #18
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@mm-sol.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan T. Ivanov Nov. 12, 2014, 9:12 a.m. UTC | #19
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



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

Patch

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);