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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGb2v64sp=fUi44Hge2qObva3XucnGXj=a+_wH-M4GraGmkYYQ@mail.gmail.com>
Date:   Thu, 18 Jan 2018 15:53:12 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Neil Armstrong <narmstrong@...libre.com>, thomas@...sch.nl
Subject: Re: [PATCH v3 08/13] drm/sun4i: Add a driver for the display frontend

On Thu, Jan 18, 2018 at 3:22 PM, Maxime Ripard
<maxime.ripard@...e-electrons.com> 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