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

[Vaittinen, Matti]

Hello Andy,

I do agree with most of the things you pointed out. I didn't comment
them.

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:
> > The ROHM BD99954 is a Battery Management LSI for 1-4 cell Lithium-
> > Ion
> > secondary battery intended to be used in space-constraint equipment
> > such
> > as Low profile Notebook PC, Tablets and other applications. BD99954
> > provides a Dual-source Battery Charger, two port BC1.2 detection
> > and a
> > Battery Monitor.
> ...
> 
> > +#include <linux/acpi.h>
> > +#include <linux/of.h>
> 
> I didn't find any evidence of use of those two, otherwise, missed
> property.h
> and perhaps mod_devicetable.h.
> 

I'll check this, thanks.

> 
> > +	.cache_type = REGCACHE_RBTREE,
> > +
> > +	.ranges = regmap_range_cfg,
> > +	.num_ranges = ARRAY_SIZE(regmap_range_cfg),
> > +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +	.wr_table = &bd9995x_writeable_regs,
> > +	.volatile_table = &bd9995x_volatile_regs,
> > +};
> 
> ...
> 
> > +#define IDI0T_BIT 0x1
> 
> In contemporary world somebody can be offended (not me).

Yeah. And when someone gets offended and complains I will change this
:) And maybe explain to him/her why I think this is an idiot bit.
Besides, it describes the bit quite well.

> ...
> 
> > +		/*
> > +		 * the actual range : 2560 to 19200 mV. No matter what
> > the
> 
> t -> T
> 
> > +		 * register says
> > +		 */
> > +		val->intval = max(val->intval, 2560);
> > +		val->intval = min(val->intval, 19200);
> 
> clamp_val()

Thanks! I didn't know of this. I learned something again :)

> ...
> 
> > +	for (i = ffs(tmp); i; i = ffs(tmp)) {
> 
> NIH of for_each_set_bit().

What does the NIH stand for?
Anyways, I agree. This is probably better if I use for_each_set_bit()

> 
> > +	do {
> > +		ret = regmap_field_read(bd->rmap_fields[F_OTPLD_STATE], 
> > &state);
> > +		if (ret)
> > +			return ret;
> > +
> > +		msleep(10);
> > +	} while (state == 0 && --rst_check_counter);
> 
> regmap_read_poll_timeout() exists, but I see you use it for field.
> Perhaps it's
> a time to introduce similar helper for field polling.

This series is again getting lengthy... I'll see if I add such an API
in this series. I've learned that adding changes will increase the time
it takes to push the series through. It might be more reviewer friendly
to add it in own patch with limited review audience (as would be the
patch 10/10 in this series). But I think your point is valid.

> ...
> 
> > +static const struct linear_range input_current_limit_ranges[] = {
> > +	{
> > +		.min = 0,
> > +		.step = 32000,
> > +		.min_sel = 0x0,
> 
> Perhaps 0x000 to match max_sel width. Same applies to other places.

I don't really see the value of this suggestion.

> > +		.max_sel = 0x1ff,
> > +	},
> > +};
> 
> ...
> 
> > +	/*
> > +	 *  the power_supply_get_battery_info does not support getting
> > values
> 
> t -> T
> Also when we refer to function use () suffix.
> 
> > +	 * from ACPI. Let's fix it if required :)
> > +	 */
> 
> And yes, good question why you simple do not fix it there?
> 

Because I don't need ACPI at the moment and because I would like to
limit the size of already lengthy series. 

> ...
> 
> > +static int bd9995x_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> 
> Can you switch to ->probe_new() (Note, it doesn't mean you need to
> drop IÂC ID table)

I can. Thanks.

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

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

Br,
	Matti Vaittinen