Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On Thu, 2022-04-14 at 11:07 +0530, Vinod Koul wrote: > On 13-04-22, 20:39, Liu Ying wrote: > > On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > > > On 13-04-22, 18:04, Liu Ying wrote: > > > > Hi Vinod, > > > > > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > > > On 02-04-22, 13:24, 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 > > > > > > Cc: Vinod Koul > > > > > > Cc: NXP Linux Team > > > > > > Signed-off-by: Liu Ying > > > > > > --- > > > > [...] > > > > > > > > + */ > > > > > > + > > > > > > +#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. > > > > > > > > > > What does it mean by bits per data lane and differential > > > > > clock > > > > > cycle? > > > > > > > > Please check > > > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means > > > > the > > > > number of slots. But, I don't find the word 'slot' in my lvds > > > > relevant > > > > specs which mentioned in lvds.yaml, so 'slots' is probably not > > > > a > > > > generic name(lvds.yaml is for display panel). So, I use > > > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > > > > variable name is fine, explanation for bit per lane and > > > differential > > > clock cycle didnt help, maybe add better explanation of what this > > > variable means > > > > I may add an example diagram as below... > > Not really a diagram, you can add if you like.. But something which > explains in a sentence or few about the variable. Ok, no diagram. > > bits per lane per differential clock cycle ? Ok, will use this explaination. Regards, Liu Ying
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On 13-04-22, 20:39, Liu Ying wrote: > On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > > On 13-04-22, 18:04, Liu Ying wrote: > > > Hi Vinod, > > > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > > On 02-04-22, 13:24, 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 > > > > > Cc: Vinod Koul > > > > > Cc: NXP Linux Team > > > > > Signed-off-by: Liu Ying > > > > > --- > > [...] > > > > > > + */ > > > > > + > > > > > +#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. > > > > > > > > What does it mean by bits per data lane and differential clock > > > > cycle? > > > > > > Please check > > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > > number of slots. But, I don't find the word 'slot' in my lvds > > > relevant > > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > > generic name(lvds.yaml is for display panel). So, I use > > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > > variable name is fine, explanation for bit per lane and differential > > clock cycle didnt help, maybe add better explanation of what this > > variable means > > I may add an example diagram as below... Not really a diagram, you can add if you like.. But something which explains in a sentence or few about the variable. bits per lane per differential clock cycle ? -- ~Vinod
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > On 13-04-22, 18:04, Liu Ying wrote: > > Hi Vinod, > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > On 02-04-22, 13:24, 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 > > > > Cc: Vinod Koul > > > > Cc: NXP Linux Team > > > > Signed-off-by: Liu Ying > > > > --- [...] > > > > + */ > > > > + > > > > +#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. > > > > > > What does it mean by bits per data lane and differential clock > > > cycle? > > > > Please check > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > number of slots. But, I don't find the word 'slot' in my lvds > > relevant > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > generic name(lvds.yaml is for display panel). So, I use > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > variable name is fine, explanation for bit per lane and differential > clock cycle didnt help, maybe add better explanation of what this > variable means I may add an example diagram as below... > > > > > > > > > > + * @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. -8< + * This is an example with 4 lanes and 7 bits per data lane and differential + * clock cycle: + * + * :: + * |<- one differential clock cycle ->| + * _ + * Clock \___/ + * __ __ __ __ __ __ __ + * Lane0 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane1 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane2 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane3 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< -8< What do you think? Regards, Liu Ying
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On 13-04-22, 18:04, Liu Ying wrote:
> Hi Vinod,
>
> On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote:
> > On 02-04-22, 13:24, 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
> > > Cc: Vinod Koul
> > > Cc: NXP Linux Team
> > > Signed-off-by: Liu Ying
> > > ---
> > > v5->v6:
> > > * Rebase upon v5.17-rc1.
> > >
> > > 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 ..7a2f4747f624
> > > --- /dev/null
> > > +++ b/include/linux/phy/phy-lvds.h
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright 2020 NXP
> >
> > 2022 now
>
> I may change it to 'Copyright 2020,2022 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.
> >
> > What does it mean by bits per data lane and differential clock cycle?
>
> Please check Documentation/devicetree/bindings/display/panel/lvds.yaml.
> lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the
> number of slots. But, I don't find the word 'slot' in my lvds relevant
> specs which mentioned in lvds.yaml, so 'slots' is probably not a
> generic name(lvds.yaml is for display panel). So, I use
> 'bits_per_lane_and_dclk_cycle' as the name tells what it means.
variable name is fine, explanation for bit per lane and differential
clock cycle didnt help, maybe add better explanation of what this
variable means
>
> >
> > > + * @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 intbits_per_lane_and_dclk_cycle;
> > > + unsigned long differential_clk_rate;
> > > + unsigned intlanes;
> > > + boolis_slave;
> > > +};
> >
> > Where is the user of this new configuration? Can you post that patch
> > for
> > reference as well please
>
> Patch 5/5 uses it. Also, I posted two drm bridge drivers[1][2] which
> use it.
That is phy driver not the user
>
> [1]
> https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-13-git-send-email-victor@nxp.com/
> [2]
> https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-14-git-send-email-victor@nxp.com/
this is helpful, thanks
--
~Vinod
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
Hi Vinod,
On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote:
> On 02-04-22, 13:24, 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
> > Cc: Vinod Koul
> > Cc: NXP Linux Team
> > Signed-off-by: Liu Ying
> > ---
> > v5->v6:
> > * Rebase upon v5.17-rc1.
> >
> > 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 ..7a2f4747f624
> > --- /dev/null
> > +++ b/include/linux/phy/phy-lvds.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2020 NXP
>
> 2022 now
I may change it to 'Copyright 2020,2022 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.
>
> What does it mean by bits per data lane and differential clock cycle?
Please check Documentation/devicetree/bindings/display/panel/lvds.yaml.
lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the
number of slots. But, I don't find the word 'slot' in my lvds relevant
specs which mentioned in lvds.yaml, so 'slots' is probably not a
generic name(lvds.yaml is for display panel). So, I use
'bits_per_lane_and_dclk_cycle' as the name tells what it means.
>
> > + * @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 intbits_per_lane_and_dclk_cycle;
> > + unsigned long differential_clk_rate;
> > + unsigned intlanes;
> > + boolis_slave;
> > +};
>
> Where is the user of this new configuration? Can you post that patch
> for
> reference as well please
Patch 5/5 uses it. Also, I posted two drm bridge drivers[1][2] which
use it.
[1]
https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-13-git-send-email-victor@nxp.com/
[2]
https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-14-git-send-email-victor@nxp.com/
Regards,
Liu Ying
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On 02-04-22, 13:24, 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
> Cc: Vinod Koul
> Cc: NXP Linux Team
> Signed-off-by: Liu Ying
> ---
> v5->v6:
> * Rebase upon v5.17-rc1.
>
> 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 ..7a2f4747f624
> --- /dev/null
> +++ b/include/linux/phy/phy-lvds.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2020 NXP
2022 now
> + */
> +
> +#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.
What does it mean by 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 intbits_per_lane_and_dclk_cycle;
> + unsigned long differential_clk_rate;
> + unsigned intlanes;
> + boolis_slave;
> +};
Where is the user of this new configuration? Can you post that patch for
reference as well please
--
~Vinod
[PATCH v6 resend 2/5] phy: Add LVDS configuration options
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
Cc: Vinod Koul
Cc: NXP Linux Team
Signed-off-by: Liu Ying
---
v5->v6:
* Rebase upon v5.17-rc1.
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 ..7a2f4747f624
--- /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 intbits_per_lane_and_dclk_cycle;
+ unsigned long differential_clk_rate;
+ unsigned intlanes;
+ boolis_slave;
+};
+
+#endif /* __PHY_LVDS_H_ */
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index f3286f4cd306..b1413757fcc3 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
struct phy;
@@ -57,10 +58,13 @@ enum phy_media {
* 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_dpdp;
+ struct phy_configure_opts_lvds lvds;
};
/**
--
2.25.1
