diff mbox

[v2,18/23] dt-bindings: thermal: armada: add reference to new bindings

Message ID 20180625151239.20976-19-miquel.raynal@bootlin.com
State New, archived
Headers show

Commit Message

Miquel Raynal June 25, 2018, 3:12 p.m. UTC
New bindings (with the syscon and the overheat interrupt) are available
for AP806 and CP110 compatibles. Add a reference to these files from the
original documentation.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Rob Herring July 3, 2018, 9:30 p.m. UTC | #1
On Mon, Jun 25, 2018 at 05:12:34PM +0200, Miquel Raynal wrote:
> New bindings (with the syscon and the overheat interrupt) are available

What interrupt? It's not in your new binding.

> for AP806 and CP110 compatibles. Add a reference to these files from the
> original documentation.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> index e0d013a2e66d..f3b441100890 100644
> --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> @@ -10,6 +10,11 @@ Required properties:
>      * marvell,armada-ap806-thermal
>      * marvell,armada-cp110-thermal

Really you should not be using the same compatible for both. Now you 
have 2 drivers matching to same compatibles.

Can't you make this a child of the syscon without breaking the binding?

>  
> +Note: these bindings are deprecated for AP806/CP110 and should instead
> +follow the rules described in:
> +Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> +Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> +
>  - reg: Device's register space.
>    Two entries are expected, see the examples below. The first one points
>    to the status register (4B). The second one points to the control
> -- 
> 2.14.1
>
Miquel Raynal July 5, 2018, 10:05 a.m. UTC | #2
Hi Rob,

Rob Herring <robh@kernel.org> wrote on Tue, 3 Jul 2018 15:30:11 -0600:

> On Mon, Jun 25, 2018 at 05:12:34PM +0200, Miquel Raynal wrote:
> > New bindings (with the syscon and the overheat interrupt) are available  
> 
> What interrupt? It's not in your new binding.
> 
> > for AP806 and CP110 compatibles. Add a reference to these files from the
> > original documentation.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > index e0d013a2e66d..f3b441100890 100644
> > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > @@ -10,6 +10,11 @@ Required properties:
> >      * marvell,armada-ap806-thermal
> >      * marvell,armada-cp110-thermal  
> 
> Really you should not be using the same compatible for both. Now you 
> have 2 drivers matching to same compatibles.
> 
> Can't you make this a child of the syscon without breaking the binding?

We are talking about only 1 driver here, so I'm not sure how I should
understand your last sentence. Do you want me to add a second
compatible (for the same piece of hardware) for thermal node declared
as a child of the syscon?

I could handle this situation in the driver by creating the missing
regmap in case we are using the old compatible. If this is the solution
you prefer, how should I name the new compatibles ?

> 
> >  
> > +Note: these bindings are deprecated for AP806/CP110 and should instead
> > +follow the rules described in:
> > +Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> > +Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> > +
> >  - reg: Device's register space.
> >    Two entries are expected, see the examples below. The first one points
> >    to the status register (4B). The second one points to the control
> > -- 
> > 2.14.1
> >   

Thanks,
Miquèl
Rob Herring July 5, 2018, 6:03 p.m. UTC | #3
On Thu, Jul 5, 2018 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> Rob Herring <robh@kernel.org> wrote on Tue, 3 Jul 2018 15:30:11 -0600:
>
> > On Mon, Jun 25, 2018 at 05:12:34PM +0200, Miquel Raynal wrote:
> > > New bindings (with the syscon and the overheat interrupt) are available
> >
> > What interrupt? It's not in your new binding.

???



> > > for AP806 and CP110 compatibles. Add a reference to these files from the
> > > original documentation.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > index e0d013a2e66d..f3b441100890 100644
> > > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > @@ -10,6 +10,11 @@ Required properties:
> > >      * marvell,armada-ap806-thermal
> > >      * marvell,armada-cp110-thermal
> >
> > Really you should not be using the same compatible for both. Now you
> > have 2 drivers matching to same compatibles.
> >
> > Can't you make this a child of the syscon without breaking the binding?
>
> We are talking about only 1 driver here, so I'm not sure how I should
> understand your last sentence. Do you want me to add a second
> compatible (for the same piece of hardware) for thermal node declared
> as a child of the syscon?

I don't know what I'm suggesting. Your changes look like they break
compatibility to me. What happens if you use this new binding without
any kernel change?

