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

On Tue, 2014-11-04 at 17:06 +0200, Stanimir Varbanov wrote:

Hi Ivan,

+

quoted

+       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>

quoted

@@ -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.

quoted

+       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>