Re: [PATCH v6 09/10] power: supply: Support ROHM bd99954 charger

[andriy.shevchenko@xxxxxxxxxxxxxxx]

On Tue, Mar 24, 2020 at 10:53:09AM +0000, Vaittinen, Matti wrote:
> On Tue, 2020-03-24 at 11:50 +0200, Andy Shevchenko wrote:
> > On Tue, Mar 24, 2020 at 10:32:19AM +0200, Matti Vaittinen wrote:

...

> > > +	for (i = ffs(tmp); i; i = ffs(tmp)) {
> > 
> > NIH of for_each_set_bit().
> 
> What does the NIH stand for?

Not Invented Here syndrome :-)

> Anyways, I agree. This is probably better if I use for_each_set_bit()

...

> > > +	if (!dev->platform_data) {
> > 
> > dev_get_platdata()
> > 
> > > +		ret = bd9995x_fw_probe(bd);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "Cannot read device
> > > properties.\n");
> > > +			return ret;
> > > +		}
> > > +	} else {
> > > +		return -ENODEV;
> > 
> > So, existing platform data leads to an error?!
> 
> Yes. As currently we only use DT. If someone needs platdata they need
> to improve the driver

I think the idea to avoid platform data in new code as much as possible.
And it's unusual to have somebody to use this driver with platform_data set.
Why not simple ignore it?

...

> > > +#ifndef BD99954_CHARGER_H
> > > +#define BD99954_CHARGER_H
> > > +
> > > +#include <linux/regmap.h>
> > 
> > It is not the header you have users for.
> > Proper one should be bits.h.
> 
> Huh? struct reg_field is in regmap.h, right?

In that long list of some enums I definitely missed something. Just double
check the users anyway.

-- 
With Best Regards,
Andy Shevchenko