Received-SPF: pass (google.com: domain of linux-kernel+bounces-139866-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <89bc03c9-9545-4886-aec7-91c9926257b3@quicinc.com>
Date: Thu, 11 Apr 2024 14:41:13 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator
 support
Content-Language: en-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>, <kernel@quicinc.com>,
        Andy Gross
	<agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Dmitry Torokhov
	<dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Dmitry Baryshkov
	<dmitry.baryshkov@linaro.org>
CC: <linux-arm-msm@vger.kernel.org>, <linux-input@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>
References: <20240401-pm8xxx-vibrator-new-design-v8-0-6f2b8b03b4c7@quicinc.com>
 <20240401-pm8xxx-vibrator-new-design-v8-3-6f2b8b03b4c7@quicinc.com>
 <3f8c970c-6a0d-4fc3-a2d3-e0797e7055cf@linaro.org>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
In-Reply-To: <3f8c970c-6a0d-4fc3-a2d3-e0797e7055cf@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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