Hi Neil,
>
> >
> > > +
> > > +struct visionox_vtdr6130 {
> > > + struct drm_panel panel;
> > > + struct mipi_dsi_device *dsi;
> > > + struct gpio_desc *reset_gpio;
> > > + struct regulator_bulk_data supplies[3];
> > > + bool prepared;
> > > +};
> > > +
> > > +static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel)
> > > +{
> > > + return container_of(panel, struct visionox_vtdr6130, panel);
> > > +}
> > > +
> > > +static inline int visionox_vtdr6130_dsi_write(struct mipi_dsi_device *dsi, const void *seq,
> > > + size_t len)
> > > +{
> > > + return mipi_dsi_dcs_write_buffer(dsi, seq, len);
> > > +}
> > > +
> > > +#define dsi_dcs_write_seq(dsi, seq...) \
> > > + { \
> > > + const u8 d[] = { seq }; \
> > > + visionox_vtdr6130_dsi_write(dsi, d, ARRAY_SIZE(d)); \
> > > + }
> > Please use mipi_dsi_dcs_write_seq()
> > No need to add your own macros here.
> >
> > This will also add a little bit of error reporting that is missing here.
>
> OK, should I add a check and return in the macro in case of error ?
> Checkpatch emits some warning when this is done.
I expect you can use the macro as-is like this:
- dsi_dcs_write_seq(dsi, 0x51, 0x00, 0x00);
+ mipi_dsi_dcs_write_seq(dsi, 0x51, 0x00, 0x00);
So no need to create your own macro at all - just use the already
existing mipi_dsi_dcs_write_seq().
>
> >
> >
> > > +
> > > +static void visionox_vtdr6130_reset(struct visionox_vtdr6130 *ctx)
> > > +{
> > > + gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > > + usleep_range(10000, 11000);
> > > + gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> > > + usleep_range(10000, 11000);
> > > + gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > > + usleep_range(10000, 11000);
> > > +}
> > I have seen this pattern before - and I am still confused if the HW
> > really requires the 0 => 1 => 0 sequence.
> > I would expect writing 1 - wait and then writing 0 would do it.
>
> It's what downstream code uses and recommend all over the place, if it's an issue
> I can try to remove the first set_value
This was a fly-by comment - do what you find best.
> > > +
> > > + ret = mipi_dsi_dcs_set_display_brightness(dsi, cpu_to_le16(brightness));
> > mipi_dsi_dcs_set_display_brightness() take u16 as brightness - so this
> > will do an implicit conversion.
>
> I know, but the panel needs an inversed value, so perhaps I should directly
> call mipi_dsi_dcs_write_buffer() here instead of needing a double
> inversion.
If the generic one cannot be used without tricks like this, then yes, it
is better to hand-roll your own with a suitable comment.
Sam
