Attention is currently required from: Bora Guvendik, Anil Kumar K, Cliff Huang, Subrata Banik, Paul Menzel, Thejaswani Putta.

View Change

3 comments:

• File src/drivers/wwan/fm/acpi_fm350gl.c:

  • Patch Set #7, Line 55:

    acpigen_write_if_lequal_op_int(LOCAL0_OP, 0);
    	if (wwan_fm350gl_get_rtd3_method_support(config) | ACPI_PCIE_RP_EMIT_L23)
    		acpigen_emit_namestring(acpi_device_path_join(parent_dev, "DL23"));
    	/* assert PERST# pin */
    	acpigen_enable_tx_gpio(&config->perst_gpio);
    

    Sorry am I misreading the logic here?

    I am reading this as:
    If PERST# is low, run the DL23() Method and then drive PERST# low? even though the TX state already was read back as low?

  • Patch Set #7, Line 64:

    /* warm reset */
    	acpigen_write_if_lequal_op_int(ARG0_OP, 0);
    	acpigen_write_sleep(FM350GL_TBTG);
    	/* cold reset */
    	acpigen_write_else();
    	acpigen_write_if_lequal_op_int(ARG0_OP, 1);
    

    suggestion:
    add some macros here like
    ```
    enum reset_type {
          RESET_TYPE_WARM = 0,
          RESET_TYPE_COLD = 1,
    };
    ```

    and then you can use those directly here

  • Patch Set #7, Line 52:

    acpigen_write_method_serialized("FHRF", 1);
    	/* LOCAL0 = PERST# */
    	acpigen_get_tx_gpio(&config->perst_gpio);
    	acpigen_write_if_lequal_op_int(LOCAL0_OP, 0);
    	if (wwan_fm350gl_get_rtd3_method_support(config) | ACPI_PCIE_RP_EMIT_L23)
    		acpigen_emit_namestring(acpi_device_path_join(parent_dev, "DL23"));
    	/* assert PERST# pin */
    	acpigen_enable_tx_gpio(&config->perst_gpio);
    	acpigen_write_if_end(); /* If */
    	acpigen_write_sleep(FM350GL_TR2B);
    	/* assert RESET# pin */
    	acpigen_enable_tx_gpio(&config->reset_gpio);
    	/* warm reset */
    	acpigen_write_if_lequal_op_int(ARG0_OP, 0);
    	acpigen_write_sleep(FM350GL_TBTG);
    	/* cold reset */
    	acpigen_write_else();
    	acpigen_write_if_lequal_op_int(ARG0_OP, 1);
    	/* disable source clock */
    	if (wwan_fm350gl_get_rtd3_method_support(config) | ACPI_PCIE_RP_EMIT_SRCK)
    		acpigen_emit_namestring(acpi_device_path_join(parent_dev, "SRCK"));
    	acpigen_emit_byte(ZERO_OP);
    	acpigen_write_sleep(FM350GL_TB2F);
    	/* assert FCPO# pin */
    	acpigen_enable_tx_gpio(&config->fcpo_gpio);
    	acpigen_write_sleep(FM350GL_TFDI);
    	acpigen_write_if_end(); /* If */
    	acpigen_pop_len(); /* Else */
    	acpigen_write_method_end(); /* Method */
    }
    

    suggestion:
    To make the If scopes and nesting read more like C, we can use arbitrary nested scopes in the actual C code to make it read similarly, e.g.

    
    ```
    {
    	acpigen_write_method_serialized("FHRF", 1);
    	/* LOCAL0 = PERST# */
    	acpigen_get_tx_gpio(&config->perst_gpio);
    	acpigen_write_if_lequal_op_int(LOCAL0_OP, 0);
    	{
    		if (wwan_fm350gl_get_rtd3_method_support(config) | ACPI_PCIE_RP_EMIT_L23)
    			acpigen_emit_namestring(acpi_device_path_join(parent_dev, "DL23"));
    	
    		/* assert PERST# pin */
    		acpigen_enable_tx_gpio(&config->perst_gpio);
    	}
    	acpigen_write_if_end(); /* If */
    	
    	acpigen_write_sleep(FM350GL_TR2B);
    	
    	/* assert RESET# pin */
    	acpigen_enable_tx_gpio(&config->reset_gpio);
    	
    	acpigen_write_if_lequal_op_int(ARG0_OP, RESET_TYPE_WARM);
    	{
    		acpigen_write_sleep(FM350GL_TBTG);
    	}
    	acpigen_write_else();
    	{
    		acpigen_write_if_lequal_op_int(ARG0_OP, RESET_TYPE_COLD);
    		{
    			/* disable source clock */
    			if (wwan_fm350gl_get_rtd3_method_support(config) | ACPI_PCIE_RP_EMIT_SRCK)
    				acpigen_emit_namestring(acpi_device_path_join(parent_dev, "SRCK"));
    			acpigen_emit_byte(ZERO_OP);
    			acpigen_write_sleep(FM350GL_TB2F);
    			/* assert FCPO# pin */
    			acpigen_enable_tx_gpio(&config->fcpo_gpio);
    			acpigen_write_sleep(FM350GL_TFDI);
    		}
    		acpigen_write_if_end(); /* If */
    	}
    	acpigen_pop_len(); /* Else */

    	acpigen_write_method_end(); /* Method */
    }

    ```

To view, visit change 61354. To unsubscribe, or for help writing mail filters, visit settings.