Re: [PATCH v2 3/6] iio: accel: mma8452: use local struct device

[Sanjay Chitroda <sanjayembeddedse@gmail.com>]

On 23 April 2026 1:05:33 am IST, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>On Wed, Apr 22, 2026 at 10:26:40PM +0530, Sanjay Chitroda wrote:
>
>> Introduce a local struct device pointer derived from &client->dev.
>> This avoids repeated &client->dev usage and improves readability.
>
>...
>
>> static int mma8452_set_runtime_pm_state(struct i2c_client *client, bool on)
>>  {
>>  #ifdef CONFIG_PM
>
>So, this seems to be a missed in the PM patch.
>
>>  	int ret;
>> +	struct device *dev = &client->dev;
>
>Keep it in reversed xmas tree order.
>
Understood, along with headers would attempt to keep all local variables also in chronological order.

>>  	if (on)
>> -		ret = pm_runtime_resume_and_get(&client->dev);
>> +		ret = pm_runtime_resume_and_get(dev);
>>  	else
>> -		ret = pm_runtime_put_autosuspend(&client->dev);
>> +		ret = pm_runtime_put_autosuspend(dev);
>>  	if (ret < 0) {
>> -		dev_err(&client->dev,
>> +		dev_err(dev,
>>  			"failed to change power state to %d\n", on);
>>  
>>  		return ret;
>
>>  }
>
>...
>
>>  static int mma8452_probe(struct i2c_client *client)
>>  {
>>  	struct mma8452_data *data;
>> +	struct device *dev = &client->dev;
>
>Same here, reversed xmas tree order.
>
>>  	struct iio_dev *indio_dev;
>>  	int ret;
>
Sure, understood.
>...
>
>>  	data->chip_info = i2c_get_match_data(client);
>>  	if (!data->chip_info)
>> -		return dev_err_probe(&client->dev, -ENODEV,
>> +		return dev_err_probe(dev, -ENODEV,
>>  				     "unknown device model\n");
>
>Now it's a single line.
>
>...
>
>>  	ret = regulator_enable(data->vdd_reg);
>>  	if (ret) {
>> -		dev_err(&client->dev, "failed to enable VDD regulator!\n");
>> +		dev_err(dev, "failed to enable VDD regulator!\n");
>>  		return ret;
>
>Convert this to dev_err_probe() before doing this patch. In that patch you
>introduce temporary dev variable already. Here will use in the other places.
>
Agree with you, will rebase in next series.
>>  	}
>>  
>>  	ret = regulator_enable(data->vddio_reg);
>>  	if (ret) {
>> -		dev_err(&client->dev, "failed to enable VDDIO regulator!\n");
>> +		dev_err(dev, "failed to enable VDDIO regulator!\n");
>>  		goto disable_regulator_vdd;
>
>Ditto.
>
Sure.
>>  	}
>
>...
>
>> +	pm_runtime_set_autosuspend_delay(dev,
>>  					 MMA8452_AUTO_SUSPEND_DELAY_MS);
>
>Now a single line.
>