linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Sebastian Götte" <linux@jaseg.net>
To: "Noralf Trønnes" <noralf@tronnes.org>, dri-devel@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/6] drm: tiny: gdepaper: add driver for 2/3 color epaper displays
Date: Sat, 10 Aug 2019 14:17:06 +0900	[thread overview]
Message-ID: <7ebf06e2-fe24-a4fb-25ff-77ce1ee3ae16@jaseg.net> (raw)
In-Reply-To: <604c82ee-af16-5f34-b229-5e919c4adfdc@tronnes.org>

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

  reply	other threads:[~2019-08-10  5:17 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30 13:46 [RFC PATCH 0/6] tiny: Add driver for gooddisplay epaper panels Jan Sebastian Götte
2019-07-30 13:47 ` [PATCH 1/6] drm: tiny: gdepaper: add TINYDRM_GDEPAPER config option Jan Sebastian Götte
2019-07-30 13:48 ` [PATCH 2/6] dt-bindings: add gooddisplay vendor prefix Jan Sebastian Götte
2019-07-30 13:48 ` [PATCH 3/6] dt-bindings: add good display epaper header Jan Sebastian Götte
2019-07-30 13:48 ` [PATCH 4/6] dt-bindings: add gdepaper documentation Jan Sebastian Götte
2019-07-30 13:48 ` [PATCH 5/6] drm: uapi: add gdepaper uapi header Jan Sebastian Götte
2019-07-30 14:08   ` Emil Velikov
2019-07-30 16:21     ` Jan Sebastian Götte
2019-07-30 16:49       ` Emil Velikov
2019-07-31  2:01         ` Jan Sebastian Götte
2019-07-31  6:47           ` Daniel Vetter
2019-07-30 13:48 ` [PATCH 6/6] drm: tiny: gdepaper: add driver for 2/3 color epaper displays Jan Sebastian Götte
2019-08-06 16:06   ` Noralf Trønnes
2019-08-10  5:17     ` Jan Sebastian Götte [this message]
2019-07-30 14:26 ` [RFC PATCH 0/6] tiny: Add driver for gooddisplay epaper panels Daniel Vetter
2019-07-30 16:24   ` Jan Sebastian Götte

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7ebf06e2-fe24-a4fb-25ff-77ce1ee3ae16@jaseg.net \
    --to=linux@jaseg.net \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=noralf@tronnes.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).