Re: [PATCH v6 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

[Vinay Simha]

On Sat, Jun 18, 2016 at 5:58 AM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> Hi Vinay,
>
> On 17 June 2016 at 19:23, Vinay Simha BN <simhavcs@xxxxxxxxx> wrote:
>
>> v6:
>>  * emil review comments incorporated
>>    PANEL_NUM_REGULATORS dropped, return ret added at necessary
>>    places, if checks dropped for backlight and gpios
>
> Looks like some of my suggestions went below the radar. Did you miss
> them or I've I lost the plot somewhere ? In case of the latter don't
> be shy to point out :-)
>
>
>> +struct jdi_panel {
>> +       struct drm_panel base;
>> +       struct mipi_dsi_device *dsi;
>> +
>> +       struct regulator_bulk_data supplies[3];
>> +
> struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
>
>
>> +static int jdi_panel_off(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_off(dsi);
>> +       if (ret < 0)
>> +               return ret;
>> +
> IMHO neither this function nor its caller jdi_panel_unprepare() should
> stop in the middle/return prematurely.
>
> Or in other words - one should change the function return type to void
> and drop the returns.
>
>
>> +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;
> As suggested above, drop this return.
>
i can make the function void for jdi_panel_off  and drop the return,
but i cannot make void for jdi_panel_unprepare,
since drm_panel_prepare requires 0 or negative value.


* call to drm_panel_prepare().
 *
 * Return: 0 on success or a negative error code on failure.
 */
static inline int drm_panel_unprepare(struct drm_panel *panel)
{
        if (panel && panel->funcs && panel->funcs->unprepare)
                return panel->funcs->unprepare(panel);

        return panel ? -ENOSYS : -EINVAL;
}

>
>
>> +static int jdi_panel_prepare(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 = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
>> +       if (ret < 0) {
>> +               dev_err(dev, "regulator enable failed, %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       msleep(20);
>> +
>> +       if (jdi->reset_gpio) {
> You can drop the check.
>
>> +               gpiod_set_value(jdi->reset_gpio, 1);
>> +               usleep_range(10, 20);
>> +       }
>> +
>> +       if (jdi->enable_gpio) {
> Ditto.
>
>
>> +
>> +poweroff:
>> +       gpiod_set_value(jdi->enable_gpio, 0);
>> +       gpiod_set_value(jdi->reset_gpio, 0);
>> +
> Just noticed that you're missing regulator_bulk_disable() here.
>
>
> Regards,
> Emil



-- 
Regards,

Vinay Simha.B.N.