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

[Liu Ying]

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@xxxxxx>
> > > Cc: Vinod Koul <vkoul@xxxxxxxxxx>
> > > Cc: NXP Linux Team <linux-imx@xxxxxxx>
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > ---
> > > 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;
> > >  };
> > >  
> > >  /**
> > >