Messages in this thread | | | Date | Thu, 11 Apr 2024 14:41:13 +0800 | Subject | Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support | From | Fenglin Wu <> |
| |
Hi Konrad,
On 4/11/2024 2:10 AM, Konrad Dybcio wrote: > > >> -#define VIB_MAX_LEVEL_mV (3100) >> -#define VIB_MIN_LEVEL_mV (1200) >> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV) >> +#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100)) > > You shouldn't need the additional inside parentheses > > Also, is this really a good discriminator for the voltage ranges? Do *all* > PMIC vibrators with a drv2_addr operate within this range? If not, consider > a struct field here > Currently, yes, all PMIC vibrators with a drv2_addr (PMI632, PM7250B, PM7325B, PM7550BA) operate within the same range because they are the same type.
> >> +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200)) >> +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - >> VIB_MIN_LEVEL_mV(vib)) > > If the ranges are supposed to be inclusive, this is off-by-one. But looking > at the driver, it seems like MIN_LEVEL may be "off"? I'm not sure though. > > Either way, this would be a separate fix. > [...] The voltage range [1504, 3544] for the new SPMI vibrator is inclusive. I checked the voltage range [1200 3100] for PM8916 SPMI vibrator is also inclusive. I couldn't find any document to confirm if the SSBI vibrator but I assume it is the same as PM8916. I will make change before the series to address it.
Thanks for reviewing the changes!
Fenglin
| |