[v10,4/6] thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver

From: Balsam CHIHI <bchihi@baylibre.com>

The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
controllers contained in a thermal domain.

A thermal domains can be the MCU or the AP.

Each thermal domains contain up to seven controllers, each thermal
controller handle up to four thermal sensors.

The LVTS has two Finite State Machines (FSM), one to handle the
functionin temperatures range like hot or cold temperature and another
one to handle monitoring trip point. The FSM notifies via interrupts
when a trip point is crossed.

The interrupt is managed at the thermal controller level, so when an
interrupt occurs, the driver has to find out which sensor triggered
such an interrupt.

The sampling of the thermal can be filtered or immediate. For the
former, the LVTS measures several points and applies a low pass
filter.

Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
---
 drivers/thermal/mediatek/Kconfig            |   15 +
 drivers/thermal/mediatek/Makefile           |    1 +
 drivers/thermal/mediatek/lvts_thermal.c     | 1244 +++++++++++++++++++
 include/dt-bindings/thermal/mediatek-lvts.h |   19 +
 4 files changed, 1279 insertions(+)
 create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
 create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h

Il 12/01/23 16:28, bchihi@baylibre.com ha scritto:
> From: Balsam CHIHI <bchihi@baylibre.com>
> 
> The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
> controllers contained in a thermal domain.
> 
> A thermal domains can be the MCU or the AP.
> 
> Each thermal domains contain up to seven controllers, each thermal
> controller handle up to four thermal sensors.
> 
> The LVTS has two Finite State Machines (FSM), one to handle the
> functionin temperatures range like hot or cold temperature and another
> one to handle monitoring trip point. The FSM notifies via interrupts
> when a trip point is crossed.
> 
> The interrupt is managed at the thermal controller level, so when an
> interrupt occurs, the driver has to find out which sensor triggered
> such an interrupt.
> 
> The sampling of the thermal can be filtered or immediate. For the
> former, the LVTS measures several points and applies a low pass
> filter.
> 
> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> ---
>   drivers/thermal/mediatek/Kconfig            |   15 +
>   drivers/thermal/mediatek/Makefile           |    1 +
>   drivers/thermal/mediatek/lvts_thermal.c     | 1244 +++++++++++++++++++
>   include/dt-bindings/thermal/mediatek-lvts.h |   19 +
>   4 files changed, 1279 insertions(+)
>   create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
>   create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h
> 

..snip..

