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;
> > > };
> > >
> > > /**
> > >
