| Message ID | 20230807131256.254243-3-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] media: dt-bindings: samsung,exynos4212-fimc-is: replace duplicate pmu node with phandle | expand |
Hi Krzysztof, [...] > +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) > +{ > + struct device_node *node; > + void __iomem *regs; > + > + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); > + if (!node) { > + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); > + node = of_get_child_by_name(dev->of_node, "pmu"); > + if (!node) > + return IOMEM_ERR_PTR(-ENODEV); in my opinion this should be: ... if (!node) return IOMEM_ERR_PTR(-ENODEV); dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); Because if you don't have both "samsung,pmu-syscon and "pmu" then the warning should not be printed and you need to return -ENODEV. ... and... "*please* update your DTB", the user might get upset and out of sheer spite, decides not to do it – just because! :) Andi > + }
On 08/08/2023 01:13, Andi Shyti wrote: > Hi Krzysztof, > > [...] > >> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) >> +{ >> + struct device_node *node; >> + void __iomem *regs; >> + >> + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); >> + if (!node) { >> + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >> + node = of_get_child_by_name(dev->of_node, "pmu"); >> + if (!node) >> + return IOMEM_ERR_PTR(-ENODEV); > > in my opinion this should be: > > ... > if (!node) > return IOMEM_ERR_PTR(-ENODEV); > > dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); > > Because if you don't have both "samsung,pmu-syscon and "pmu" then > the warning should not be printed and you need to return -ENODEV. Why not? Warning is correct - the driver is trying to find, thus continuous tense "Finding", PMU node via old method. > > ... and... "*please* update your DTB", the user might get upset > and out of sheer spite, decides not to do it – just because! :) The message is already long enough, why making it longer? Anyone who ships DTS outside of Linux kernel is doomed anyway... Best regards, Krzysztof
> >> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) > >> +{ > >> + struct device_node *node; > >> + void __iomem *regs; > >> + > >> + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); > >> + if (!node) { > >> + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); > >> + node = of_get_child_by_name(dev->of_node, "pmu"); > >> + if (!node) > >> + return IOMEM_ERR_PTR(-ENODEV); > > > > in my opinion this should be: > > > > ... > > if (!node) > > return IOMEM_ERR_PTR(-ENODEV); > > > > dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); > > > > Because if you don't have both "samsung,pmu-syscon and "pmu" then > > the warning should not be printed and you need to return -ENODEV. > > Why not? Warning is correct - the driver is trying to find, thus > continuous tense "Finding", PMU node via old method. Alright, I'll go along with what you're suggesting, but I have to say, I find it misleading. From what I understand, you're requesting an update to the dtb because it's using deprecated methods. However, the reality might be that the node is not present in any method at all. Your statement would be accurate if you failed to find the previous method but then did end up finding it. Relying on the present continuous tense for clarity is a bold move, don't you think? :) Andi > > ... and... "*please* update your DTB", the user might get upset > > and out of sheer spite, decides not to do it – just because! :) > > The message is already long enough, why making it longer? Anyone who > ships DTS outside of Linux kernel is doomed anyway... > > Best regards, > Krzysztof >
On 08/08/2023 13:42, Andi Shyti wrote: >>>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) >>>> +{ >>>> + struct device_node *node; >>>> + void __iomem *regs; >>>> + >>>> + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); >>>> + if (!node) { >>>> + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >>>> + node = of_get_child_by_name(dev->of_node, "pmu"); >>>> + if (!node) >>>> + return IOMEM_ERR_PTR(-ENODEV); >>> >>> in my opinion this should be: >>> >>> ... >>> if (!node) >>> return IOMEM_ERR_PTR(-ENODEV); >>> >>> dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >>> >>> Because if you don't have both "samsung,pmu-syscon and "pmu" then >>> the warning should not be printed and you need to return -ENODEV. >> >> Why not? Warning is correct - the driver is trying to find, thus >> continuous tense "Finding", PMU node via old method. > > Alright, I'll go along with what you're suggesting, but I have to > say, I find it misleading. > > From what I understand, you're requesting an update to the dtb > because it's using deprecated methods. However, the reality might > be that the node is not present in any method at all. > > Your statement would be accurate if you failed to find the > previous method but then did end up finding it. > > Relying on the present continuous tense for clarity is a bold > move, don't you think? :) I just don't think it matters and is not worth resending. Best regards, Krzysztof
Hi Krzysztof, On 08/08/2023 01:13, Andi Shyti wrote: > Hi Krzysztof, > > [...] > >> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) >> +{ >> + struct device_node *node; >> + void __iomem *regs; >> + >> + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); >> + if (!node) { >> + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >> + node = of_get_child_by_name(dev->of_node, "pmu"); >> + if (!node) >> + return IOMEM_ERR_PTR(-ENODEV); > > in my opinion this should be: > > ... > if (!node) > return IOMEM_ERR_PTR(-ENODEV); > > dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); > > Because if you don't have both "samsung,pmu-syscon and "pmu" then > the warning should not be printed and you need to return -ENODEV. I agree with Andi for this part. The only time you want to see this message is if samsung,pmu-syscon is missing AND pmu is present. If both are missing, then just return ENODEV as it was before. > > ... and... "*please* update your DTB", the user might get upset > and out of sheer spite, decides not to do it – just because! :) I don't care about this bit. I guess it doesn't hurt to add 'please', but I accept it either way. Regards, Hans > > Andi > >> + }
On 11/08/2023 11:49, Hans Verkuil wrote: > Hi Krzysztof, > > On 08/08/2023 01:13, Andi Shyti wrote: >> Hi Krzysztof, >> >> [...] >> >>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) >>> +{ >>> + struct device_node *node; >>> + void __iomem *regs; >>> + >>> + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); >>> + if (!node) { >>> + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >>> + node = of_get_child_by_name(dev->of_node, "pmu"); >>> + if (!node) >>> + return IOMEM_ERR_PTR(-ENODEV); >> >> in my opinion this should be: >> >> ... >> if (!node) >> return IOMEM_ERR_PTR(-ENODEV); >> >> dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); >> >> Because if you don't have both "samsung,pmu-syscon and "pmu" then >> the warning should not be printed and you need to return -ENODEV. > > I agree with Andi for this part. > > The only time you want to see this message is if samsung,pmu-syscon is > missing AND pmu is present. If both are missing, then just return ENODEV as > it was before. OK, understood. I will send a v3. Best regards, Krzysztof
diff --git a/drivers/media/platform/samsung/exynos4-is/fimc-is.c b/drivers/media/platform/samsung/exynos4-is/fimc-is.c index 530a148fe4d3..c995b1226ca3 100644 --- a/drivers/media/platform/samsung/exynos4-is/fimc-is.c +++ b/drivers/media/platform/samsung/exynos4-is/fimc-is.c @@ -767,12 +767,32 @@ static void fimc_is_debugfs_create(struct fimc_is *is) static int fimc_is_runtime_resume(struct device *dev); static int fimc_is_runtime_suspend(struct device *dev); +static void __iomem *fimc_is_get_pmu_regs(struct device *dev) +{ + struct device_node *node; + void __iomem *regs; + + node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0); + if (!node) { + dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n"); + node = of_get_child_by_name(dev->of_node, "pmu"); + if (!node) + return IOMEM_ERR_PTR(-ENODEV); + } + + regs = of_iomap(node, 0); + of_node_put(node); + if (!regs) + return IOMEM_ERR_PTR(-ENOMEM); + + return regs; +} + static int fimc_is_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct fimc_is *is; struct resource res; - struct device_node *node; int ret; is = devm_kzalloc(&pdev->dev, sizeof(*is), GFP_KERNEL); @@ -794,14 +814,9 @@ static int fimc_is_probe(struct platform_device *pdev) if (IS_ERR(is->regs)) return PTR_ERR(is->regs); - node = of_get_child_by_name(dev->of_node, "pmu"); - if (!node) - return -ENODEV; - - is->pmu_regs = of_iomap(node, 0); - of_node_put(node); - if (!is->pmu_regs) - return -ENOMEM; + is->pmu_regs = fimc_is_get_pmu_regs(dev); + if (IS_ERR(is->pmu_regs)) + return PTR_ERR(is->pmu_regs); is->irq = irq_of_parse_and_map(dev->of_node, 0); if (!is->irq) {
Devicetree for the FIMC IS camera included duplicated PMU node as its child like: soc@0 { system-controller@10020000 { ... }; // Real PMU camera@11800000 { fimc-is@12000000 { // FIMC IS camera node pmu@10020000 { reg = <0x10020000 0x3000>; // Fake PMU node }; }; }; }; This is not a correct representation of the hardware. Mapping the PMU (Power Management Unit) IO memory should be via syscon-like phandle (samsung,pmu-syscon, already used for other drivers), not by duplicating "pmu" Devicetree node inside the FIMC IS. Backward compatibility is preserved. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Changes in v2: 1. Use IOMEM_ERR_PTR (Hans) --- .../platform/samsung/exynos4-is/fimc-is.c | 33 ++++++++++++++----- 1 file changed, 24 insertions(+), 9 deletions(-)