Re: [PATCH 2/5] drm/sun4i: Support LVDS using MIPI DSI combo D-PHY

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 4063 bytes --]

Hi,

Some comments below.

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> On Allwinner chips with a combo D-PHY, the TCON LCD0 should fetch it
> from device tree, in order to enable LVDS. Since the PHY also needs to
> be powered off to disable LVDS, add a function to the quirks.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 12 ++++++++++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 960e83c82..ccf335a61 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> @@ -183,6 +184,8 @@ static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
>  	} else {
>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
>  				   SUN4I_TCON0_LVDS_IF_EN, 0);

Does the T113-S3/D1 also have LVDS support in TCON regsiters?
My understanding is that it only has the combo-phy, so this register write
should not take place.

I think it would be easier to just move this to a common callback for users of
the tcon0 lvds phy and always call 

> +		if (tcon->quirks->disable_lvds_phy)
> +			tcon->quirks->disable_lvds_phy(tcon, encoder);

Also "disable" is often used as opposite of "enable", so I would suggest using
"teardown" as opposite of "setup" instead.

>  	}
>  }
>  
> @@ -1245,6 +1248,15 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  		goto err_free_dclk;
>  	}
>  
> +	if (tcon->quirks->has_combo_dphy) {
> +		tcon->dphy = devm_phy_get(dev, "dphy");

I don't think it makes sense to call this dphy since it's used in LVDS mode
and not in D-PHY mode. I would suggest using "combo" as phy name instead.

It would have made more sense to also have the compatible reflect this
(by calling it combo-phy instead of mipi-dphy) but it's too late now.

> +		if (IS_ERR(tcon->dphy)) {
> +			dev_err(dev, "Couldn't get the combo D-PHY\n");
> +			ret = PTR_ERR(tcon->dphy);
> +			goto err_free_dclk;
> +		}
> +	}
> +
>  	if (tcon->quirks->has_channel_0) {
>  		/*
>  		 * If we have an LVDS panel connected to the TCON, we should
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index fa23aa23f..b270d1a13 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -239,6 +239,7 @@ struct sun4i_tcon_quirks {
>  	bool	has_channel_0;	/* a83t does not have channel 0 on second TCON */
>  	bool	has_channel_1;	/* a33 does not have channel 1 */
>  	bool	has_lvds_alt;	/* Does the LVDS clock have a parent other than the TCON clock? */
> +	bool	has_combo_dphy; /* Is the D-PHY used for LVDS output? */
>  	bool	needs_de_be_mux; /* sun6i needs mux to select backend */
>  	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
>  	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
> @@ -250,6 +251,8 @@ struct sun4i_tcon_quirks {
>  	/* handler for LVDS setup routine */
>  	void	(*setup_lvds_phy)(struct sun4i_tcon *tcon,
>  				  const struct drm_encoder *encoder);
> +	void	(*disable_lvds_phy)(struct sun4i_tcon *tcon,
> +				    const struct drm_encoder *encoder);
>  };
>  
>  struct sun4i_tcon {
> @@ -282,6 +285,9 @@ struct sun4i_tcon {
>  	/* Associated crtc */
>  	struct sun4i_crtc		*crtc;
>  
> +	/* Associated D-PHY */
> +	struct phy			*dphy;
> +
>  	int				id;
>  
>  	/* TCON list management */
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]