Hi Stefan, > > + */ > > +static u32 swap_bytes(u16 var) > > We certainly don't want a byte swapping function in the driver. I am sure > Linux > has appropriate functions for that already, however, I am not convinced that > we need that at all. >
Yeah, sure. Actually I had sent the V2 for this feature, which just using "<<24" to do this https://patchwork.kernel.org/patch/9252389/ ... > > + }; > > + > > + struct drm_device *dev = crtc->dev; > > + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private; > > + unsigned int i; > > + struct rgb glut; > > + > > + for (i = 0; i < size; i++) { > > + glut.r[i] = swap_bytes(r[i]); > > + glut.g[i] = swap_bytes(g[i]); > > + glut.b[i] = swap_bytes(b[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_R + 4 * i, > glut.r[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_G + 4 * i, > glut.g[i]); > > + regmap_write(fsl_dev->regmap, FSL_GAMMA_B + 4 * i, > glut.b[i]); > > I guess the problem is that regmap_write does byte swapping because > ls1021a.dtsi defines the whole DCU register space to be big-endian. So you end > up doing byte swapping twice. > > If the gamma area is really little-endian, then DCU on LS1021a seems to be > quite a mess... :-( > > In this case, we probably should create a second regmap for the different > areas > specifically, e.g. change the device tree: > > reg = <0x0 0x2ce0000 0x0 0x2000 > 0x0 0x2ce2000 0x0 0x2000 > 0x0 0x2ce4000 0x0 0xc00 > 0x0 0x2ce4c00 0x0 0x400>; > > reg-names = "regs", "palette", "gamma", "cursor"; > i > /* Use Gamma is always little endian */ > static const struct regmap_config fsl_dcu_regmap_gamma_config = { ... > .val_format_endian = REGMAP_ENDIAN_LITTLE, ... > }; > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gamma"); > base_gamma = devm_ioremap_resource(dev, res); > > fsl_dev->regmap_gamma = devm_regmap_init_mmio(...) > > > regmap_write(fsl_dev->regmap_gamma, ...) > This is a errta for DCU, Gamma registers are supposed to be big endian, but actually it is little endian, do we Really need to create a second regmap? > > @Mark, what do you think? Do we have a (better) solution for such cases? > > > + } > > + > > + return 0; > > +} > > + > > static const struct drm_crtc_funcs fsl_dcu_drm_crtc_funcs = { > > .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > > .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > > @@ -135,6 +197,7 @@ static const struct drm_crtc_funcs > > fsl_dcu_drm_crtc_funcs = { > > .page_flip = drm_atomic_helper_page_flip, > > .reset = drm_atomic_helper_crtc_reset, > > .set_config = drm_atomic_helper_set_config, > > + .gamma_set = fsl_crtc_gamma_set, > > }; > > > > int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev) diff > > --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > index 3b371fe7..d3bc540 100644 > > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > @@ -25,6 +25,9 @@ > > #define DCU_MODE_NORMAL 1 > > #define DCU_MODE_TEST 2 > > #define DCU_MODE_COLORBAR 3 > > +#define DCU_MODE_EN_GAMMA_MASK 0x04 > > Nit: In cases where MASK is a single bit, you can use BIT(..)... > > > +#define DCU_MODE_GAMMA_ENABLE BIT(2) > > +#define DCU_MODE_GAMMA_DISABLE 0 > > That sounds like a useless define to me. In the disable case, just use 0 in > regmap_update_bits. The .._MASK shows which bit you clear. > Yeah, sure. Thanks. Best Regards, Meng
