lkml.org 
[lkml]   [2023]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v10 4/6] thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver
From

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

\
 
 \ /
  Last update: 2023-03-26 23:46    [W:0.057 / U:0.452 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site