Hi Miquel,
On jeu., déc. 14 2017, Miquel RAYNAL <miquel.raynal@xxxxxxxxxxxxxxxxxx> wrote:
> 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).
Actually I was concerned by the "once" for me it means "only one time",
but maybe it just meant it was useless to reset it again but not
harmful.
Gregory
>
>>
>> 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
>
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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
