On 2016-09-04 19:28, Meng Yi wrote: > 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/ > Oh sorry, missed that patch. However, the discussion below is still valid: > ... > >> > + }; >> > + >> > + 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? Do you have the exact wording of the errata? There are two problems to the current approach: First, the two byte swaps (yours and then the byte swap due to the big-endian configuration of regmap) seems ugly to me. Second, the gamma value is the lowest byte in Vybrid, and on Vybrid the regmap is configured little-endian. Hence your code won't work on Vybrid... Therefor I still think that using a second regmap would be nicer. -- Stefan _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
