From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752895AbdGEM2E (ORCPT ); Wed, 5 Jul 2017 08:28:04 -0400 Received: from conssluserg-03.nifty.com ([210.131.2.82]:62783 "EHLO conssluserg-03.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752044AbdGEM2C (ORCPT ); Wed, 5 Jul 2017 08:28:02 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-03.nifty.com v65CRw60026471 X-Nifty-SrcIP: [209.85.161.178] MIME-Version: 1.0 In-Reply-To: References: <1499255419-31684-1-git-send-email-hayashi.kunihiko@socionext.com> <1499255419-31684-3-git-send-email-hayashi.kunihiko@socionext.com> From: Masahiro Yamada Date: Wed, 5 Jul 2017 21:27:57 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 2/2] thermal: uniphier: add UniPhier thermal driver To: Kunihiko Hayashi Cc: rui.zhang@intel.com, Eduardo Valentin , linux-pm@vger.kernel.org, linux-arm-kernel , Linux Kernel Mailing List , Rob Herring , Mark Rutland , Masami Hiramatsu , Jassi Brar Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2017-07-05 21:20 GMT+09:00 Masahiro Yamada : > 2017-07-05 20:50 GMT+09:00 Kunihiko Hayashi : > >> + >> +#define TMOD 0x0928 >> +#define TMOD_MASK GENMASK(9, 0) >> + > [ snip ] >> + >> + /* >> + * The bit[8:0] of TMOD register represents 2's complement value >> + * of temperature in Celsius. Since bit8 of TMOD shows a sign bit, >> + * 32bit temperature value is obtained by sign extension. >> + */ > > > Apparently, this comment does not match your code: > > #define TMOD_MASK GENMASK(9, 0) > > > TMOD_MASK is indicating bit[9:0]. > > > > Digging into the patch history, now I understood what happened. > > > > In v1, you described > #define TMOD_MASK 0x1ff > > This was correct. > > > In v2, you converted it into > #define TMOD_MASK GENMASK(9, 0) > > This was misconversion. It should be GENMASK(8, 0) > > > > Anyway, TMOD_MASK is not used any more. > > >> + *out_temp = sign_extend32(temp, 8) * 1000; > > > Why magic number here? > > /* MSB of the TMOD field is a sign bit */ > *out_temp = sign_extend32(temp, TMOD_WIDTH) * 1000; No. sign_extend32(temp, TMOD_WIDTH - 1) or sign_extend32(temp, TMOD_MSB) or whatever. -- Best Regards Masahiro Yamada From mboxrd@z Thu Jan 1 00:00:00 1970 From: yamada.masahiro@socionext.com (Masahiro Yamada) Date: Wed, 5 Jul 2017 21:27:57 +0900 Subject: [PATCH v3 2/2] thermal: uniphier: add UniPhier thermal driver In-Reply-To: References: <1499255419-31684-1-git-send-email-hayashi.kunihiko@socionext.com> <1499255419-31684-3-git-send-email-hayashi.kunihiko@socionext.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 2017-07-05 21:20 GMT+09:00 Masahiro Yamada : > 2017-07-05 20:50 GMT+09:00 Kunihiko Hayashi : > >> + >> +#define TMOD 0x0928 >> +#define TMOD_MASK GENMASK(9, 0) >> + > [ snip ] >> + >> + /* >> + * The bit[8:0] of TMOD register represents 2's complement value >> + * of temperature in Celsius. Since bit8 of TMOD shows a sign bit, >> + * 32bit temperature value is obtained by sign extension. >> + */ > > > Apparently, this comment does not match your code: > > #define TMOD_MASK GENMASK(9, 0) > > > TMOD_MASK is indicating bit[9:0]. > > > > Digging into the patch history, now I understood what happened. > > > > In v1, you described > #define TMOD_MASK 0x1ff > > This was correct. > > > In v2, you converted it into > #define TMOD_MASK GENMASK(9, 0) > > This was misconversion. It should be GENMASK(8, 0) > > > > Anyway, TMOD_MASK is not used any more. > > >> + *out_temp = sign_extend32(temp, 8) * 1000; > > > Why magic number here? > > /* MSB of the TMOD field is a sign bit */ > *out_temp = sign_extend32(temp, TMOD_WIDTH) * 1000; No. sign_extend32(temp, TMOD_WIDTH - 1) or sign_extend32(temp, TMOD_MSB) or whatever. -- Best Regards Masahiro Yamada