Re: [PATCH 3/4] input: misc: Add Qualcomm SPMI PMIC haptics driver

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 6/16/26 12:08 PM, Fenglin Wu wrote:
> Add an initial driver for the Qualcomm PMIH010x PMIC haptics module,
> named as HAP530_HV. This module supports several play modes, including
> DIRECT_PLAY, FIFO, PAT_MEM, and SWR, each with distinct data sourcing
> and hardware data handling logic. Currently, the driver provides support
> for two play modes using the input force-feedback framework: FF_CONSTANT
> effect for DIRECT_PLAY mode and FF_PERIODIC effect with FF_CUSTOM
> waveform for FIFO mode.
> 
> Assisted-by: Claude:claude-4-6-sonnet
> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com>
> ---

[...]

> +static int cfg_write(struct qcom_haptics *h, u32 off, u32 val)

static inline

although I have mixed feelings about having so many accessors

[...]

> +static int haptics_write_fifo_chunk(struct qcom_haptics *h,
> +				    const s8 *data, u32 len)
> +{
> +	u32 i, bulk_len = ALIGN_DOWN(len, 4);

Please avoid mixing multiple declarations and assignments

> +	int ret;
> +
> +	for (i = 0; i < bulk_len; i += 4) {

You can do 'int i' in loops nowadays

> +		ret = ptn_bulk_write(h, HAP_PTN_FIFO_DIN_0_REG, &data[i], 4);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (; i < len; i++) {
> +		ret = ptn_write(h, HAP_PTN_FIFO_DIN_1B_REG, (u8)data[i]);
> +		if (ret)
> +			return ret;
> +	}

So if i'm reading this right, the first loop will always write
4*(len//4) bytes and the second one will be entered at most once,
to write len rem 4 bytes.. should this be an if instead?

> +
> +	return 0;
> +}
> +
> +/*
> + * Configure the hardware FIFO memory boundary.
> + * FIFO occupies addresses [0, fifo_len).
> + */
> +static int haptics_configure_fifo_mmap(struct qcom_haptics *h)
> +{
> +	u32 fifo_len, fifo_units;
> +
> +	/* Config all memory space for FIFO usage for now */

What's the not-"for now" endgame for this?

> +	fifo_len = HAP530_MEM_TOTAL_BYTES;
> +	fifo_len = ALIGN_DOWN(fifo_len, 64);
> +	fifo_units = fifo_len / 64;
> +	h->fifo_len = fifo_len;
> +
> +	return ptn_write(h, HAP_PTN_MMAP_FIFO_REG,
> +			 MMAP_FIFO_EXIST_BIT |
> +			 FIELD_PREP(MMAP_FIFO_LEN_MASK, fifo_units - 1));
> +}
> +
> +static u32 haptics_gain_scaled_vmax(struct qcom_haptics *h, u32 vmax_mv)
> +{
> +	u32 v = (u32)((u64)vmax_mv * h->gain / 0xFFFF);

mult_frac()

> +
> +	return max_t(u32, v, VMAX_STEP_MV);
> +}
> +
> +static void haptics_fifo_irq_enable(struct qcom_haptics *h, bool enable)
> +{
> +	if (h->irq_enabled == enable)
> +		return;
> +
> +	if (enable)
> +		enable_irq(h->fifo_empty_irq);
> +	else
> +		disable_irq_nosync(h->fifo_empty_irq);

This is called in the .remove() path, I think you may need the
sync variant as the underlying device may be destroyed before the
ISR completes if there's a late interrupt


[...]

> +static int haptics_playback(struct input_dev *dev, int effect_id, int val)
> +{
> +	struct qcom_haptics *h = input_get_drvdata(dev);
> +
> +	h->cur_effect_id = effect_id;
> +	h->play_request  = (val > 0);
> +	schedule_work(&h->play_work);
> +	return 0;

nit: \n before return is 'nice'

[...]

> +	ret = device_property_read_u32_array(&pdev->dev, "reg", regs,
> +					     ARRAY_SIZE(regs));

Here you use device_property_

> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to read 'reg' property\n");
> +
> +	h->cfg_base = regs[0];
> +	h->ptn_base = regs[1];
> +
> +	ret = of_property_read_u32(h->dev->of_node, "qcom,lra-period-us",
> +				   &h->t_lra_us);

And here you use of_property_ (please use device_)

[...]

> +	input->name     = "qcom-spmi-haptics";

Odd whitespace

Konrad