Hello Thomas,
On 7/20/22 16:27, Thomas Zimmermann wrote:
> Open Firmware provides basic display output via the 'display' node.
> DT platform code already provides a device that represents the node's
> framebuffer. Add a DRM driver for the device. The display mode and
> color format is pre-initialized by the system's firmware. Runtime
> modesetting via DRM is not possible. The display is useful during
> early boot stages or as error fallback.
>
I'm not familiar with OF display but the driver looks good to me.
Reviewed-by: Javier Martinez Canillas <javi...@redhat.com>
I just have a few questions below.
[...]
> +static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_state
> *new_state)
> +{
> + struct drm_plane_state *new_plane_state =
> drm_atomic_get_new_plane_state(new_state, plane);
> + struct drm_crtc_state *new_crtc_state;
> + int ret;
> +
> + if (!new_plane_state->fb)
> + return 0;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(new_state,
> new_plane_state->crtc);
> +
> + ret = drm_atomic_helper_check_plane_state(new_plane_state,
> new_crtc_state,
> + DRM_PLANE_HELPER_NO_SCALING,
> + DRM_PLANE_HELPER_NO_SCALING,
> + false, false);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
This seems to be exactly the same check than used in the simpledrm driver.
Maybe could be moved to the fwfb helper library too ?
[...]
> +
> +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_state *old_state)
> +{
> + /*
> + * Always enabled; disabling clears the screen in the
> + * primary plane's atomic_disable function.
> + */
> +}
> +
Same comment than for simpledrm, are these no-op helpers really needed ?
[...]
> +static const struct of_device_id ofdrm_of_match_display[] = {
> + { .compatible = "display", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display);
> +
I don't see a binding for this in Documentation/devicetree/bindings/display.
Do we need one or it's that only required for FDT and not Open Firmware DT ?
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
