On 11/04/2014 05:22 PM, Ivan T. Ivanov wrote:

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

quoted

Hi Ivan,

+

quoted

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

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

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.

might be better idea to use devm_kzalloc and devm_kstrdup?

-- 
regards,
Stan