Received-SPF: pass (google.com: domain of linux-kernel+bounces-41024-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240106221907.325127-1-aford173@gmail.com> <20240106221907.325127-2-aford173@gmail.com>
 <13783659.uLZWGnKmhe@steina-w>
In-Reply-To: <13783659.uLZWGnKmhe@steina-w>
From: Adam Ford <aford173@gmail.com>
Date: Fri, 26 Jan 2024 20:56:47 -0600
Message-ID: <CAHCN7xLJ7fXePrjuXOo4WJovAG1vZm1LjJGJP29aEfPO8oJL+g@mail.gmail.com>
Subject: Re: [PATCH V2 2/2] phy: freescale: add Samsung HDMI PHY
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: dri-devel@lists.freedesktop.org, 
	Kishon Vijay Abraham I <kishon@kernel.org>, devicetree@vger.kernel.org, 
	Conor Dooley <conor+dt@kernel.org>, Fabio Estevam <festevam@gmail.com>, 
	Pengutronix Kernel Team <kernel@pengutronix.de>, Shawn Guo <shawnguo@kernel.org>, 
	Sascha Hauer <s.hauer@pengutronix.de>, linux-kernel@vger.kernel.org, 
	Vinod Koul <vkoul@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	NXP Linux Team <linux-imx@nxp.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, 
	linux-phy@lists.infradead.org, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 8, 2024 at 9:03=E2=80=AFAM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi Adam,
>
> thanks for pushing this forward.
>
> Am Samstag, 6. Januar 2024, 23:19:05 CET schrieb Adam Ford:
> > From: Lucas Stach <l.stach@pengutronix.de>
> >
> > This adds the driver for the Samsung HDMI PHY found on the
> > i.MX8MP SoC.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > ---
> > V2:  Fixed some whitespace found from checkpatch
> >      Change error handling when enabling apbclk to use dev_err_probe
> >      Rebase on Linux-Next
> >
> >      I (Adam) tried to help move this along, so I took Lucas' patch and
> >      attempted to apply fixes based on feedback.  I don't have
> >      all the history, so apologies for that.
> >
> > diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kcon=
fig
> > index 853958fb2c06..5c2b73042dfc 100644
> > --- a/drivers/phy/freescale/Kconfig
> > +++ b/drivers/phy/freescale/Kconfig
> > @@ -35,6 +35,12 @@ config PHY_FSL_IMX8M_PCIE
> >         Enable this to add support for the PCIE PHY as found on
> >         i.MX8M family of SOCs.
> >
> > +config PHY_FSL_SAMSUNG_HDMI_PHY
> > +     tristate "Samsung HDMI PHY support"
> > +     depends on OF && HAS_IOMEM
> > +     help
> > +       Enable this to add support for the Samsung HDMI PHY in i.MX8MP.
> > +
> >  endif
> >
> >  config PHY_FSL_LYNX_28G
> > diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Mak=
efile
> > index cedb328bc4d2..dbcafdcc8751 100644
> > --- a/drivers/phy/freescale/Makefile
> > +++ b/drivers/phy/freescale/Makefile
> > @@ -3,4 +3,5 @@ obj-$(CONFIG_PHY_FSL_IMX8MQ_USB)      +=3D phy-fsl-imx8=
mq-usb.o
> >  obj-$(CONFIG_PHY_MIXEL_LVDS_PHY)     +=3D phy-fsl-imx8qm-lvds-phy.o
> >  obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY)    +=3D phy-fsl-imx8-mipi-dphy.o
> >  obj-$(CONFIG_PHY_FSL_IMX8M_PCIE)     +=3D phy-fsl-imx8m-pcie.o
> > +obj-$(CONFIG_PHY_FSL_SAMSUNG_HDMI_PHY)  +=3D phy-fsl-samsung-hdmi.o
> >  obj-$(CONFIG_PHY_FSL_LYNX_28G)               +=3D phy-fsl-lynx-28g.o
>
> I don't know if there was different feedback already. But I would have ad=
ded
> the entry sorted alphabetically, thus after CONFIG_PHY_FSL_LYNX_28G. Same=
 goes,
> for Kconfig as well.

The Makefile is easy to rearrange, but Kconfig is already out of
alphabetical order, and PHY_FSL_SAMSUNG_HDMI_PHY is encapsulated in an
if statement, so it cannot go after PHY_FSL_LYNX_28G.  It is
alphabetical after PHY_FSL_IMX8M.

>
> > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c new file mode 100644
> > index 000000000000..54e93ea898f7
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -0,0 +1,1078 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2020 NXP
> > + * Copyright 2022 Pengutronix, Lucas Stach <kernel@pengutronix.de>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#define HDMI_TX_CONTROL0             0x200
> > +#define  HDMI_TX_CONTROL_PHY_PWRDWN  BIT(3)
>
> These defines are unused here.

I can drop these.
>
> > +
> > +#define PHY_REG_33           0x84
> > +#define  REG33_MODE_SET_DONE BIT(7)
> > +#define  REG33_FIX_DA                BIT(1)
> > +
> > +#define PHY_REG_34           0x88
> > +#define  REG34_PHY_READY     BIT(7)
> > +#define  REG34_PLL_LOCK              BIT(6)
> > +#define  REG34_PHY_CLK_READY BIT(5)
> > +
> > +
> > +#define PHY_PLL_REGS_NUM 48
> > +
> > +struct phy_config {
> > +     u32     clk_rate;
> > +     u8 regs[PHY_PLL_REGS_NUM];
>
> Shouldn't reg be aligned along clk_rate?

Why so?  They appear to just be structures where individual parts are
read/written individually.  Looking at another HDMI phy driver, it's
not really any different.

>
> Despite that. Tested on TQMa8MPQL/MBa8MPxL + Full-HD HDMI monitor.
>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>

Thanks for testing.

adam
> Best regards,
> Alexander
>
<snip>

>
>
> --
> TQ-Systems GmbH | M=C3=BChlstra=C3=9Fe 2, Gut Delling | 82229 Seefeld, Ge=
rmany
> Amtsgericht M=C3=BCnchen, HRB 105018
> Gesch=C3=A4ftsf=C3=BChrer: Detlef Schneider, R=C3=BCdiger Stahl, Stefan S=
chneider
> http://www.tq-group.com/
>
>