Re: [PATCH v7 6/7] thermal/drivers/exynos: Handle temperature threshold IRQs with SoC-specific mapping

[Mateusz Majewski <m.majewski2@samsung.com>]

Hello :)

> +/* Map Rise and Falling edges for IRQ Clean */
> +struct tmu_irq_map {
> +	u32 fall[3];
> +	u32 rise[3];
> +};

Hmm, we can probably get away with less interrupts. We actually only
enable one fall interrupt in tmu_set_low_temp and one rise interrupt in
tmu_set_high_temp.

Regarding tmu_set_crit_temp, on SoCs that have hardware thermal tripping
there is nothing to clear. On others we will reboot immediately anyway,
though maybe there is nothing wrong with clearing the interrupt
beforehand? Regardless of this, there is only a rise critical
temperature interrupt, we never set a matching fall interrupt.

Maybe it would also be good to add a bool to this struct containing
information about whether a fall interrupt is in use, and reuse
the same logic for 4210?

(Nitpick: I am not a native speaker of English, but I think "clean" and
"clear" have slightly different meanings, and the rest of the code
consistently uses "clear", so it would be worthwhile to also use "clear"
here.)

> +	/* Set SoC-specific interrupt bit mappings */
> +	switch (data->soc) {
> +	case SOC_ARCH_EXYNOS3250:
> +	case SOC_ARCH_EXYNOS4412:
> +	case SOC_ARCH_EXYNOS5250:
> +	case SOC_ARCH_EXYNOS5260:
> +		irq_map.fall[2] = BIT(20);
> +		irq_map.fall[1] = BIT(16);
> +		irq_map.fall[0] = BIT(12);
> +		irq_map.rise[2] = BIT(8);
> +		irq_map.rise[1] = BIT(4);
> +		irq_map.rise[0] = BIT(0);
> +		break;
> +	case SOC_ARCH_EXYNOS5420:
> +	case SOC_ARCH_EXYNOS5420_TRIMINFO:
> +		irq_map.fall[2] = BIT(24);
> +		irq_map.fall[1] = BIT(20);
> +		irq_map.fall[0] = BIT(16);
> +		irq_map.rise[2] = BIT(8);
> +		irq_map.rise[1] = BIT(4);
> +		irq_map.rise[0] = BIT(0);
> +		break;
> +	case SOC_ARCH_EXYNOS5433:
> +	case SOC_ARCH_EXYNOS7:
> +		irq_map.fall[2] = BIT(23);
> +		irq_map.fall[1] = BIT(17);
> +		irq_map.fall[0] = BIT(16);
> +		irq_map.rise[2] = BIT(7);
> +		irq_map.rise[1] = BIT(1);
> +		irq_map.rise[0] = BIT(0);
> +		break;
> +	default:
> +		pr_warn("exynos-tmu: Unknown SoC type %d, using fallback IRQ mapping\n", soc);
> +		break;

Maybe put irq_map inside exynos_tmu_data? exynos_map_dt_data has a
switch block that is quite similar, in that it also matches on the SoC
type. This way also there is no need to have a fallback.

Kind regards,
Mateusz Majewski