Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 29 May 2023 14:09:34 +0200
From:   Marijn Suijten <marijn.suijten@somainline.org>
To:     Neil Armstrong <neil.armstrong@linaro.org>
Cc:     Jessica Zhang <quic_jesszhan@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        devicetree@vger.kernel.org, Sam Ravnborg <sam@ravnborg.org>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Subject: Re: [PATCH 2/2] drm/panel: Add driver for Visionox r66451 panel
Message-ID: <6enrlcmw6a2vxwzocsdbcx7yaltzh3kv4w3yiydzx7ylrtdozn@xzwdj35uj7na>
References: <20230516-b4-r66451-panel-driver-v1-0-4210bcbb1649@quicinc.com>
 <20230516-b4-r66451-panel-driver-v1-2-4210bcbb1649@quicinc.com>
 <3cca2809-fa26-f0cf-2ccc-6737d150b43d@linaro.org>
 <kc65fjf5xibvszx4afomzbmfrlld4vcnwfxqxefrupkaxw74fa@j2op4bj2recm>
 <2d9d7454-0e7d-b6f2-93ff-9eca69380e47@quicinc.com>
 <9d50bc4e-e747-47fa-03cf-4dbd33f778dd@quicinc.com>
 <5becf303-8a23-6ace-2379-0a002e9b48f7@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5becf303-8a23-6ace-2379-0a002e9b48f7@linaro.org>
Precedence: bulk

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.

- Marijn