[PATCH 15/15] drm/panel: Add Sharp LQ101R1SX01 support

[Thierry Reding]

On Mon, Oct 20, 2014 at 05:56:57PM +0200, Andrzej Hajda wrote:
> On 10/13/2014 12:16 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding at nvidia.com>
> >
> > This panel requires dual-channel mode. The device accepts command-mode
> > data on 8 lanes and will therefore need a dual-channel DSI controller.
> > The two interfaces that make up this device need to be instantiated in
> > the controllers that gang up to provide the dual-channel DSI host.
> >
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> >  .../bindings/panel/sharp,lq101r1sx01.txt           |  46 +++
> >  drivers/gpu/drm/panel/Kconfig                      |  13 +
> >  drivers/gpu/drm/panel/Makefile                     |   1 +
> >  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c    | 426 +++++++++++++++++++++
> >  4 files changed, 486 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> >  create mode 100644 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> >
> > diff --git a/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> > new file mode 100644
> > index 000000000000..4ab4380ddac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/sharp,lq101r1sx01.txt
> > @@ -0,0 +1,46 @@
> > +Sharp Microelectronics 10.1" WQXGA TFT LCD panel
> > +
> > +This panel requires a dual-channel DSI host to operate. It supports two modes:
> > +- left-right: each channel drives the left or right half of the screen
> > +- even-odd: each channel drives the even or odd lines of the screen
> > +
> > +Each of the DSI channels controls a separate DSI peripheral. The peripheral
> > +driven by the first link (DSI-LINK1), left or even, is considered the primary
> > +peripheral and controls the device. The 'link2' property contains a phandle
> > +to the peripheral driven by the second link (DSI-LINK2, right or odd).
> > +
> > +Note that in video mode the DSI-LINK1 interface always provides the left/even
> > +pixels and DSI-LINK2 always provides the right/odd pixels. In command mode it
> > +is possible to program either link to drive the left/even or right/odd pixels
> > +but for the sake of consistency this binding assumes that the same assignment
> > +is chosen as for video mode.
> > +
> > +Required properties:
> > +- compatible: should be "sharp,lq101r1sx01"
> > +- link2: phandle to the DSI peripheral on the secondary link. Note that the
> > +  presence of this property marks the containing node as DSI-LINK1.
> > +- power-supply: phandle of the regulator that provides the supply voltage
> > +
> > +Optional properties:
> > +- backlight: phandle of the backlight device attached to the panel
> > +
> > +Example:
> > +
> > +	dsi at 54300000 {
> > +		panel: panel at 0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +
> > +			link2 = <&secondary>;
> > +
> > +			power-supply = <...>;
> > +			backlight = <...>;
> > +		};
> > +	};
> > +
> > +	dsi at 54400000 {
> > +		secondary: panel at 0 {
> > +			compatible = "sharp,lq101r1sx01";
> > +			reg = <0>;
> > +		};
> > +	};
> 
> The example does not follow rules above, 2nd node does not contain
> required properties.
> Maybe it should be clearly stated that power-supply, link2 and backlight
> properties can
> be present only in LINK1 node; LINK2 node should contain nothing more
> than compatible and reg.

I've updated the binding documentation to clarify this.

> I guess if it wouldn't be better if different compatibles could be used
> to distinguish LINK1 and LINK2 nodes,
> this way you can provide different sets of required/optional properties
> for both nodes. As a bonus you can
> have different probe/remove/shutdown callback per link.

I think having separate compatibles isn't entirely accurate. They're
both really the same device. There's already the link2 property that
distinguishes both halves.

Having separate probe/remove/shutdown isn't something I consider a
bonus. It would mean that we need to register a second driver. That is,
the device will no longer be driven by a single driver, but rather two
drivers that then need to talk to one another again. And it's not like
there's a lot to do for DSI-LINK2 in the first place, as you point out
yourself.

I'd rather stick with just a single driver and handle the distinction
via the link2 property, which we need to get access to the second
peripheral anyway.

> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
[...]
> > +static int sharp_panel_write(struct sharp_panel *sharp, u16 offset, u8 value)
> > +{
> > +	u8 payload[3] = { offset >> 8, offset & 0xff, value };
> > +	struct mipi_dsi_device *dsi = sharp->link1;
> > +	ssize_t err;
> > +
> > +	err = mipi_dsi_generic_write(dsi, payload, sizeof(payload));
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to write %02x to %04x: %d\n",
> > +			value, offset, err);
> > +
> > +	err = mipi_dsi_dcs_nop(dsi);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to send DCS nop: %d\n", err);
> > +
> 
> If mipi_dsi_generic_write fails and mipi_dsi_dcs_nop succeeds function
> return success, is it OK?

No, this should always return an error if it fails. If these writes
don't succeed the panel won't be usable anyway. Also returning an error
will actually allow us to decide what to do about the failure in the
caller, where more context exists to make a proper decision.

I've fixed this to propagate the error from mipi_dsi_generic_write() and
mipi_dsi_dcs_nop().