> +
> +static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high)
> +{
> +	struct lvts_sensor *lvts_sensor = tz->devdata;
> +	void __iomem *base = lvts_sensor->base;
> +	u32 raw_low = lvts_temp_to_raw(low);
> +	u32 raw_high = lvts_temp_to_raw(high);
> +
> +	/*
> +	 * Hot to normal temperature threshold
> +	 *
> +	 * LVTS_H2NTHRE
> +	 *
> +	 * Bits:
> +	 *
> +	 * 14-0 : Raw temperature for threshold
> +	 */
> +	if (low != -INT_MAX) {
> +		dev_dbg(&tz->device, "Setting low limit temperature interrupt: %d\n", low);
> +		writel(raw_low, LVTS_H2NTHRE(base));
> +	}
> +
> +	/*
> +	 * Hot temperature threshold
> +	 *
> +	 * LVTS_HTHRE
> +	 *
> +	 * Bits:
> +	 *
> +	 * 14-0 : Raw temperature for threshold
> +	 */
> +	dev_dbg(&tz->device, "Setting high limit temperature interrupt: %d\n", high);
> +	writel(raw_high, LVTS_HTHRE(base));
> +
> +	return 0;
> +}
> +
> +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
> +{
> +	irqreturn_t iret = IRQ_NONE;
> +	u32 value, masks[] = { 0x0009001F, 0X000881F0, 0x00247C00, 0x1FC00000 };

Please, no magic numbers around.

> +	int i;
> +
> +	/*
> +	 * Interrupt monitoring status
> +	 *
> +	 * LVTS_MONINTST
> +	 *
> +	 * Bits:

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)




> +	 *
> +	 * 31 : Interrupt for stage 3
> +	 * 30 : Interrupt for stage 2
> +	 * 29 : Interrupt for state 1
> +	 * 28 : Interrupt using filter on sensor 3
> +	 *
> +	 * 27 : Interrupt using immediate on sensor 3
> +	 * 26 : Interrupt normal to hot on sensor 3
> +	 * 25 : Interrupt high offset on sensor 3
> +	 * 24 : Interrupt low offset on sensor 3
> +	 *
> +	 * 23 : Interrupt hot threshold on sensor 3
> +	 * 22 : Interrupt cold threshold on sensor 3
> +	 * 21 : Interrupt using filter on sensor 2
> +	 * 20 : Interrupt using filter on sensor 1
> +	 *
> +	 * 19 : Interrupt using filter on sensor 0
> +	 * 18 : Interrupt using immediate on sensor 2
> +	 * 17 : Interrupt using immediate on sensor 1
> +	 * 16 : Interrupt using immediate on sensor 0
> +	 *
> +	 * 15 : Interrupt device access timeout interrupt
> +	 * 14 : Interrupt normal to hot on sensor 2
> +	 * 13 : Interrupt high offset interrupt on sensor 2
> +	 * 12 : Interrupt low offset interrupt on sensor 2
> +	 *
> +	 * 11 : Interrupt hot threshold on sensor 2
> +	 * 10 : Interrupt cold threshold on sensor 2
> +	 *  9 : Interrupt normal to hot on sensor 1
> +	 *  8 : Interrupt high offset interrupt on sensor 1
> +	 *
> +	 *  7 : Interrupt low offset interrupt on sensor 1
> +	 *  6 : Interrupt hot threshold on sensor 1
> +	 *  5 : Interrupt cold threshold on sensor 1
> +	 *  4 : Interrupt normal to hot on sensor 0
> +	 *
> +	 *  3 : Interrupt high offset interrupt on sensor 0
> +	 *  2 : Interrupt low offset interrupt on sensor 0
> +	 *  1 : Interrupt hot threshold on sensor 0
> +	 *  0 : Interrupt cold threshold on sensor 0
> +	 *
> +	 * We are interested in the sensor(s) responsible of the
> +	 * interrupt event. We update the thermal framework with the
> +	 * thermal zone associated with the sensor. The framework will
> +	 * take care of the rest whatever the kind of interrupt, we
> +	 * are only interested in which sensor raised the interrupt.
> +	 *
> +	 * sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
> +	 *                  => 0x1FC00000
> +	 * sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
> +	 *                  => 0x00247C00
> +	 * sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
> +	 *                  => 0X000881F0
> +	 * sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
> +	 *                  => 0x0009001F
> +	 */
> +	value = readl(LVTS_MONINTSTS(lvts_ctrl->base));
> +
> +	/*
> +	 * Let's figure out which sensors raised the interrupt
> +	 *
> +	 * NOTE: the masks array must be ordered with the index
> +	 * corresponding to the sensor id eg. index=0, mask for
> +	 * sensor0.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(masks); i++) {
> +
> +		if (!(value & masks[i]))

Questions for you:

1. Are the masks always the same for all SoCs?
2. Do they correspond to what we set in lvts_irq_init()?

I'd expect future new SoCs to have different masks... and since lvts_irq_init() is
actually "playing with" interrupts register(s), with one of them (LVTS_MONINT)
having the same layout as this one, I would place all of the initialization in
that function instead.

This means that we'd initialize those masks at lvts_irq_init() time, in a struct
member, and read it back in this interrupt handler: like that, we get that a bit
more ordered and generally more readable.

> +			continue;
> +
> +		thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
> +					   THERMAL_TRIP_VIOLATED);
> +		iret |= IRQ_HANDLED;
> +	}
> +
> +	/*
> +	 * Write back to clear the interrupt status (W1C)
> +	 */
> +	writel(value, LVTS_MONINTSTS(lvts_ctrl->base));
> +
> +	return iret;
> +}
> +
> +/*
> + * Temperature interrupt handler. Even if the driver supports more
> + * interrupt modes, we use the interrupt when the temperature crosses
> + * the hot threshold the way up and the way down (modulo the
> + * hysteresis).
> + *
> + * Each thermal domain has a couple of interrupts, one for hardware
> + * reset and another one for all the thermal events happening on the
> + * different sensors.
> + *
> + * The interrupt is configured for thermal events when crossing the
> + * hot temperature limit. At each interrupt, we check in every
> + * controller if there is an interrupt pending.
> + */
> +static irqreturn_t lvts_irq_handler(int irq, void *data)
> +{
> +	struct lvts_domain *lvts_td = data;
> +	irqreturn_t iret = IRQ_NONE;
> +	int i;
> +
> +	for (i = 0; i < lvts_td->num_lvts_ctrl; i++)
> +		iret |= lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);

Please do *not* OR your function calls! While this is surely fine here in
this function and for this particular case, it's generally bad practice
and shall be avoided.

> +
> +	return iret;
> +}
> +
> +static struct thermal_zone_device_ops lvts_ops = {
> +	.get_temp = lvts_get_temp,
> +	.set_trips = lvts_set_trips,
> +};
> +

..snip..

> +
> +static int lvts_irq_init(struct lvts_ctrl *lvts_ctrl)
> +{
> +	u32 value;
> +
> +	/*
> +	 * LVTS_PROTCTL : Thermal Protection Sensor Selection
> +	 *
> +	 * Bits:
> +	 *
> +	 * 19-18 : Sensor to base the protection on
> +	 * 17-16 : Strategy:
> +	 *         00 : Average of 4 sensors
> +	 *         01 : Max of 4 sensors
> +	 *         10 : Selected sensor with bits 19-18
> +	 *         11 : Reserved
> +	 */
> +	value = BIT(16);
> +	writel(value, LVTS_PROTCTL(lvts_ctrl->base));
> +
> +	/*
> +	 * LVTS_PROTTA : Stage 1 temperature threshold
> +	 * LVTS_PROTTB : Stage 2 temperature threshold
> +	 * LVTS_PROTTC : Stage 3 temperature threshold
> +	 *
> +	 * Bits:
> +	 *
> +	 * 14-0: Raw temperature threshold
> +	 *
> +	 * writel(0x0, LVTS_PROTTA(lvts_ctrl->base));
> +	 * writel(0x0, LVTS_PROTTB(lvts_ctrl->base));
> +	 */
> +	writel(lvts_ctrl->hw_tshut_raw_temp, LVTS_PROTTC(lvts_ctrl->base));
> +
> +	/*
> +	 * LVTS_MONINT : Interrupt configuration register
> +	 *
> +	 * The LVTS_MONINT register layout is the same as the LVTS_MONINTSTS
> +	 * register, except we set the bits to enable the interrupt.
> +	 */
> +	value = 0x9FBF7BDE;

	u32 val;

	val = FIELD_PREP(LVTS_MONINT_SNS0_MASK, LVTS_MONINT_EN_IRQS);
	val |= FIELD_PREP(LVTS_MONINT_SNS1_MASK, LVTS_MONINT_EN_IRQS);

	... etc

	writel(val, ...... )

> +	writel(value, LVTS_MONINT(lvts_ctrl->base));
> +
> +	return 0;
> +}
> +

