Re: [PATCH 2/2] drm/panel: add visionox vtdr6130 DSI panel driver

[Sam Ravnborg <sam@ravnborg.org>]

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