W dniu 08.01.2016 o 18:16, Laxman Dewangan pisze:

Hi Krzysztof,
Thanks for review.
I will fix most of your comment on my next patch.

Answering to some of comment/query.

On Friday 08 January 2016 07:05 AM, Krzysztof Kozlowski wrote:

quoted

()2016-01-07 23:38 GMT+09:00 Laxman Dewangan [off-list ref]:
+                               dev_err(dev,
+                                   "FPS enable-input %u is not
supported\n",
+                                       pval);
Indentation of arguments does not seem equal here or maybe this is
just my email client. Have you run this through checkpatch? And
sparse? And coccicheck (that one definitely not because kbuild is
complaining)?

I ran checkpatch before I sent.

Anyway please be sure that indentation is consistent.

quoted

+               chip->rmap[i] = devm_regmap_init_i2c(chip->clients[i],
+               (const struct regmap_config
*)&max77620_regmap_config[i]);
Indentation looks weird here (or again this is my email client...).
The cast is even weirder?!? Why casting?

There is some parameter difference for MAX77620 and MAX20024. I have
only one structure for it and changing tun time so I have not define
this structure as constant.
Now API needs const type structure and hence casting it.

I don't quite get... usually there is no need of casting pointer to a
writable memory when function accepts pointer to const.

However, I have  define different structure for MAX77620 and MAX20024
which are const type and hence no need to explicitly casting here. This
will be in my next patch.

You mean v2? Okay, let's wait for that...

+static inline int max77620_reg_update(struct device *dev, int sid,
+               unsigned int reg, unsigned int mask, unsigned int val)
+{
+       struct max77620_chip *chip = dev_get_drvdata(dev);
+
+       return regmap_update_bits(chip->rmap[sid], reg, mask, val);
+}

quoted

I think all these shouldn't be static inlines in header. Although some
of them are one-liners but rest are not. Let the compiler decide what
to do with these wrappers.

If I dont make inline from header then this will complain as unused
static function on related C compilation if it is not used on C. This
header included from all sub module driver and they are not using all
these APIs.

To avoid compilation warning,  I need to use inline here.

Because this shouldn't be defined in header at the first place. Instead
define it in main MFD driver with EXPORT_SYMBOL() and put in headers
just declaration.

Best regards,
Krzysztof