Re: [PATCH 6/6] drm: tiny: gdepaper: add driver for 2/3 color epaper displays

[Jan Sebastian GÃtte]

Hi Noralf,

thank you for your comments. I've incorporated your suggestions into my draft.

On 8/7/19 1:06 AM, Noralf TrÃnnes wrote:
[...]

>> +static int gdepaper_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct device_node *np = dev->of_node;
>> +	const struct of_device_id *of_id;
>> +	struct drm_device *drm;
>> +	struct drm_display_mode *mode;
>> +	struct gdepaper *epap;
>> +	const struct gdepaper_type_descriptor *type_desc;
>> +	int ret;
>> +	size_t bufsize;
>> +
>> +	of_id = of_match_node(gdepaper_of_match, np);
>> +	if (WARN_ON(of_id == NULL)) {
>> +		dev_warn(dev, "dt node didn't match, aborting probe\n");
>> +		return -EINVAL;
>> +	}
>> +	type_desc = of_id->data;
>> +
>> +	dev_dbg(dev, "Probing gdepaper module\n");
>> +	epap = kzalloc(sizeof(*epap), GFP_KERNEL);
>> +	if (!epap)
>> +		return -ENOMEM;
>> +
>> +	epap->enabled = false;
>> +	mutex_init(&epap->cmdlock);
>> +	epap->tx_buf = NULL;
>> +	epap->spi = spi;
>> +
>> +	drm = &epap->drm;
>> +	ret = devm_drm_dev_init(dev, drm, &gdepaper_driver);
>> +	if (ret) {
>> +		dev_warn(dev, "failed to init drm dev\n");
>> +		goto err_free;
>> +	}
>> +
>> +	drm_mode_config_init(drm);
>> +
>> +	epap->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(epap->reset)) {
>> +		dev_err(dev, "Failed to get reset GPIO\n");
>> +		ret = PTR_ERR(epap->reset);
>> +		goto err_free;
>> +	}
>> +
>> +	epap->busy = devm_gpiod_get(dev, "busy", GPIOD_IN);
>> +	if (IS_ERR(epap->busy)) {
>> +		dev_err(dev, "Failed to get busy GPIO\n");
>> +		ret = PTR_ERR(epap->busy);
>> +		goto err_free;
>> +	}
>> +
>> +	epap->dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW);
>> +	if (IS_ERR(epap->dc)) {
>> +		dev_err(dev, "Failed to get dc GPIO\n");
>> +		ret = PTR_ERR(epap->dc);
>> +		goto err_free;
>> +	}
>> +
>> +	epap->spi_speed_hz = 2000000;
>> +	epap->pll_div = 1;
>> +	epap->framerate_mHz = 81850;
>> +	epap->rfp.vg_lv = GDEP_PWR_VGHL_16V;
>> +	epap->rfp.vcom_sel = 0;
>> +	epap->rfp.vdh_bw_mv = 11000; /* drive high level, b/w pixel */
>> +	epap->rfp.vdh_col_mv = 4200; /* drive high level, red/yellow pixel */
>> +	epap->rfp.vdl_mv = -11000; /* drive low level */
>> +	epap->rfp.border_data_sel = 2; /* "vbd" */
>> +	epap->rfp.data_polarity = 0; /* "ddx" */
>> +	epap->rfp.vcom_dc_mv = -1000;
>> +	epap->rfp.vcom_data_ivl_hsync = 10; /* hsync periods */
>> +	epap->rfp.use_otp_luts_flag = 1;
>> +	epap->ss_param[0] = 0x07;
>> +	epap->ss_param[1] = 0x07;
>> +	epap->ss_param[2] = 0x17;
>> +	epap->controller_res = GDEP_CTRL_RES_320X300;
>> +
>> +	ret = gdepaper_of_read_luts(epap, np, dev);
>> +	if (ret) {
>> +		dev_warn(dev, "can't read LUTs from dt\n");
>> +		goto err_free;
>> +	}
>> +
>> +	of_property_read_u32(np, "controller-resolution",
>> +			&epap->controller_res);
>> +	of_property_read_u32(np, "spi-speed-hz", &epap->spi_speed_hz);
>> +	epap->partial_update_en = of_property_read_bool(np, "partial-update");
>> +	ret = of_property_read_u32(np, "colors", &epap->display_colors);
>> +	if (ret == -EINVAL) {
>> +		if (type_desc) {
>> +			epap->display_colors = type_desc->colors;
>> +
>> +		} else {
>> +			dev_err(dev, "colors must be set in dt\n");
>> +			ret = -EINVAL;
>> +			goto err_free;
>> +		}
>> +	} else if (ret) {
>> +		dev_err(dev, "Invalid dt colors property\n");
>> +		goto err_free;
>> +	}
>> +	if (epap->display_colors < 0 ||
>> +			epap->display_colors >= GDEPAPER_COL_END) {
>> +		dev_err(dev, "invalid colors value\n");
>> +		ret = -EINVAL;
>> +		goto err_free;
>> +	}
>> +	epap->mirror_x = of_property_read_bool(np, "mirror-x");
>> +	epap->mirror_y = of_property_read_bool(np, "mirror-y");
>> +	of_property_read_u32(np, "pll-div", &epap->pll_div);
>> +	of_property_read_u32(np, "fps-millihertz", &epap->framerate_mHz);
>> +	of_property_read_u32(np, "vghl-level", &epap->rfp.vg_lv);
>> +	epap->vds_en = !of_property_read_bool(np, "vds-external");
>> +	epap->vdg_en = !of_property_read_bool(np, "vdg-external");
>> +	of_property_read_u32(np, "vcom", &epap->rfp.vcom_sel);
>> +	of_property_read_u32(np, "vdh-bw-millivolts", &epap->rfp.vdh_bw_mv);
>> +	of_property_read_u32(np, "vdh-color-millivolts", &epap->rfp.vdh_col_mv);
>> +	of_property_read_u32(np, "vdl-millivolts", &epap->rfp.vdl_mv);
>> +	of_property_read_u32(np, "border-data", &epap->rfp.border_data_sel);
>> +	of_property_read_u32(np, "data-polarity", &epap->rfp.data_polarity);
>> +	ret = of_property_read_u8_array(np, "boost-soft-start",
>> +			(u8 *)&epap->ss_param, sizeof(epap->ss_param));
>> +	if (ret && ret != -EINVAL)
>> +		dev_err(dev, "invalid boost-soft-start value, ignoring\n");
>> +	of_property_read_u32(np, "vcom-data-interval-periods",
>> +			&epap->rfp.vcom_data_ivl_hsync);
> 
> Why do you need these DT properties when you define compatibles for all
> the panels, can't you include these settings in the type descriptor?
	I allowed for these to be overridden in case there is some panel that's not listed on the mfg's (quite chaotic) website. Looking at this some more I think I'll remove some of these though.

