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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
