| Message ID | 20180625151239.20976-19-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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 >
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
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
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
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
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
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
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 --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
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(+)