Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register

From: Yicong Yang <yangyicong@hisilicon.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: <linux-iio@vger.kernel.org>, <lars@metafoo.de>,
	<Michael.Hennerich@analog.com>, <pmeerw@pmeerw.net>,
	<prime.zeng@huawei.com>, <tiantao6@hisilicon.com>
Subject: Re: [PATCH 1/7] iio: adc: adi-axi-adc: simplify devm_adi_axi_adc_conv_register
Date: Mon, 12 Apr 2021 17:10:17 +0800	[thread overview]
Message-ID: <e58b460c-33c7-e544-539c-2d25f96fa17a@hisilicon.com> (raw)
In-Reply-To: <20210411151215.5d3e5494@jic23-huawei>

On 2021/4/11 22:12, Jonathan Cameron wrote:
> On Thu, 8 Apr 2021 19:38:10 +0800
> Yicong Yang <yangyicong@hisilicon.com> wrote:
> 
>> Use devm_add_action_or_reset() instead of devres_alloc() and
>> devres_add(), which works the same. This will simplify the
>> code. There is no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/iio/adc/adi-axi-adc.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
>> index 9109da2..575a63f 100644
>> --- a/drivers/iio/adc/adi-axi-adc.c
>> +++ b/drivers/iio/adc/adi-axi-adc.c
>> @@ -210,29 +210,25 @@ static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
>>  	kfree(cl);
>>  }
>>  
>> -static void devm_adi_axi_adc_conv_release(struct device *dev, void *res)
>> +static void devm_adi_axi_adc_conv_release(void *conv)
>>  {
>> -	adi_axi_adc_conv_unregister(*(struct adi_axi_adc_conv **)res);
>> +	adi_axi_adc_conv_unregister(conv);
>>  }
>>  
>>  struct adi_axi_adc_conv *devm_adi_axi_adc_conv_register(struct device *dev,
>>  							size_t sizeof_priv)
>>  {
>> -	struct adi_axi_adc_conv **ptr, *conv;
>> -
>> -	ptr = devres_alloc(devm_adi_axi_adc_conv_release, sizeof(*ptr),
>> -			   GFP_KERNEL);
>> -	if (!ptr)
>> -		return ERR_PTR(-ENOMEM);
>> +	struct adi_axi_adc_conv *conv;
>> +	int ret;
>>  
>>  	conv = adi_axi_adc_conv_register(dev, sizeof_priv);
>> -	if (IS_ERR(conv)) {
>> -		devres_free(ptr);
>> +	if (IS_ERR(conv))
>>  		return ERR_CAST(conv);
> 
> Is that ERR_CAST() needed here?  conv is already of the
> right type so we don't need to cast it to a void * and back gain.
> Obviously was there before an not needed either, but might as well
> tidy it up whilst we are here!

sure. thanks for the hint. I didn't notice this. will drop the ERR_CAST.

thanks!

> 
> Thanks,
> 
> Jonathan
> 
> 
> 
>> -	}
>>  
>> -	*ptr = conv;
>> -	devres_add(dev, ptr);
>> +	ret = devm_add_action_or_reset(dev, devm_adi_axi_adc_conv_release,
>> +				       conv);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>>  
>>  	return conv;
>>  }
> 
> 
> .
> 