Received-SPF: pass (google.com: domain of linux-kernel+bounces-139170-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <3f8c970c-6a0d-4fc3-a2d3-e0797e7055cf@linaro.org>
Date: Wed, 10 Apr 2024 20:10:42 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator
 support
To: quic_fenglinw@quicinc.com, 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>
Content-Language: en-US
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20240401-pm8xxx-vibrator-new-design-v8-3-6f2b8b03b4c7@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/1/24 10:38, Fenglin Wu via B4 Relay wrote:
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
> 
> Add support for a new SPMI vibrator module which is very similar
> to the vibrator module inside PM8916 but has a finer drive voltage
> step and different output voltage range, its drive level control
> is expanded across 2 registers. The vibrator module can be found
> in following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
> 
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---

[...]

>   
> -#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


> +#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.
[...]

> +static struct pm8xxx_regs pmi632_regs = {
> +	.enable_offset = 0x46,
> +	.enable_mask = BIT(7),
> +	.drv_offset = 0x40,
> +	.drv_mask = 0xFF,

GENMASK(7, 0)

> +	.drv_shift = 0,
> +	.drv2_offset = 0x41,
> +	.drv2_mask = 0x0F,

GENMASK(3, 0)

[...]

>   
> +	if (regs->drv2_mask) {
> +		if (on)
> +			val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
> +		else
> +			val = 0;
> +		rc = regmap_write(vib->regmap, vib->drv2_addr, val);

Are you purposefuly zeroing out the other bits?

If yes, consider regmap_write_bits here
If not, consider regmap_update_bits here

> +		if (rc < 0)
> +			return rc;

Ignore regmap_r/w errors, these mean a complete failure of the API and
we don't generally assume MMIO accesses can fail

Unless SPMI is known to have issues here

> +	}
> +
>   	if (regs->enable_mask)
>   		rc = regmap_update_bits(vib->regmap, vib->enable_addr,
>   					regs->enable_mask, on ? ~0 : 0);
> @@ -114,19 +141,22 @@ static void pm8xxx_work_handler(struct work_struct *work)
>   		return;
>   
>   	/*
> -	 * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
> +	 * pmic vibrator supports voltage ranges from MIN_LEVEL to MAX_LEVEL, so
>   	 * scale the level to fit into these ranges.
>   	 */
>   	if (vib->speed) {
>   		vib->active = true;
> -		vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> -						VIB_MIN_LEVEL_mV;
> -		vib->level /= 100;
> +		vib->level = ((VIB_MAX_LEVELS(vib) * vib->speed) / MAX_FF_SPEED) +

mult_frac()

> +						VIB_MIN_LEVEL_mV(vib);

vib->level = VIB_MIN_LEVEL_mV;
vib->level += the other thing

for readability?

>   	} else {
>   		vib->active = false;
> -		vib->level = VIB_MIN_LEVEL_mV / 100;
> +		vib->level = VIB_MIN_LEVEL_mV(vib);
> +
>   	}
>   
> +	if (!vib->drv2_addr)
> +		vib->level /= 100;

Maybe this could be moved to pm8xxx_vib_set() instead

> +
>   	pm8xxx_vib_set(vib, vib->active);
>   }
>   
> @@ -202,7 +232,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>   
>   	vib->enable_addr = reg_base + regs->enable_offset;
>   	vib->drv_addr = reg_base + regs->drv_offset;
> -
> +	vib->drv2_addr = reg_base + regs->drv2_offset;

It would be nice to preserve a newline between assignments and rw
functions here

Thanks for working on this!

Konrad