On Thu, 14 Dec 2017 12:11:49 +0100
Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Miquel,
>
> On jeu., déc. 14 2017, Miquel Raynal
> <miquel.raynal@xxxxxxxxxxxxxxxxxx> wrote:
>
> > From: Baruch Siach <baruch@xxxxxxxxxx>
> >
> > The CP110 component is integrated in the Armada 8k and 7k lines of
> > processors.
> >
> > Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>
> > [<miquel.raynal@xxxxxxxxxxxxxxxxxx>: renamed the register
> > pointers]
>
> Actually you did more thant this see below
>
>
> > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/thermal/armada_thermal.c | 30
> > ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6
> > deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 279d01937bb8..f5c911524656
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -37,7 +37,6 @@
> > #define A375_UNIT_CONTROL_MASK 0x7
> > #define A375_READOUT_INVERT BIT(15)
> > #define A375_HW_RESETn BIT(8)
> > -#define A380_HW_RESET BIT(8)
> >
> > /* Legacy bindings */
> > #define LEGACY_CONTROL_MEM_LEN 0x4
> > @@ -52,6 +51,10 @@
> > #define CONTROL0_TSEN_RESET BIT(1)
> > #define CONTROL0_TSEN_ENABLE BIT(2)
> >
> > +/* EXT_TSEN refers to the external temperature sensors, out of the
> > AP */ +#define CONTROL1_EXT_TSEN_SW_RESET BIT(7)
> > +#define CONTROL1_EXT_TSEN_HW_RESETn BIT(8)
> You added or rename these values
>
> > +
> > struct armada_thermal_data;
> >
> > /* Marvell EBU Thermal Sensor Dev Structure */
> > @@ -153,11 +156,10 @@ static void armada380_init_sensor(struct
> > platform_device *pdev, u32 reg = readl_relaxed(priv->control1);
> >
> > /* Reset hardware once */
> > - if (!(reg & A380_HW_RESET)) {
> > - reg |= A380_HW_RESET;
> > - writel(reg, priv->control1);
> > - msleep(10);
> > - }
> > + reg |= CONTROL1_EXT_TSEN_HW_RESETn;
> > + reg &= ~CONTROL1_EXT_TSEN_SW_RESET;
> > + writel(reg, priv->control1);
>
> And here you modified the behavior of this function.
> Did you checked that it is valid for Armada 38x?
There is nothing about it the documentation and anyway this register
can be accessed later, so writing it is harmless ayway.
>
> Given the comment we had, I thought we should not do anything if
> CONTROL1_EXT_TSEN_HW_RESETn was not set.
That is the opposite, if it is not set (ie. reset is active), you have
to set it (reset is then disabled).
>
> By the way, if the new sequence is valid, this comment should be
> removed or at least updated.
That's right, I will in v4.
Thanks for reviewing,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
