Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180118072258.ucbchh32mhyln2qn@flea.lan>
References: <cover.8538fc1191390239557a21e2b0444c81eef1c581.1515492513.git-series.maxime.ripard@free-electrons.com>
 <3b7164f4a5199f0499c6f41201b31bc471cb3af9.1515492513.git-series.maxime.ripard@free-electrons.com>
 <CAGb2v65o-fYvP6Qf8gq=eceAG4oqq7E-r4hyxq4xe+fm0sKfqg@mail.gmail.com> <20180118072258.ucbchh32mhyln2qn@flea.lan>
From:   Chen-Yu Tsai <wens@csie.org>
Date:   Thu, 18 Jan 2018 15:53:12 +0800
Message-ID: <CAGb2v64sp=fUi44Hge2qObva3XucnGXj=a+_wH-M4GraGmkYYQ@mail.gmail.com>
Subject: Re: [PATCH v3 08/13] drm/sun4i: Add a driver for the display frontend
To:     Maxime Ripard <maxime.ripard@free-electrons.com>
Cc:     Daniel Vetter <daniel.vetter@intel.com>,
        David Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
        Neil Armstrong <narmstrong@baylibre.com>, thomas@vitsch.nl
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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