Re: [PATCH V5 5/5] iio: adc: Clean up ADC code common to PMIC5 and PMIC7

[Jishnu Prakash <jprakash@codeaurora.org>]

Hi Jonathan,

On 5/24/2020 5:34 PM, Jonathan Cameron wrote:
> On Fri, 22 May 2020 19:54:12 +0530
> Jishnu Prakash <jprakash@codeaurora.org> wrote:
>
>> This commit includes the following changes:
>>
>> Add a common function used for read_raw callback for both PMIC5
>> and PMIC7 ADCs.
>>
>> Add exit function for ADC.
> Hi Jishnu,
>
> I don't understand why one is needed, and if it is you can't do
> what you have here without introducing some nasty races.
> So if you need it clearly explain why in comments in the code
> and also consider how it may race with new requests coming in etc
> as the userspace interfaces are still visible.
>
> Move the eoc_irq addition to the structure here as well as makes
> no sense in earlier patch.
>
> Thanks,
>
> Jonathan
>
>
>> Add info_property under adc_data to more efficiently distinguish
>> PMIC5 and PMIC7 ADCs.
>>
>> Signed-off-by: Jishnu Prakash <jprakash@codeaurora.org>
>> ---
>>   drivers/iio/adc/qcom-spmi-adc5.c   | 81 +++++++++++++++++++++-----------------
>>   drivers/iio/adc/qcom-vadc-common.h |  1 +
>>   2 files changed, 46 insertions(+), 36 deletions(-)
>>
>>   
>> +static int adc5_exit(struct platform_device *pdev)
>> +{
>> +	struct adc5_chip *adc = platform_get_drvdata(pdev);
>> +
>> +	if (adc->irq_eoc >= 0)
>> +		disable_irq(adc->irq_eoc);
> So here you are disabling an irq?  Why.  We should be removing it
> cleanly in the managed flow shortly anyway.  If you did do this
> here for some reason I'm not thinking of then you would have
> a race against the userspace being removed on the unwind
> of the iio device register.
>
>> +	return 0;
>> +}
>> +
You're right about the exit function, the actions done in it are not 
strictly required, so I'll remove it in the next post.