| Message ID | 20200220145127.21273-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/3] regulator: max14577: Add proper dt-compatible strings | expand |
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.
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
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.
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
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.
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
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
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
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
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",
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(+)