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

[Jishnu Prakash]

Hi Jonathan,

On 5/24/2020 5:34 PM, Jonathan Cameron wrote:
> On Fri, 22 May 2020 19:54:12 +0530
> Jishnu Prakash <jprakash@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> >   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.