[PATCH v2] drm/panel: db7430: Add driver for Samsung DB7430

[Doug Anderson]

Hi,

On Mon, May 31, 2021 at 3:30 AM Linus Walleij <linus.walleij at linaro.org> wrote:
>
> +/**
> + * struct db7430 - state container for the Db7430 panel

super nitty: s/Db7430/DB7430/

Also, it's not technically "the DB7430 panel" but instead "a panel
controlled by the DB7430 controller".


> +static int db7430_dcs_write(struct db7430 *lms, const u8 *data, size_t len)
> +{
> +       int ret;
> +
> +       dev_dbg(lms->dev, "SPI writing dcs seq: %*ph\n", (int)len, data);
> +
> +       /*
> +        * This sends 9 bits with the first bit (bit 8) set to 0
> +        * This indicates that this is a command. Anything after the
> +        * command is data.
> +        */
> +       ret = db7430_write_word(lms, *data);
> +
> +       while (!ret && --len) {
> +               ++data;
> +               /* This sends 9 bits with the first bit (bit 8) set to 1 */
> +               ret = db7430_write_word(lms, *data | DATA_MASK);
> +       }
> +
> +       if (ret) {
> +               dev_err(lms->dev, "SPI error %d writing dcs seq: %*ph\n", ret,
> +                       (int)len, data);
> +       }
> +
> +       return ret;
> +}

Still hoping that this can work atop DBI so we can avoid the raw SPI
writes. You said you're trying for it for v3 so I'm looking forward to
checking it out there.


> +static int db7430_power_on(struct db7430 *lms)
> +{
> +       int ret;
> +
> +       /* Power up */
> +       ret = regulator_bulk_enable(ARRAY_SIZE(lms->regulators),
> +                                   lms->regulators);
> +       if (ret) {
> +               dev_err(lms->dev, "failed to enable regulators: %d\n", ret);
> +               return ret;
> +       }
> +       msleep(50);
> +
> +       /* Assert reset >=1 ms */
> +       gpiod_set_value_cansleep(lms->reset, 1);
> +       usleep_range(1000, 5000);
> +       /* De-assert reset */
> +       gpiod_set_value_cansleep(lms->reset, 0);
> +       /* Wait >= 10 ms */
> +       msleep(10);
> +       dev_dbg(lms->dev, "de-asserted RESET\n");
> +
> +       /*
> +        * This is set to 0x0a (RGB/BGR order + horizontal flip) in order
> +        * to make the display behave normally. If this is not set the displays
> +        * normal output behaviour is horizontally flipped and BGR ordered. Do
> +        * it twice because the first message doesn't always "take".
> +        */
> +       db7430_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);

In response to v1, I asked:

Also: should we be error-checking lms397kf04_dcs_write_seq_static()
return values in this function? spi_write() can fail...

It still seems like it'd be nice to error check, even if you just
print a message in the helper function and then go on with the rest of
the function (to simplify control flow).


> +       db7430_dcs_write_seq_static(lms, MIPI_DCS_SET_ADDRESS_MODE, 0x0a);
> +       /* Called "Access protection off" in vendor code */
> +       db7430_dcs_write_seq_static(lms, DB7430_ACCESS_PROT_OFF, 0x00);

Now that you've updated the #define to include the words
"ACCESS_PROT_OFF" you probably don't need the comment.


> +/**
> + * db7430_get_modes() - return the mode
> + * @panel: the panel to get the mode for
> + * @connector: reference to the central DRM connector control structure
> + */
> +static int db7430_get_modes(struct drm_panel *panel,
> +                           struct drm_connector *connector)
> +{
> +       struct db7430 *lms = to_db7430(panel);
> +       struct drm_display_mode *mode;
> +       static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +
> +       mode = drm_mode_duplicate(connector->dev, &db7430_mode);
> +       if (!mode) {
> +               dev_err(lms->dev, "failed to add mode\n");
> +               return -ENOMEM;
> +       }
> +
> +       connector->display_info.width_mm = mode->width_mm;
> +       connector->display_info.height_mm = mode->height_mm;
> +       connector->display_info.bus_flags =
> +               DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +       drm_display_info_set_bus_formats(&connector->display_info,
> +                                        &bus_format, 1);

In my review of v1, I asked:

panel-simple also sets the bpc in the "display_info". Do you need to?

I didn't see a reply, so I'm still curious about the answer.


-Doug