[PATCH v2 12/19] video: sunxi: de2: switch to DT probing

[Simon Glass]

Hi Andre,

On Tue, 6 Apr 2021 at 13:09, Andre Przywara <andre.przywara at arm.com> wrote:
>
> On Sat,  6 Mar 2021 20:54:30 +0100
> Jernej Skrabec <jernej.skrabec at siol.net> wrote:
>
> Hi Jernej,
>
> (CC:ing Simon for some DM issues below)
>
> > Currently DE2 driver is probed via driver info. Switch probing to device
> > tree compatible string method.
> >
> > Display is now searched via driver name which has same limitation as
> > previous method. This can be improved only when all drivers in chain are
> > probed via device tree compatible strings.
>
> So on a first glance this looks alright (also the fixed version on your
> github). However it doesn't work on the Pine64-LTS (or A64 in general),
> and I identified at least two problems below:
>
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec at siol.net>
> > ---
> >  drivers/video/sunxi/sunxi_de2.c | 88 +++++++++++++++++++--------------
> >  1 file changed, 52 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/video/sunxi/sunxi_de2.c b/drivers/video/sunxi/sunxi_de2.c
> > index e02d359cd259..81576e45e9ef 100644
> > --- a/drivers/video/sunxi/sunxi_de2.c
> > +++ b/drivers/video/sunxi/sunxi_de2.c
> > @@ -31,6 +31,11 @@ enum {
> >       LCD_MAX_LOG2_BPP        = VIDEO_BPP32,
> >  };
> >
> > +struct sunxi_de2_data {
> > +     int id;
>
> nit: can you rename this to something less general, like mux or
> pipeline_id?
>
> > +     const char *disp_drv_name;
> > +};
> > +
> >  static void sunxi_de2_composer_init(void)
> >  {
> >       struct sunxi_ccm_reg * const ccm =
> > @@ -228,51 +233,34 @@ static int sunxi_de2_init(struct udevice *dev, ulong fbbase,
> >
> >  static int sunxi_de2_probe(struct udevice *dev)
> >  {
> > +     const struct sunxi_de2_data *data =
> > +             (const struct sunxi_de2_data *)dev_get_driver_data(dev);
> >       struct video_uc_plat *plat = dev_get_uclass_plat(dev);
> >       struct udevice *disp;
> > -     int ret;
> > +     int ret, index = 0;
> >
> >       /* Before relocation we don't need to do anything */
> >       if (!(gd->flags & GD_FLG_RELOC))
> >               return 0;
> >
> > -     ret = uclass_get_device_by_driver(UCLASS_DISPLAY,
> > -                                       DM_DRIVER_GET(sunxi_lcd), &disp);
> > -     if (!ret) {
> > -             int mux;
> > +     while (!(ret = uclass_get_device(UCLASS_DISPLAY, index++, &disp))) {
>
> So this call tries to enumerate all devices registered as
> UCLASS_DISPLAY. For the A64 the sunxi_lcd driver provides one device,
> however it only probes correctly when there is bridge configured (for
> the Pinebook, for instance). There is code to read timings from DT,
> but I don't find any of the required properties (simple-panel) in any
> Allwinner DTs (using DE2, at least).
> As a consequence the probe in sunxi_lcd.c fails (except for the
> Pinebook), and it returns -ENODEV. This is unfortunately the same error
> code that uclass_get_device_by_driver() returns when there are no
> (more) devices providing UCLASS_DISPLAY, so the while loop stops here,
> before having considered the HDMI devices (which are enumerated *after*
> the LCD device). This is the same problem with your change to use
> uclass_id_foreach_dev(), btw.

Well, good to check the error to something different. The -ENODEV
error is reserved for driver model. It can be returned by the bind()
method, but not probe().

>
> Not sure how to best solve this, it sounds like a general DM related
> problem (failing probe() ending iterating all devices in one class).

That's intended though. I suppose we could change it. But you can
iterate through devices with uclass_first_device_err() if you want to
do them all..

Is this in core DM code or somewhere else?

>
> I hacked this a bit (using a different error code in sunxi_lcd_probe()
> and catching/translating this here), but this revealed another problem:
> I only ever see the mixer0 device being probed (actually multiple
> times). I could debug that both mixers are detected in the DT, created
> as two devices and bound (using the right .id value and the right DT
> offset), but only the first one is ever probed through this function
> here. Unfortunately this is the wrong one (HDMI is on mixer1), so
> de2_probe() fails :-(
> Disabling mixer0 in the DT makes it work (on top of the hack above).
>
> No real clue about this second problem, but I will debug further.
>
[..]

Regards,
Simon