> Do you know what are the possible implications of not using ACPM? As I > understand, ACPM is a Samsung's downstream framework which uses APM > (Active Power Management) IP block internally to act as an IPC > mechanism, which makes it possible to offload any PM related > operations (which might get quite heavy, if we are to belive the TRM > description of APM) from CPU to APM. I'm not against the direct > registers access based implementation (in fact, I'm not sure how that > APM/ACPM thing can be implemented in upstreamable way and if it's > worth it at all). Just curious if we understand what we are > potentially missing out, and if at some point we'll be forced to > implement that ACPM thing anyway (for something else)? Not sure honestly. The downstream v4.10 driver does many operations on registers anyway...? > Not sure if that's true, as already discussed in my comments for the > previous patches. Looks like one clock is still needed, which is the > PCLK bus clock (to interface registers) which might simultaneously act > as an operating (functional) clock. The code seems to be working correctly without this clock, both register reads and writes. Maybe the support for extra sensors, which I couldn't get to work, would require this clock? > Exynos850 TRM says AVG_CONTROL offset is 0x38, and 0x58 is actually > for THRESHOLD0_TEMP_RISE3_2 register. Thank you so much! Will fix in v2. Though writing to the right place doesn't seem to change much in practice, probably just means that the correct mode is being used. > Something seems off to me here. How come the shift value for EXYNOS7 > case is 8, but the mask is actually 9 bits long? Does it mean the > first error field is 8 bits long, and the second error field is 9 bits > long for EXYNOS7? I don't have the Exynos7 manual, so it's just a > hunch. But if it's true, maybe this shift value has to be added in > your [PATCH 2/6] to fix Exynos7 case? I did not really want to mess with Exynos7 code, as we don't have an Exynos7 board sadly. Honestly I feel like I should drop the 2/6 patch completely and only modify the code to run on 850 correctly. > Also, just an idea: those values (and other similar values) could be > pre-calculated somewhere during the probe, stored in some struct (e.g. > _variant or _chip) and then just used here. sanitize_temp_error is only called one per probe and once per resume, so probably little to gain? Will also do all other.
