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

[Fenglin Wu <fenglin.wu@oss.qualcomm.com>]

>> +		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?

I should put a comment for clarification. Here’s some background: FIFO 
data writing supports both 4-byte bulk writes using registers 
[HAP_PTN_FIFO_DIN_0_REG ... HAP_PTN_FIFO_DIN_3_REG], and 1-byte writes 
using the HAP_PTN_FIFO_DIN_1B_REG register. The 4-byte bulk write is 
more efficient, especially for waveform which has several Kb data, and 
it helps to reduce software latency when loading effects and reduce the 
delay in triggering vibration. It also helps prevent the FIFO from 
running dry during data refill in FIFO-empty interrupts. Typically, we 
use 4-byte writes for the initial 4-byte aligned data, and 1-byte writes 
for any trailing remainder.

So it still needs a 'for' loop here since the remainder could be more 
than 1 byte.

>> +
>> +	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?

The hardware supports more modes than the two currently supported in the 
driver. One of these, called 'PAT_MEM' mode, also shares memory space 
with FIFO mode. However, 'PAT_MEM' requires memory to be pre-reserved 
and waveform data to be pre-loaded. The entire 8K bytes of memory can be 
divided into partitions, and it is configurable, with FIFO mode always 
using the first partition [0, fifo_len], where 'fifo_len' is set via the 
'MMAP_FIFO_REG' register. 'PAT_MEM' mode plays waveform using data 
preloaded in a memory bank defined by the registers 
'PATX_MEM_START_ADDR_REG' and 'PATTERN_SPMI_PATX_LEN_REG' (they are not 
defined in the driver). Since PAT_MEM is mainly intended for 
hardware-triggered vibrations, such as a signal from a dedicated GPIO 
triggering a short vibration with a preloaded waveform, and although it 
also supports software triggers, I haven't found a suitable way to 
support it well into the driver under input FF framework yet. So, I am 
currently allocating the entire 8K FIFO memory for FIFO mode only. We 
can adjust this later if we find a better way to incorporate 'PAT_MEM' 
mode into the driver.