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 <[email protected]>
---
Changes in v2:
1. Use IOMEM_ERR_PTR (Hans)
---
.../platform/samsung/exynos4-is/fimc-is.c | 33 ++++++++++++++-----
1 file changed, 24 insertions(+), 9 deletions(-)
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) {
--
2.34.1
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 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
> >> +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 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
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
>
>> + }