Re: [PATCH v6 0/4] Exynos Thermal code improvement

[Daniel Lezcano]

On 5/15/25 20:01, Anand Moon wrote:
> Hi Daniel,
>
> On Thu, 15 May 2025 at 18:59, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> > On 5/15/25 13:10, Anand Moon wrote:
> > > Hi Daniel,
> > >
> > > On Wed, 14 May 2025 at 16:53, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
> > > > On Wed, Apr 30, 2025 at 06:02:56PM +0530, Anand Moon wrote:
> > > > > Hi All,
> > > >
> > > > Hi Anand,
> > > >
> > > > if the goal of the changes is to do cleanups, I recommend to rework
> > > > how the code is organized. Instead of having the data->soc check all
> > > > around the functions, write per platform functions and store them in
> > > > struct of_device_id data field instead of the soc version.
> > > >
> > > > Basically get rid of exynos_map_dt_data by settings the different ops
> > > > in a per platform structure.
> > > >
> > > > Then the initialization routine would be simpler to clean.
> > >
> > > Thanks, I had previously attempted this approach.
> > > The goal is to split the exynos_tmu_data structure to accommodate
> > > SoC-specific callbacks for initialization and configuration.
> > >
> > > In my earlier attempt, I tried to refactor the code to achieve this.
> > > However, the main challenge I encountered was that the
> > > exynos_sensor_ops weren’t being correctly mapped for each SoC.
> > >
> > > Some SoC have multiple sensor
> > > exynos4x12
> > >                       tmu: tmu@100c0000
> > > exynos5420
> > >                   tmu_cpu0: tmu@10060000
> > >                   tmu_cpu1: tmu@10064000
> > >                   tmu_cpu2: tmu@10068000
> > >                   tmu_cpu3: tmu@1006c000
> > >                   tmu_gpu: tmu@100a0000
> > >    exynos5433
> > >                   tmu_atlas0: tmu@10060000
> > >                   tmu_atlas1: tmu@10068000
> > >                   tmu_g3d: tmu@10070000
> > > exynos7
> > >                   tmu@10060000
> > >
> > > It could be a design issue of the structure.or some DTS issue.
> > > So what I found in debugging it is not working correctly.
> > >
> > > static const struct thermal_zone_device_ops exynos_sensor_ops = {
> > >           .get_temp = exynos_get_temp,
> > >           .set_emul_temp = exynos_tmu_set_emulation,
> > >           .set_trips = exynos_set_trips,
> > > };
> > >
> > > The sensor callback will not return a valid pointer and soc id for the get_temp.
> > >
> > > Here is my earlier version of local changes.
> > > [1] https://pastebin.com/bbEP04Zh exynos_tmu.c
> > > [2] https://pastebin.com/PzNz5yve Odroid U3 dmesg.log
> > > [3] https://pastebin.com/4Yjt2d2u    Odroid Xu4 dmesg.log
> > >
> > > I want to re-model the structure to improve the code.
> > > Once Its working condition I will send this for review.
> > >
> > > If you have some suggestions please let me know.
> >
> > I suggest to do the conversion step by step beginning by
> > exynos4210_tmu_clear_irqs, then by exynos_map_dt_data as the first
> > cleanup iteration
> Ok you want IRQ handle per SoC call back functions?
> so on all the changes depending on SoC id should be moved to
> respective callback functions to reduce the code.

I think you can keep the same irq handler function but move the 
tmu_intstat, tmu_intclear in the persoc structure and remove the 
exynos4210_tmu_clear_irqs function.

You should end up with something like:

static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
{
	struct exynos_tmu_data *data = id;
	unsigned int val_irq;

	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);

	mutex_lock(&data->lock);
	clk_enable(data->clk);

	val_irq = readl(data->base + data->tmu_intstat);
        writel(val_irq, data->base + data->tmu_intclear);

	clk_disable(data->clk);
	mutex_unlock(&data->lock);

	return IRQ_HANDLED;
}

But if the irq handler has some soc specific code, then it should be a 
separate function

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