[PATCH 0/4] mfd: max77686: Allow the driver to be built as a module

[PATCH 0/4] mfd: max77686: Allow the driver to be built as a module

[Javier Martinez Canillas]

Hello,

The Maxim77802 PMIC driver uses a boolean Kconfig symbol but there isn't
really a reason to require the driver to be built-in.

It is true that since the PMIC provides clocks and regulators that could
be critical on a system, most integrators are not going to build it as a
module but it could be useful for multi-platform or distribution kernels
that support different systems by building as much as possible as module.

Besides this series, a patch for the max77686 regulator driver is needed
or the MFD max77686 driver module won't be installed by kbuild. I didn't
include the patch in this series to avoid cross-subsystems churn for the
maintainers and also because I don't consider that a regression since it
was not possible to build the driver as a module before anyways.

Also, all the defconfigs in mainline that enable the MFD max77686 driver
Kconfig symbol are using =y of course so it shouldn't be an issue to get
this set merged, before the max77686 regulator patch already posted [0].

[0]: https://patchwork.kernel.org/patch/8287431/

Best regards,
Javier


Javier Martinez Canillas (4):
  mfd: max77686: Add max77802 to I2C device ID table
  mfd: max77686: Use module_i2c_driver() instead of subsys initcall
  mfd: max77686: Allow driver to be built as a module
  mfd: max77686: Export OF module alias information

 drivers/mfd/Kconfig    |  4 ++--
 drivers/mfd/max77686.c | 15 +++------------
 2 files changed, 5 insertions(+), 14 deletions(-)

-- 
2.5.0

[PATCH 1/4] mfd: max77686: Add max77802 to I2C device ID table

[Javier Martinez Canillas]

The max77686 MFD driver supports both the Maxim 77686 and Maxim 77802
PMICs but only the OF device table contains entries for both devices.

The max77802 entry is missing in the I2C device ID table which isn't
a problem currently since the driver only supports DT but it will be
needed if the driver is changed to be built as a module since the I2C
core always reports a I2C modalias uevent so auto-load will not work.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/mfd/max77686.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index d959ebbb2194..1f30c97d6d4e 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -345,6 +345,7 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
 
 static const struct i2c_device_id max77686_i2c_id[] = {
 	{ "max77686", TYPE_MAX77686 },
+	{ "max77802", TYPE_MAX77802 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max77686_i2c_id);
-- 
2.5.0

Re: [PATCH 1/4] mfd: max77686: Add max77802 to I2C device ID table

[Krzysztof Kozlowski]

On 12.02.2016 13:30, Javier Martinez Canillas wrote:
> The max77686 MFD driver supports both the Maxim 77686 and Maxim 77802
> PMICs but only the OF device table contains entries for both devices.
> 
> The max77802 entry is missing in the I2C device ID table which isn't
> a problem currently since the driver only supports DT but it will be
> needed if the driver is changed to be built as a module since the I2C
> core always reports a I2C modalias uevent so auto-load will not work.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/max77686.c | 1 +
>  1 file changed, 1 insertion(+)


Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 1/4] mfd: max77686: Add max77802 to I2C device ID table

[Lee Jones]

On Fri, 12 Feb 2016, Javier Martinez Canillas wrote:

