[PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Add device tree compatible strings and create proper modalias structures
to let this driver load automatically if compiled as module, because
max14577 MFD driver creates MFD cells with such compatible strings.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/max14577-regulator.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/regulator/max14577-regulator.c b/drivers/regulator/max14577-regulator.c
index 07a150c9bbf2..19e779dd961e 100644
--- a/drivers/regulator/max14577-regulator.c
+++ b/drivers/regulator/max14577-regulator.c
@@ -238,6 +238,15 @@ static const struct platform_device_id max14577_regulator_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, max14577_regulator_id);
 
+static const struct of_device_id of_max14577_regulator_dt_match[] = {
+	{ .compatible = "maxim,max77836-regulator",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
+	{ .compatible = "maxim,max14577-regulator",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_max14577_regulator_dt_match);
+
 static struct platform_driver max14577_regulator_driver = {
 	.driver = {
 		   .name = "max14577-regulator",
-- 
2.17.1

[PATCH 2/3] extcon: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Add device tree compatible strings and create proper modalias structures
to let this driver load automatically if compiled as module, because
max14577 MFD driver creates MFD cells with such compatible strings.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/extcon/extcon-max14577.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
index 32f663436e6e..6df814cffe37 100644
--- a/drivers/extcon/extcon-max14577.c
+++ b/drivers/extcon/extcon-max14577.c
@@ -782,6 +782,15 @@ static const struct platform_device_id max14577_muic_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, max14577_muic_id);
 
+static const struct of_device_id of_max14577_muic_dt_match[] = {
+	{ .compatible = "maxim,max77836-muic",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
+	{ .compatible = "maxim,max14577-muic",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_max14577_muic_dt_match);
+
 static struct platform_driver max14577_muic_driver = {
 	.driver		= {
 		.name	= "max14577-muic",
-- 
2.17.1

Re: [PATCH 2/3] extcon: max14577: Add proper dt-compatible strings

[Krzysztof Kozlowski]

On Thu, Feb 20, 2020 at 03:51:26PM +0100, Marek Szyprowski wrote:
> Add device tree compatible strings and create proper modalias structures
> to let this driver load automatically if compiled as module, because
> max14577 MFD driver creates MFD cells with such compatible strings.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/extcon/extcon-max14577.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

The approach is still being discussed (in patch #1) so this should be
applied if patch #1 is also accepted. In such case:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[PATCH 3/3] power: charger: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Add device tree compatible strings and create proper modalias structures
to let this driver load automatically if compiled as module, because
max14577 MFD driver creates MFD cells with such compatible strings.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/power/supply/max14577_charger.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/power/supply/max14577_charger.c b/drivers/power/supply/max14577_charger.c
index 8a59feac6468..891ba9f6f295 100644
--- a/drivers/power/supply/max14577_charger.c
+++ b/drivers/power/supply/max14577_charger.c
@@ -623,6 +623,15 @@ static const struct platform_device_id max14577_charger_id[] = {
 };
 MODULE_DEVICE_TABLE(platform, max14577_charger_id);
 
+static const struct of_device_id of_max14577_charger_dt_match[] = {
+	{ .compatible = "maxim,max77836-charger",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
+	{ .compatible = "maxim,max14577-charger",
+	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_max14577_charger_dt_match);
+
 static struct platform_driver max14577_charger_driver = {
 	.driver = {
 		.name	= "max14577-charger",
-- 
2.17.1

Re: [PATCH 3/3] power: charger: max14577: Add proper dt-compatible strings

[Krzysztof Kozlowski]

On Thu, Feb 20, 2020 at 03:51:27PM +0100, Marek Szyprowski wrote:
> Add device tree compatible strings and create proper modalias structures
> to let this driver load automatically if compiled as module, because
> max14577 MFD driver creates MFD cells with such compatible strings.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/power/supply/max14577_charger.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

The approach is still being discussed (in patch #1) so this should be
applied if patch #1 is also accepted. In such case:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 3/3] power: charger: max14577: Add proper dt-compatible strings

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

Hi,

On Thu, Feb 20, 2020 at 03:51:27PM +0100, Marek Szyprowski wrote:
> Add device tree compatible strings and create proper modalias structures
> to let this driver load automatically if compiled as module, because
> max14577 MFD driver creates MFD cells with such compatible strings.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/power/supply/max14577_charger.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/power/supply/max14577_charger.c b/drivers/power/supply/max14577_charger.c
> index 8a59feac6468..891ba9f6f295 100644
> --- a/drivers/power/supply/max14577_charger.c
> +++ b/drivers/power/supply/max14577_charger.c
> @@ -623,6 +623,15 @@ static const struct platform_device_id max14577_charger_id[] = {
>  };
>  MODULE_DEVICE_TABLE(platform, max14577_charger_id);
>  
> +static const struct of_device_id of_max14577_charger_dt_match[] = {
> +	{ .compatible = "maxim,max77836-charger",
> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
> +	{ .compatible = "maxim,max14577-charger",
> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_max14577_charger_dt_match);
> +
>  static struct platform_driver max14577_charger_driver = {
>  	.driver = {
>  		.name	= "max14577-charger",

Independently of the discussion in patch 1 this is missing the link
to the of table in platform_driver->driver->of_match_table.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Thu, Feb 20, 2020 at 03:51:25PM +0100, Marek Szyprowski wrote:
> Add device tree compatible strings and create proper modalias structures
> to let this driver load automatically if compiled as module, because
> max14577 MFD driver creates MFD cells with such compatible strings.

> +static const struct of_device_id of_max14577_regulator_dt_match[] = {
> +	{ .compatible = "maxim,max77836-regulator",
> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
> +	{ .compatible = "maxim,max14577-regulator",
> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },

Why would we want to encode the particular way Linux happens to
represent regulators on a MFD into the DT binding?  It's not clear that
this is a generic thing (another OS might choose to have a separate
object for each regulator with no parent for example) and the compatible
isn't adding any information we didn't have already knowing about the
parent device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Hi Mark,

On 20.02.2020 17:56, Mark Brown wrote:
> On Thu, Feb 20, 2020 at 03:51:25PM +0100, Marek Szyprowski wrote:
>> Add device tree compatible strings and create proper modalias structures
>> to let this driver load automatically if compiled as module, because
>> max14577 MFD driver creates MFD cells with such compatible strings.
>> +static const struct of_device_id of_max14577_regulator_dt_match[] = {
>> +	{ .compatible = "maxim,max77836-regulator",
>> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX77836, },
>> +	{ .compatible = "maxim,max14577-regulator",
>> +	  .data = (void *)MAXIM_DEVICE_TYPE_MAX14577, },
> Why would we want to encode the particular way Linux happens to
> represent regulators on a MFD into the DT binding?  It's not clear that
> this is a generic thing (another OS might choose to have a separate
> object for each regulator with no parent for example) and the compatible
> isn't adding any information we didn't have already knowing about the
> parent device.

Well, that's how the bindings for max14577/max77836 are defined:

Documentation/devicetree/bindings/mfd/max14577.txt

I've only fixed regulator, charger and extcon drivers to match the cells 
created by the current mfd driver.

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

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

On Fri, Feb 21, 2020 at 11:44:03AM +0100, Marek Szyprowski wrote:
> On 20.02.2020 17:56, Mark Brown wrote:

> > Why would we want to encode the particular way Linux happens to
> > represent regulators on a MFD into the DT binding?  It's not clear that
> > this is a generic thing (another OS might choose to have a separate
> > object for each regulator with no parent for example) and the compatible
> > isn't adding any information we didn't have already knowing about the
> > parent device.

> Well, that's how the bindings for max14577/max77836 are defined:

> Documentation/devicetree/bindings/mfd/max14577.txt

> I've only fixed regulator, charger and extcon drivers to match the cells 
> created by the current mfd driver.

We could just remove the compatible strings from the binding
documentation, they won't do any harm if we don't use them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Hi Mark,

+CC: Rob Herring, here is the whole thread:

https://lore.kernel.org/lkml/20200221123813.GB5546@sirena.org.uk/T/#t

On 21.02.2020 13:38, Mark Brown wrote:
> On Fri, Feb 21, 2020 at 11:44:03AM +0100, Marek Szyprowski wrote:
>> On 20.02.2020 17:56, Mark Brown wrote:
>>
>>> Why would we want to encode the particular way Linux happens to
>>> represent regulators on a MFD into the DT binding?  It's not clear that
>>> this is a generic thing (another OS might choose to have a separate
>>> object for each regulator with no parent for example) and the compatible
>>> isn't adding any information we didn't have already knowing about the
>>> parent device.
>>>
>> Well, that's how the bindings for max14577/max77836 are defined:
>>
>> Documentation/devicetree/bindings/mfd/max14577.txt
>>
>> I've only fixed regulator, charger and extcon drivers to match the cells
>> created by the current mfd driver.
> We could just remove the compatible strings from the binding
> documentation, they won't do any harm if we don't use them.

Frankly I have no strong opinion on this. I've just wanted to fix the 
broken autoloading of the drivers compiled as modules.

Best regards

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

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Fri, Feb 21, 2020 at 02:23:57PM +0100, Marek Szyprowski wrote:
> On 21.02.2020 13:38, Mark Brown wrote:

> > We could just remove the compatible strings from the binding
> > documentation, they won't do any harm if we don't use them.

> Frankly I have no strong opinion on this. I've just wanted to fix the 
> broken autoloading of the drivers compiled as modules.

Shouldn't adding the relevant module table for the platform devices work
just as well for that?  Possibly also deleting the of_compatible bits in
the MFD as well, ISTR that's needed to make the platform device work.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Hi Mark,

On 21.02.2020 18:13, Mark Brown wrote:
> On Fri, Feb 21, 2020 at 02:23:57PM +0100, Marek Szyprowski wrote:
>> On 21.02.2020 13:38, Mark Brown wrote:
>>> We could just remove the compatible strings from the binding
>>> documentation, they won't do any harm if we don't use them.
>> Frankly I have no strong opinion on this. I've just wanted to fix the
>> broken autoloading of the drivers compiled as modules.
> Shouldn't adding the relevant module table for the platform devices work
> just as well for that?  Possibly also deleting the of_compatible bits in
> the MFD as well, ISTR that's needed to make the platform device work.

Right. This will work too. MFD cells will match to their drivers by the 
name and modalias strings will be correct. The question is which 
approach is preffered? Krzysztof? I've checked other mfd drivers, but I 
cannot find any pattern in this area.

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

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Krzysztof Kozlowski]

On Mon, Feb 24, 2020 at 03:08:05PM +0100, Marek Szyprowski wrote:
> Hi Mark,
> 
> On 21.02.2020 18:13, Mark Brown wrote:
> > On Fri, Feb 21, 2020 at 02:23:57PM +0100, Marek Szyprowski wrote:
> >> On 21.02.2020 13:38, Mark Brown wrote:
> >>> We could just remove the compatible strings from the binding
> >>> documentation, they won't do any harm if we don't use them.
> >> Frankly I have no strong opinion on this. I've just wanted to fix the
> >> broken autoloading of the drivers compiled as modules.
> > Shouldn't adding the relevant module table for the platform devices work
> > just as well for that?  Possibly also deleting the of_compatible bits in
> > the MFD as well, ISTR that's needed to make the platform device work.
> 
> Right. This will work too. MFD cells will match to their drivers by the 
> name and modalias strings will be correct. The question is which 
> approach is preffered? Krzysztof? I've checked other mfd drivers, but I 
> cannot find any pattern in this area.

I would guess that adding MODULE_DEVICE_TABLE() for OF-matches in main
MFD driver would fix the issue... otherwise the same problem we have
with max77693 (also MUIC/extcon/regulator/charger).

Some of these drivers (I guess only charger) bind to a OF node so they
need a compatible. I think we added this to regulators and extcon for
symmetry.
Without this binding, the charger would need to read a specific child
node from parent. This make them tightly coupled. It seems to me more
robust for each component to bind to his own node, when needed.

Another reason of adding compatibles was an idea of reusability of
MFD children (between different MFD drivers or even standalone) but it
never got implemented (children still depend on parent significantly).

In general, I like the approach of children with compatibles but I will
not argue against changing the drivers. They could really use some
cleanup :)
Long time I tried to remove the support for platform_data [1] - maybe
let's continue?

[1] https://lore.kernel.org/lkml/20170217200200.4521-1-krzk@kernel.org/

Best regards,
Krzysztof

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Marek Szyprowski]

Hi Krzysztof,

On 24.02.2020 21:12, Krzysztof Kozlowski wrote:
> On Mon, Feb 24, 2020 at 03:08:05PM +0100, Marek Szyprowski wrote:
>> On 21.02.2020 18:13, Mark Brown wrote:
>>> On Fri, Feb 21, 2020 at 02:23:57PM +0100, Marek Szyprowski wrote:
>>>> On 21.02.2020 13:38, Mark Brown wrote:
>>>>> We could just remove the compatible strings from the binding
>>>>> documentation, they won't do any harm if we don't use them.
>>>> Frankly I have no strong opinion on this. I've just wanted to fix the
>>>> broken autoloading of the drivers compiled as modules.
>>> Shouldn't adding the relevant module table for the platform devices work
>>> just as well for that?  Possibly also deleting the of_compatible bits in
>>> the MFD as well, ISTR that's needed to make the platform device work.
>> Right. This will work too. MFD cells will match to their drivers by the
>> name and modalias strings will be correct. The question is which
>> approach is preffered? Krzysztof? I've checked other mfd drivers, but I
>> cannot find any pattern in this area.
> I would guess that adding MODULE_DEVICE_TABLE() for OF-matches in main
> MFD driver would fix the issue... otherwise the same problem we have
> with max77693 (also MUIC/extcon/regulator/charger).

Indeed, there is a same problem with max77963:

max77963-muic driver lacks compatible and has wrong platform modalias 
("extcon-max77963"),

max77963-charger driver lacks compatible,

max77963-haptic driver lacks compatible.

> Some of these drivers (I guess only charger) bind to a OF node so they
> need a compatible. I think we added this to regulators and extcon for
> symmetry.
> Without this binding, the charger would need to read a specific child
> node from parent. This make them tightly coupled. It seems to me more
> robust for each component to bind to his own node, when needed.

Extcon would also need its node when support for it will be added to 
dwc2 driver. Having compatible strings in the nodes simplifies matching 
and makes it almost automatic.

> Another reason of adding compatibles was an idea of reusability of
> MFD children (between different MFD drivers or even standalone) but it
> never got implemented (children still depend on parent significantly).

So far, there is no such case.

> In general, I like the approach of children with compatibles but I will
> not argue against changing the drivers. They could really use some
> cleanup :)
> Long time I tried to remove the support for platform_data [1] - maybe
> let's continue?
>
> [1] https://lore.kernel.org/lkml/20170217200200.4521-1-krzk@kernel.org/

Cleanup of the driver is another story, completely independent of fixing 
this issue imho.

krzk: could you then specify if you are against or after the proposed 
changes?

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

Re: [PATCH 1/3] regulator: max14577: Add proper dt-compatible strings

[Krzysztof Kozlowski]

On Fri, Mar 06, 2020 at 02:51:22PM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 24.02.2020 21:12, Krzysztof Kozlowski wrote:
> > On Mon, Feb 24, 2020 at 03:08:05PM +0100, Marek Szyprowski wrote:
> >> On 21.02.2020 18:13, Mark Brown wrote:
> >>> On Fri, Feb 21, 2020 at 02:23:57PM +0100, Marek Szyprowski wrote:
> >>>> On 21.02.2020 13:38, Mark Brown wrote:
> >>>>> We could just remove the compatible strings from the binding
> >>>>> documentation, they won't do any harm if we don't use them.
> >>>> Frankly I have no strong opinion on this. I've just wanted to fix the
> >>>> broken autoloading of the drivers compiled as modules.
> >>> Shouldn't adding the relevant module table for the platform devices work
> >>> just as well for that?  Possibly also deleting the of_compatible bits in
> >>> the MFD as well, ISTR that's needed to make the platform device work.
> >> Right. This will work too. MFD cells will match to their drivers by the
> >> name and modalias strings will be correct. The question is which
> >> approach is preffered? Krzysztof? I've checked other mfd drivers, but I
> >> cannot find any pattern in this area.
> > I would guess that adding MODULE_DEVICE_TABLE() for OF-matches in main
> > MFD driver would fix the issue... otherwise the same problem we have
> > with max77693 (also MUIC/extcon/regulator/charger).
> 
> Indeed, there is a same problem with max77963:
> 
> max77963-muic driver lacks compatible and has wrong platform modalias 
> ("extcon-max77963"),
> 
> max77963-charger driver lacks compatible,
> 
> max77963-haptic driver lacks compatible.
> 
> > Some of these drivers (I guess only charger) bind to a OF node so they
> > need a compatible. I think we added this to regulators and extcon for
> > symmetry.
> > Without this binding, the charger would need to read a specific child
> > node from parent. This make them tightly coupled. It seems to me more
> > robust for each component to bind to his own node, when needed.
> 
> Extcon would also need its node when support for it will be added to 
> dwc2 driver. Having compatible strings in the nodes simplifies matching 
> and makes it almost automatic.
> 
> > Another reason of adding compatibles was an idea of reusability of
> > MFD children (between different MFD drivers or even standalone) but it
> > never got implemented (children still depend on parent significantly).
> 
> So far, there is no such case.
> 
> > In general, I like the approach of children with compatibles but I will
> > not argue against changing the drivers. They could really use some
> > cleanup :)
> > Long time I tried to remove the support for platform_data [1] - maybe
> > let's continue?
> >
> > [1] https://lore.kernel.org/lkml/20170217200200.4521-1-krzk@kernel.org/
> 
> Cleanup of the driver is another story, completely independent of fixing 
> this issue imho.
> 
> krzk: could you then specify if you are against or after the proposed 
> changes?

Since we already have compatibles and some of the children require them
(charger), I vote in favor of this patch and for keeping compatibles.

Best regards,
Krzysztof