11 comments:
File src/drivers/intel/mipi_camera/camera.c:
method_params[0] = config->clk_panel.clks[index].clknum;
method_params[1] = config->clk_panel.clks[index].freq;
acpigen_call_method("MCON", method_params, 2);
This is why I don't particularly like acpigen_call_method(), because I don't find this any easier to read (and I'd argue less readable) than:
acpigen_emit_namestring("MCON");
acpigen_write_integer(config->clk_panel.clks[index].clknum);
acpigen_write_integer(config->clk_panel.clks[]index].freq);
Patch Set #12, Line 512: (config->pr0
This should check for again NULL and supply a default. Or is a custom name always required?
acpigen_write_name_integer("STA", 0);
acpigen_write_STA_ext("STA");
acpigen_write_method_serialized("_ON", 0);
acpigen_write_if();
acpigen_emit_byte(LEQUAL_OP);
acpigen_emit_namestring("STA");
acpigen_write_integer(0);
add_power_resource(config, &config->on_seq);
acpigen_write_store_op_to_namestr(1, "STA");
acpigen_pop_len(); /* if */
acpigen_pop_len(); /* _ON */
/* _OFF operations */
acpigen_write_method_serialized("_OFF", 0);
acpigen_write_if();
acpigen_emit_byte(LEQUAL_OP);
acpigen_emit_namestring("STA");
acpigen_write_integer(1);
add_power_resource(config, &config->off_seq);
acpigen_write_store_op_to_namestr(0, "STA");
Why is the extra "STA" "variable" necessary? The OS should only call _ON and _OFF as appropriate, I don't think you need to guard against the OS calling it multiple times per suspend or resume.
acpigen_write_name("_DEP");
acpigen_write_package(1);
acpigen_emit_namestring(config->dep);
acpigen_pop_len();
I still don't understand this. The ACPI spec says this about _DEP:
_DEP evaluates to a package and designates device objects that OSPM should assign a higher priority in start ordering due to future operation region accesses.
What operation region is being accessed from 2 places ?
File src/drivers/intel/mipi_camera/chip.h:
Patch Set #12, Line 69: clk_ct
there could be a better name here, I'm not sure what the 'ct' refers to. maybe clk_config?
symbolic constant please
Patch Set #12, Line 84: gp_ctrl_panel
gpio_... ?
symbolic constant please
Patch Set #12, Line 85: struct gp_ct gps[4];
is the struct for gp_ct necessary? could just have uint8_t gpios[4].
Patch Set #12, Line 90: index
what is the index for? Doesn't their relative order in operation_seq define the order they're applied in?
Patch Set #12, Line 97: delay_ms
should specify (in a comment or in the name) if it is pre- or post-op delay
To view, visit change 41624. To unsubscribe, or for help writing mail filters, visit settings.