Re: [PATCH] omapdss: Add new panel driver for Topolly td028ttec1 LCD.

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5821 bytes --]

On 10/10/13 12:34, Dr. H. Nikolaus Schaller wrote:
> Hi Tomi,
> 
> Am 10.10.2013 um 10:19 schrieb Tomi Valkeinen:
> 
>> Hi,
>>
>> On 10/10/13 00:08, Marek Belisko wrote:
>>> For communicating with driver is used gpio bitbanging because TD028 does
>>> not have a standard compliant SPI interface. It is a 3-wire thing with
>>> direction reversal.
>>
>> Isn't that SPI_3WIRE?
> 
> Maybe - but we have no complete datasheet and I don't know an official spec of
> a 3-wire SPI protocol to compare how 3-wire should work.

Yep, and I know only what I read on wikipedia a few hours ago =).

> And I think (but I may be wrong) that SPI_3WIRE is an optional feature of the SoC
> specific SPI drivers and in my understanding the OMAP has no such driver:
> 
> http://lxr.free-electrons.com/source/drivers/spi/spi-omap2-mcspi.c#L874
> 
> And spi-bitbang.c hasn't either.
> 
> So I would prefer to leave this as an area for optimizations for future work.
> Maybe we can add some more comments on this.

Ok. Well, I guess it's not too bad. But it's really not something that
should be in the panel driver (assuming it's "standard" 3-wire SPI).
Some SoC could support 3-wire SPI, and then it'd be good to use the SoCs
hardware for SPI communication. Having bitbanging hardcoded into the
driver will prevent that.

>>> +	int cs_gpio;
>>> +	int scl_gpio;
>>> +	int din_gpio;
>>> +	int dout_gpio;
>>
>> Three wires, but four gpios? What am I missing here...
> 
> We have wired up the 3-wire SPI interface of the display
> to 4 wires of the McSPI interface because we initially thought
> that it could work in 4 wire mode as well.
> 
> The next step we did was to port the driver code from the
> Openmoko Neo1973 U-Boot which used the gpio-bitbang
> mechanism.
> 
> Since it worked and is used only for enabling/disabling the
> display and for no other purpose we never felt it important
> to check or modify the code to use SPI.
> 
> But you are right - we don't use the din_gpio really since
> we never read registers from the chip. So 3 gpios could be
> sufficient.
> 
> Or we must handle the case that din_gpio == dout_gpio
> in panel_probe to avoid duplicate use of the same gpio.

The panel hardware has three wires, so the panel driver (if it does
handle the bus by bitbanging) can only refer to three gpios. So either
the bus details should be hidden by using the normal spi framework, or
if the driver does the bitbanging, use the gpios as specified in the
panel spec. The panel driver cannot contain any board specific stuff.

>>> +static int jbt_spi_xfer(struct panel_drv_data *data, int wordnum, int bitlen)
>>
>> Hmm, if it's not SPI, maybe it shouldn't be called SPI?
> 
> Yes, we can remove the _spi_ in this name.

Or maybe use "spi3w" or such, just to make it clear it's not plain
standard spi. Again, presuming it's really 3-wire spi =).

>>> +static int td028ttec1_panel_probe(struct platform_device *pdev)
>>> +{
>>> +	struct panel_drv_data *ddata;
>>> +	struct omap_dss_device *dssdev;
>>> +	int r;
>>> +
>>> +	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
>>> +	if (ddata == NULL)
>>> +		return -ENOMEM;
>>> +
>>> +	platform_set_drvdata(pdev, ddata);
>>> +
>>> +	if (dev_get_platdata(&pdev->dev)) {
>>> +		r = td028ttec1_panel_probe_pdata(pdev);
>>> +		if (r)
>>> +			return r;
>>> +	} else {
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	if (gpio_is_valid(ddata->cs_gpio)) {
>>> +		r = devm_gpio_request_one(&pdev->dev, ddata->cs_gpio,
>>> +				GPIOF_OUT_INIT_HIGH, "lcd cs");
>>> +		if (r)
>>> +			goto err_gpio;
>>> +	}
>>> +
>>> +	if (gpio_is_valid(ddata->scl_gpio)) {
>>> +		r = devm_gpio_request_one(&pdev->dev, ddata->scl_gpio,
>>> +				GPIOF_OUT_INIT_HIGH, "lcd scl");
>>> +		if (r)
>>> +			goto err_gpio;
>>> +	}
>>> +
>>> +	if (gpio_is_valid(ddata->dout_gpio)) {
>>> +		r = devm_gpio_request_one(&pdev->dev, ddata->dout_gpio,
>>> +				GPIOF_OUT_INIT_LOW, "lcd dout");
>>> +		if (r)
>>> +			goto err_gpio;
>>> +	}
>>> +
>>> +	if (gpio_is_valid(ddata->din_gpio)) {
>>> +		r = devm_gpio_request_one(&pdev->dev, ddata->din_gpio,
>>> +				GPIOF_IN, "lcd din");
>>> +		if (r)
>>> +			goto err_gpio;
>>> +	}
>>> +
>>> +	/* according to data sheet: wait 50ms (Tpos of LCM). However, 50ms
>>> +	 * seems unreliable with later LCM batches, increasing to 90ms */
>>> +	mdelay(90);
>>
>> What is this delay for? Usually sleeps/delays should be between two "HW
>> actions". Here there's nothing below this line that would count as such
>> an action.
> 
> I guess that this delay is intended to power on the display first, then wait some

I'm not very comfortable with merging a driver, when the driver author
guesses what parts of the driver do =).

> time and after that delay enable the DSS clocks and signals and make the
> controller chip in the panel initialize.

I don't see the code before the delay doing any power up, it just sets
the data bus gpios to certain state. Do those gpio initializations power
up the panel?

> But this of course depends on the order the DSS components are probed
> and enabled and how power is controlled (we don't interrupt power at all).
> 
> I think we can move this delay (sleep) to the beginning of 
> 
> td028ttec1_panel_enable() before /* three times command zero */
>
> to make sure that a freshly powered display has enough time to do its
> internal reset and initializations before registers are programmed.

The probe function should not enable the panel (well, it can enable the
panel, but only to read an ID or such and then disable it before exiting
probe).

So after probe, the panel should be as dead as possible, power disabled
etc. All the powering up should happen in enable().

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]