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.
...
> + * Bugs added by:
> + * Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
Bugs or not, but some style improvements can be done.
...
> +#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.
...
> +static const struct regmap_config bd9995x_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .reg_stride = 1,
> + .max_register = 3*0x100,
Spaces, please.
> + .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).
...
> +static int bd9995x_get_prop_batt_present(struct bd9995x_device *bd)
> +{
> + int ret, tmp;
> +
> + ret = regmap_field_read(bd->rmap_fields[F_BATTEMP], &tmp);
> + if (ret)
> + return false;
> +
> + return (tmp != BATT_OPEN) ? true : false;
C 101 (I saw somewhere coccinelle script for this):
return tmp != BATT_OPEN;
> +}
...
> + /*
> + * 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()
> + val->intval *= 1000;
...
> + /* The idiot bit does not represent any sub-registers */
> + tmp &= ~(IDI0T_BIT);
Besides above, this has too many parentheses.
...
> + for (i = ffs(tmp); i; i = ffs(tmp)) {
NIH of for_each_set_bit().
> + int sub_status, sub_mask;
> + int sub_status_reg[] = {
> + -1, INT1_STATUS, INT2_STATUS, INT3_STATUS, INT4_STATUS,
> + INT5_STATUS, INT6_STATUS, INT7_STATUS,
> + };
> + struct regmap_field *sub_mask_f[] = {
> + NULL, bd->rmap_fields[F_INT1_SET],
> + bd->rmap_fields[F_INT2_SET],
> + bd->rmap_fields[F_INT3_SET],
> + bd->rmap_fields[F_INT4_SET],
> + bd->rmap_fields[F_INT5_SET],
> + bd->rmap_fields[F_INT6_SET],
> + bd->rmap_fields[F_INT7_SET],
> + };
> +
> + tmp &= ~(1 << (i - 1));
See above.
> +
> + if (i - 1 > ARRAY_SIZE(sub_status_reg))
> + break;
See above. You may prepare tmp beforehand.
> + }
...
> + 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.
...
> +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.
> + .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?
...
> +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)
...
> + 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?!
> + }
...
> + /*
> + * reset all registers to default values. This should also disable
r -> R. Check all your messages for proper grammar.
> + * CHG_EN bit. If this is not required we can get rid of the remove.
> + */
...
> + dev_info(bd->dev, "Driver for DB99954 chip removed\n");
Noise. You can easily check this by using initcall_debug facility.
...
> +static const struct of_device_id bd9995x_of_match[] = {
> + { .compatible = "rohm,bd99954", },
> + { },
Terminator lines better without comma. Preventing new potential bugs in the future :-)
> +};
> +MODULE_DEVICE_TABLE(of, bd9995x_of_match);
> +
> +static const struct acpi_device_id bd9995x_acpi_match[] = {
> + {"BD999540", 0},
> + {},
Ditto.
> +};
> +MODULE_DEVICE_TABLE(acpi, bd9995x_acpi_match);
...
> +#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.
--
With Best Regards,
Andy Shevchenko
