[PATCH 0/5] drm/tinydrm: Add tinydrm_panel abstraction

[Noralf Trønnes]

Den 12.03.2017 20.16, skrev Daniel Vetter:
> On Sun, Mar 12, 2017 at 06:50:17PM +0100, Daniel Vetter wrote:
>> Hi Noralf,
>>
>> On Sat, Mar 11, 2017 at 10:35:31PM +0100, Noralf Trønnes wrote:
>>> Add support for displays that have a register interface and can be
>>> operated using a simple vtable.
>>>
>>> I have looked through the staging/fbtft drivers and it seems that,
>>> except the MIPI controllers, most if not all controllers are operated
>>> through a register. And since most controllers have more than one bus
>>> interface option, regmap seems like a good choice to describe the
>>> interface (tested[1,2]).
>>> MIPI DCS can't be represented using regmap since some commands doesn't
>>> have a parameter. That would be like a register without a value, which
>>> doesn't make sense.
>>>
>>> In my second RFC of tinydrm I used drm_panel to decribe the panels
>>> since it was a good match to the fbtft displays. I was then told that
>>> drm_panel wasn't supposed to used like that, so I dropped it and have
>>> tried to use the drm_simple_display_pipe_funcs vtable directly. This
>>> hasn't been all successful, since I ended up using devm_add_action() to
>>> power down the controller at the right time. Thierry Reding wasn't
>>> happy with this and suggested "to add an explicit callback somewhere".
>>> My solution has been to copy the drm_panel_funcs vtable.
>>> Since I now have a vtable, I also added a callback to flush the
>>> framebuffer. So presumably all the fbtft drivers can now be operated
>>> through the tinydrm_panel_funcs vtable.
>> Ehrm, what? I admit I didn't follow the discussion in-depth, but if
>> drm_panel can't be used to describe a panel, it's not fit for the job and
>> needs to be extended. Adding even more abstraction, matroschka-style,
>> isn't a solution.
>>
>> Personally I think tinydrm itself is already a bit much wrapping, but I
>> see that for really simple drivers it has it's uses. But more layers feels
>> like going in the wrong direction.
>>
>> For the callback you're looking for (i.e. the regulator_disable call) I
>> think the correct place is to enable/disable the regulator in the
>> enable/disable hooks of the drm_simple_display_pipe functions. Or maybe in
>> their equivalent in drm_panel (well, probably pre_enable and post_disable,
>> since I guess you need that regulator to driver anything). Same for _init,
>> if the display is completely off there's no point in keeping the hw
>> running. Enabling/disabling the entire hw is pretty much what ->enable and
>> ->disable are for. This also gives you proper runtime pm for almost for
>> free ...
>>
>> Also, since the regulator is actually stored in struct mipi_dbi, it's
>> probably best to handle it in the mipi_dbi helpers too. You do that
>> already with the backlight anyway.
>>
>> I noticed that the version of tinydrm that landed doesn't use drm_panel
>> anymore, I thought that was the case once, and for the version I acked?
> Self-correct, there never was a version with drm_panel. tbh I think that's
> perfectly fine, tinydrm is aimed at simple panels behind spi/i2c buses
> (where also the entire video data is uploaded through spi/i2c, not just
> control information). Not changing anything like I recommend seems like
> the right action still (well, shuffling the regulator into
> simple_pipe->enable/disable like I think it should be).

I have looked at the emails, and I used drm_panel in the first RFC,
but I got the impression that Thierry didn't like it so it was dropped
in RFC v2.

The reason for making this patchset was to solve a problem of power
management that Thierry pointed out in the mi0283qt driver where I use
devm_add_action() to disable the regulator on module/device unload.
I haven't found a way to do PM in the simple drm pipeline.

I use drm_simple_display_pipe.enable to enable backlight since it's
called after drm_simple_display_pipe.update. If it was called before,
then I could use it to prepare the panel/controller. I remember having
seen some comments in the atomic code about reordering something to
make it match PM better. But if .enable() could be called before
.update(), how then do I control backlight?

I need to first power on and configure the controller, then it can
receive the initial framebuffer, and lastly the backlight is enabled.

No problem with you shredding this, if I can do this with much less
code, then all the better.

Thank you for spending time on this.

Noralf.