From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 1/1] drm/panel: Add support for Olimex LCD-OLinuXino panel Date: Tue, 10 Jul 2018 16:48:45 +0200 Message-ID: <20180710144845.GA25784@ulmo> References: <1529909093-17021-1-git-send-email-stefan@olimex.com> <20180710103250.GG1504@ulmo> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0068614652==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Stefan Mavrodiev Cc: Mark Rutland , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Stefan Mavrodiev , David Airlie , Greg Kroah-Hartman , Randy Dunlap , open list , "open list:DRM PANEL DRIVERS" , Rob Herring , Mauro Carvalho Chehab , Andrew Morton , "David S. Miller" List-Id: devicetree@vger.kernel.org --===============0068614652== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="T4sUOijqQbZv57TR" Content-Disposition: inline --T4sUOijqQbZv57TR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 10, 2018 at 04:08:54PM +0300, Stefan Mavrodiev wrote: > On 07/10/2018 01:32 PM, Thierry Reding wrote: > > On Mon, Jun 25, 2018 at 09:44:35AM +0300, Stefan Mavrodiev wrote: [...] > > > diff --git a/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c b/dri= vers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c [...] > > > +static int lcd_olinuxino_get_modes(struct drm_panel *panel) > > > +{ > > > + struct lcd_olinuxino *lcd =3D to_lcd_olinuxino(panel); > > > + struct drm_connector *connector =3D lcd->panel.connector; > > > + struct lcd_olinuxino_info *lcd_info =3D &lcd->eeprom.info; > > > + struct drm_device *drm =3D lcd->panel.drm; > > > + struct lcd_olinuxino_mode *lcd_mode; > > > + struct drm_display_mode *mode; > > > + int i, num =3D 0; > > These two can be unsigned. > >=20 > > > + > > > + /* Read up to 4 modes */ > > > + for (i =3D 0; i < lcd->eeprom.num_modes && i < 4; i++) { > > Can it happen that lcd->eeprom.num_modes >=3D 4? Seems to me like that > > would be a serious bug. Perhaps move that check to where the EEPROM is > > read and output a warning, then overwrite lcd->eeprom.num_modes with 4? > If num_modes is bigger than 4, then lcd_mode pointer will point to invalid > location. This can happen if someone overwrite the eeprom and apply > correct checksum at the end. Then i < 4 makes sure this won't happen. I still think that this should be checked earlier in the code, like at ->probe() time. That way you can output a warning once so that people have a chance to notice a broken EEPROM and potentially do something about it. You can then also sanitize the EEPROM contents so that the rest of the code (i.e. ->get_modes()) doesn't have to check for this error case. > >=20 > > > + lcd_mode =3D (struct lcd_olinuxino_mode *) > > > + &lcd->eeprom.reserved[i * sizeof(*lcd_mode)]; > > > + > > > + mode =3D drm_mode_create(drm); > > > + if (!mode) { > > > + dev_err(drm->dev, "failed to add mode %ux%u@%u\n", > > > + lcd_mode->hactive, > > > + lcd_mode->vactive, > > > + lcd_mode->refresh); > > > + continue; > > > + } > > > + > > > + mode->clock =3D lcd_mode->pixelclock; > > > + mode->hdisplay =3D lcd_mode->hactive; > > > + mode->hsync_start =3D lcd_mode->hactive + lcd_mode->hfp; > > > + mode->hsync_end =3D lcd_mode->hactive + lcd_mode->hfp + > > > + lcd_mode->hpw; > > > + mode->htotal =3D lcd_mode->hactive + lcd_mode->hfp + > > > + lcd_mode->hpw + lcd_mode->hbp; > > > + mode->vdisplay =3D lcd_mode->vactive; > > > + mode->vsync_start =3D lcd_mode->vactive + lcd_mode->vfp; > > > + mode->vsync_end =3D lcd_mode->vactive + lcd_mode->vfp + > > > + lcd_mode->vpw; > > > + mode->vtotal =3D lcd_mode->vactive + lcd_mode->vfp + > > > + lcd_mode->vpw + lcd_mode->vbp; > > > + mode->vrefresh =3D lcd_mode->refresh; > > > + > > > + if (lcd->eeprom.num_modes =3D=3D 1) > > > + mode->type |=3D DRM_MODE_TYPE_PREFERRED; > > Is there no way to determine the preferred mode if there are more than > > one? Perhaps always prefer the first mode? > My idea here is what if the first mode is not supported by the host? That= 's > why > we will be storing multiple modes, one with the most strict timings > (e.g rounded to 10kHz or less, according to the lcd datesheet), and others > with less strict. Its not the panel driver's responsibility to take into account the host capabilities. The panel driver should, to the best of its abilities, report the supported modes and the host driver is responsible for dealing with the mode list. If any of the modes are not within its capabilities, then it can filter them out (using the ->mode_valid() callback). In this particular case, if there is no clearly defined preferred mode I'd argue that either you don't mark any mode as preferred, or you choose the one with the strictest timings. I had hoped that perhaps the first mode would always be the one with the strictest timings and hence would be the preferred mode, but it seems like that's not a guarantee. If so, it's pretty much impossible for the driver to determine a preferred mode, so can't do much about it. As for the special case of a single mode being present in the EEPROM, I'm not sure if that's something worth considering. If there's only one it doesn't really matter that it's preferred. Thierry --T4sUOijqQbZv57TR Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAltEx0sACgkQ3SOs138+ s6ECKQ//dBryPS/+3xB2zXCWHrz7hkRTj+r4FZHQg9NWA7MlEA9bRMeamyfh2PyV uYvUGl7ihg3uDx3hm1lE7+DESVQbJCVfBJ+0K9gKf4Gb7gluuFcSP0niyhd6Qlbt pldetrdcG1CSnbEvPSv73Fg2vEaS6DUrJ861swcSgYfToXaAmyj/v+JuCRXMqvkm 5BU/ZRLfZrsfCJyMe83enoqPaW44MY/p5DzGq3nVDSAPOJ49n/iX4g+DeqEzzZJt bru3GaeL9ovU6iR4H6M1p0vsjN73ESTqCFzuWElc8WEHDzBjUEcJW2YTy4yoOeAp yrPJ8YMiQkBWUDMKN59lMK/ZtgBvaVcyxDnjnC2dn1oME2Rh/tFYQgTBEVA882qA lSe0+8dam7P0XoSAy2nE2FQIHmJQ/6ijU2N2op8a08lXRfSNjWvr2tn3/kxBuJQx 1LtdUr9ARS/mo+Ru0HiyuaHv/3BN+Gi42w+H8sZFxrJlGKZs/dv0dw8bypkjw/8Y P6KT4Epd7eJBLEWvxWDmvoaYINzGvlH48ORiQhpOJe6l2Wt0flL3djPImlYI2qaB QINz+eosPS6fb7Fc2XeR4+yUhp/HpDZvrbZZGC9/xrwTWfzRGtExfS5PyxXwsrr0 cC2A3nnHg3YBtbXSF56mpL5y7Duv6tXE2VTFI621CRqNKQLga5M= =8uar -----END PGP SIGNATURE----- --T4sUOijqQbZv57TR-- --===============0068614652== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0068614652==--