diff mbox series

[v5,2/5] phy: Add LVDS configuration options

Message ID 1616662832-27048-3-git-send-email-victor.liu@nxp.com
State Accepted
Commit 3abfaefb9a6dda5e0bfa6160f55abfd0e32d8b0a
Headers show
Series [v5,1/5] drm/bridge: nwl-dsi: Set PHY mode in nwl_dsi_enable() | expand

Commit Message

Liu Ying March 25, 2021, 9 a.m. UTC
This patch allows LVDS PHYs to be configured through
the generic functions and through a custom structure
added to the generic union.

The parameters added here are based on common LVDS PHY
implementation practices.  The set of parameters
should cover all potential users.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Cc: Vinod Koul <vkoul@kernel.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v4->v5:
* Align kernel-doc style to include/linux/phy/phy.h. (Vinod)
* Trivial tweaks.
* Drop Robert's R-b tag.

v3->v4:
* Add Robert's R-b tag.

v2->v3:
* No change.

v1->v2:
* No change.

 include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h      |  4 ++++
 2 files changed, 36 insertions(+)
 create mode 100644 include/linux/phy/phy-lvds.h

Comments

Kishon Vijay Abraham I March 31, 2021, 1:32 p.m. UTC | #1
Hi,

On 25/03/21 2:30 pm, Liu Ying wrote:
> This patch allows LVDS PHYs to be configured through

> the generic functions and through a custom structure

> added to the generic union.

> 

> The parameters added here are based on common LVDS PHY

> implementation practices.  The set of parameters

> should cover all potential users.

> 

> Cc: Kishon Vijay Abraham I <kishon@ti.com>

> Cc: Vinod Koul <vkoul@kernel.org>

> Cc: NXP Linux Team <linux-imx@nxp.com>

> Signed-off-by: Liu Ying <victor.liu@nxp.com>

> ---

> v4->v5:

> * Align kernel-doc style to include/linux/phy/phy.h. (Vinod)

> * Trivial tweaks.

> * Drop Robert's R-b tag.

> 

> v3->v4:

> * Add Robert's R-b tag.

> 

> v2->v3:

> * No change.

> 

> v1->v2:

> * No change.

> 

>  include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++

>  include/linux/phy/phy.h      |  4 ++++

>  2 files changed, 36 insertions(+)

>  create mode 100644 include/linux/phy/phy-lvds.h

> 

> diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h

> new file mode 100644

> index 00000000..7a2f474

> --- /dev/null

> +++ b/include/linux/phy/phy-lvds.h

> @@ -0,0 +1,32 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Copyright 2020 NXP

> + */

> +

> +#ifndef __PHY_LVDS_H_

> +#define __PHY_LVDS_H_

> +

> +/**

> + * struct phy_configure_opts_lvds - LVDS configuration set

> + * @bits_per_lane_and_dclk_cycle:	Number of bits per data lane and

> + *					differential clock cycle.


phy_set_bus_width() instead?
> + * @differential_clk_rate:		Clock rate, in Hertz, of the LVDS

> + *					differential clock.


Please use clk API's to get rate.
> + * @lanes:				Number of active, consecutive,

> + *					data lanes, starting from lane 0,

> + *					used for the transmissions.

> + * @is_slave:				Boolean, true if the phy is a slave

> + *					which works together with a master

> + *					phy to support dual link transmission,

> + *					otherwise a regular phy or a master phy.


For parameters that are known at design time, it doesn't have to be
passed from consumer driver. So all these parameters do they really have
to be passed at runtime?

Thanks
Kishon
> + *

> + * This structure is used to represent the configuration state of a LVDS phy.

> + */

> +struct phy_configure_opts_lvds {

> +	unsigned int	bits_per_lane_and_dclk_cycle;

> +	unsigned long	differential_clk_rate;

> +	unsigned int	lanes;

> +	bool		is_slave;

> +};

> +

> +#endif /* __PHY_LVDS_H_ */

> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h

> index e435bdb..d450b44 100644

> --- a/include/linux/phy/phy.h

> +++ b/include/linux/phy/phy.h

> @@ -17,6 +17,7 @@

>  #include <linux/regulator/consumer.h>

>  

>  #include <linux/phy/phy-dp.h>

> +#include <linux/phy/phy-lvds.h>

>  #include <linux/phy/phy-mipi-dphy.h>

>  

>  struct phy;

