Re: [PATCH 2/2] power: supply: Add bd718(15/28/78) charger driver

[Andreas Kemnade <andreas@kemnade.info>]

Am Mon, 18 Aug 2025 13:33:05 +0300
schrieb Matti Vaittinen <mazziesaccount@gmail.com>:

[...]
> > +#define BD7182x_MASK_CCNTRST			0x80
> > +#define BD7182x_MASK_CCNTENB			0x40
> > +#define BD7182x_MASK_CCCALIB			0x20  
> 
> I suppose unused defines could get cleared. (At least the CC related 
> ones)...
> 
Hmm, I don't think having unused register defines is a problem at least
that is not an uncommon thing.

> > +#define BD7182x_MASK_WDT_AUTO			0x40
> > +#define BD7182x_MASK_VBAT_ALM_LIMIT_U		0x01
> > +#define BD7182x_MASK_CHG_EN			0x01
> > +
> > +#define BD7182x_DCIN_COLLAPSE_DEFAULT		0x36
> > +
> > +/* Measured min and max value clear bits */
> > +#define BD718XX_MASK_VSYS_MIN_AVG_CLR		0x10
> > +
> > +#define JITTER_DEFAULT				3000  
> 
> ...Also the loop jitter
> 
yes, there I agree.

> > +#define MAX_CURRENT_DEFAULT			890000		/* uA */
> > +#define AC_NAME					"bd71828_ac"
> > +#define BAT_NAME				"bd71828_bat"
> > +
> > +/*
> > + * VBAT Low voltage detection Threshold
> > + * 0x00D4*16mV = 212*0.016 = 3.392v
> > + */
> > +#define VBAT_LOW_TH			0x00D4
> > +
> > +#define THR_RELAX_CURRENT_DEFAULT		5		/* mA */
> > +#define THR_RELAX_TIME_DEFAULT			(60 * 60)	/* sec. */
> > +
> > +#define DGRD_CYC_CAP_DEFAULT			88	/* 1 micro Ah */
> > +
> > +#define DGRD_TEMP_H_DEFAULT			450	/* 0.1 degrees C */
> > +#define DGRD_TEMP_M_DEFAULT			250	/* 0.1 degrees C */
> > +#define DGRD_TEMP_L_DEFAULT			50	/* 0.1 degrees C */
> > +#define DGRD_TEMP_VL_DEFAULT			0	/* 0.1 degrees C */
> > +
> > +#define SOC_EST_MAX_NUM_DEFAULT			5
> > +#define PWRCTRL_NORMAL				0x22
> > +#define PWRCTRL_RESET				0x23
> > +
> > +/*
> > + * Originally we relied upon a fixed size table of OCV and VDR params.
> > + * However the exising linux power-supply batinfo interface for getting the OCV
> > + * values from DT does not have fixed amount of OCV values. Thus we use fixed
> > + * parameter amount only for values provided as module params - and use this
> > + * only as maximum number of parameters when values come from DT.
> > + */
> > +#define NUM_BAT_PARAMS				23
> > +#define MAX_NUM_VDR_VALUES NUM_BAT_PARAMS
> > +
> > +struct pwr_regs {
> > +	int used_init_regs;
> > +	u8 vbat_init;
> > +	u8 vbat_init2;
> > +	u8 vbat_init3;
> > +	u8 vbat_avg;
> > +	u8 ibat;
> > +	u8 ibat_avg;
> > +	u8 meas_clear;
> > +	u8 vsys_min_avg;
> > +	u8 btemp_vth;
> > +	u8 chg_state;
> > +	u8 coulomb3;
> > +	u8 coulomb2;
> > +	u8 coulomb1;
> > +	u8 coulomb0;
> > +	u8 coulomb_ctrl;
>  > +	u8 vbat_rex_avg;
> > +	u8 coulomb_full3;
> > +	u8 cc_full_clr;
> > +	u8 coulomb_chg3;  
> 
> The coulomb counter and vbat_rex related entries could be dropped as 
> they're now unused. Actually, dropping all unused members would simplify 
> this quite a bit.
> 
hmm, removing it just to add it back tomorrow again... that are not so
much bytes. I would not remove them.

Regards,
Andreas