[PATCH] regulator: max77650: add of_match table

[PATCH] regulator: max77650: add of_match table

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We need the of_match table if we want to use the compatible string in
the pmic's child node and get the regulator driver loaded automatically.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/regulator/max77650-regulator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/regulator/max77650-regulator.c b/drivers/regulator/max77650-regulator.c
index e57fc9197d62..ac89a412f665 100644
--- a/drivers/regulator/max77650-regulator.c
+++ b/drivers/regulator/max77650-regulator.c
@@ -386,9 +386,16 @@ static int max77650_regulator_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id max77650_regulator_of_match[] = {
+	{ .compatible = "maxim,max77650-regulator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max77650_regulator_of_match);
+
 static struct platform_driver max77650_regulator_driver = {
 	.driver = {
 		.name = "max77650-regulator",
+		.of_match_table = max77650_regulator_of_match,
 	},
 	.probe = max77650_regulator_probe,
 };
-- 
2.23.0

Re: [PATCH] regulator: max77650: add of_match table

[Mark Brown]

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

On Tue, Dec 10, 2019 at 11:07:25AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We need the of_match table if we want to use the compatible string in
> the pmic's child node and get the regulator driver loaded automatically.

Why would we need to use a compatible string in a child node to load the
regulator driver, surely we can just register a platform device in the
MFD?

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

Re: [PATCH] regulator: max77650: add of_match table

[Bartosz Golaszewski]

wt., 10 gru 2019 o 13:12 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Dec 10, 2019 at 11:07:25AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We need the of_match table if we want to use the compatible string in
> > the pmic's child node and get the regulator driver loaded automatically.
>
> Why would we need to use a compatible string in a child node to load the
> regulator driver, surely we can just register a platform device in the
> MFD?

The device is registered all right from MFD code, but the module won't
be loaded automatically from user-space even with the right
MODULE_ALIAS() for sub-nodes unless we define the
MODULE_DEVICE_TABLE().

Besides: the DT bindings define the compatible for sub-nodes already.
We should probably conform to that.

Bart

Re: [PATCH] regulator: max77650: add of_match table

[Mark Brown]

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

On Tue, Dec 10, 2019 at 01:51:38PM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 13:12 Mark Brown <broonie@kernel.org> napisał(a):

> > Why would we need to use a compatible string in a child node to load the
> > regulator driver, surely we can just register a platform device in the
> > MFD?

> The device is registered all right from MFD code, but the module won't
> be loaded automatically from user-space even with the right
> MODULE_ALIAS() for sub-nodes unless we define the
> MODULE_DEVICE_TABLE().

This seems to work fine for other drivers and the platform bus has to be
usable on systems that don't use DT so that doesn't sound right.  Which
MODULE_ALIAS() are you using exactly?

> Besides: the DT bindings define the compatible for sub-nodes already.
> We should probably conform to that.

I would say that's a mistake and should be fixed, this particular way of
loading the regulators is a Linux implementation detail.

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

Re: [PATCH] regulator: max77650: add of_match table

[Bartosz Golaszewski]

wt., 10 gru 2019 o 14:02 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Dec 10, 2019 at 01:51:38PM +0100, Bartosz Golaszewski wrote:
> > wt., 10 gru 2019 o 13:12 Mark Brown <broonie@kernel.org> napisał(a):
>
> > > Why would we need to use a compatible string in a child node to load the
> > > regulator driver, surely we can just register a platform device in the
> > > MFD?
>
> > The device is registered all right from MFD code, but the module won't
> > be loaded automatically from user-space even with the right
> > MODULE_ALIAS() for sub-nodes unless we define the
> > MODULE_DEVICE_TABLE().
>
> This seems to work fine for other drivers and the platform bus has to be
> usable on systems that don't use DT so that doesn't sound right.  Which
> MODULE_ALIAS() are you using exactly?
>

MODULE_ALIAS("platform:max77650-regulator");

> > Besides: the DT bindings define the compatible for sub-nodes already.
> > We should probably conform to that.
>
> I would say that's a mistake and should be fixed, this particular way of
> loading the regulators is a Linux implementation detail.

Fixed by removing this from the bindings?

Bartosz

Re: [PATCH] regulator: max77650: add of_match table

[Mark Brown]

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

On Tue, Dec 10, 2019 at 02:10:15PM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 14:02 Mark Brown <broonie@kernel.org> napisał(a):

> > This seems to work fine for other drivers and the platform bus has to be
> > usable on systems that don't use DT so that doesn't sound right.  Which
> > MODULE_ALIAS() are you using exactly?

> MODULE_ALIAS("platform:max77650-regulator");

Huh, that should work...  I wonder if adding a compatible to the DT has
messed it up, does it work without the compatible in the .dts (and with
the of_compatible removed from the MFD driver I guess)?

> > > Besides: the DT bindings define the compatible for sub-nodes already.
> > > We should probably conform to that.

> > I would say that's a mistake and should be fixed, this particular way of
> > loading the regulators is a Linux implementation detail.

> Fixed by removing this from the bindings?

Yeah.  Though it may be too late, shame I didn't catch this when it was
merged :(

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

Re: [PATCH] regulator: max77650: add of_match table

[Bartosz Golaszewski]

wt., 10 gru 2019 o 14:21 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Dec 10, 2019 at 02:10:15PM +0100, Bartosz Golaszewski wrote:
> > wt., 10 gru 2019 o 14:02 Mark Brown <broonie@kernel.org> napisał(a):
>
> > > This seems to work fine for other drivers and the platform bus has to be
> > > usable on systems that don't use DT so that doesn't sound right.  Which
> > > MODULE_ALIAS() are you using exactly?
>
> > MODULE_ALIAS("platform:max77650-regulator");
>
> Huh, that should work...  I wonder if adding a compatible to the DT has
> messed it up, does it work without the compatible in the .dts (and with
> the of_compatible removed from the MFD driver I guess)?
>

It does work when removing the compatible strings.

> > > > Besides: the DT bindings define the compatible for sub-nodes already.
> > > > We should probably conform to that.
>
> > > I would say that's a mistake and should be fixed, this particular way of
> > > loading the regulators is a Linux implementation detail.
>
> > Fixed by removing this from the bindings?
>
> Yeah.  Though it may be too late, shame I didn't catch this when it was
> merged :(

Yeah, I didn't know any better too.

Rob: the bindings for this were released some time ago and they define
compatible strings for MFD sub-nodes, but using compatible strings for
sub-nodes is apparently incorrect and the drivers don't support it
either. Can we remove them from the bindings?

Best regards,
Bartosz Golaszewski

Re: [PATCH] regulator: max77650: add of_match table

[Mark Brown]

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

On Tue, Dec 10, 2019 at 05:53:11PM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 14:21 Mark Brown <broonie@kernel.org> napisał(a):
> > On Tue, Dec 10, 2019 at 02:10:15PM +0100, Bartosz Golaszewski wrote:
> > > wt., 10 gru 2019 o 14:02 Mark Brown <broonie@kernel.org> napisał(a):

> > > MODULE_ALIAS("platform:max77650-regulator");

> > Huh, that should work...  I wonder if adding a compatible to the DT has
> > messed it up, does it work without the compatible in the .dts (and with
> > the of_compatible removed from the MFD driver I guess)?

> It does work when removing the compatible strings.

Ah, that explains it.

> > Yeah.  Though it may be too late, shame I didn't catch this when it was
> > merged :(

> Yeah, I didn't know any better too.

> Rob: the bindings for this were released some time ago and they define
> compatible strings for MFD sub-nodes, but using compatible strings for
> sub-nodes is apparently incorrect and the drivers don't support it
> either. Can we remove them from the bindings?

I think at this point it's better to just keep compatibility rather than
disrupt anything, I'll go ahead and apply your patch.

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

Applied "regulator: max77650: add of_match table" to the regulator tree

[Mark Brown]

The patch

   regulator: max77650: add of_match table

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 100a21100bbb2bbc82fc4273e152c96e5c6c5d12 Mon Sep 17 00:00:00 2001
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date: Tue, 10 Dec 2019 11:07:25 +0100
Subject: [PATCH] regulator: max77650: add of_match table

We need the of_match table if we want to use the compatible string in
the pmic's child node and get the regulator driver loaded automatically.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Link: https://lore.kernel.org/r/20191210100725.11005-1-brgl@bgdev.pl
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/max77650-regulator.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/regulator/max77650-regulator.c b/drivers/regulator/max77650-regulator.c
index e57fc9197d62..ac89a412f665 100644
--- a/drivers/regulator/max77650-regulator.c
+++ b/drivers/regulator/max77650-regulator.c
@@ -386,9 +386,16 @@ static int max77650_regulator_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id max77650_regulator_of_match[] = {
+	{ .compatible = "maxim,max77650-regulator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max77650_regulator_of_match);
+
 static struct platform_driver max77650_regulator_driver = {
 	.driver = {
 		.name = "max77650-regulator",
+		.of_match_table = max77650_regulator_of_match,
 	},
 	.probe = max77650_regulator_probe,
 };
-- 
2.20.1