> @@ -51,10 +52,13 @@ enum phy_mode {

>   *		the MIPI_DPHY phy mode.

>   * @dp:		Configuration set applicable for phys supporting

>   *		the DisplayPort protocol.

> + * @lvds:	Configuration set applicable for phys supporting

> + *		the LVDS phy mode.

>   */

>  union phy_configure_opts {

>  	struct phy_configure_opts_mipi_dphy	mipi_dphy;

>  	struct phy_configure_opts_dp		dp;

> +	struct phy_configure_opts_lvds		lvds;

>  };

>  

>  /**

>
Liu Ying June 10, 2021, 9:38 a.m. UTC | #2
Hi Kishon, Vinod,

Any follow-up comments/suggestions based on my previous reply?
Or, perhaps, just keep the patch as-is to support the generic lvds phy
configuration structure?

Thanks,
Liu Ying

On Thu, 2021-04-01 at 16:36 +0800, Liu Ying wrote:
> Hi Kishon,
> 
> First of all, thanks for your review.
> 
> On Wed, 2021-03-31 at 19:02 +0530, Kishon Vijay Abraham I wrote:
> > Hi,
> > 
> > On 25/03/21 2:30 pm, Liu Ying wrote:
> > > This patch allows LVDS PHYs to be configured through
> > > the generic functions and through a custom structure
> > > added to the generic union.
> > > 
> > > The parameters added here are based on common LVDS PHY
> > > implementation practices.  The set of parameters
> > > should cover all potential users.
> > > 
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > Cc: Vinod Koul <vkoul@kernel.org>
> > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > ---
> > > v4->v5:
> > > * Align kernel-doc style to include/linux/phy/phy.h. (Vinod)
> > > * Trivial tweaks.
> > > * Drop Robert's R-b tag.
> > > 
> > > v3->v4:
> > > * Add Robert's R-b tag.
> > > 
> > > v2->v3:
> > > * No change.
> > > 
> > > v1->v2:
> > > * No change.
> > > 
> > >  include/linux/phy/phy-lvds.h | 32 ++++++++++++++++++++++++++++++++
> > >  include/linux/phy/phy.h      |  4 ++++
> > >  2 files changed, 36 insertions(+)
> > >  create mode 100644 include/linux/phy/phy-lvds.h
> > > 
> > > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h
> > > new file mode 100644
> > > index 00000000..7a2f474
> > > --- /dev/null
> > > +++ b/include/linux/phy/phy-lvds.h
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright 2020 NXP
> > > + */
> > > +
> > > +#ifndef __PHY_LVDS_H_
> > > +#define __PHY_LVDS_H_
> > > +
> > > +/**
> > > + * struct phy_configure_opts_lvds - LVDS configuration set
> > > + * @bits_per_lane_and_dclk_cycle:	Number of bits per data lane and
> > > + *					differential clock cycle.
> > 
> > phy_set_bus_width() instead?
> 
> This member aims to configure the number of bits transmitted during a
> period of time(a clock cycle). It doesn't sound like a concept of 'bus
> width'?
> 
> > > + * @differential_clk_rate:		Clock rate, in Hertz, of the LVDS
> > > + *					differential clock.
> > 
> > Please use clk API's to get rate.
> 
> I like your idea. But, this rate is likely runtime-configurable, e.g.,
> a LVDS to HDMI chip is connected. It seems that there is no appropriate
> driver to set the rate by calling clk_set_rate() then?
> 
> > > + * @lanes:				Number of active, consecutive,
> > > + *					data lanes, starting from lane 0,
> > > + *					used for the transmissions.
> > > + * @is_slave:				Boolean, true if the phy is a slave
> > > + *					which works together with a master
> > > + *					phy to support dual link transmission,
> > > + *					otherwise a regular phy or a master phy.
> > 
> > For parameters that are known at design time, it doesn't have to be
> > passed from consumer driver. So all these parameters do they really have
> > to be passed at runtime?
> 
> Yes for all, perhaps. Details below:
> 
> 1) bits_per_lane_and_dclk_cycle
> i.MX8qxp LVDS phy can only do 7, while i.MX8qm LVDS phy(a different IP)
> can do either 7 or 10(configurable by setting a phy register).
> 
> 2) differential_clk_rate
> It's likely runtime-configurable, as I mentioned above.
> 
> 3) lanes
> The higher color depth is, the more lanes are used:
> RGB666 - 3 lanes
> RGB888 - 4 lanes
> RGB101010 - 5 lanes
> 
> That means a phy with 5 lanes(like i.MX8qm LVDS phy) support up to the
> 3 types of RGB pixels.
> 
> Though the i.MX LVDS phys don't have any register to configure the
> lanes to be used, it would be good to define it for phy_validate() or
> other potential phys?
> 
> 4) is_slave
> Any i.MX8qxp LVDS phy instance can act as a regular phy or a master phy
> or a slave phy. Changing mode at runtime is probably unneeded. But,
> it's difficult for the phy driver to get the mode from device tree(see
> drm_of_lvds_get_port_pixels_type()), I think. Export an i.MX8qxp LVDS
> phy specific function to set this instead?
> 
> Regards,
> Liu Ying
> 
> > Thanks
> > Kishon
> > > + *
> > > + * This structure is used to represent the configuration state of a LVDS phy.
> > > + */
> > > +struct phy_configure_opts_lvds {
> > > +	unsigned int	bits_per_lane_and_dclk_cycle;
> > > +	unsigned long	differential_clk_rate;
> > > +	unsigned int	lanes;
> > > +	bool		is_slave;
> > > +};
> > > +
> > > +#endif /* __PHY_LVDS_H_ */
> > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > > index e435bdb..d450b44 100644
> > > --- a/include/linux/phy/phy.h
> > > +++ b/include/linux/phy/phy.h
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/regulator/consumer.h>
> > >  
> > >  #include <linux/phy/phy-dp.h>
> > > +#include <linux/phy/phy-lvds.h>
> > >  #include <linux/phy/phy-mipi-dphy.h>
> > >  
> > >  struct phy;
> > > @@ -51,10 +52,13 @@ enum phy_mode {
> > >   *		the MIPI_DPHY phy mode.
> > >   * @dp:		Configuration set applicable for phys supporting
> > >   *		the DisplayPort protocol.
> > > + * @lvds:	Configuration set applicable for phys supporting
> > > + *		the LVDS phy mode.
> > >   */
> > >  union phy_configure_opts {
> > >  	struct phy_configure_opts_mipi_dphy	mipi_dphy;
> > >  	struct phy_configure_opts_dp		dp;
> > > +	struct phy_configure_opts_lvds		lvds;
> > >  };
> > >  
> > >  /**
> > >
diff mbox series

Patch

diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-lvds.h
new file mode 100644
index 00000000..7a2f474
--- /dev/null
+++ b/include/linux/phy/phy-lvds.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2020 NXP
+ */
+
+#ifndef __PHY_LVDS_H_
+#define __PHY_LVDS_H_
+
+/**
+ * struct phy_configure_opts_lvds - LVDS configuration set
+ * @bits_per_lane_and_dclk_cycle:	Number of bits per data lane and
+ *					differential clock cycle.
+ * @differential_clk_rate:		Clock rate, in Hertz, of the LVDS
+ *					differential clock.
+ * @lanes:				Number of active, consecutive,
+ *					data lanes, starting from lane 0,
+ *					used for the transmissions.
+ * @is_slave:				Boolean, true if the phy is a slave
+ *					which works together with a master
+ *					phy to support dual link transmission,
+ *					otherwise a regular phy or a master phy.
+ *
+ * This structure is used to represent the configuration state of a LVDS phy.
+ */
+struct phy_configure_opts_lvds {
+	unsigned int	bits_per_lane_and_dclk_cycle;
+	unsigned long	differential_clk_rate;
+	unsigned int	lanes;
+	bool		is_slave;
+};
+
+#endif /* __PHY_LVDS_H_ */
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index e435bdb..d450b44 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -17,6 +17,7 @@ 
 #include <linux/regulator/consumer.h>
 
 #include <linux/phy/phy-dp.h>
+#include <linux/phy/phy-lvds.h>
 #include <linux/phy/phy-mipi-dphy.h>
 
 struct phy;
@@ -51,10 +52,13 @@  enum phy_mode {
  *		the MIPI_DPHY phy mode.
  * @dp:		Configuration set applicable for phys supporting
  *		the DisplayPort protocol.
+ * @lvds:	Configuration set applicable for phys supporting
+ *		the LVDS phy mode.
  */
 union phy_configure_opts {
 	struct phy_configure_opts_mipi_dphy	mipi_dphy;
 	struct phy_configure_opts_dp		dp;
+	struct phy_configure_opts_lvds		lvds;
 };
 
 /**