> I could handle this situation in the driver by creating the missing
> regmap in case we are using the old compatible. If this is the solution
> you prefer, how should I name the new compatibles ?
>
> >
> > >
> > > +Note: these bindings are deprecated for AP806/CP110 and should instead
> > > +follow the rules described in:
> > > +Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
> > > +Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> > > +
> > >  - reg: Device's register space.
> > >    Two entries are expected, see the examples below. The first one points
> > >    to the status register (4B). The second one points to the control
> > > --
> > > 2.14.1
> > >
>
> Thanks,
> Miquèl
Miquel Raynal July 6, 2018, 6:54 a.m. UTC | #4
Hi Rob,

Rob Herring <robh@kernel.org> wrote on Thu, 5 Jul 2018 12:03:04 -0600:

> On Thu, Jul 5, 2018 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> > Rob Herring <robh@kernel.org> wrote on Tue, 3 Jul 2018 15:30:11 -0600:
> >  
> > > On Mon, Jun 25, 2018 at 05:12:34PM +0200, Miquel Raynal wrote:  
> > > > New bindings (with the syscon and the overheat interrupt) are available  
> > >
> > > What interrupt? It's not in your new binding.  
> 
> ???

I forgot that point, my bad.

Initially this series introduced an overheat interrupt. Due to some
dependencies I split the original series in two: this one and another
one just to add support for the interrupt.

This is a stale comment I forgot to remove (again in the v3), sorry.

I'll wait for Eduardo's feedback and repost eventually without the
parenthesis.

Thanks,
Miquèl
Miquel Raynal July 6, 2018, 7:49 a.m. UTC | #5
Hi Rob,

[...]

> > > > for AP806 and CP110 compatibles. Add a reference to these files from the
> > > > original documentation.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > index e0d013a2e66d..f3b441100890 100644
> > > > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > @@ -10,6 +10,11 @@ Required properties:
> > > >      * marvell,armada-ap806-thermal
> > > >      * marvell,armada-cp110-thermal  
> > >
> > > Really you should not be using the same compatible for both. Now you
> > > have 2 drivers matching to same compatibles.
> > >
> > > Can't you make this a child of the syscon without breaking the binding?  
> >
> > We are talking about only 1 driver here, so I'm not sure how I should
> > understand your last sentence. Do you want me to add a second
> > compatible (for the same piece of hardware) for thermal node declared
> > as a child of the syscon?  
> 
> I don't know what I'm suggesting. Your changes look like they break
> compatibility to me.

There is code in the driver to handle the legacy non-syscon-ish thermal
node ("armada_thermal_probe_legacy()"). An old DT with a new kernel
would work fine.

> What happens if you use this new binding without any kernel change?

I suppose you mean "during the merge window"? In this case the driver
of the thermal IP will not probe (and spawn an error in the dmesg) with
ap806/cp110 IPs. As the support is pretty new for them, this is maybe
not a huge issue. Also, before the DT changes adding thermal zones,
just retrieving the temperature might be pretty useless. However, it
is always possible to merge the driver changes in 4.19 and the DT
changes in 4.20.


Thanks,
Miquèl
Rob Herring July 6, 2018, 5:51 p.m. UTC | #6
On Fri, Jul 6, 2018 at 12:55 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> Rob Herring <robh@kernel.org> wrote on Thu, 5 Jul 2018 12:03:04 -0600:
>
> > On Thu, Jul 5, 2018 at 4:05 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > Rob Herring <robh@kernel.org> wrote on Tue, 3 Jul 2018 15:30:11 -0600:
> > >
> > > > On Mon, Jun 25, 2018 at 05:12:34PM +0200, Miquel Raynal wrote:
> > > > > New bindings (with the syscon and the overheat interrupt) are available
> > > >
> > > > What interrupt? It's not in your new binding.
> >
> > ???
>
> I forgot that point, my bad.
>
> Initially this series introduced an overheat interrupt. Due to some
> dependencies I split the original series in two: this one and another
> one just to add support for the interrupt.

You can split up the driver support, but please make the binding
complete as possible.

