Hi Vinay,
I belive I've spotted a few issues. If my understanding is correct,
then I'll defer to Thierry if he'd like them fixed here, or as
follow-ups.
On 16 June 2016 at 04:00, Vinay Simha BN <simhavcs@xxxxxxxxx> wrote:
> +#define PANEL_NUM_REGULATORS 3
> +
Nit: #define PANEL_NUM_REGULATORS ARRAY_SIZE(regulator_names) or just
drop the extra define and use the latter directly ?
> +static int jdi_panel_init(struct jdi_panel *jdi)
> +{
> + if (ret < 0)
> + return ret;
> +
> + return 0;
Nit: The above three lines can become one - "return ret;"
> +}
> +
> +static int jdi_panel_on(struct jdi_panel *jdi)
> +{
> + struct mipi_dsi_device *dsi = jdi->dsi;
> + int ret;
> +
> + dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> + ret = mipi_dsi_dcs_set_display_on(dsi);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
Ditto.
> +static int jdi_panel_disable(struct drm_panel *panel)
> +{
> + struct jdi_panel *jdi = to_jdi_panel(panel);
> +
> + if (!jdi->enabled)
> + return 0;
> +
Thinking out loud:
Thierry,
Shouldn't we fold 'enabled' and 'prepared' in struct drm_panel and
tweak the helpers respectively ? Is there any specific reason for
keeping these in the drivers ?
> + if (jdi->backlight) {
We seems to be bailing out of jdi_panel_add() when this is NULL. Thus
we can omit the check.
> +static int jdi_panel_unprepare(struct drm_panel *panel)
> +{
> + struct jdi_panel *jdi = to_jdi_panel(panel);
> + struct device *dev = &jdi->dsi->dev;
> + int ret;
> +
> + if (!jdi->prepared)
> + return 0;
> +
> + ret = jdi_panel_off(jdi);
> + if (ret) {
> + dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regulator_bulk_disable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
> + if (ret < 0) {
> + dev_err(dev, "regulator disable failed, %d\n", ret);
> + return ret;
> + }
> +
Since we cannot recover from most/all of the above I'm thinking if one
shouldn't drop the "return ret" lines. Same goes for jdi_panel_off().
> + if (jdi->reset_gpio)
> + gpiod_set_value(jdi->reset_gpio, 0);
> +
> + if (jdi->enable_gpio)
Drop these two checks. The gpios are required, thus one should bail
out in jdi_panel_add()
> +static int jdi_panel_prepare(struct drm_panel *panel)
> +{
> + if (jdi->reset_gpio) {
> + gpiod_set_value(jdi->reset_gpio, 1);
> + usleep_range(10, 20);
> + }
> +
> + if (jdi->enable_gpio) {
> + gpiod_set_value(jdi->enable_gpio, 1);
> + usleep_range(10, 20);
> + }
> +
> +poweroff:
> + if (jdi->reset_gpio)
> + gpiod_set_value(jdi->reset_gpio, 0);
> + if (jdi->enable_gpio)
> + gpiod_set_value(jdi->enable_gpio, 0);
> +
Generic suggestion/nitpick: Please keep the teardown order the inverse
of the setup one. In here one could/should handle enable_gpio first
and then reset_gpio.
> + return ret;
> +}
> +
> +static int jdi_panel_enable(struct drm_panel *panel)
> +{
> + struct jdi_panel *jdi = to_jdi_panel(panel);
> +
> + if (jdi->enabled)
> + return 0;
> +
> + if (jdi->backlight) {
Analogous to jdi_panel_disable - drop the check ?
> +static int jdi_panel_add(struct jdi_panel *jdi)
> +{
> + 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));
> + jdi->reset_gpio = NULL;
> + } else {
> + gpiod_direction_output(jdi->reset_gpio, 0);
> + }
> +
> + jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR(jdi->enable_gpio)) {
> + dev_err(dev, "cannot get enable-gpio %ld\n",
> + PTR_ERR(jdi->enable_gpio));
> + jdi->enable_gpio = NULL;
> + } else {
> + gpiod_direction_output(jdi->enable_gpio, 0);
> + }
> +
As mentioned above - since these two are required, thus we should
error out of this function. Right ?
> + jdi->backlight = drm_panel_create_dsi_backlight(jdi->dsi);
> + if (!jdi->backlight)
> + return -EPROBE_DEFER;
> +
> + drm_panel_init(&jdi->base);
> + jdi->base.funcs = &jdi_panel_funcs;
> + jdi->base.dev = &jdi->dsi->dev;
> +
> + ret = drm_panel_add(&jdi->base);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
return ret;
Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
