linux-kernel - Re: [PATCH v10 4/6] thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 Jan 2023 15:30:34 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Balsam CHIHI <bchihi@...libre.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     rafael@...nel.org, amitk@...nel.org, rui.zhang@...el.com,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        khilman@...libre.com, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, james.lo@...iatek.com,
        rex-bc.chen@...iatek.com
Subject: Re: [PATCH v10 4/6] thermal/drivers/mediatek: Add the Low Voltage
 Thermal Sensor driver


Hi Balsam,

On 18/01/2023 14:58, Balsam CHIHI wrote:

[ ... ]

>> You're describing the register with nice words, but there's another way to do
>> the same that will be even more effective.
>>
>> /*
>>    * LVTS MONINT: Interrupt Monitoring register
>>    * Each bit describes the enable status of per-sensor interrupts.
>>    */
>> #define LVTS_MONINT_THRES_COLD  BIT(0)  /* Cold threshold */
>> #define LVTS_MONINT_THRES_HOT   BIT(1)  /* Hot threshold */
>> #define LVTS_MONINT_OFFST_LOW   BIT(2)  /* Low offset */
>> #define LVTS_MONINT_OFFST_HIGH  BIT(3)  /* High offset */
>> #define LVTS_MONINT_OFFST_NTH   BIT(4)  /* Normal To Hot */
>> #define EVERYTHING_ELSE ........................
>>
>> #define LVTS_MONINT_SNS0_MASK   GENMASK( ... )
>> #define LVTS_MONINT_SNS1_MASK   GENMASK .....
>>
>> /* Find a better name for this one */
>> #define LVTS_MONINT_EN_IRQS     ( LVTS_MONINT_THRES_COLD | LVTS_MONINT_THRES_HOT |
>>                                    LVTS_MONINT_OFFST_LOW ..... etc etc)
>>
> 
> Given the complexity of the controller and the number of registers,
> if we create a define per bits, we will end up with a huge list of
> defines (~300).

Yeah, that is too much for a little gain.

However, a few can be added for the interrupt only.

Instead of LVTS_MONINT_THRES ..., it could be LVTS_INT_THRES_... and 
reused for LVTS_MONINTSTS and LVTS_MONINT setup as the bits position are 
the same?

[ ... ]


>>> +static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
>>> +{
>>> +     u32 period_unit = (118 * 1000) / (256 * 38);
>>
>> #define SOMETHING       118
>> #define SOMETHING_ELSE  1000
>> #define ....
>>
>> const u32 period_unit = (SOMETHING * SOMETHING_ELSE) / ....
>>
> 
> Constifying "u32 period_unit" generates the following compilation warning :
> ./include/asm-generic/io.h:273:61: note: expected ‘volatile void *’
> but argument is of type ‘const void *’
>    273 | static inline void writel(u32 value, volatile void __iomem *addr)
>        |                                      ~~~~~~~~~~~~~~~~~~~~~~~^~~~

That is strange. period_unit is the 'value', not the 'addr'. Are you 
sure about the warning?




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog