Messages in this thread | | | From | Balsam CHIHI <> | Date | Wed, 18 Jan 2023 15:52:46 +0100 | Subject | Re: [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
| |