lkml.org 
[lkml]   [2024]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From
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

\
 
 \ /
  Last update: 2024-05-27 16:35    [W:0.048 / U:1.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site