Messages in this thread | | | Date | Sat, 21 Oct 2023 03:07:30 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCH 2/2] power: supply: bq24190_charger: Add support for BQ24296 |
| |
Hi,
On Sat, Oct 07, 2023 at 10:07:01AM +0800, Hermes Zhang wrote: > From: Hermes Zhang <chenhuiz@axis.com> > > The BQ24296 is most similar to the BQ24196, but the: > 1. OTG config is split from CHG config (REG01) > 2. ICHG (Fast Charge Current limit) range is smaller (<=3008mA) > 3. NTC fault is simplified to 2 bits > > Signed-off-by: Hermes Zhang <chenhuiz@axis.com> > ---
Thanks for your patch. I did not look into tiny details, because I think this needs to be restructured first. Please follow the style of bq256xx_charger.c, bq24257_charger.c and bq2515x_charger.c and use a struct together with i2c_get_match_data().
Also put device specific functions into the struct to avoid a switch case structure like the following:
> [...] > +static int bq24190_hw_init(struct bq24190_dev_info *bdi) > +{ > + int ret; > + > + /* First check that the device really is what its supposed to be */ > + switch (bdi->chip) { > + case BQ24190: > + case BQ24192: > + case BQ24192i: > + case BQ24196: > + ret = bq24190_check_chip(bdi); > + break; > + case BQ24296: > + ret = bq24296_check_chip(bdi); > + break; > + default: > + dev_err(bdi->dev, "Error unsupported model: %d\n", bdi->chip); > + return -EINVAL; > + } > [...]
Instead it should look like this:
struct info { ... int (*check_chip)(struct bq24190_dev_info *bdi); ... };
static int bq24190_hw_init(struct bq24190_dev_info *bdi) { return bdi->info->check_chip(bdi); }
Thanks,
-- Sebastian [unhandled content-type:application/pgp-signature] | |