lkml.org 
[lkml]   [2024]   [Apr]   [10]   [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


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

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