> The max77686 MFD driver supports both the Maxim 77686 and Maxim 77802
> PMICs but only the OF device table contains entries for both devices.
> 
> The max77802 entry is missing in the I2C device ID table which isn't
> a problem currently since the driver only supports DT but it will be
> needed if the driver is changed to be built as a module since the I2C
> core always reports a I2C modalias uevent so auto-load will not work.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/max77686.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index d959ebbb2194..1f30c97d6d4e 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -345,6 +345,7 @@ static int max77686_i2c_remove(struct i2c_client *i2c)
>  
>  static const struct i2c_device_id max77686_i2c_id[] = {
>  	{ "max77686", TYPE_MAX77686 },
> +	{ "max77802", TYPE_MAX77802 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, max77686_i2c_id);

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

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

[Javier Martinez Canillas]

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@osg.samsung.com>
---

 drivers/mfd/max77686.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 1f30c97d6d4e..2f563d0f83cc 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -400,18 +400,7 @@ static struct i2c_driver max77686_i2c_driver = {
 	.id_table = max77686_i2c_id,
 };
 
-static int __init max77686_i2c_init(void)
-{
-	return i2c_add_driver(&max77686_i2c_driver);
-}
-/* init early so consumer devices can complete system boot */
-subsys_initcall(max77686_i2c_init);
-
-static void __exit max77686_i2c_exit(void)
-{
-	i2c_del_driver(&max77686_i2c_driver);
-}
-module_exit(max77686_i2c_exit);
+module_i2c_driver(max77686_i2c_driver);
 
 MODULE_DESCRIPTION("MAXIM 77686/802 multi-function core driver");
 MODULE_AUTHOR("Chiwoong Byun <woong.byun@samsung.com>");
-- 
2.5.0

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

[Krzysztof Kozlowski]

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@osg.samsung.com>
> ---
> 
>  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".

My fast tests of this patch shown that it works good... but some more
thorough tests should be done.

Best regards,
Krzysztof

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

[Lee Jones]

On Mon, 15 Feb 2016, 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@osg.samsung.com>
> > ---
> > 
> >  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".
> 
> My fast tests of this patch shown that it works good... but some more
> thorough tests should be done.

See to all of this, collect the Acks you've received and re-submit
please.

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

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

[Javier Martinez Canillas]

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@osg.samsung.com>
>> ---
>>
>>   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.

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?

> 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.
  
> Best regards,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

[Krzysztof Kozlowski]

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@osg.samsung.com>
>>> ---
>>>
>>>   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.

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,
Krzysztof

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

[Javier Martinez Canillas]

Hello Krzysztof,

On 02/15/2016 08:20 PM, 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@osg.samsung.com>
>>>> ---
>>>>
>>>>    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.
>
> By "please check" in this case I mean - look if every regulator/clock
> consumer using stuff exposed by PMIC, supports properly deferred probing.
>

Got it, I checked and all but one consumer driver that use resources
provided by the max77686 defer probe when this is not found AFAICT.

Clocks:

drivers/mmc/core/pwrseq_simple.c
drivers/rtc/rtc-s3c.c

Regulators:

drivers/cpufreq/cpufreq-dt.c
drivers/gpu/drm/exynos/exynos_drm_dsi.c
drivers/gpu/drm/exynos/exynos_hdmi.c
drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
drivers/iio/adc/exynos_adc.c
drivers/input/misc/max77693-haptic.c
drivers/input/touchscreen/mms114.c
drivers/media/i2c/s5c73m3/s5c73m3-core.c
drivers/media/i2c/s5k6a3.c
drivers/media/platform/exynos4-is/mipi-csis.c
drivers/mfd/wm8994-core.c
drivers/mmc/host/dw_mmc.c
drivers/mmc/host/sdhci.c
drivers/usb/dwc2/platform.c

The only driver that does not defer probe when an input supply
isn't found is drivers/thermal/samsung/exynos_tmu.c (vtmu-supply).

So that should be handled before this series are merged.

>> 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...
>

Ok.
  
>>
>>> 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

Yes, problem is that I don't have access to boards with a max77686,
and that's why I asked for help to test the series on those :)

> 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. :)

Agreed, I never said that adding regressions was justified. I just wanted
to get rid of the initcall levels ordering hack but of course only after
the patches have been tested and found to not cause regressions.

But feel free to nack the series if you consider this to be too risky.

>
> Best regards,
> Krzysztof
>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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

[Krzysztof Kozlowski]

On 17.02.2016 05:45, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 02/15/2016 08:20 PM, 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@osg.samsung.com>
>>>>> ---
>>>>>
>>>>>    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.
>>
>> By "please check" in this case I mean - look if every regulator/clock
>> consumer using stuff exposed by PMIC, supports properly deferred probing.
>>
> 
> Got it, I checked and all but one consumer driver that use resources
> provided by the max77686 defer probe when this is not found AFAICT.
> 
> Clocks:
> 
> drivers/mmc/core/pwrseq_simple.c
> drivers/rtc/rtc-s3c.c
> 
> Regulators:
> 
> drivers/cpufreq/cpufreq-dt.c
> drivers/gpu/drm/exynos/exynos_drm_dsi.c
> drivers/gpu/drm/exynos/exynos_hdmi.c
> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> drivers/iio/adc/exynos_adc.c
> drivers/input/misc/max77693-haptic.c
> drivers/input/touchscreen/mms114.c
> drivers/media/i2c/s5c73m3/s5c73m3-core.c
> drivers/media/i2c/s5k6a3.c
> drivers/media/platform/exynos4-is/mipi-csis.c
> drivers/mfd/wm8994-core.c
> drivers/mmc/host/dw_mmc.c
> drivers/mmc/host/sdhci.c
> drivers/usb/dwc2/platform.c
> 
> The only driver that does not defer probe when an input supply
> isn't found is drivers/thermal/samsung/exynos_tmu.c (vtmu-supply).
> 
> So that should be handled before this series are merged.

