Hi Richard, On Fri, Jan 13, 2023 at 04:24:23PM -0500, Richard Acayan wrote: > On Fri, Jan 13, 2023 at 05:34:55PM +0100, Sam Ravnborg wrote: > > Hi Richard/Daniel. > Not sure if you saw the original commit that I linked. > > This patch was written in 2017 for the Pixel 3 and Pixel 3 XL > smartphones. I didn't contact the original author before sending this, > since the kernel that this patch originated from is EOL. > > This greeting is still fine because it would be great if the original > author took responsibility for getting this patch applied. > > > > > On Thu, Jan 12, 2023 at 11:18:48PM -0500, Richard Acayan wrote: > >> From: Daniel Mentz <danielmentz@xxxxxxxxxx> > >> > >> The MIPI DCS specification demands that brightness values are sent in > >> big endian byte order. It also states that one parameter (i.e. one byte) > >> shall be sent/received for 8 bit wide values, and two parameters shall > >> be used for values that are between 9 and 16 bits wide. > > It is only a few week ago someone write they needed a byteswap to use > > the kernel provided mipi_dsi_dcs_set_display_brightness(). I did not > > realize then that this is because it is buggy. > I didn't even consider sending this patch when I initially saw it. I > would have performed the byte swap in a new S6E3FA7 panel driver just > like in SOFEF00. > > > > > Some comments on the patch: > > - It would be nice to split it up so first patch fixes that the > > parameters are in big-endian. This would go for both set and get. > The brightness is only big endian if it's 16-bit. > > Most MIPI panel drivers in the kernel tree only send an 8-bit > brightness, or none at all. For these panels, the brightness can be sent > as a 16-bit integer in little endian, as it has the same effect as an > 8-bit integer, with a zero byte appended. The original MIPI brightness > functions work properly with no changes for 8-bit brightness panels > only. > > If we want separate 8-bit (original) and 16-bit functions, the 8-bit > functions shouldn't pass the brightness in big endian. That doesn't mean > we shouldn't change them at all. Agree, the brightness is passed exactly as returned by backlight_get_brightness(), and the helper take care of the correct byte ordering. > > > > > - Second patch could then introduce the possibility to a driver to > > use a variant that matches the display. > > I would suggest to go for two functions: > > mipi_dsi_dcs_set_display_brightness() > > mipi_dsi_dcs_set_display_brightness_two() > I'm thinking of a "_large" or "_wide" suffix instead. This would make a > 32-bit suffix tricky though, if it is ever needed. _large is way better than _two, and would be my choice. > > > So it is obvious when you go for the two byte version without adding a > > bool parameter. > > If the implementation is done using a helper or two independent > > functions are up to you. > These get_display_brightness and set_display_brightness functions are > already helpers. I'll have them duplicated. > > > > > The patch split would be nice, but at least we should have two functions > > and not a bool parameter. > Yes, that was a bad idea on my part. With separate functions for 16-bit > brightness, there is no API change, less panel driver touching, and > panel driver changes can happen in separate patches. Thank you for the > suggestion. Great, looks forward for the patches. Sam
