From: Matthias Brugger <[email protected]>
Binding documentation mentions that a compatible is required for the
MT6360 device node, but the driver doesn't provide a OF match table.
Fixes: d321571d5e4c ("regulator: mt6360: Add support for MT6360 regulator")
Signed-off-by: Matthias Brugger <[email protected]>
---
Changes in v2:
- check for CONFIG_OF
- add Fixes tag
drivers/regulator/mt6360-regulator.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/regulator/mt6360-regulator.c b/drivers/regulator/mt6360-regulator.c
index 15308ee29c13..f7b2514feabf 100644
--- a/drivers/regulator/mt6360-regulator.c
+++ b/drivers/regulator/mt6360-regulator.c
@@ -445,9 +445,18 @@ static const struct platform_device_id mt6360_regulator_id_table[] = {
};
MODULE_DEVICE_TABLE(platform, mt6360_regulator_id_table);
+#ifdef CONFIG_OF
+static const struct of_device_id mt6360_of_match[] = {
+ { .compatible = "mediatek,mt6360-regulator", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mt6360_of_match);
+#endif
+
static struct platform_driver mt6360_regulator_driver = {
.driver = {
.name = "mt6360-regulator",
+ .of_match_table = of_match_ptr(mt6360_of_match),
},
.probe = mt6360_regulator_probe,
.id_table = mt6360_regulator_id_table,
--
2.29.2
From: Matthias Brugger <[email protected]>
The binding documentation mentions that a compatible is required for the
MT6358 device node. But the driver does not provide a OF match table.
This way auto-loading is broken as the MFD driver that registers the
device has a .of_compatible set which makes the platform .uevent
callback report a OF modalias, but that's not in the module.
Fixes: f67ff1bd58f0 ("regulator: mt6358: Add support for MT6358 regulator")
Signed-off-by: Matthias Brugger <[email protected]>
---
Changes in v2:
- check for CONFIG_OF
- add Fixes tag
drivers/regulator/mt6358-regulator.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/regulator/mt6358-regulator.c b/drivers/regulator/mt6358-regulator.c
index 13cb6ac9a892..a4ed19a54ec6 100644
--- a/drivers/regulator/mt6358-regulator.c
+++ b/drivers/regulator/mt6358-regulator.c
@@ -534,9 +534,18 @@ static const struct platform_device_id mt6358_platform_ids[] = {
};
MODULE_DEVICE_TABLE(platform, mt6358_platform_ids);
+#ifdef CONFIG_OF
+static const struct of_device_id mt6358_of_match[] = {
+ { .compatible = "mediatek,mt6358-regulator", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mt6358_of_match);
+#endif
+
static struct platform_driver mt6358_regulator_driver = {
.driver = {
.name = "mt6358-regulator",
+ .of_match_table = of_match_ptr(mt6358_of_match),
},
.probe = mt6358_regulator_probe,
.id_table = mt6358_platform_ids,
--
2.29.2
From: Matthias Brugger <[email protected]>
The binding documentation mentions that a compatible is required for the
MT6323 device node. But the driver does not provide a OF match table.
This way auto-loading is broken as the MFD driver that registers the
device has a .of_compatible set which makes the platform .uevent
callback report a OF modalias, but that's not in the module.
Fixes: 2fdf82923618 ("regulator: mt6323: Add support for MT6323 regulator")
Signed-off-by: Matthias Brugger <[email protected]>
---
Changes in v2:
- check for CONFIG_OF
- add Fixes tag
drivers/regulator/mt6323-regulator.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/regulator/mt6323-regulator.c b/drivers/regulator/mt6323-regulator.c
index ff9016170db3..646742d4db74 100644
--- a/drivers/regulator/mt6323-regulator.c
+++ b/drivers/regulator/mt6323-regulator.c
@@ -406,9 +406,18 @@ static const struct platform_device_id mt6323_platform_ids[] = {
};
MODULE_DEVICE_TABLE(platform, mt6323_platform_ids);
+#ifdef CONFIG_OF
+static const struct of_device_id mt6323_of_match[] = {
+ { .compatible = "mediatek,mt6323-regulator", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mt6323_of_match);
+#endif
+
static struct platform_driver mt6323_regulator_driver = {
.driver = {
.name = "mt6323-regulator",
+ .of_match_table = of_match_ptr(mt6323_of_match),
},
.probe = mt6323_regulator_probe,
.id_table = mt6323_platform_ids,
--
2.29.2
Hi
> Gesendet: Samstag, 09. Januar 2021 um 12:26 Uhr
> Von: [email protected]
> Changes in v2:
> - check for CONFIG_OF
> - add Fixes tag
> --- a/drivers/regulator/mt6323-regulator.c
> +++ b/drivers/regulator/mt6323-regulator.c
> @@ -406,9 +406,18 @@ static const struct platform_device_id mt6323_platform_ids[] = {
> };
> MODULE_DEVICE_TABLE(platform, mt6323_platform_ids);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id mt6323_of_match[] = {
> + { .compatible = "mediatek,mt6323-regulator", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mt6323_of_match);
> +#endif
> +
> static struct platform_driver mt6323_regulator_driver = {
> .driver = {
> .name = "mt6323-regulator",
> + .of_match_table = of_match_ptr(mt6323_of_match),
imho check for CONFIG_OF is here needed too, else mt6323_of_match is referenced but undefined
> },
> .probe = mt6323_regulator_probe,
> .id_table = mt6323_platform_ids,
regards Frank
On 09/01/2021 17:44, Frank Wunderlich wrote:
> Hi
>
>> Gesendet: Samstag, 09. Januar 2021 um 12:26 Uhr
>> Von: [email protected]
>
>> Changes in v2:
>> - check for CONFIG_OF
>> - add Fixes tag
>
>> --- a/drivers/regulator/mt6323-regulator.c
>> +++ b/drivers/regulator/mt6323-regulator.c
>> @@ -406,9 +406,18 @@ static const struct platform_device_id mt6323_platform_ids[] = {
>> };
>> MODULE_DEVICE_TABLE(platform, mt6323_platform_ids);
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id mt6323_of_match[] = {
>> + { .compatible = "mediatek,mt6323-regulator", },
>> + { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, mt6323_of_match);
>> +#endif
>> +
>> static struct platform_driver mt6323_regulator_driver = {
>> .driver = {
>> .name = "mt6323-regulator",
>> + .of_match_table = of_match_ptr(mt6323_of_match),
>
> imho check for CONFIG_OF is here needed too, else mt6323_of_match is referenced but undefined
>
No, of_match_ptr already takes care of this:
https://elixir.bootlin.com/linux/latest/source/include/linux/of.h#L998
Regards,
Matthias
[ Internal Use - External ]
Hi Matthias,
I discussed OF match table with Mark in previous mail in our PATCH v3,
MFD should just instantiate the platform device.
> Mark Brown <[email protected]> ?? 2020?~8??20?? ?g?| ?U??7:45?g?D?G
> > This device only exists in the context of a single parent device, there
> > should be no need for a compatible string here - this is just a detail
> > of how Linux does things. The MFD should just instntiate the platform
> > device.
> Trying to autoload module without of_id_table will cause run-time error:
> ueventd: LoadWithAliases was unable to load
> of:NregulatorT(null)Cmediatek,mt6360-regulator
You shouldn't have this described in the device tree at all, like I say
the MFD should just instantiate the platform device.
> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Saturday, January 9, 2021 7:26 PM
> To: Liam Girdwood <[email protected]>; Mark Brown
> <[email protected]>
> Cc: Hsin-Hsiung Wang <[email protected]>; Axel Lin
> <[email protected]>; Chen Zhong <[email protected]>;
> gene_chen(???T?t) <[email protected]>; [email protected];
> [email protected]; [email protected];
> Matthias Brugger <[email protected]>; Matti Vaittinen
> <[email protected]>; Matthias Brugger
> <[email protected]>
> Subject: [PATCH v2 1/3] regulator: mt6360: Add OF match table
>
> From: Matthias Brugger <[email protected]>
>
> Binding documentation mentions that a compatible is required for the
> MT6360 device node, but the driver doesn't provide a OF match table.
>
> Fixes: d321571d5e4c ("regulator: mt6360: Add support for MT6360 regulator")
> Signed-off-by: Matthias Brugger <[email protected]>
>
> ---
>
> Changes in v2:
> - check for CONFIG_OF
> - add Fixes tag
>
> drivers/regulator/mt6360-regulator.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/regulator/mt6360-regulator.c
> b/drivers/regulator/mt6360-regulator.c
> index 15308ee29c13..f7b2514feabf 100644
> --- a/drivers/regulator/mt6360-regulator.c
> +++ b/drivers/regulator/mt6360-regulator.c
> @@ -445,9 +445,18 @@ static const struct platform_device_id
> mt6360_regulator_id_table[] = { }; MODULE_DEVICE_TABLE(platform,
> mt6360_regulator_id_table);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id mt6360_of_match[] = {
> +{ .compatible = "mediatek,mt6360-regulator", },
> +{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mt6360_of_match); #endif
> +
> static struct platform_driver mt6360_regulator_driver = {
> .driver = {
> .name = "mt6360-regulator",
> +.of_match_table = of_match_ptr(mt6360_of_match),
> },
> .probe = mt6360_regulator_probe,
> .id_table = mt6360_regulator_id_table,
> --
> 2.29.2
************* Email Confidentiality Notice ********************
The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!
On 11/01/2021 03:18, gene_chen(???T?t) wrote:
> [ Internal Use - External ]
>
Please don't top-post in the future.
> Hi Matthias,
>
> I discussed OF match table with Mark in previous mail in our PATCH v3,
> MFD should just instantiate the platform device.
Did you ever test that? Which MFD driver should instantiate the device?
I had a look at mt6360-core.c (the obvious one) but I don't see any reference to
the regulator [1]. What am I missing?
>
>> Mark Brown <[email protected]> ?? 2020?~8??20?? ?g?| ?U??7:45?g?D?G
>
>>> This device only exists in the context of a single parent device, there
>>> should be no need for a compatible string here - this is just a detail
>>> of how Linux does things. The MFD should just instntiate the platform
>>> device.
>
>> Trying to autoload module without of_id_table will cause run-time error:
>> ueventd: LoadWithAliases was unable to load
>> of:NregulatorT(null)Cmediatek,mt6360-regulator
>
> You shouldn't have this described in the device tree at all, like I say
> the MFD should just instantiate the platform device.
>
Well from my understanding the regulator has a device-tree entry [2], so it
needs to match against a device-tree node. My understanding is, that you need a
the devicetree node to describe the regulators provided by the device. TBH I'm a
bit puzzled about the comment from Mark here. How does another DT node be able
to reference a regulator if this is not described in DT? I think we need a DT
node here and the matching in the regulator and MFD driver to get the regulator
loaded via udev.
Regards,
Matthias
[1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/mt6360-core.c#L294
[2]
https://elixir.bootlin.com/linux/v5.10.6/source/Documentation/devicetree/bindings/regulator/mt6360-regulator.yaml
>> -----Original Message-----
>> From: [email protected] <[email protected]>
>> Sent: Saturday, January 9, 2021 7:26 PM
>> To: Liam Girdwood <[email protected]>; Mark Brown
>> <[email protected]>
>> Cc: Hsin-Hsiung Wang <[email protected]>; Axel Lin
>> <[email protected]>; Chen Zhong <[email protected]>;
>> gene_chen(???T?t) <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> Matthias Brugger <[email protected]>; Matti Vaittinen
>> <[email protected]>; Matthias Brugger
>> <[email protected]>
>> Subject: [PATCH v2 1/3] regulator: mt6360: Add OF match table
>>
>> From: Matthias Brugger <[email protected]>
>>
>> Binding documentation mentions that a compatible is required for the
>> MT6360 device node, but the driver doesn't provide a OF match table.
>>
>> Fixes: d321571d5e4c ("regulator: mt6360: Add support for MT6360 regulator")
>> Signed-off-by: Matthias Brugger <[email protected]>
>>
>> ---
>>
>> Changes in v2:
>> - check for CONFIG_OF
>> - add Fixes tag
>>
>> drivers/regulator/mt6360-regulator.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/regulator/mt6360-regulator.c
>> b/drivers/regulator/mt6360-regulator.c
>> index 15308ee29c13..f7b2514feabf 100644
>> --- a/drivers/regulator/mt6360-regulator.c
>> +++ b/drivers/regulator/mt6360-regulator.c
>> @@ -445,9 +445,18 @@ static const struct platform_device_id
>> mt6360_regulator_id_table[] = { }; MODULE_DEVICE_TABLE(platform,
>> mt6360_regulator_id_table);
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id mt6360_of_match[] = {
>> +{ .compatible = "mediatek,mt6360-regulator", },
>> +{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, mt6360_of_match); #endif
>> +
>> static struct platform_driver mt6360_regulator_driver = {
>> .driver = {
>> .name = "mt6360-regulator",
>> +.of_match_table = of_match_ptr(mt6360_of_match),
>> },
>> .probe = mt6360_regulator_probe,
>> .id_table = mt6360_regulator_id_table,
>> --
>> 2.29.2
> ************* Email Confidentiality Notice ********************
>
> The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!
>
Hello Matthias & All,
On Mon, 2021-01-11 at 11:08 +0100, Matthias Brugger wrote:
>
> On 11/01/2021 03:18, gene_chen(陳俊宇) wrote:
> > [ Internal Use - External ]
> >
>
> Please don't top-post in the future.
>
> > Hi Matthias,
> >
> > I discussed OF match table with Mark in previous mail in our PATCH
> > v3,
> > MFD should just instantiate the platform device.
>
> Did you ever test that? Which MFD driver should instantiate the
> device?
> I had a look at mt6360-core.c (the obvious one) but I don't see any
> reference to
> the regulator [1]. What am I missing?
>
> > > Mark Brown <[email protected]> 於 2020年8月20日 週四 下午7:45寫道：
> > > > This device only exists in the context of a single parent
> > > > device, there
> > > > should be no need for a compatible string here - this is just a
> > > > detail
> > > > of how Linux does things. The MFD should just instntiate the
> > > > platform
> > > > device.
> > > Trying to autoload module without of_id_table will cause run-time
> > > error:
> > > ueventd: LoadWithAliases was unable to load
> > > of:NregulatorT(null)Cmediatek,mt6360-regulator
> >
> > You shouldn't have this described in the device tree at all, like I
> > say
> > the MFD should just instantiate the platform device.
> >
>
> Well from my understanding the regulator has a device-tree entry [2],
> so it
> needs to match against a device-tree node. My understanding is, that
> you need a
> the devicetree node to describe the regulators provided by the
> device. TBH I'm a
> bit puzzled about the comment from Mark here. How does another DT
> node be able
> to reference a regulator if this is not described in DT? I think we
> need a DT
> node here and the matching in the regulator and MFD driver to get the
> regulator
> loaded via udev.
Others are better to answer - but as I spotted this from my inbox I'll
give my 2 cents :)
This can be done W/O regulators having own compatible. Please see
following:
drivers/mfd/rohm-bd718x7.c
drivers/regulator/bd718x7-regulator.c
Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
Documentation/devicetree/bindings/regulator/rohm,bd71847-regulator.yaml
as example.
The device matching can be done via platform_device_id table.
I think the MODULE_ALIAS() is needed for module matching - but I don't
remember without further code browsing.
Anyways, the BD71837/47/50 DT entry without compatible for
clk/regulators do probe and load the sub-devices.
As a "tradeoff" subdevices must retrieve the DT node from the parent
device.
For my uneducated eyes the DT binding for regulators should be changed.
Compatible should not be required and the example node should be moved
to MFD binding document in the MFD node. But that's just my view on
this - not willing to push this to any direction!
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Hi Matti,
On 11/01/2021 11:32, Vaittinen, Matti wrote:
>
> Hello Matthias & All,
>
> On Mon, 2021-01-11 at 11:08 +0100, Matthias Brugger wrote:
>>
>> On 11/01/2021 03:18, gene_chen(陳俊宇) wrote:
>>> [ Internal Use - External ]
>>>
>>
>> Please don't top-post in the future.
>>
>>> Hi Matthias,
>>>
>>> I discussed OF match table with Mark in previous mail in our PATCH
>>> v3,
>>> MFD should just instantiate the platform device.
>>
>> Did you ever test that? Which MFD driver should instantiate the
>> device?
>> I had a look at mt6360-core.c (the obvious one) but I don't see any
>> reference to
>> the regulator [1]. What am I missing?
>>
>>>> Mark Brown <[email protected]> 於 2020年8月20日 週四 下午7:45寫道：
>>>>> This device only exists in the context of a single parent
>>>>> device, there
>>>>> should be no need for a compatible string here - this is just a
>>>>> detail
>>>>> of how Linux does things. The MFD should just instntiate the
>>>>> platform
>>>>> device.
>>>> Trying to autoload module without of_id_table will cause run-time
>>>> error:
>>>> ueventd: LoadWithAliases was unable to load
>>>> of:NregulatorT(null)Cmediatek,mt6360-regulator
>>>
>>> You shouldn't have this described in the device tree at all, like I
>>> say
>>> the MFD should just instantiate the platform device.
>>>
>>
>> Well from my understanding the regulator has a device-tree entry [2],
>> so it
>> needs to match against a device-tree node. My understanding is, that
>> you need a
>> the devicetree node to describe the regulators provided by the
>> device. TBH I'm a
>> bit puzzled about the comment from Mark here. How does another DT
>> node be able
>> to reference a regulator if this is not described in DT? I think we
>> need a DT
>> node here and the matching in the regulator and MFD driver to get the
>> regulator
>> loaded via udev.
>
> Others are better to answer - but as I spotted this from my inbox I'll
> give my 2 cents :)
>
> This can be done W/O regulators having own compatible. Please see
> following:
>
> drivers/mfd/rohm-bd718x7.c
> drivers/regulator/bd718x7-regulator.c
> Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> Documentation/devicetree/bindings/regulator/rohm,bd71847-regulator.yaml
>
> as example.
Thanks for the links. Yes that's' a way to go, but...
>
> The device matching can be done via platform_device_id table.
> I think the MODULE_ALIAS() is needed for module matching - but I don't
> remember without further code browsing.
>
> Anyways, the BD71837/47/50 DT entry without compatible for
> clk/regulators do probe and load the sub-devices.
>
> As a "tradeoff" subdevices must retrieve the DT node from the parent
> device.
>
> For my uneducated eyes the DT binding for regulators should be changed.
> Compatible should not be required and the example node should be moved
> to MFD binding document in the MFD node. But that's just my view on
> this - not willing to push this to any direction!
>
Generally speaking, DT bindings are stable and can't be changed in a
none-compatible way. We could argue if there is any user of the compatible out
there (probably there isn't any, as the code seems to not even be working).
Apart from that all other cells in the MFD driver are defined as OF_MFD_CELL
[1]. Actually I suspect that mt6360_ldo should represent mt6360-regulator.
Regards,
Matthias
[1] https://elixir.bootlin.com/linux/latest/source/drivers/mfd/mt6360-core.c#L294
> Best Regards
> Matti Vaittinen
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland
> SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
>
> Simon says - in Latin please.
> "non cogito me" dixit Rene Descarte, deinde evanescavit
>
> (Thanks for the translation Simon)
>
On 11/01/2021 03:18, gene_chen(???T?t) wrote:
> [ Internal Use - External ]
>
> Hi Matthias,
>
> I discussed OF match table with Mark in previous mail in our PATCH v3,
> MFD should just instantiate the platform device.
>
>> Mark Brown <[email protected]> ?? 2020?~8??20?? ?g?| ?U??7:45?g?D?G
>
>>> This device only exists in the context of a single parent device, there
>>> should be no need for a compatible string here - this is just a detail
>>> of how Linux does things. The MFD should just instntiate the platform
>>> device.
>
>> Trying to autoload module without of_id_table will cause run-time error:
>> ueventd: LoadWithAliases was unable to load
>> of:NregulatorT(null)Cmediatek,mt6360-regulator
>
> You shouldn't have this described in the device tree at all, like I say
> the MFD should just instantiate the platform device.
>
Well with the patch you send to enable regulators [1] you use OF_MFD_CELL. So
my understanding is, if you don't provide an of_match_table in the regulator
driver, the loading via udev is broken (like you mentioned in the cite from an
older email).
Regards,
Matthias
[1]
https://lore.kernel.org/linux-mediatek/[email protected]/
>> -----Original Message-----
>> From: [email protected] <[email protected]>
>> Sent: Saturday, January 9, 2021 7:26 PM
>> To: Liam Girdwood <[email protected]>; Mark Brown
>> <[email protected]>
>> Cc: Hsin-Hsiung Wang <[email protected]>; Axel Lin
>> <[email protected]>; Chen Zhong <[email protected]>;
>> gene_chen(???T?t) <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> Matthias Brugger <[email protected]>; Matti Vaittinen
>> <[email protected]>; Matthias Brugger
>> <[email protected]>
>> Subject: [PATCH v2 1/3] regulator: mt6360: Add OF match table
>>
>> From: Matthias Brugger <[email protected]>
>>
>> Binding documentation mentions that a compatible is required for the
>> MT6360 device node, but the driver doesn't provide a OF match table.
>>
>> Fixes: d321571d5e4c ("regulator: mt6360: Add support for MT6360 regulator")
>> Signed-off-by: Matthias Brugger <[email protected]>
>>
>> ---
>>
>> Changes in v2:
>> - check for CONFIG_OF
>> - add Fixes tag
>>
>> drivers/regulator/mt6360-regulator.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/regulator/mt6360-regulator.c
>> b/drivers/regulator/mt6360-regulator.c
>> index 15308ee29c13..f7b2514feabf 100644
>> --- a/drivers/regulator/mt6360-regulator.c
>> +++ b/drivers/regulator/mt6360-regulator.c
>> @@ -445,9 +445,18 @@ static const struct platform_device_id
>> mt6360_regulator_id_table[] = { }; MODULE_DEVICE_TABLE(platform,
>> mt6360_regulator_id_table);
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id mt6360_of_match[] = {
>> +{ .compatible = "mediatek,mt6360-regulator", },
>> +{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, mt6360_of_match); #endif
>> +
>> static struct platform_driver mt6360_regulator_driver = {
>> .driver = {
>> .name = "mt6360-regulator",
>> +.of_match_table = of_match_ptr(mt6360_of_match),
>> },
>> .probe = mt6360_regulator_probe,
>> .id_table = mt6360_regulator_id_table,
>> --
>> 2.29.2
> ************* Email Confidentiality Notice ********************
>
> The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!
>
On Sat, Jan 09, 2021 at 12:26:09PM +0100, [email protected] wrote:
> From: Matthias Brugger <[email protected]>
>
> Binding documentation mentions that a compatible is required for the
> MT6360 device node, but the driver doesn't provide a OF match table.
The binding should be fixed to remove the requirement for a compatible
here, this is both redundant since we already know we have a mt6380 from
the core MFD and encoding details of how Linux implements things into
the DT bindings.
On Mon, 2021-01-11 at 13:38 +0100, Matthias Brugger wrote:
> Hi Matti,
>
> On 11/01/2021 11:32, Vaittinen, Matti wrote:
> > Hello Matthias & All,
> >
> > On Mon, 2021-01-11 at 11:08 +0100, Matthias Brugger wrote:
> > > On 11/01/2021 03:18, gene_chen(陳俊宇) wrote:
> > > > [ Internal Use - External ]
> > > >
> > >
> > > Please don't top-post in the future.
> > >
> > > > Hi Matthias,
> > > >
> > > > I discussed OF match table with Mark in previous mail in our
> > > > PATCH
> > > > v3,
> > > > MFD should just instantiate the platform device.
> > >
> > > Did you ever test that? Which MFD driver should instantiate the
> > > device?
> > > I had a look at mt6360-core.c (the obvious one) but I don't see
> > > any
> > > reference to
> > > the regulator [1]. What am I missing?
> > >
> > > > > Mark Brown <[email protected]> 於 2020年8月20日 週四 下午7:45寫道：
> > > > > > This device only exists in the context of a single parent
> > > > > > device, there
> > > > > > should be no need for a compatible string here - this is
> > > > > > just a
> > > > > > detail
> > > > > > of how Linux does things. The MFD should just instntiate
> > > > > > the
> > > > > > platform
> > > > > > device.
> > > > > Trying to autoload module without of_id_table will cause run-
> > > > > time
> > > > > error:
> > > > > ueventd: LoadWithAliases was unable to load
> > > > > of:NregulatorT(null)Cmediatek,mt6360-regulator
> > > >
> > > > You shouldn't have this described in the device tree at all,
> > > > like I
> > > > say
> > > > the MFD should just instantiate the platform device.
> > > >
> > >
> > > Well from my understanding the regulator has a device-tree entry
> > > [2],
> > > so it
> > > needs to match against a device-tree node. My understanding is,
> > > that
> > > you need a
> > > the devicetree node to describe the regulators provided by the
> > > device. TBH I'm a
> > > bit puzzled about the comment from Mark here. How does another DT
> > > node be able
> > > to reference a regulator if this is not described in DT? I think
> > > we
> > > need a DT
> > > node here and the matching in the regulator and MFD driver to get
> > > the
> > > regulator
> > > loaded via udev.
> >
> > Others are better to answer - but as I spotted this from my inbox
> > I'll
> > give my 2 cents :)
> >
> > This can be done W/O regulators having own compatible. Please see
> > following:
> >
> > drivers/mfd/rohm-bd718x7.c
> > drivers/regulator/bd718x7-regulator.c
> > Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> > Documentation/devicetree/bindings/regulator/rohm,bd71847-
> > regulator.yaml
> >
> > as example.
>
> Thanks for the links. Yes that's' a way to go, but...
>
> > The device matching can be done via platform_device_id table.
> > I think the MODULE_ALIAS() is needed for module matching - but I
> > don't
> > remember without further code browsing.
> >
> > Anyways, the BD71837/47/50 DT entry without compatible for
> > clk/regulators do probe and load the sub-devices.
> >
> > As a "tradeoff" subdevices must retrieve the DT node from the
> > parent
> > device.
> >
> > For my uneducated eyes the DT binding for regulators should be
> > changed.
> > Compatible should not be required and the example node should be
> > moved
> > to MFD binding document in the MFD node. But that's just my view on
> > this - not willing to push this to any direction!
> >
>
> Generally speaking, DT bindings are stable and can't be changed in a
> none-compatible way. We could argue if there is any user of the
> compatible out
> there (probably there isn't any, as the code seems to not even be
> working).
I think this depends on the device where driver(s) are used. Quite a
few devices allow updating the device-tree when SW is updated. But as I
said, I don't want to try pushing this one direction or other - I leave
it for those who have broader shoulders for pushing XD.
I was merely trying to answer your question :)
Best Regards
Matti
On Mon, Jan 11, 2021 at 04:41:18PM +0000, Mark Brown wrote:
> On Sat, Jan 09, 2021 at 12:26:09PM +0100, [email protected] wrote:
> > From: Matthias Brugger <[email protected]>
> >
> > Binding documentation mentions that a compatible is required for the
> > MT6360 device node, but the driver doesn't provide a OF match table.
>
> The binding should be fixed to remove the requirement for a compatible
> here, this is both redundant since we already know we have a mt6380 from
> the core MFD and encoding details of how Linux implements things into
> the DT bindings.
Ok, I'll leave that to the driver author to fix this.
Just be aware that there is series for the MFD part which adds
OF_MFD_CELL for the regulator.
Regards,
Matthias
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek