Re: [PATCH 2/3] Input - aw8695: Add driver for AW8695 haptics

[Krzysztof Kozlowski]

On Mon, Sep 14, 2026 at 04:37:48PM +0200, Griffin Kroah-Hartman wrote:
> From: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>
> 
> Add a driver for interfacing with the Awinic AW8695 LRA Haptic Driver.
> 
> The chip supports multiple modes of which only RAM mode is implemented.
> RTP mode would enable a user to "stream" waveform data but to my
> knowledge no such user space API exists in the kernel yet.
> 
> We upload a basic sine wave to the chip and play this on request.
> 
> Co-developed-by: Griffin Kroah-Hartman <griffin.kroah@xxxxxxxxxxxxx>
> Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@xxxxxxxxxxxxx>
> Signed-off-by: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>

Your SoB should be the last, after your codev, after author's.

> +static irqreturn_t aw8695_irq(int irq, void *data)
> +{
> +	struct aw8695_data *haptics = data;
> +	struct device *dev = &haptics->client->dev;
> +	unsigned int read_buf;
> +	int err;
> +
> +	err = regmap_read(haptics->regmap, AW8695_SYSINT_REG, &read_buf);
> +	if (err) {
> +		dev_err(dev, "Failed to read SYSINT register: %d\n", err);
> +		return IRQ_NONE;
> +	}
> +	dev_dbg(dev, "Interrupt: SYSINT=0x%x\n", read_buf);
> +
> +	if (read_buf & AW8695_SYSINT_BSTERRI)
> +		dev_err(dev, "Received boost short circuit protection or over-voltage protection interrupt!\n");
> +	if (read_buf & AW8695_SYSINT_OVI)
> +		dev_err(dev, "Received wave data overflow or DPWM DC error interrupt!\n");
> +	if (read_buf & AW8695_SYSINT_UVLI)
> +		dev_err(dev, "Received under voltage lock out interrupt!\n");
> +	if (read_buf & AW8695_SYSINT_OCDI)
> +		dev_err(dev, "Received over current interrupt!\n");
> +	if (read_buf & AW8695_SYSINT_OTI)
> +		dev_err(dev, "Received over temperature interrupt!\n");
> +
> +	if (read_buf & AW8695_SYSINT_DONEI)
> +		dev_dbg(dev, "Received playback done interrupt\n");
> +	/* FIFO mode is not (yet) implemented in this driver */
> +	if (read_buf & AW8695_SYSINT_FF_AEI)
> +		dev_dbg(dev, "Received FIFO almost empty interrupt\n");
> +	if (read_buf & AW8695_SYSINT_FF_AFI)
> +		dev_dbg(dev, "Received FIFO almost full interrupt\n");
> +
> +	err = regmap_read(haptics->regmap, AW8695_DBGSTAT_REG, &read_buf);
> +	if (err) {
> +		dev_err(dev, "Failed to read DBGSTAT register: %d\n", err);
> +		return IRQ_NONE;
> +	}
> +	dev_dbg(dev, "Interrupt: DBGSTAT=0x%x\n", read_buf);
> +
> +	err = regmap_read(haptics->regmap, AW8695_SYSST_REG, &read_buf);
> +	if (err) {
> +		dev_err(dev, "Failed to read SYSST register: %d\n", err);
> +		return IRQ_NONE;
> +	}
> +	dev_dbg(dev, "Interrupt: SYSST=0x%x\n", read_buf);

You should not have three debugs (and possibly +3 more in if()
conditions) in interrupt handler. In case of irq
storm this still will overwhelm the log. This should be only one and
most likely dev_dbg_ratelimited(). Errors should also have ratelimit,
even if IRQ storm with errors is unlikely or even drop them completely -
isn't regmap already going to print some errors in such case?

> +
> +	return IRQ_HANDLED;
> +}

Best regards,
Krzysztof