Re: [PATCH v5 2/3] mfd: add support for Allwinner SoCs ADC

[Lee Jones <lee.jones@linaro.org>]

On Tue, 13 Sep 2016, Quentin Schulz wrote:
> On 12/09/2016 15:56, Lee Jones wrote:
> > On Mon, 12 Sep 2016, Quentin Schulz wrote:
> >> On 12/09/2016 11:59, Lee Jones wrote:
> >>> On Mon, 12 Sep 2016, Quentin Schulz wrote:
> >>>
> >>>> On 12/09/2016 11:18, Lee Jones wrote:
> >>>>> On Thu, 08 Sep 2016, Quentin Schulz wrote:
> >>>>> [...]
> >>>>>> +
> >>>>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match);
> >>>>>
> >>>>> Place this directly under the table.
> >>>>>
> >>>>>> +static struct platform_driver sun4i_gpadc_mfd_driver = {
> >>>>>> +	.driver = {
> >>>>>> +		.name = "sun4i-adc-mfd",
> >>>>>> +		.of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match),
> >>>>>> +	},
> >>>>>> +	.probe = sun4i_gpadc_mfd_probe,
> >>>>>
> >>>>> No .remove?
> >>>>>
> >>>>
> >>>> No, everything in probe is handled with devm functions.
> >>>
> >>> Don't you need to undo the register write you did?
> >>>
> >>
> >> The regmap_write I use is there to disable all interrupts on hardware
> >> side before the irq_chip handles all interrupts by itself. The
> >> interrupts are not used in the MFD driver.
> >>
> >> Thus, I chose to disable the hardware interrupts in the remove function
> >> of drivers using the interrupts (only the IIO yet but the touchscreen
> >> driver later also which will be using a third interrupt). When the MFD
> >> driver is removed, the MFD cells will all be removed, thus calling their
> >> own remove functions, thus disabling hardware interrupts used in each
> >> driver. So the hardware interrupts disabling would be called twice.
> > 
> > This does send some little alarm bells ringing.  I'd normally expect
> > the .remove function to undo everything you did in .probe.  So, if you
> > are disabling the IRQs from within the leaf drivers, shouldn't you be
> > initialising them in the leaf driver's respective .probes?
> > 
> 
> I use the regmap_write in the MFD driver's probe to disable all
> interrupts before requesting irq_chip to guarantee the interrupts are in
> a known state, being disabled. It is to insure no interrupt will occur
> unwittingly before we want the leaf drivers to handle them.
> 
> The disabling of irqs in the remove is handled rather by
> devm_regmap_del_irq_chip than by an explicit regmap_write in the
> driver's removal function. It performs the exact same thing.
> 
> I always use devm functions for requesting either an irq_chip or the
> irqs themselves. In that case, when the device is removed, the irqs are
> freed on leaf drivers' (where the irqs are requested) removal while the
> removal of irq_chip in the MFD driver will also free all irqs mapped to
> this irq_chip thanks to devm_regmap_del_irq_chip. Therefore, the
> interrupts are disabled by devm functions.
> 
> The regmap_update_bits in probe and removal of the ADC driver to disable
> irqs are actually redundant because the devm functions already handle
> the irqs disabling.

Okay.  So long as you've thought about it, I'm happy.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog