Re: [PATCH v2 2/3] Input: aw86927 - add driver for Awinic AW86927

[Griffin Kroah-Hartman <griffin.kroah@fairphone.com>]

Hello Dmitry!

On 8/11/25 18:35, Dmitry Torokhov wrote:
> Hi Griffin,
> 
> On Mon, Aug 11, 2025 at 01:12:02PM +0200, Griffin Kroah-Hartman wrote:
(..)
>> +struct aw86927_sram_waveform_header {
>> +	uint8_t version;
>> +	struct {
>> +		__be16 start_address;
>> +		__be16 end_address;
>> +	} __packed waveform_address[1];
> 
> Why does this need to be an array? 

Great question, during development this was used to include multiple 
wave-forms, but it was decided that the feature was unnecessary as of 
now. I will remove the array for simplicity.

>> +static int aw86927_haptics_play(struct input_dev *dev, void *data, struct ff_effect *effect)
>> +{
>> +	struct aw86927_data *haptics = input_get_drvdata(dev);
>> +	int level;
>> +
>> +	level = effect->u.rumble.strong_magnitude;
>> +	if (!level)
>> +		level = effect->u.rumble.weak_magnitude;
>> +
>> +	/* If already running, don't restart playback */
> 
> Why not if effect parameters are changing? Also what if someone is
> issuing stop for already stopped effect?

It it's current state, the driver only has one level of vibration, 
therefore adjusting the effect does not matter as it is either vibrating 
or off.
For your second question I don't think I understand what you are asking. 
If someone gives magnitude 0 to a vibration that has ceased playback, 
the vibration has already stopped, so we don't need to stop it again.

>> +
>> +	haptics->regmap = devm_regmap_init_i2c(client, &aw86927_regmap_config);
>> +	if (IS_ERR(haptics->regmap))
>> +		return dev_err_probe(haptics->dev, PTR_ERR(haptics->regmap),
>> +					"Failed to allocate register map\n");
>> +
>> +	haptics->input_dev = devm_input_allocate_device(haptics->dev);
>> +	if (!haptics->input_dev)
>> +		return -ENOMEM;
>> +
>> +	haptics->reset_gpio = devm_gpiod_get(haptics->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(haptics->reset_gpio))
>> +		return dev_err_probe(haptics->dev, PTR_ERR(haptics->reset_gpio),
>> +				     "Failed to get reset gpio\n");
> 
> Is it mandatory to wire the reset pin? I see the chip supports software
> reset so maybe this can be optional?

In the datasheet, it is never explicitly mentioned that the pin is 
optional, and in the downstream driver it is mandatory.
If the reset pin is not found the probe will fail.

If somebody has a board with that use-case they can modify this driver 
easily.

>> +
>> +	/* Hardware reset */
>> +	aw86927_hw_reset(haptics);
>> +
>> +	/* Software reset */
>> +	err = regmap_write(haptics->regmap, AW86927_RSTCFG, AW86927_RSTCFG_SOFTRST);
>> +	if (err)
>> +		return dev_err_probe(haptics->dev, PTR_ERR(haptics->regmap),
>> +					"Failed Software reset\n");
> 
> Do you need to issue software reset together with hardware reset? Is
> one or the other not enough?

The datasheet does not mention whether one is enough, the downstream 
driver does it with both so I have assumed that this would be best 
practice with this chip.

>> +	err = devm_request_threaded_irq(haptics->dev, client->irq, NULL,
>> +					aw86927_irq, IRQF_ONESHOT, NULL, haptics);
> 
> Error handling? Also it looks like here it is safe to register the
> interrupt handler early since it does not actually do anything, but
> better to move it after the bulk of initialization in case it will get
> expanded.

I will fix the error handling :). If someone were to implement FIFO mode 
then this would be the correct placement of the initialization, so I 
think for now it would make sense to keep it here.

> 
> Thanks.
> 

Thank you for your comments!