> > +	usleep_range(10, 20);
> > +
> > +	return err;
> > +}
> > +
> > +static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,
> > +					   u16 offset, u8 *value)
> > +{
> > +	ssize_t err;
> > +
> > +	cpu_to_be16s(&offset);
> > +
> > +	err = mipi_dsi_generic_read(sharp->link1, &offset, sizeof(offset),
> > +				    value, sizeof(*value));
> > +	if (err < 0)
> > +		dev_err(&sharp->link1->dev, "failed to read from %04x: %d\n",
> > +			offset, err);
> > +
> > +	return err;
> > +}
> > +
> > +static int sharp_panel_disable(struct drm_panel *panel)
> > +{
> > +	struct sharp_panel *sharp = to_sharp_panel(panel);
> > +
> > +	if (!sharp->enabled)
> > +		return 0;
> 
> Shouldn't we assume disable callback can be called only on enabled panel?

That's not something the core does for us currently. We might want to
change that at some point, but until then we can simply do what all
other panels do.

> > +
> > +	if (sharp->backlight) {
> > +		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> > +		backlight_update_status(sharp->backlight);
> > +	}
> > +
> > +	sharp->enabled = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static int sharp_panel_unprepare(struct drm_panel *panel)
> > +{
> > +	struct sharp_panel *sharp = to_sharp_panel(panel);
> > +	int err;
> > +
> > +	if (!sharp->prepared)
> > +		return 0;
> 
> Shouldn't we assume unprepare callback can be called only on enabled panel?

See above.

> > +static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> > +					 struct mipi_dsi_device *right,
> > +					 const struct drm_display_mode *mode)
> > +{
> > +	int err;
> > +
> > +	err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
> > +	if (err < 0)
> > +		dev_err(&left->dev, "failed to set column address: %d\n", err);
> > +
> > +	err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
> > +	if (err < 0)
> > +		dev_err(&left->dev, "failed to set page address: %d\n", err);
> > +
> > +	err = mipi_dsi_dcs_set_column_address(right, mode->hdisplay / 2,
> > +					      mode->hdisplay - 1);
> > +	if (err < 0)
> > +		dev_err(&right->dev, "failed to set column address: %d\n", err);
> > +
> > +	err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
> > +	if (err < 0)
> > +		dev_err(&right->dev, "failed to set page address: %d\n", err);
> > +
> > +	return 0;
> 
> It always returns 0, maybe it could be void?

I think this should also propagate all errors.

> > +static int sharp_panel_prepare(struct drm_panel *panel)
> > +{
[...]
> > +	sharp->prepared = true;
> > +
> > +	return err;
> 
> You should return 0 here, or if you want to signal error to caller you
> should unwind
> changes, I guess regulator_disable should be enough.

Done.

> > +static int sharp_panel_probe(struct mipi_dsi_device *dsi)
> > +{
> > +	struct mipi_dsi_device *secondary = NULL;
> > +	struct sharp_panel *sharp;
> > +	struct device_node *np;
> > +	int err;
> > +
> > +	dsi->lanes = 4;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = 0;
> > +
> > +	/* Find DSI-LINK1 */
> > +	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
> > +	if (np) {
> > +		secondary = of_find_mipi_dsi_device_by_node(np);
> > +		of_node_put(np);
> > +
> > +		if (!secondary)
> > +			return -EPROBE_DEFER;
> > +	}
> > +
> > +	sharp = devm_kzalloc(&dsi->dev, sizeof(*sharp), GFP_KERNEL);
> > +	if (!sharp)
> > +		return -ENOMEM;
> 
> Do we really need this for LINK2 device? You can just check if
> mipi_dsi_get_drvdata
> is not null to distinguish them, I suppose.

Done.

> > +static int sharp_panel_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
> > +	int err;
> > +
> > +	/* only detach from host for the DSI-LINK2 interface */
> > +	if (!sharp->link2) {
> > +		mipi_dsi_detach(dsi);
> 
> There is no locking mechanism here, so it is possible that
> everything can crash if someone unbind driver from LINK2 and then try to
> enable the panel.

I don't think so. Since we're not doing anything with the DSI-LINK2
device anymore it's irrelevant whether it is bound to the driver or
not.

> > +		return 0;
> > +	}
> > +
> > +	err = sharp_panel_disable(&sharp->base);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
> 
> IMHO calling mipi_dsi_detach below should cause connector to call panel
> disable and unprepare so the call above seems to me unnecessary.

I don't think the connector has any business doing anything with the
panel on mipi_dsi_detach(). I suppose we could implement something like
that as part of drm_panel_detach(), but that's not the case today, so
this simply follows what every other panel has done so far.

> > +
> > +	err = mipi_dsi_detach(dsi);
> > +	if (err < 0)
> > +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
> > +
> > +	drm_panel_detach(&sharp->base);
> 
> drm_panel_attach is called from tegra_dsi_host_attach,
> wouldn't be more 'symmetrical' to call drm_panel_detach from
> tegra_dsi_host_detach :)

No, it's not called from tegra_dsi_host_attach(), it's called as part of
the DSI output initialization at DRM load time.

drm_panel_detach() really needs to be called from two places: when the
panel driver is unloaded and when the connector is unloaded. It seems
like this is another area where we may have to put more thought into how
to handle it more uniformly across drivers.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141031/a8398dbe/attachment.sig>