Re: [PATCH v3 08/13] drm/sun4i: Add a driver for the display frontend

[Chen-Yu Tsai]

On Thu, Jan 18, 2018 at 3:22 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> Hi,
>
> On Wed, Jan 17, 2018 at 09:43:31PM +0800, Chen-Yu Tsai wrote:
>> >         if (sun4i_drv_node_is_connector(node))
>> >                 return 0;
>> >
>> > -       if (!sun4i_drv_node_is_frontend(node)) {
>> > +       /*
>> > +        * If the device is either just a regular device, or an
>> > +        * enabled frontend supported by the driver, we add it to our
>> > +        * component list.
>> > +        */
>> > +       if (!sun4i_drv_node_is_frontend(node) ||
>> > +           (sun4i_drv_node_is_supported_frontend(node) &&
>> > +            of_device_is_available(node))) {
>>
>> Nit: sun4i_drv_node_is_supported_frontend should be a subset of
>> sun4i_drv_node_is_frontend, so of_device_is_available should always
>> be true at this point.
>
> That's not really the condition though :)
>
> It's if the device is *not* a frontend or if it is a supported
> frontend that is available, add it to the endpoints list.

Right. I got confused by the inverted logic. Sorry.

>
>> > +       regmap_write(frontend->regs, SUN4I_FRONTEND_INPUT_FMT_REG,
>> > +                    SUN4I_FRONTEND_INPUT_FMT_DATA_MOD(1) |
>> > +                    SUN4I_FRONTEND_INPUT_FMT_DATA_FMT(in_fmt_val) |
>> > +                    SUN4I_FRONTEND_INPUT_FMT_PS(1));
>> > +       regmap_write(frontend->regs, SUN4I_FRONTEND_OUTPUT_FMT_REG,
>> > +                    SUN4I_FRONTEND_OUTPUT_FMT_DATA_FMT(out_fmt_val));
>>
>> Seems that you also need to set the "ALPHA_EN" bit for ARGB.
>
> I have not seen that bit documented anywhere. Where is it coming from?

The A31's user manual. I just checked all the datasheets, and only
the A31 and the A80 have this bit (bit 7) defined. It says if the
bit is cleared, alpha is replaced with 0xff. I assume it works either
way on the A33, or you wouldn't be suprised. Leave it out for now then.
(Or maybe a TODO note now that we know about it.)

ChenYu

>
>> > +       frontend->reset = devm_reset_control_get(dev, NULL);
>> > +       if (IS_ERR(frontend->reset)) {
>> > +               dev_err(dev, "Couldn't get our reset line\n");
>> > +               return PTR_ERR(frontend->reset);
>> > +       }
>> > +       reset_control_reset(frontend->reset);
>>
>> reset_control_reset leaves the reset control deasserted. At this
>> point the clock might not be running, which might mean the internal
>> state is not completely wiped out. (Though this really depends on
>> the design of the internal logic.)
>>
>> Maybe just assert it? It gets deasserted in the runtime PM callback
>> later. And just to be safe, I would move it close to the end of the
>> probe path, past all possible errors, so the hardware doesn't get
>> touched until everything is ready. Or don't touch it anywhere in
>> the probe path, and have the runtime PM resume function do a reset.
>
> That seems like the best solution yes.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com