I'll leave vds-external/vgs-external since they depend on circuitry around the panel and thus should be in DT. boost-soft-start is largely undocumented and I don't know what they might be useful for, but I feel it could depend on the booster inductors and voltage regulator connected to the panel, so it should be in DT.

Those ending up in the refresh params struct are refresh-related and thus application-specific. Most of these come with probably sane defaults, so to initialize a display at a minimum you only need either the type (compatible=gdew...) or the dimensions (px, mm) and color scheme.

>> +
>> +	/* Accept both positive and negative notation */
>> +	if (epap->rfp.vdl_mv < 0)
>> +		epap->rfp.vdl_mv = -epap->rfp.vdl_mv;
>> +	if (epap->rfp.vcom_dc_mv < 0)
>> +		epap->rfp.vcom_dc_mv = -epap->rfp.vcom_dc_mv;
>> +
>> +	/* (from mipi-dbi.c:)
>> +	 * Even though it's not the SPI device that does DMA (the master does),
>> +	 * the dma mask is necessary for the dma_alloc_wc() in
>> +	 * drm_gem_cma_create(). The dma_addr returned will be a physical
>> +	 * address which might be different from the bus address, but this is
>> +	 * not a problem since the address will not be used.
>> +	 * The virtual address is used in the transfer and the SPI core
>> +	 * re-maps it on the SPI master device using the DMA streaming API
>> +	 * (spi_map_buf()).
>> +	 */
>> +	if (!dev->coherent_dma_mask) {
>> +		ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> +		if (ret) {
>> +			dev_warn(dev, "Failed to set dma mask %d\n", ret);
>> +			goto err_free;
>> +		}
>> +	}
>> +
>> +	mode = gdepaper_of_read_mode(type_desc, np, dev);
>> +	if (IS_ERR(mode)) {
>> +		dev_warn(dev, "Failed to read mode: %ld\n", PTR_ERR(mode));
>> +		ret = PTR_ERR(mode);
>> +		goto err_free;
>> +	}
>> +
>> +	/* 8 pixels per byte, bit-packed */
>> +	bufsize = (mode->vdisplay * mode->hdisplay + 7)/8;
> 
> DIV_ROUND_UP(mode->vdisplay * mode->hdisplay, 8)
> 
>> +	epap->tx_buf = devm_kmalloc(drm->dev, bufsize, GFP_KERNEL);
>> +	if (!epap->tx_buf) {
>> +		ret = -ENOMEM;
>> +		goto err_free;
>> +	}
>> +
>> +	/* TODO rotation support? */
>> +	ret = tinydrm_display_pipe_init(drm, &epap->pipe, &gdepaper_pipe_funcs,
>> +					DRM_MODE_CONNECTOR_VIRTUAL,
>> +					gdepaper_formats,
>> +					ARRAY_SIZE(gdepaper_formats), mode, 0);
> 
> tinydrm_display_pipe_init() is gone now, here's how I replaced it in the
> other e-ink driver:
> 
> drm/tinydrm/repaper: Don't use tinydrm_display_pipe_init()
> https://cgit.freedesktop.org/drm/drm-misc/commit?id=1321db837549a0ff9dc2c95ff76c46770f7f02a1
Thank you. I found an almost identical solution.

- Jan