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

On Wed, Jan 18, 2023 at 3:30 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> 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?
>

OK, I will add a few for the interrupts.

> [ ... ]
>
>
> >>> +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?

I double-checked and you are right.
The warning is not related to "const u32 period_unit".
It is related to the previous one, which is "const void __iomem *lvts_edata".
And this is the full log :

drivers/thermal/mediatek/lvts_thermal.c: In function ‘lvts_ctrl_calibrate’:
drivers/thermal/mediatek/lvts_thermal.c:835:47: warning: passing
argument 2 of ‘writel’ discards ‘const’ qualifier from pointer target
type [-Wdiscarded-qualifiers]
835 | writel(lvts_ctrl->calibration[i], lvts_edata[i]);
| ~~~~~~~~~~^~~
In file included from ./arch/arm64/include/asm/io.h:163,
from ./include/linux/io.h:13,
from ./include/linux/irq.h:20,
from ./include/asm-generic/hardirq.h:17,
from ./arch/arm64/include/asm/hardirq.h:17,
from ./include/linux/hardirq.h:11,
from ./include/linux/interrupt.h:11,
from drivers/thermal/mediatek/lvts_thermal.c:12:
./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)
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~

>
>
>
>
> --
> <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
>

Best regards,
Balsam

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