Rob
Rob Herring July 6, 2018, 5:56 p.m. UTC | #7
On Fri, Jul 6, 2018 at 1:49 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> [...]
>
> > > > > for AP806 and CP110 compatibles. Add a reference to these files from the
> > > > > original documentation.
> > > > >
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > index e0d013a2e66d..f3b441100890 100644
> > > > > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > @@ -10,6 +10,11 @@ Required properties:
> > > > >      * marvell,armada-ap806-thermal
> > > > >      * marvell,armada-cp110-thermal
> > > >
> > > > Really you should not be using the same compatible for both. Now you
> > > > have 2 drivers matching to same compatibles.
> > > >
> > > > Can't you make this a child of the syscon without breaking the binding?
> > >
> > > We are talking about only 1 driver here, so I'm not sure how I should
> > > understand your last sentence. Do you want me to add a second
> > > compatible (for the same piece of hardware) for thermal node declared
> > > as a child of the syscon?
> >
> > I don't know what I'm suggesting. Your changes look like they break
> > compatibility to me.
>
> There is code in the driver to handle the legacy non-syscon-ish thermal
> node ("armada_thermal_probe_legacy()"). An old DT with a new kernel
> would work fine.

Okay, that's good.

> > What happens if you use this new binding without any kernel change?
>
> I suppose you mean "during the merge window"? In this case the driver
> of the thermal IP will not probe (and spawn an error in the dmesg) with
> ap806/cp110 IPs. As the support is pretty new for them, this is maybe
> not a huge issue. Also, before the DT changes adding thermal zones,
> just retrieving the temperature might be pretty useless. However, it
> is always possible to merge the driver changes in 4.19 and the DT
> changes in 4.20.

Not the merge window, but distros (SUSE at least) want to be able to
boot with current DTs and their distro (old) kernel. Maybe for this
platform you don't care.

Rob
Miquel Raynal July 16, 2018, 2:32 p.m. UTC | #8
Hi Rob,

> > > > > > for AP806 and CP110 compatibles. Add a reference to these files from the
> > > > > > original documentation.
> > > > > >
> > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/thermal/armada-thermal.txt | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > > index e0d013a2e66d..f3b441100890 100644
> > > > > > --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > > +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
> > > > > > @@ -10,6 +10,11 @@ Required properties:
> > > > > >      * marvell,armada-ap806-thermal
> > > > > >      * marvell,armada-cp110-thermal  
> > > > >
> > > > > Really you should not be using the same compatible for both. Now you
> > > > > have 2 drivers matching to same compatibles.
> > > > >
> > > > > Can't you make this a child of the syscon without breaking the binding?  
> > > >
> > > > We are talking about only 1 driver here, so I'm not sure how I should
> > > > understand your last sentence. Do you want me to add a second
> > > > compatible (for the same piece of hardware) for thermal node declared
> > > > as a child of the syscon?  
> > >
> > > I don't know what I'm suggesting. Your changes look like they break
> > > compatibility to me.  
> >
> > There is code in the driver to handle the legacy non-syscon-ish thermal
> > node ("armada_thermal_probe_legacy()"). An old DT with a new kernel
> > would work fine.  
> 
> Okay, that's good.
> 
> > > What happens if you use this new binding without any kernel change?  
> >
> > I suppose you mean "during the merge window"? In this case the driver
> > of the thermal IP will not probe (and spawn an error in the dmesg) with
> > ap806/cp110 IPs. As the support is pretty new for them, this is maybe
> > not a huge issue. Also, before the DT changes adding thermal zones,
> > just retrieving the temperature might be pretty useless. However, it
> > is always possible to merge the driver changes in 4.19 and the DT
> > changes in 4.20.  
> 
> Not the merge window, but distros (SUSE at least) want to be able to
> boot with current DTs and their distro (old) kernel. Maybe for this
> platform you don't care.

Indeed, I think we don't care that much for this platform.

Let me resend the series with the 'interrupt' reference removed as I
think it was the last thing to address.

Thanks,
Miquèl
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
index e0d013a2e66d..f3b441100890 100644
--- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt
@@ -10,6 +10,11 @@  Required properties:
     * marvell,armada-ap806-thermal
     * marvell,armada-cp110-thermal
 
+Note: these bindings are deprecated for AP806/CP110 and should instead
+follow the rules described in:
+Documentation/devicetree/bindings/arm/marvell/ap806-system-controller.txt
+Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
+
 - reg: Device's register space.
   Two entries are expected, see the examples below. The first one points
   to the status register (4B). The second one points to the control