Thanks for the analysis. Indeed the exynos_tmu does not support probe
deferral. Instead it just ignores the error and skips the regulator. It
would be good to fix this before applying this patch.

Rest looks indeed good so I don't have objections (beside tmu case).

Best regards,
Krzysztof

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

[Javier Martinez Canillas]

Hello Krzysztof,

On 02/17/2016 04:49 AM, Krzysztof Kozlowski wrote:
> On 17.02.2016 05:45, Javier Martinez Canillas wrote:

[snip]

>>>
>>> By "please check" in this case I mean - look if every regulator/clock
>>> consumer using stuff exposed by PMIC, supports properly deferred probing.
>>>
>>
>> Got it, I checked and all but one consumer driver that use resources
>> provided by the max77686 defer probe when this is not found AFAICT.
>>
>> Clocks:
>>
>> drivers/mmc/core/pwrseq_simple.c
>> drivers/rtc/rtc-s3c.c
>>
>> Regulators:
>>
>> drivers/cpufreq/cpufreq-dt.c
>> drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> drivers/gpu/drm/exynos/exynos_hdmi.c
>> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
>> drivers/iio/adc/exynos_adc.c
>> drivers/input/misc/max77693-haptic.c
>> drivers/input/touchscreen/mms114.c
>> drivers/media/i2c/s5c73m3/s5c73m3-core.c
>> drivers/media/i2c/s5k6a3.c
>> drivers/media/platform/exynos4-is/mipi-csis.c
>> drivers/mfd/wm8994-core.c
>> drivers/mmc/host/dw_mmc.c
>> drivers/mmc/host/sdhci.c
>> drivers/usb/dwc2/platform.c
>>
>> The only driver that does not defer probe when an input supply
>> isn't found is drivers/thermal/samsung/exynos_tmu.c (vtmu-supply).
>>
>> So that should be handled before this series are merged.
>
> Thanks for the analysis. Indeed the exynos_tmu does not support probe
> deferral. Instead it just ignores the error and skips the regulator. It
> would be good to fix this before applying this patch.
>

Ok. I'll take care of that, probably tomorrow.
  
> Rest looks indeed good so I don't have objections (beside tmu case).
>

Thanks a lot for your feedback and suggestions.

> Best regards,
> Krzysztof
> --

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

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@osg.samsung.com>
>>>> ---
>>>>
>>>>    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

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

[Krzysztof Kozlowski]

On 17.02.2016 18:27, Marek Szyprowski wrote:
> 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@osg.samsung.com>
>>>>> ---
>>>>>
>>>>>    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").

Thanks for the confirmation!

Best regards,
Krzysztof

[PATCH 3/4] mfd: max77686: Allow driver to be built as a module

[Javier Martinez Canillas]

The driver's Kconfig symbol is a boolean but nothing prevents the driver
to be built as a module instead of built-in. It is true that most system
integrators will choose the latter but the config should not restrict it.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---

 drivers/mfd/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6c4ebd9f4cb6..005ace26230e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -493,8 +493,8 @@ config MFD_MAX14577
 	  of the device.
 
 config MFD_MAX77686
-	bool "Maxim Semiconductor MAX77686/802 PMIC Support"
-	depends on I2C=y
+	tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
+	depends on I2C
 	depends on OF
 	select MFD_CORE
 	select REGMAP_I2C
-- 
2.5.0

Re: [PATCH 3/4] mfd: max77686: Allow driver to be built as a module

[Lee Jones]

On Fri, 12 Feb 2016, Javier Martinez Canillas wrote:

> The driver's Kconfig symbol is a boolean but nothing prevents the driver
> to be built as a module instead of built-in. It is true that most system
> integrators will choose the latter but the config should not restrict it.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> 
>  drivers/mfd/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6c4ebd9f4cb6..005ace26230e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -493,8 +493,8 @@ config MFD_MAX14577
>  	  of the device.
>  
>  config MFD_MAX77686
> -	bool "Maxim Semiconductor MAX77686/802 PMIC Support"
> -	depends on I2C=y
> +	tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
> +	depends on I2C
>  	depends on OF
>  	select MFD_CORE
>  	select REGMAP_I2C

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

