Re: [PATCH 2/2] drm/panel: Add driver for Visionox r66451 panel

[Jessica Zhang]

On 5/29/2023 5:09 AM, Marijn Suijten wrote:
> On 2023-05-26 09:32:45, Neil Armstrong wrote:
> <snip>
> > > > > > > +static int visionox_r66451_bl_update_status(struct backlight_device *bl)
> > > > > > > +{
> > > > > > > +    struct mipi_dsi_device *dsi = bl_get_data(bl);
> > > > > > > +    u16 brightness = backlight_get_brightness(bl);
> > > > > > > +
> > > > > > > +    return mipi_dsi_dcs_set_display_brightness(dsi, cpu_to_le16(brightness));
> > > > > >
> > > > > > mipi_dsi_dcs_set_display_brightness() already converts the brightness,
> > > > > > so you don't need cpu_to_le16 here.
> > > > >
> > > > > Tread carefully here: we've had the same issue and conversation on our
> > > > > Sony panels where this extra inversion is required.
> > > > > set_display_brightness() sends the bytes as little-endian to the panel
> > > > > (and it even assumes little-endian in get_display_brightness()) but the
> > > > > spec for 16-bit brightness values states that they have to be sent in
> > > > > big-endian.  This is why c9d27c6be518b ("drm/mipi-dsi: Fix byte order of
> > > > > 16-bit DCS set/get brightness") added
> > > > > mipi_dsi_dcs_set_display_brightness_large().
> > > > >
> > > > > Jessica, if you need to have the endian swap here (should be very easy
> > > > > to test with a real panel, but it should be given the max_brightness
> > > > > value being over 8 bits) please switch to the _large() variant.
> > > >
> > > > Got it, thanks for the heads up!
> > >
> > > Hi Marijn,
> > >
> > > Just wanted to update this thread -- I've checked the backlight brightness values in the sysfs and it matches the value being given in the panel driver (255), so I think it should be fine to use *_set_display_brightness() without the _large() variant.
> >
> > Sure, I was also misleaded by you using cpu_to_le16() but as Dmitry said it's already
> > done in mipi_dsi_dcs_set_display_brightness() and a no-op on LE arm64 platforms anyway.
>
> Yuck, right, it's cpu_to_le16 here and not cpu_to_be16.  @Jessica, can
> you please remove this misleading conversion?
> mipi_dsi_dcs_set_display_brightness() takes a native u16, not a specific
> __le16.

Hi Marijn,

Acked, will drop the conversion.

Thanks,

Jessica Zhang

> - Marijn