..snip..

> +
> +static int lvts_ctrl_initialize(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> +{
> +	/*
> +	 * Write device mask: 0xC1030000
> +	 */
> +	u32 cmds[] = {
> +		0xC1030E01, 0xC1030CFC, 0xC1030A8C, 0xC103098D, 0xC10308F1,
> +		0xC10307A6, 0xC10306B8, 0xC1030500, 0xC1030420, 0xC1030300,
> +		0xC1030030, 0xC10300F6, 0xC1030050, 0xC1030060, 0xC10300AC,
> +		0xC10300FC, 0xC103009D, 0xC10300F1, 0xC10300E1
> +	};
...what is this long list of commands?

Why 0xC103_0000? Describe that please.

Also, why is this not a platform data constant?

Example:
struct lvts_plat {
	const struct lvts_ctrl_data *ctrl_data;
	u8 num_ctrl_data;
	const u16 device_mask;
	const u16 *init_cmds;
	u8 num_init_cmds;
}

where device_mask gets set to 0xc103 and init_cmds is an array containing
the low-16 (0x0e01, 0x0cfc, ...), and where this function would simply do
something like

	lvts_write_config(lvts_ctrl, plat->device_mask, init_cmds, num_init_cmds);

... and where lvts_write_config() does something like:

	for (i = 0; i < num_cmds; i++) {
		u32 val = device_mask | init_cmds[i];
		writel(val, LVTS_CONFIG ...)
	}
> +
> +	lvts_write_config(lvts_ctrl, cmds, ARRAY_SIZE(cmds));
> +
> +	return 0;
> +}
> +
> +static int lvts_ctrl_calibrate(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> +{
> +	int i;
> +	void __iomem *lvts_edata[] = {

Can we constify this?

> +		LVTS_EDATA00(lvts_ctrl->base),
> +		LVTS_EDATA01(lvts_ctrl->base),
> +		LVTS_EDATA02(lvts_ctrl->base),
> +		LVTS_EDATA03(lvts_ctrl->base)
> +	};
> +
> +	/*
> +	 * LVTS_EDATA0X : Efuse calibration reference value for sensor X
> +	 *
> +	 * Bits:
> +	 *
> +	 * 20-0 : Efuse value for normalization data
> +	 */
> +	for (i = 0; i < LVTS_SENSOR_MAX; i++)
> +		writel(lvts_ctrl->calibration[i], lvts_edata[i]);
> +
> +	return 0;
> +}
> +
> +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) / ....

> +	u32 grp_interval = 1;
> +	u32 flt_interval = 1;
> +	u32 sensor_interval = 1;
> +	u32 hw_filter = 0x2;
> +	u32 value;
> +

...snip...

> +
> +static struct lvts_ctrl_data mt819x_lvts_data_ctrl[] = {

No wildcards. Please, rename this to give the name of the oldest SoC
that uses these values. Assuming that it is MT8192.... mt8192_lvts_data_ctrl[]

> +	{
> +		.cal_offset = { 0x4, 0x7 },
> +		.lvts_sensor = {
> +			{ .dt_id = MT819x_MCU_BIG_CPU0 },
> +			{ .dt_id = MT819x_MCU_BIG_CPU1 }
> +		},
> +		.num_lvts_sensor = 2,
> +		.offset = 0x0,
> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> +	},
> +
> +	{
> +		.cal_offset = { 0xd, 0x10 },
> +		.lvts_sensor = {
> +			{ .dt_id = MT819x_MCU_BIG_CPU2 },
> +			{ .dt_id = MT819x_MCU_BIG_CPU3 }
> +		},
> +		.num_lvts_sensor = 2,
> +		.offset = 0x100,
> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> +	},
> +
> +	{
> +		.cal_offset = { 0x16, 0x19, 0x1c, 0x1f },
> +		.lvts_sensor = {
> +			{ .dt_id = MT819x_MCU_LITTLE_CPU0 },
> +			{ .dt_id = MT819x_MCU_LITTLE_CPU1 },
> +			{ .dt_id = MT819x_MCU_LITTLE_CPU2 },
> +			{ .dt_id = MT819x_MCU_LITTLE_CPU3 }
> +		},
> +		.num_lvts_sensor = 4,
> +		.offset = 0x200,
> +		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> +	}
> +};
> +
> +static struct lvts_data mt819x_lvts_mcu_data = {

Same here.

> +	.lvts_ctrl	= mt819x_lvts_data_ctrl,
> +	.num_lvts_ctrl	= ARRAY_SIZE(mt819x_lvts_data_ctrl),
> +};
> +
> +static const struct of_device_id lvts_of_match[] = {
> +	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt819x_lvts_mcu_data },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, lvts_of_match);
> +
> +static struct platform_driver lvts_driver = {
> +	.probe = lvts_probe,
> +	.remove = lvts_remove,
> +	.driver = {
> +		.name = "mtk-lvts-thermal",
> +		.of_match_table = lvts_of_match,
> +	},
> +};
> +module_platform_driver(lvts_driver);
> +
> +MODULE_AUTHOR("Balsam CHIHI <bchihi@baylibre.com>");
> +MODULE_DESCRIPTION("MediaTek LVTS Thermal Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/dt-bindings/thermal/mediatek-lvts.h b/include/dt-bindings/thermal/mediatek-lvts.h
> new file mode 100644
> index 000000000000..80d060400236
> --- /dev/null
> +++ b/include/dt-bindings/thermal/mediatek-lvts.h

Bindings go in a different commit: add this in your patch [2/6], where you are
adding the yaml binding.

Also, please follow binding names: rename this file to mediatek,mt8192-lvts.h.

> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2023 MediaTek Inc.
> + * Author: Balsam CHIHI <bchihi@baylibre.com>
> + */
> +
> +#ifndef __MEDIATEK_LVTS_DT_H
> +#define __MEDIATEK_LVTS_DT_H
> +
> +#define MT819x_MCU_BIG_CPU0	0

No wildcards: MT8192_MCU_BIG_CPU0

> +#define MT819x_MCU_BIG_CPU1	1
> +#define	MT819x_MCU_BIG_CPU2	2
> +#define	MT819x_MCU_BIG_CPU3	3
> +#define	MT819x_MCU_LITTLE_CPU0	4
> +#define	MT819x_MCU_LITTLE_CPU1	5
> +#define	MT819x_MCU_LITTLE_CPU2	6
> +#define	MT819x_MCU_LITTLE_CPU3	7
> +
> +#endif /* __MEDIATEK_LVTS_DT_H */

Regards,
Angelo

On 16/01/2023 11:50, AngeloGioacchino Del Regno wrote:
> Il 12/01/23 16:28, bchihi@baylibre.com ha scritto:
>> From: Balsam CHIHI <bchihi@baylibre.com>
>>
>> The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
>> controllers contained in a thermal domain.
>>
>> A thermal domains can be the MCU or the AP.
>>
>> Each thermal domains contain up to seven controllers, each thermal
>> controller handle up to four thermal sensors.
>>
>> The LVTS has two Finite State Machines (FSM), one to handle the
>> functionin temperatures range like hot or cold temperature and another
>> one to handle monitoring trip point. The FSM notifies via interrupts
>> when a trip point is crossed.
>>
>> The interrupt is managed at the thermal controller level, so when an
>> interrupt occurs, the driver has to find out which sensor triggered
>> such an interrupt.
>>
>> The sampling of the thermal can be filtered or immediate. For the
>> former, the LVTS measures several points and applies a low pass
>> filter.
>>
>> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
>> ---
>>   drivers/thermal/mediatek/Kconfig            |   15 +
>>   drivers/thermal/mediatek/Makefile           |    1 +
>>   drivers/thermal/mediatek/lvts_thermal.c     | 1244 +++++++++++++++++++
>>   include/dt-bindings/thermal/mediatek-lvts.h |   19 +
>>   4 files changed, 1279 insertions(+)
>>   create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
>>   create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h
>>
> 
> ..snip..
> 
>> +
>> +static int lvts_set_trips(struct thermal_zone_device *tz, int low, 
>> int high)
>> +{
>> +    struct lvts_sensor *lvts_sensor = tz->devdata;
>> +    void __iomem *base = lvts_sensor->base;
>> +    u32 raw_low = lvts_temp_to_raw(low);
>> +    u32 raw_high = lvts_temp_to_raw(high);
>> +
>> +    /*
>> +     * Hot to normal temperature threshold
>> +     *
>> +     * LVTS_H2NTHRE
>> +     *
>> +     * Bits:
>> +     *
>> +     * 14-0 : Raw temperature for threshold
>> +     */
>> +    if (low != -INT_MAX) {
>> +        dev_dbg(&tz->device, "Setting low limit temperature 
>> interrupt: %d\n", low);
>> +        writel(raw_low, LVTS_H2NTHRE(base));
>> +    }
>> +
>> +    /*
>> +     * Hot temperature threshold
>> +     *
>> +     * LVTS_HTHRE
>> +     *
>> +     * Bits:
>> +     *
>> +     * 14-0 : Raw temperature for threshold
>> +     */
>> +    dev_dbg(&tz->device, "Setting high limit temperature interrupt: 
>> %d\n", high);
>> +    writel(raw_high, LVTS_HTHRE(base));
>> +
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
>> +{
>> +    irqreturn_t iret = IRQ_NONE;
>> +    u32 value, masks[] = { 0x0009001F, 0X000881F0, 0x00247C00, 
>> 0x1FC00000 };
> 
> Please, no magic numbers around.
> 
>> +    int i;
>> +
>> +    /*
>> +     * Interrupt monitoring status
>> +     *
>> +     * LVTS_MONINTST
>> +     *
>> +     * Bits:
> 
> 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 ........................

I don't see how this is more effective than describing the register 
layout. If someone wants to hack the driver, it is much better to have 
the layout than this long list of defines for every bits of every registers.

Il 16/01/23 12:08, Daniel Lezcano ha scritto:
> On 16/01/2023 11:50, AngeloGioacchino Del Regno wrote:
>> Il 12/01/23 16:28, bchihi@baylibre.com ha scritto:
>>> From: Balsam CHIHI <bchihi@baylibre.com>
>>>
>>> The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
>>> controllers contained in a thermal domain.
>>>
>>> A thermal domains can be the MCU or the AP.
>>>
>>> Each thermal domains contain up to seven controllers, each thermal
>>> controller handle up to four thermal sensors.
>>>
>>> The LVTS has two Finite State Machines (FSM), one to handle the
>>> functionin temperatures range like hot or cold temperature and another
>>> one to handle monitoring trip point. The FSM notifies via interrupts
>>> when a trip point is crossed.
>>>
>>> The interrupt is managed at the thermal controller level, so when an
>>> interrupt occurs, the driver has to find out which sensor triggered
>>> such an interrupt.
>>>
>>> The sampling of the thermal can be filtered or immediate. For the
>>> former, the LVTS measures several points and applies a low pass
>>> filter.
>>>
>>> Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
>>> ---
>>>   drivers/thermal/mediatek/Kconfig            |   15 +
>>>   drivers/thermal/mediatek/Makefile           |    1 +
>>>   drivers/thermal/mediatek/lvts_thermal.c     | 1244 +++++++++++++++++++
>>>   include/dt-bindings/thermal/mediatek-lvts.h |   19 +
>>>   4 files changed, 1279 insertions(+)
>>>   create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
>>>   create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h
>>>
>>
>> ..snip..
>>
>>> +
>>> +static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high)
>>> +{
>>> +    struct lvts_sensor *lvts_sensor = tz->devdata;
>>> +    void __iomem *base = lvts_sensor->base;
>>> +    u32 raw_low = lvts_temp_to_raw(low);
>>> +    u32 raw_high = lvts_temp_to_raw(high);
>>> +
>>> +    /*
>>> +     * Hot to normal temperature threshold
>>> +     *
>>> +     * LVTS_H2NTHRE
>>> +     *
>>> +     * Bits:
>>> +     *
>>> +     * 14-0 : Raw temperature for threshold
>>> +     */
>>> +    if (low != -INT_MAX) {
>>> +        dev_dbg(&tz->device, "Setting low limit temperature interrupt: %d\n", 
>>> low);
>>> +        writel(raw_low, LVTS_H2NTHRE(base));
>>> +    }
>>> +
>>> +    /*
>>> +     * Hot temperature threshold
>>> +     *
>>> +     * LVTS_HTHRE
>>> +     *
>>> +     * Bits:
>>> +     *
>>> +     * 14-0 : Raw temperature for threshold
>>> +     */
>>> +    dev_dbg(&tz->device, "Setting high limit temperature interrupt: %d\n", high);
>>> +    writel(raw_high, LVTS_HTHRE(base));
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
>>> +{
>>> +    irqreturn_t iret = IRQ_NONE;
>>> +    u32 value, masks[] = { 0x0009001F, 0X000881F0, 0x00247C00, 0x1FC00000 };
>>
>> Please, no magic numbers around.
>>
>>> +    int i;
>>> +
>>> +    /*
>>> +     * Interrupt monitoring status
>>> +     *
>>> +     * LVTS_MONINTST
>>> +     *
>>> +     * Bits:
>>
>> 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 ........................
> 
> I don't see how this is more effective than describing the register layout. If 
> someone wants to hack the driver, it is much better to have the layout than this 
> long list of defines for every bits of every registers.
> 

This also describes the register layout, with the difference that we can use those
definitions with bitfield macros to avoid writing magic numbers around.

Anyway, I'm not against the long comment that is describing the layout with nice
words - my suggestion was just only about one of the ways to replace the magic
numbers with actual definitions.

Regards,
Angelo

Hi Angelo,

On Mon, Jan 16, 2023 at 11:50 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 12/01/23 16:28, bchihi@baylibre.com ha scritto:
> > From: Balsam CHIHI <bchihi@baylibre.com>
> >
> > The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
> > controllers contained in a thermal domain.
> >
> > A thermal domains can be the MCU or the AP.
> >
> > Each thermal domains contain up to seven controllers, each thermal
> > controller handle up to four thermal sensors.
> >
> > The LVTS has two Finite State Machines (FSM), one to handle the
> > functionin temperatures range like hot or cold temperature and another
> > one to handle monitoring trip point. The FSM notifies via interrupts
> > when a trip point is crossed.
> >
> > The interrupt is managed at the thermal controller level, so when an
> > interrupt occurs, the driver has to find out which sensor triggered
> > such an interrupt.
> >
> > The sampling of the thermal can be filtered or immediate. For the
> > former, the LVTS measures several points and applies a low pass
> > filter.
> >
> > Signed-off-by: Balsam CHIHI <bchihi@baylibre.com>
> > ---
> >   drivers/thermal/mediatek/Kconfig            |   15 +
> >   drivers/thermal/mediatek/Makefile           |    1 +
> >   drivers/thermal/mediatek/lvts_thermal.c     | 1244 +++++++++++++++++++
> >   include/dt-bindings/thermal/mediatek-lvts.h |   19 +
> >   4 files changed, 1279 insertions(+)
> >   create mode 100644 drivers/thermal/mediatek/lvts_thermal.c
> >   create mode 100644 include/dt-bindings/thermal/mediatek-lvts.h
> >

..snip..

> > +
> > +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
> > +{
> > +     irqreturn_t iret = IRQ_NONE;
> > +     u32 value, masks[] = { 0x0009001F, 0X000881F0, 0x00247C00, 0x1FC00000 };
>
> Please, no magic numbers around.
>

These number are explained in the comment bellow.
This is part of it :
      * sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
      *                  => 0x1FC00000
      * sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
      *                  => 0x00247C00
      * sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
      *                  => 0X000881F0
      * sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
      *                  => 0x0009001F

> > +     int i;
> > +
> > +     /*
> > +      * Interrupt monitoring status
> > +      *
> > +      * LVTS_MONINTST
> > +      *
> > +      * Bits:
>
> 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).
I don't think that will help for the readability.

> > +      *
> > +      * 31 : Interrupt for stage 3
> > +      * 30 : Interrupt for stage 2
> > +      * 29 : Interrupt for state 1
> > +      * 28 : Interrupt using filter on sensor 3
> > +      *

..snip..

> > +      *
> > +      *  3 : Interrupt high offset interrupt on sensor 0
> > +      *  2 : Interrupt low offset interrupt on sensor 0
> > +      *  1 : Interrupt hot threshold on sensor 0
> > +      *  0 : Interrupt cold threshold on sensor 0
> > +      *
> > +      * We are interested in the sensor(s) responsible of the
> > +      * interrupt event. We update the thermal framework with the
> > +      * thermal zone associated with the sensor. The framework will
> > +      * take care of the rest whatever the kind of interrupt, we
> > +      * are only interested in which sensor raised the interrupt.
> > +      *
> > +      * sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
> > +      *                  => 0x1FC00000
> > +      * sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
> > +      *                  => 0x00247C00
> > +      * sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
> > +      *                  => 0X000881F0
> > +      * sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
> > +      *                  => 0x0009001F
> > +      */
> > +     value = readl(LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > +     /*
> > +      * Let's figure out which sensors raised the interrupt
> > +      *
> > +      * NOTE: the masks array must be ordered with the index
> > +      * corresponding to the sensor id eg. index=0, mask for
> > +      * sensor0.
> > +      */
> > +     for (i = 0; i < ARRAY_SIZE(masks); i++) {
> > +
> > +             if (!(value & masks[i]))
>
> Questions for you:
>
> 1. Are the masks always the same for all SoCs?

The LVTS controller is not SoC specific.
The mask is controller specific whatever the SoC version.

> 2. Do they correspond to what we set in lvts_irq_init()?

Not exactly, we set LVTS_MONINT and the controller sets the LVTS_MONINTSTS.
The content will be different with what we set and what we get.

>
> I'd expect future new SoCs to have different masks... and since lvts_irq_init() is
> actually "playing with" interrupts register(s), with one of them (LVTS_MONINT)
> having the same layout as this one, I would place all of the initialization in
> that function instead.
>
> This means that we'd initialize those masks at lvts_irq_init() time, in a struct
> member, and read it back in this interrupt handler: like that, we get that a bit
> more ordered and generally more readable.
>

No. Actually, what will change is on which sensor a thermal zone is tie to,
and that is handled already by the device tree configuration.

> > +                     continue;
> > +
> > +             thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
> > +                                        THERMAL_TRIP_VIOLATED);
> > +             iret |= IRQ_HANDLED;
> > +     }
> > +
> > +     /*
> > +      * Write back to clear the interrupt status (W1C)
> > +      */
> > +     writel(value, LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > +     return iret;
> > +}
> > +
> > +/*
> > + * Temperature interrupt handler. Even if the driver supports more
> > + * interrupt modes, we use the interrupt when the temperature crosses
> > + * the hot threshold the way up and the way down (modulo the
> > + * hysteresis).
> > + *
> > + * Each thermal domain has a couple of interrupts, one for hardware
> > + * reset and another one for all the thermal events happening on the
> > + * different sensors.
> > + *
> > + * The interrupt is configured for thermal events when crossing the
> > + * hot temperature limit. At each interrupt, we check in every
> > + * controller if there is an interrupt pending.
> > + */
> > +static irqreturn_t lvts_irq_handler(int irq, void *data)
> > +{
> > +     struct lvts_domain *lvts_td = data;
> > +     irqreturn_t iret = IRQ_NONE;
> > +     int i;
> > +
> > +     for (i = 0; i < lvts_td->num_lvts_ctrl; i++)
> > +             iret |= lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);
>
> Please do *not* OR your function calls! While this is surely fine here in
> this function and for this particular case, it's generally bad practice
> and shall be avoided.
>

I understand that could be prone to errors.
I can propose this alternative but it looks less elegant than OR'ing the result.
Do you have a suggestion to improve this code snippet?

> > +
> > +     return iret;
> > +}
> > +
> > +static struct thermal_zone_device_ops lvts_ops = {
> > +     .get_temp = lvts_get_temp,
> > +     .set_trips = lvts_set_trips,
> > +};
> > +
>
> ..snip..
>
> > +
> > +static int lvts_irq_init(struct lvts_ctrl *lvts_ctrl)
> > +{
> > +     u32 value;
> > +
> > +     /*
> > +      * LVTS_PROTCTL : Thermal Protection Sensor Selection
> > +      *
> > +      * Bits:
> > +      *
> > +      * 19-18 : Sensor to base the protection on
> > +      * 17-16 : Strategy:
> > +      *         00 : Average of 4 sensors
> > +      *         01 : Max of 4 sensors
> > +      *         10 : Selected sensor with bits 19-18
> > +      *         11 : Reserved
> > +      */
> > +     value = BIT(16);
> > +     writel(value, LVTS_PROTCTL(lvts_ctrl->base));
> > +
> > +     /*
> > +      * LVTS_PROTTA : Stage 1 temperature threshold
> > +      * LVTS_PROTTB : Stage 2 temperature threshold
> > +      * LVTS_PROTTC : Stage 3 temperature threshold
> > +      *
> > +      * Bits:
> > +      *
> > +      * 14-0: Raw temperature threshold
> > +      *
> > +      * writel(0x0, LVTS_PROTTA(lvts_ctrl->base));
> > +      * writel(0x0, LVTS_PROTTB(lvts_ctrl->base));
> > +      */
> > +     writel(lvts_ctrl->hw_tshut_raw_temp, LVTS_PROTTC(lvts_ctrl->base));
> > +
> > +     /*
> > +      * LVTS_MONINT : Interrupt configuration register
> > +      *
> > +      * The LVTS_MONINT register layout is the same as the LVTS_MONINTSTS
> > +      * register, except we set the bits to enable the interrupt.
> > +      */
> > +     value = 0x9FBF7BDE;
>
>         u32 val;
>
>         val = FIELD_PREP(LVTS_MONINT_SNS0_MASK, LVTS_MONINT_EN_IRQS);
>         val |= FIELD_PREP(LVTS_MONINT_SNS1_MASK, LVTS_MONINT_EN_IRQS);
>
>         ... etc
>
>         writel(val, ...... )
>

OK, I'll change it accordingly.

> > +     writel(value, LVTS_MONINT(lvts_ctrl->base));
> > +
> > +     return 0;
> > +}
> > +
>
> ..snip..
>
> > +
> > +static int lvts_ctrl_initialize(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > +     /*
> > +      * Write device mask: 0xC1030000
> > +      */
> > +     u32 cmds[] = {
> > +             0xC1030E01, 0xC1030CFC, 0xC1030A8C, 0xC103098D, 0xC10308F1,
> > +             0xC10307A6, 0xC10306B8, 0xC1030500, 0xC1030420, 0xC1030300,
> > +             0xC1030030, 0xC10300F6, 0xC1030050, 0xC1030060, 0xC10300AC,
> > +             0xC10300FC, 0xC103009D, 0xC10300F1, 0xC10300E1
> > +     };
> ...what is this long list of commands?
>
> Why 0xC103_0000? Describe that please.
>

AFAIU, based on the documentation, the configuration register can be
read or write.
When we write it, we set the different bits corresponding to a write sequence
which is 0xC1030000.
The documentation gives the register layout but does not explain how it works.

> Also, why is this not a platform data constant?
>

It is not a platform data, it is a controller data.
Whatever the SoC the configuration sequence will be the same.

> Example:
> struct lvts_plat {
>         const struct lvts_ctrl_data *ctrl_data;
>         u8 num_ctrl_data;
>         const u16 device_mask;
>         const u16 *init_cmds;
>         u8 num_init_cmds;
> }
>
> where device_mask gets set to 0xc103 and init_cmds is an array containing
> the low-16 (0x0e01, 0x0cfc, ...), and where this function would simply do
> something like
>
>         lvts_write_config(lvts_ctrl, plat->device_mask, init_cmds, num_init_cmds);
>
> ... and where lvts_write_config() does something like:
>
>         for (i = 0; i < num_cmds; i++) {
>                 u32 val = device_mask | init_cmds[i];
>                 writel(val, LVTS_CONFIG ...)
>         }
> > +
> > +     lvts_write_config(lvts_ctrl, cmds, ARRAY_SIZE(cmds));
> > +
> > +     return 0;
> > +}
> > +
> > +static int lvts_ctrl_calibrate(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > +     int i;
> > +     void __iomem *lvts_edata[] = {
>
> Can we constify this?
>

Constifying "void __iomem *lvts_edata[]" generates the following
compilation warning :
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]);
      |                                     ~~~~~~~~~~^~~

> > +             LVTS_EDATA00(lvts_ctrl->base),
> > +             LVTS_EDATA01(lvts_ctrl->base),
> > +             LVTS_EDATA02(lvts_ctrl->base),
> > +             LVTS_EDATA03(lvts_ctrl->base)
> > +     };
> > +
> > +     /*
> > +      * LVTS_EDATA0X : Efuse calibration reference value for sensor X
> > +      *
> > +      * Bits:
> > +      *
> > +      * 20-0 : Efuse value for normalization data
> > +      */
> > +     for (i = 0; i < LVTS_SENSOR_MAX; i++)
> > +             writel(lvts_ctrl->calibration[i], lvts_edata[i]);
> > +
> > +     return 0;
> > +}
> > +
> > +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)
      |                                      ~~~~~~~~~~~~~~~~~~~~~~~^~~~

> > +     u32 grp_interval = 1;
> > +     u32 flt_interval = 1;
> > +     u32 sensor_interval = 1;
> > +     u32 hw_filter = 0x2;
> > +     u32 value;
> > +
>
> ...snip...
>
> > +
> > +static struct lvts_ctrl_data mt819x_lvts_data_ctrl[] = {
>
> No wildcards. Please, rename this to give the name of the oldest SoC
> that uses these values. Assuming that it is MT8192.... mt8192_lvts_data_ctrl[]
>

OK, it Will be mt8195_lvts_data_ctrl[].

> > +     {
> > +             .cal_offset = { 0x4, 0x7 },
> > +             .lvts_sensor = {
> > +                     { .dt_id = MT819x_MCU_BIG_CPU0 },
> > +                     { .dt_id = MT819x_MCU_BIG_CPU1 }
> > +             },
> > +             .num_lvts_sensor = 2,
> > +             .offset = 0x0,
> > +             .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> > +     },
> > +

..snip..

> > +static struct lvts_data mt819x_lvts_mcu_data = {
>
> Same here.
>

OK, it Will be mt8195_lvts_mcu_data.

> > +     .lvts_ctrl      = mt819x_lvts_data_ctrl,
> > +     .num_lvts_ctrl  = ARRAY_SIZE(mt819x_lvts_data_ctrl),
> > +};
> > +
> > +static const struct of_device_id lvts_of_match[] = {
> > +     { .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt819x_lvts_mcu_data },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, lvts_of_match);
> > +
> > +static struct platform_driver lvts_driver = {
> > +     .probe = lvts_probe,
> > +     .remove = lvts_remove,
> > +     .driver = {
> > +             .name = "mtk-lvts-thermal",
> > +             .of_match_table = lvts_of_match,
> > +     },
> > +};
> > +module_platform_driver(lvts_driver);
> > +
> > +MODULE_AUTHOR("Balsam CHIHI <bchihi@baylibre.com>");
> > +MODULE_DESCRIPTION("MediaTek LVTS Thermal Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/dt-bindings/thermal/mediatek-lvts.h b/include/dt-bindings/thermal/mediatek-lvts.h
> > new file mode 100644
> > index 000000000000..80d060400236
> > --- /dev/null
> > +++ b/include/dt-bindings/thermal/mediatek-lvts.h
>
> Bindings go in a different commit: add this in your patch [2/6], where you are
> adding the yaml binding.
>

OK, it will be moved to :
[2/6] dt-bindings/thermal/mediatek: Add dt-binding document for LVTS
thermal controllers

> Also, please follow binding names: rename this file to mediatek,mt8192-lvts.h.
>

LVTS is SoC independent (only available on MT8192 and MT8195).
Should not we leave this file name SoC indemendent too "mediatek-lvts.h",
just like "mediatek,lvts-thermal.yaml"?

> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (c) 2023 MediaTek Inc.
> > + * Author: Balsam CHIHI <bchihi@baylibre.com>
> > + */
> > +
> > +#ifndef __MEDIATEK_LVTS_DT_H
> > +#define __MEDIATEK_LVTS_DT_H
> > +
> > +#define MT819x_MCU_BIG_CPU0  0
>
> No wildcards: MT8192_MCU_BIG_CPU0
>

OK, it Will be MT8195_MCU_BIG_CPU0.

>
> > +#define MT819x_MCU_BIG_CPU1  1
> > +#define      MT819x_MCU_BIG_CPU2     2
> > +#define      MT819x_MCU_BIG_CPU3     3
> > +#define      MT819x_MCU_LITTLE_CPU0  4
> > +#define      MT819x_MCU_LITTLE_CPU1  5
> > +#define      MT819x_MCU_LITTLE_CPU2  6
> > +#define      MT819x_MCU_LITTLE_CPU3  7
> > +
> > +#endif /* __MEDIATEK_LVTS_DT_H */
>
> Regards,
> Angelo

Best regards,
Balsam

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?

Sorry for forgetting this comment.

> > +static int lvts_ctrl_initialize(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > +     /*
> > +      * Write device mask: 0xC1030000
> > +      */
> > +     u32 cmds[] = {
> > +             0xC1030E01, 0xC1030CFC, 0xC1030A8C, 0xC103098D, 0xC10308F1,
> > +             0xC10307A6, 0xC10306B8, 0xC1030500, 0xC1030420, 0xC1030300,
> > +             0xC1030030, 0xC10300F6, 0xC1030050, 0xC1030060, 0xC10300AC,
> > +             0xC10300FC, 0xC103009D, 0xC10300F1, 0xC10300E1
> > +     };
> ...what is this long list of commands?
>

I can not give an answer for that.
The LVTS programming guide is short in explanation.
It just gives this code sequence to configure it without any explanation.

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