Re: [PATCH] thermal: rockchip: fix handling of invalid readings

[Caesar Wang]

Hello Eduardo,

I guess you forgot to review/pick up this series patchs.

Another patch as follows:

[PATCH] thermal: rockhip: fix setting thermal shutdown polarity
https://patchwork.kernel.org/patch/6973131/

----
Thanks,
Caesar

å 2015å08æ10æ 18:22, Caesar Wang åé:
> å 2015å08æ10æ 15:59, Dmitry Torokhov åé:
> > Hi Caesar,
> >
> > On Sun, Aug 9, 2015 at 11:30 PM, Caesar Wang <wxt@xxxxxxxxxxxxxx> wrote:
> > > Dear Dmitry,
> > >
> > > Thanks your patch.
> > >
> > > The code looks like fine,but I don't think the TS-ADC will work on 
> > > rk3288
> > > SoC.
> > What do you mean? It seems to work when I ran it...
>
> Making a mistake.:-(
>
> > > å 2015å08æ08æ 04:59, Dmitry Torokhov åé:
> > > > We attempted to signal invalid code by returning -EAGAIN from
> > > > rk_tsadcv2_code_to_temp(), unfortunately the return value was stuffed
> > > > directly into the temperature pointer, potentially confusing upper
> > > > layers with temperature of -EINVAL.
> > > >
> > > > Let's split temperature from error/success indicator to avoid such
> > > > confusion.
> > > >
> > > > Also change the way we scan the temperature table to start with the 
> > > > 2nd
> > > > element so that we do not need to worry that we may reference out of
> > > > bounds element while doing binary search and keep checking that we end
> > > > up with 'mid' equal to 0 (since we are looking for the temperature 
> > > > that
> > > > would fall into interval between the 'mid' and 'mid - 1') .
> > > >
> > > > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxxxx>
> > > > ---
> > > >    drivers/thermal/rockchip_thermal.c | 28 
> > > > +++++++++++++---------------
> > > >    1 file changed, 13 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/rockchip_thermal.c
> > > > b/drivers/thermal/rockchip_thermal.c
> > > > index c89ffb2..93ee307 100644
> > > > --- a/drivers/thermal/rockchip_thermal.c
> > > > +++ b/drivers/thermal/rockchip_thermal.c
> > > > @@ -124,7 +124,7 @@ struct rockchip_thermal_data {
> > > >    #define TSADCV2_AUTO_PERIOD_HT_TIME           50  /* msec */
> > > >      struct tsadc_table {
> > > > -       unsigned long code;
> > > > +       u32 code;
> > > >          long temp;
> > > >    };
> > > >    @@ -164,7 +164,6 @@ static const struct tsadc_table 
> > > > v2_code_table[] = {
> > > >          {3452, 115000},
> > > >          {3437, 120000},
> > > >          {3421, 125000},
> > > > -       {0, 125000},
> > > >    };
> > > >      static u32 rk_tsadcv2_temp_to_code(long temp)
> > > > @@ -191,19 +190,21 @@ static u32 rk_tsadcv2_temp_to_code(long temp)
> > > >          return 0;
> > > >    }
> > > >    -static int rk_tsadcv2_code_to_temp(u32 code)
> > > > +static int rk_tsadcv2_code_to_temp(u32 code, int *temp)
>
> It's wired that can't git-am this patch.
>
> I search the Eduardo'branch[0] for rockchip-thermal.c driver. The 
> function is "static long rk_tsadcv2_code_to_temp(u32 code)"
>
> Maybe i'm missing something, I verified this patch on my board but 
> this fixed.
>
> So you can free add it:
> Tested-by: Caesar Wang <wxt@xxxxxxxxxxxxxx>
>
> [0]:
> url = 
> git://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git
>
>
>
> ---
> Thanks,
> Caesar
>
>
>
> > > >    {
> > > > -       unsigned int low = 0;
> > > > +       unsigned int low = 1;
> > > >          unsigned int high = ARRAY_SIZE(v2_code_table) - 1;
> > > >          unsigned int mid = (low + high) / 2;
> > > >          unsigned int num;
> > > >          unsigned long denom;
> > > >    -     /* Invalid code, return -EAGAIN */
> > > > -       if (code > TSADCV2_DATA_MASK)
> > > > -               return -EAGAIN;
> > > > +       BUILD_BUG_ON(ARRAY_SIZE(v2_code_table) < 2);
> > > >    -     while (low <= high && mid) {
> > > > +       code &= TSADCV2_DATA_MASK;
> > > > +       if (code < v2_code_table[high].code)
> > > > +               return -EAGAIN;         /* Incorrect reading */
> > > > +
> > > > +       while (low <= high) {
> > > >                  if (code >= v2_code_table[mid].code &&
> > > >                      code < v2_code_table[mid - 1].code)
> > > >                          break;
> > > > @@ -223,7 +224,9 @@ static int rk_tsadcv2_code_to_temp(u32 code)
> > > >          num = v2_code_table[mid].temp - v2_code_table[mid - 1].temp;
> > > >          num *= v2_code_table[mid - 1].code - code;
> > > >          denom = v2_code_table[mid - 1].code - 
> > > > v2_code_table[mid].code;
> > > > -       return v2_code_table[mid - 1].temp + (num / denom);
> > > > +       *temp = v2_code_table[mid - 1].temp + (num / denom);
> > > > +
> > > > +       return 0;
> > > >    }
> > > >      /**
> > > > @@ -281,14 +284,9 @@ static int rk_tsadcv2_get_temp(int chn, void 
> > > > __iomem
> > > > *regs, int *temp)
> > > >    {
> > > >          u32 val;
> > > >    -     /* the A/D value of the channel last conversion need some 
> > > > time */
> > > >          val = readl_relaxed(regs + TSADCV2_DATA(chn));
> > > > -       if (val == 0)
> > > > -               return -EAGAIN;
> > > > -
> > >
> > > if we reserve the above code, that will get the ADC value.
> > But if we pass  0 into rk_tsadcv2_code_to_temp() it will trigger:
> >
> > > > +       if (code < v2_code_table[high].code)
> > > > +               return -EAGAIN;         /* Incorrect reading */
> > and still return -EAGAIN, as it was. There is no behavior change as
> > far as I can see.
> Yup, that's ok for me.
>
> > > > -       *temp = rk_tsadcv2_code_to_temp(val);
> > > >    -     return 0;
> > > > +       return rk_tsadcv2_code_to_temp(val, temp);
> > > >    }
> > > >      static void rk_tsadcv2_tshut_temp(int chn, void __iomem *regs, 
> > > > long
> > > > temp)
> > Thanks,
> > Dmitry


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/