On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote: [...]
On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
[...]
+const char *regs[] = {
"vddp","dcdc_en","vcc"+};
This should be static. Also use a more sensible name, such as regulator_names, please.
it kept as regs, to keep constant names as used in the dsi_cfg file drivers/gpu/drm/msm/dsi/dsi_cfg.c
That's a completely different driver, no need to be consistent.
+static int jdi_panel_init(struct jdi_panel *jdi) +{
[...]
struct mipi_dsi_device *dsi = jdi->dsi;int ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_soft_reset(dsi);if (ret < 0)return ret;usleep_range(10000, 20000);ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);if (ret < 0)return ret;Please use the existing symbolic constants for this.
i am not clear on the symbolic constants for pixel_format ?
See include/video/mipi_display.h
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,(u8[]){ 0x24 }, 1);if (ret < 0)return ret;brightness control setting, to enable pwm/backlight
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,(u8[]){ 0x00 }, 1);if (ret < 0)return ret;this is to set cabc off/on
Can you point me to the specification of these? I'd like to investigate whether or not we can turn these into more sensible commands. As-is, it is completely obfuscated and I'd like to avoid that where possible.
ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);if (ret < 0)return ret;mdelay(10);Same here. This also needs at least a comment, though ideally you'd use symbolic names for those magic numbers.
i do not have the datasheet to give more description. this is for interface setting, either command mode/video mode
Okay, please add a comment on what this is supposed to do, then.
jdi->mode = &default_mode;for (i = 0; i < num; i++)s[i].supply = regs[i];ret = devm_regulator_bulk_get(dev, num, s);if (ret < 0) {dev_err(dev, "%s: failed to init regulator, ret=%d\n",__func__, ret);return ret;}jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);if (IS_ERR(jdi->reset_gpio)) {dev_err(dev, "cannot get reset-gpios %ld\n",PTR_ERR(jdi->reset_gpio));This is a third variant of error reporting. Please stick to one.
for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting cannot be changed to ret, others error reporting incorporated consistently.
PTR_ERR() returns signed long, not unsigned.
You can still use the same format for the message and substitute the %ld printk specifier to match the type.
Thierry