Hello Gregory,
Thank you for reviewing the series.
On Mon, 18 Dec 2017 17:05:07 +0100
Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> wrote:
> Hi Miquel,
>
> On lun., déc. 18 2017, Miquel Raynal
> <miquel.raynal@xxxxxxxxxxxxxxxxxx> wrote:
>
> > From: Baruch Siach <baruch@xxxxxxxxxx>
> >
> > The AP806 component is integrated in the Armada 8K and 7K lines of
> > processors.
> >
> > The thermal sensor sample field on the status register is a signed
> > value. Extend armada_get_temp() and the driver structure to handle
> > signed values.
> >
> > Signed-off-by: Baruch Siach <baruch@xxxxxxxxxx>
> > [<miquel.raynal@xxxxxxxxxxxxxxxxxx>: Changes when applying over the
> > previous patches, including the register names changes, also
> > switched the coefficients values to s64 instead of unsigned long to
> > deal with negative values and used do_div instead of the
> > traditionnal '/'] Signed-off-by: Miquel Raynal
> > <miquel.raynal@xxxxxxxxxxxxxxxxxx>
>
> I am just a little concerned by the fac that the value pass though
> get_temp() is an int and now the intermediate calculation are in
> s64. But maybe I'm too picky for this,
I know, but I thought having a temporary s64 variable and casting it
when setting *temp would have been exactly the same so I decided to let
it that way, but if you have something in mind (maybe a check that a
cast into a smaller type is valid? or limiting the temperature to
some boundaries?) I can update.
Thanks,
Miquèl
>
> Reviewed-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
>
> Gregory
>
>
> > ---
> > drivers/thermal/armada_thermal.c | 80
> > ++++++++++++++++++++++++++++++++-------- 1 file changed, 65
> > insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/thermal/armada_thermal.c
> > b/drivers/thermal/armada_thermal.c index 198485fa77f2..ec29ea76b818
> > 100644 --- a/drivers/thermal/armada_thermal.c
> > +++ b/drivers/thermal/armada_thermal.c
> > @@ -47,6 +47,11 @@
> > #define CONTROL0_OFFSET 0x0
> > #define CONTROL1_OFFSET 0x4
> >
> > +/* TSEN refers to the temperature sensors within the AP */
> > +#define CONTROL0_TSEN_START BIT(0)
> > +#define CONTROL0_TSEN_RESET BIT(1)
> > +#define CONTROL0_TSEN_ENABLE BIT(2)
> > +
> > struct armada_thermal_data;
> >
> > /* Marvell EBU Thermal Sensor Dev Structure */
> > @@ -66,15 +71,17 @@ struct armada_thermal_data {
> > bool (*is_valid)(struct armada_thermal_priv *);
> >
> > /* Formula coeficients: temp = (b - m * reg) / div */
> > - unsigned long coef_b;
> > - unsigned long coef_m;
> > - unsigned long coef_div;
> > + s64 coef_b;
> > + s64 coef_m;
> > + u32 coef_div;
> > bool inverted;
> > + bool signed_sample;
> >
> > /* Register shift and mask to access the sensor
> > temperature */ unsigned int temp_shift;
> > unsigned int temp_mask;
> > u32 is_valid_bit;
> > + bool needs_control0;
> > };
> >
> > static void armadaxp_init_sensor(struct platform_device *pdev,
> > @@ -154,6 +161,18 @@ static void armada380_init_sensor(struct
> > platform_device *pdev, }
> > }
> >
> > +static void armada_ap806_init_sensor(struct platform_device *pdev,
> > + struct armada_thermal_priv
> > *priv) +{
> > + u32 reg;
> > +
> > + reg = readl_relaxed(priv->control0);
> > + reg &= ~CONTROL0_TSEN_RESET;
> > + reg |= CONTROL0_TSEN_START | CONTROL0_TSEN_ENABLE;
> > + writel(reg, priv->control0);
> > + msleep(10);
> > +}
> > +
> > static bool armada_is_valid(struct armada_thermal_priv *priv)
> > {
> > u32 reg = readl_relaxed(priv->status);
> > @@ -165,8 +184,8 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, int *temp)
> > {
> > struct armada_thermal_priv *priv = thermal->devdata;
> > - unsigned long reg;
> > - unsigned long m, b, div;
> > + u32 reg, div;
> > + s64 sample, b, m;
> >
> > /* Valid check */
> > if (priv->data->is_valid && !priv->data->is_valid(priv)) {
> > @@ -177,6 +196,11 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal,
> > reg = readl_relaxed(priv->status);
> > reg = (reg >> priv->data->temp_shift) &
> > priv->data->temp_mask;
> > + if (priv->data->signed_sample)
> > + /* The most significant bit is the sign bit */
> > + sample = sign_extend32(reg,
> > fls(priv->data->temp_mask) - 1);
> > + else
> > + sample = reg;
> >
> > /* Get formula coeficients */
> > b = priv->data->coef_b;
> > @@ -184,9 +208,12 @@ static int armada_get_temp(struct
> > thermal_zone_device *thermal, div = priv->data->coef_div;
> >
> > if (priv->data->inverted)
> > - *temp = ((m * reg) - b) / div;
> > + *temp = (m * sample) - b;
> > else
> > - *temp = (b - (m * reg)) / div;
> > + *temp = b - (m * sample);
> > +
> > + do_div(*temp, div);
> > +
> > return 0;
> > }
> >
> > @@ -198,8 +225,8 @@ static const struct armada_thermal_data
> > armadaxp_data = { .init_sensor = armadaxp_init_sensor,
> > .temp_shift = 10,
> > .temp_mask = 0x1ff,
> > - .coef_b = 3153000000UL,
> > - .coef_m = 10000000UL,
> > + .coef_b = 3153000000ULL,
> > + .coef_m = 10000000ULL,
> > .coef_div = 13825,
> > };
> >
> > @@ -209,8 +236,8 @@ static const struct armada_thermal_data
> > armada370_data = { .is_valid_bit = BIT(9),
> > .temp_shift = 10,
> > .temp_mask = 0x1ff,
> > - .coef_b = 3153000000UL,
> > - .coef_m = 10000000UL,
> > + .coef_b = 3153000000ULL,
> > + .coef_m = 10000000ULL,
> > .coef_div = 13825,
> > };
> >
> > @@ -220,8 +247,8 @@ static const struct armada_thermal_data
> > armada375_data = { .is_valid_bit = BIT(10),
> > .temp_shift = 0,
> > .temp_mask = 0x1ff,
> > - .coef_b = 3171900000UL,
> > - .coef_m = 10000000UL,
> > + .coef_b = 3171900000ULL,
> > + .coef_m = 10000000ULL,
> > .coef_div = 13616,
> > };
> >
> > @@ -231,12 +258,26 @@ static const struct armada_thermal_data
> > armada380_data = { .is_valid_bit = BIT(10),
> > .temp_shift = 0,
> > .temp_mask = 0x3ff,
> > - .coef_b = 1172499100UL,
> > - .coef_m = 2000096UL,
> > + .coef_b = 1172499100ULL,
> > + .coef_m = 2000096ULL,
> > .coef_div = 4201,
> > .inverted = true,
> > };
> >
> > +static const struct armada_thermal_data armada_ap806_data = {
> > + .is_valid = armada_is_valid,
> > + .init_sensor = armada_ap806_init_sensor,
> > + .is_valid_bit = BIT(16),
> > + .temp_shift = 0,
> > + .temp_mask = 0x3ff,
> > + .coef_b = -150000LL,
> > + .coef_m = 423ULL,
> > + .coef_div = 1,
> > + .inverted = true,
> > + .signed_sample = true,
> > + .needs_control0 = true,
> > +};
> > +
> > static const struct of_device_id armada_thermal_id_table[] = {
> > {
> > .compatible = "marvell,armadaxp-thermal",
> > @@ -255,6 +296,10 @@ static const struct of_device_id
> > armada_thermal_id_table[] = { .data = &armada380_data,
> > },
> > {
> > + .compatible = "marvell,armada-ap806-thermal",
> > + .data = &armada_ap806_data,
> > + },
> > + {
> > /* sentinel */
> > },
> > };
> > @@ -296,6 +341,11 @@ static int armada_thermal_probe(struct
> > platform_device *pdev) */
> > if (resource_size(res) == LEGACY_CONTROL_MEM_LEN) {
> > /* ->control0 unavailable in this configuration */
> > + if (priv->data->needs_control0) {
> > + dev_err(&pdev->dev, "No access to control0
> > register\n");
> > + return -EINVAL;
> > + }
> > +
> > priv->control1 = control + LEGACY_CONTROL1_OFFSET;
> > } else {
> > priv->control0 = control + CONTROL0_OFFSET;
> > --
> > 2.11.0
> >
>
--
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
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