[PATCH 4/4] mfd: max77686: Export OF module alias information

[Javier Martinez Canillas]

When the device is registered via OF, the OF table is used to match the
driver instead of the I2C device ID table but the entries in the latter
are used as aliasses to load the module if the driver was not built-in.

This is because the I2C core always reports an I2C module alias instead
of an OF one but that could change so it is better to always export it.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/mfd/max77686.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
index 2f563d0f83cc..b1ed9ed0828c 100644
--- a/drivers/mfd/max77686.c
+++ b/drivers/mfd/max77686.c
@@ -204,6 +204,7 @@ static const struct of_device_id max77686_pmic_dt_match[] = {
 	},
 	{ },
 };
+MODULE_DEVICE_TABLE(of, max77686_pmic_dt_match);
 
 static int max77686_i2c_probe(struct i2c_client *i2c,
 			      const struct i2c_device_id *id)
-- 
2.5.0

Re: [PATCH 4/4] mfd: max77686: Export OF module alias information

[Krzysztof Kozlowski]

On 12.02.2016 13:30, Javier Martinez Canillas wrote:
> When the device is registered via OF, the OF table is used to match the
> driver instead of the I2C device ID table but the entries in the latter
> are used as aliasses to load the module if the driver was not built-in.
> 
> This is because the I2C core always reports an I2C module alias instead
> of an OF one but that could change so it is better to always export it.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
>  drivers/mfd/max77686.c | 1 +
>  1 file changed, 1 insertion(+)
>

Makes sense,
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 4/4] mfd: max77686: Export OF module alias information

[Lee Jones]

On Fri, 12 Feb 2016, Javier Martinez Canillas wrote:

> When the device is registered via OF, the OF table is used to match the
> driver instead of the I2C device ID table but the entries in the latter
> are used as aliasses to load the module if the driver was not built-in.
> 
> This is because the I2C core always reports an I2C module alias instead
> of an OF one but that could change so it is better to always export it.

I do have a set which changes this behaviour.

Annoyingly it is still not applied. :(

> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> 
>  drivers/mfd/max77686.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
> index 2f563d0f83cc..b1ed9ed0828c 100644
> --- a/drivers/mfd/max77686.c
> +++ b/drivers/mfd/max77686.c
> @@ -204,6 +204,7 @@ static const struct of_device_id max77686_pmic_dt_match[] = {
>  	},
>  	{ },
>  };
> +MODULE_DEVICE_TABLE(of, max77686_pmic_dt_match);
>  
>  static int max77686_i2c_probe(struct i2c_client *i2c,
>  			      const struct i2c_device_id *id)

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

Re: [PATCH 0/4] mfd: max77686: Allow the driver to be built as a module

[Andi Shyti]

I like all the four patches

Reviewed-by: Andi Shyti <andi.shyti@samsung.com>

Thanks,
Andi

On Fri, Feb 12, 2016 at 01:30:15AM -0300, Javier Martinez Canillas wrote:
> Hello,
> 
> The Maxim77802 PMIC driver uses a boolean Kconfig symbol but there isn't
> really a reason to require the driver to be built-in.
> 
> It is true that since the PMIC provides clocks and regulators that could
> be critical on a system, most integrators are not going to build it as a
> module but it could be useful for multi-platform or distribution kernels
> that support different systems by building as much as possible as module.
> 
> Besides this series, a patch for the max77686 regulator driver is needed
> or the MFD max77686 driver module won't be installed by kbuild. I didn't
> include the patch in this series to avoid cross-subsystems churn for the
> maintainers and also because I don't consider that a regression since it
> was not possible to build the driver as a module before anyways.
> 
> Also, all the defconfigs in mainline that enable the MFD max77686 driver
> Kconfig symbol are using =y of course so it shouldn't be an issue to get
> this set merged, before the max77686 regulator patch already posted [0].
> 
> [0]: https://patchwork.kernel.org/patch/8287431/
> 
> Best regards,
> Javier
> 
> 
> Javier Martinez Canillas (4):
>   mfd: max77686: Add max77802 to I2C device ID table
>   mfd: max77686: Use module_i2c_driver() instead of subsys initcall
>   mfd: max77686: Allow driver to be built as a module
>   mfd: max77686: Export OF module alias information
> 
>  drivers/mfd/Kconfig    |  4 ++--
>  drivers/mfd/max77686.c | 15 +++------------
>  2 files changed, 5 insertions(+), 14 deletions(-)
> 
> -- 
> 2.5.0
>