Re: [PATCH 2/4] mfd: max77686: Use module_i2c_driver() instead of subsys initcall

[Marek Szyprowski]

Hello,

On 2016-02-16 00:20, Krzysztof Kozlowski wrote:
> On 16.02.2016 00:21, Javier Martinez Canillas wrote:
> > Hello Krzysztof,
> >
> > On 02/15/2016 03:54 AM, Krzysztof Kozlowski wrote:
> > > On 12.02.2016 13:30, Javier Martinez Canillas wrote:
> > > > The driver's init and exit function don't do anything besides adding and
> > > > deleting the I2C driver so the module_i2c_driver() macro could be used.
> > > >
> > > > Currently is not being used because the driver is initialized at subsys
> > > > initcall level, claiming that this is done to allow consumers devices to
> > > > use the resources provided by this driver. But dependencies should be in
> > > > the DT and consumers drivers should not rely in the registration order.
> > > >
> > > > Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> > > > ---
> > > >
> > > >    drivers/mfd/max77686.c | 13 +------------
> > > >    1 file changed, 1 insertion(+), 12 deletions(-)
> > > In the past not all dependencies supported deferred probing so such
> > > ordering was required.
> > >
> > > I don't like the "dependencies should be in DT" reason for the change...
> > > because it is kind of wishful thinking. Yeah, the dependencies should be
> > > in DT, but are they?
> > >
> > > Instead *please check it* and write:
> > > "Dependencies are in DT so manual ordering of init calls is not
> > > necessary any more".
> > For the max77802 I know that's the case since the only two DTS in mainline
> > that use it are the Peach Pit and Pi and I'm very familiar with those two.
> >
> > But I wonder how can I check that this is the case for the max77686. Most
> > DTS in mainline have nodes that use some clocks and regulators provided by
> > the PMIC, only arch/arm/boot/dts/exynos5250-smdk5250.dts doesn't have one
> > of the regulators as input supply or clock consumer defined.
> +Cc Marek Szyprowski, who may know a lot more about dependencies between
> these.
>
> I wouldn't care for drivers not taking references to regulators/clocks.
> Most of necessary regulators and clocks are turned on by bootloader or
> by default values in PMIC. This means that later probing of PMIC
> shouldn't influence drivers which are not using it.
>
> The remaining problem was unsupported deferred probing by some of the
> drivers using regulators/clocks (drivers being consumers of regulators
> or clocks). AFAIR one of example was USB OTG.

USB OTG has been recently fixed to finally support deferred probing, see
commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget: udc-core:
independent registration of gadgets and gadget drivers").

> By "please check" in this case I mean - look if every regulator/clock
> consumer using stuff exposed by PMIC, supports properly deferred probing.
>
> > For the clock, I guess the RTC is just broken since it's using the s3c6410
> > controller that requires a source clock and this is not defined.
> >
> > Now the question is if it doesn't really need the regulators or is that
> > the DTS isn't correctly defined and some drivers were relying on the MFD
> > and regulator drivers to be registered at subsys initcall level?
> I suspect the consumers are not defined in DTS. However I wouldn't care
> about such issue. If there is no consumer, then probe order shouldn't
> matter...
>
> > > My fast tests of this patch shown that it works good... but some more
> > > thorough tests should be done.
> > What do you suggest? The drivers now support deferred probing but as said,
> > I don't know how I can be sure that drivers aren't missing input supplies
> > and relying in regulators being registered early and marked as always-on.
> So test it... You are posting a small improvement without any important
> benefit but in the same time it might broke existing platforms. Perfect
> is the enemy of the good (or if it ain't broken, don't touch it), so
> please be sure that max77686 still works. :)

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland