On 24.01.2023 08:27, Devi Priya wrote:

On 1/13/2023 7:39 PM, Konrad Dybcio wrote:

quoted

On 13.01.2023 14:21, Devi Priya wrote:

quoted

On 1/10/2023 6:07 PM, Konrad Dybcio wrote:

quoted

On 10.01.2023 13:13, devi priya wrote:

quoted

Add Global Clock Controller (GCC) driver for ipq9574 based devices

Co-developed-by: Anusha Rao <redacted>
Signed-off-by: Anusha Rao <redacted>
Signed-off-by: devi priya <redacted>
---

[...]

quoted

quoted

quoted

+static struct clk_branch gcc_blsp1_qup6_i2c_apps_clk = {
+    .halt_reg = 0x07024,
+    .clkr = {
+        .enable_reg = 0x07024,
+        .enable_mask = BIT(0),
+        .hw.init = &(struct clk_init_data) {
+            .name = "gcc_blsp1_qup6_i2c_apps_clk",
+            .parent_hws = (const struct clk_hw *[]) {
+                    &blsp1_qup6_i2c_apps_clk_src.clkr.hw },
+            .num_parents = 1,
+            .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,

Sounds very much like a hack..

Got it, will remove the clock entry as it is not being used in linux

I'm not sure removing it is the best option, somebody might have a
funky board where they use this particular QUP for I2C for whatever
reason and then the clock would have to be re-added..

Sure, Understood
This clock is used by the RPM component to communicate with PMIC and we
would add the critical flag here

Okay, so this SoC is intended to ship with some RPM PMICs and
*always* with an I2C companion that's required for some basic
functionality, correct?

Otherwise, if it's just for wifi/multimedia/etc (like PM8008ij
on some newer devices), you should not make it critical and
simply rely on Linux keeping it alive like so:

consumer takes a regulator
the regulator does not go to sleep because it's consumed
the PMIC is active because a regulator on it is being used
the I2C bus is active because its child PMIC is used
the I2C clocks are alive because there's an active user

Konrad

quoted

Thanks for addressing all of the review comments so thoroughly!

Konrad

Best Regards,
Devi Priya