Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Togorean, Bogdan" <Bogdan.Togorean@analog.com>
To:     Sam Ravnborg <sam@ravnborg.org>
CC:     "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Mike Looijmans <mike.looijmans@topic.nl>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        David Airlie <airlied@linux.ie>,
        Rob Herring <robh+dt@kernel.org>,
        "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Subject: RE: [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP
 core
Thread-Topic: [PATCH 1/2] drm/adi: axi-hdmi-tx: Add support for AXI HDMI TX IP
 core
Thread-Index: AQHWmwh5xmwgGy1BWUuP733ye4eTYKmaYpMAgAqabZA=
Date:   Fri, 23 Oct 2020 08:51:48 +0000
Message-ID: <DM6PR03MB52748E16A02E8AE049379D569B1A0@DM6PR03MB5274.namprd03.prod.outlook.com>
References: <20201005141226.180655-1-bogdan.togorean@analog.com>
 <20201016145500.GA1325536@ravnborg.org>
In-Reply-To: <20201016145500.GA1325536@ravnborg.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0cef92a6-9ee1-4443-adae-08d87730e0b4
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Oct 2020 08:51:48.4201
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ttvosz1ZIjlzQ5vGfu7Wp/MB6g2YB8yBjg8bQwspN9TyYraR6ETjbhXa6Uz5B1NP2RMkDFn50z40JN3Vmbf1AeDjgv08eHYTISwdJPH+f78=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR03MB2601
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.235,18.0.737
 definitions=2020-10-23_04:2020-10-23,2020-10-23 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 adultscore=0
 priorityscore=1501 phishscore=0 spamscore=0 malwarescore=0 mlxlogscore=999
 lowpriorityscore=0 bulkscore=0 impostorscore=0 suspectscore=0
 clxscore=1011 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2010230061
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thank you Sam for your review,
I'll send now V2 implementing all your remarks.

Best regards,
Bogdan

>=20
> Hi Bogdan
>=20
> On Mon, Oct 05, 2020 at 05:12:08PM +0300, Bogdan Togorean wrote:
> > From: Lars-Peter Clausen <lars@metafoo.de>
> >
> > The AXI HDMI HDL driver is the driver for the HDL graphics core which i=
s
> > used on various FPGA designs. It's mostly used to interface with the
> > ADV7511 driver on some Zynq boards (e.g. ZC702 & ZedBoard).
> >
> > Link: https://wiki.analog.com/resources/tools-software/linux-drivers/dr=
m/hdl-
> axi-hdmi
> > Link: https://wiki.analog.com/resources/fpga/docs/axi_hdmi_tx
>=20
> Thanks for submitting the driver - a few high level comments after
> browsing the driver:
>=20
> - Use drmm_mode_config_init() to utilize new cleanup
> - Look at other uses of drm_driver - there is macros that makes this
>   much simpler / smaller
> - Use devm_drm_dev_alloc() to allocate axi_hdmi_tx_private
>   To do so embed drm_device in axi_hdmi_tx_private - which is the way to
>   do it today
> - Do not use ddev->dev_private, it is deprecated
> - Use dev_err_probe() when you risk to see a PROBE_DEFER
> - In all include blocks sort the include alphabetically
> - Use the new interface to drm_bridge_attach() - where display driver
>   creates the connector
> - See if the Kconfig selects can be trimmed - the framebuffer releated
>   selects looks wrong (others get it wrong too)
> - Check if you can use the simple encoder - see
>   drm_simple_encoder_init()
>=20
> If this is a simple one plane, one crtc display driver then it should
> use the drm_simple_* support. Or the changelog should explain why not.
>=20
> We want the drivers as simple as we can - and they shall use as much of
> the helper infrastructure as they can.
>=20
> We continue to develop the helper infrastructure so it is expected that
> there is some lacking behind as is the case here.
>=20
>         Sam
>=20