Hi Nicholas. On Mon, Aug 13, 2018 at 05:54:48PM +0200, Nicolas Ferre wrote: > On 12/08/2018 at 20:41, Sam Ravnborg wrote: > >New DRM based driver for at91sam9 SOC's that uses the > >Atmel LCDC IP core. > > I'm delighted to see this work: Thanks a lot Sam! Glad to hear. I was a bit worried that the response would be "this is waste of time as we have a working driver already". > > >This is first version of a patch set that adds > >drivers for the Atmel LCDC IP core. > >Posted for review as the basics works now. > > > >The LCDC IP core contains two devices: > >- a PWM often used for backlight > >- a LCD display controller > > > >Both devices are supported today by the atmel_lcdfb driver. > >For this new set of drivers the compatible strings was > >selected to avoid clash with the existing compatible > >strings used for the atmel_lcdfb driver to allow them > >to co-exist. > > Would be good to have a plan to phase-out the old atmel_lcdfb fbdev > driver when this one addresses some TODO items that make sense. Agree on this. One approach could be to say that when all in-kernel users of atmel_lcdfb are ported, then the old driver could be dropped after a kernel release. > The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" > => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). The "-mfd" suffix was added to avoid clashing with the current compatible string used by the atmel_lcdfb driver. I susggest we do the following: - use the microchip prefix, as this is now owned by microchip - and add the driver to a drm/microchip/ directory (Then we can only hope that microchip do not change name or are purchased by someone else). > > >The DRM implementation has a few shortcomings compared to the > >existing fbdev based driver: > > - STN displays are not supported > > Binding support is missing but most of the > > STN specific functionality is otherwise ported > > from the fbdev driver. > > I assume the info should come from the panel > > but as I lack HW I have not looked too much > > into what is required. > > Yes, I'm not even sure if STN displays are still available those > days... Might not be worth it spending time on this. Then I will delete all the STN bits that was ported over. If we need them later they can be found on the mailign list. > > > - gamma support is missing > > The driver utilises drm_simple_kms_helper and > > this helper lacks support for setting up gamma. > > If this is useful please let me know and I > > will extend drm_simple_kms_helper to support this > > and update the driver. > > - modesetting is not checked (see TODO in file) > > Is this required for such a simple setup? > > - support for extra modes as applicable (and lcd-wiring-mode) > > - support for AVR32 (is it relevant?) > > No, AVR32 is not relevant anymore as the core and SoC were removed > some years ago from Linux. > All mention of AT32 or AVR32 can be remove then. Ok, I will delete these. > >The drivers _works_ on a proprietary at91sam9263 based board > >and I will eventually migrate the at91sam9263ek over > >to use it as well. > > We'll be able to test it on other SoCs and boards. Thanks! > > >Works is maybe a stretch - the following was tested: > > modetest shows reasonable output > > modetest -s shows some nice colored squares > > You must see something like this (without the overlay additional > black square): > http://www.at91.com/linux4sam/bin/view/Linux4SAM/UsingAtmelDRMDriver#Some_modetest_commands I posted a picture to G+ here: https://plus.google.com/+SamRavnborg/posts/YBt4jUGLgWa They look similar but are not equal. For now I assume this is OK. I will do some testing with a Qt app, and will test colors with this too. > >All feedback (from spelling errors to general structure) appreciated! > > On my side, it's mostly Nitpicking ;-) > Now that we're Microchip for a little bit more than 2 years, I tend > to make this name prevalent compared to Atmel for new work around > our products... But I know this driver is for older SoCs and that it > got inspired by two former drivers that have this "atmel" naming > everywhere. MAINTAINERS and Kconfig changes could include Microchip > name, so that it's obvious for the newcomers... Will fix, as described above. Thanks for the inputs! Sam
