Re: [PATCH V8 02/12] phy: freescale: add Samsung HDMI PHY

Dmitry Baryshkov
2024-02-04T09:23:18Z

On Sat, 3 Feb 2024 at 17:53, Adam Ford <aford173@gmail.com> wrote:
>
> 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>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> V4: Make lookup table hex values lower case.
>
> V3: Re-order the Makefile to keep it alphabetical
> Remove unused defines
>
> 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.
> ---
> drivers/phy/freescale/Kconfig | 6 +
> drivers/phy/freescale/Makefile | 1 +
> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1075 ++++++++++++++++++
> 3 files changed, 1082 insertions(+)
> create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>
> diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
> 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/Makefile
> index cedb328bc4d2..79e5f16d3ce8 100644
> --- a/drivers/phy/freescale/Makefile
> +++ b/drivers/phy/freescale/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PHY_MIXEL_LVDS_PHY) += phy-fsl-imx8qm-lvds-phy.o
> obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o
> obj-$(CONFIG_PHY_FSL_IMX8M_PCIE) += phy-fsl-imx8m-pcie.o
> obj-$(CONFIG_PHY_FSL_LYNX_28G) += phy-fsl-lynx-28g.o
> +obj-$(CONFIG_PHY_FSL_SAMSUNG_HDMI_PHY) += phy-fsl-samsung-hdmi.o
> 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..bf0e2299d00f
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -0,0 +1,1075 @@
> +// 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 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];
> +};
> +
> +const struct phy_config phy_pll_cfg[] = {
> + { 22250000, {
> + 0x00, 0xd1, 0x4b, 0xf1, 0x89, 0x88, 0x80, 0x40,
> + 0x4f, 0x30, 0x33, 0x65, 0x00, 0x15, 0x25, 0x80,
> + 0x6c, 0xf2, 0x67, 0x00, 0x10, 0x8f, 0x30, 0x32,
> + 0x60, 0x8f, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00,
> + 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0xe0, 0x83, 0x0f, 0x3e, 0xf8, 0x00, 0x00,
> + },
> + }, {
> + 23750000, {
> + 0x00, 0xd1, 0x50, 0xf1, 0x86, 0x85, 0x80, 0x40,
> + 0x4f, 0x30, 0x33, 0x65, 0x00, 0x03, 0x25, 0x80,
> + 0x6c, 0xf2, 0x67, 0x00, 0x10, 0x8f, 0x30, 0x32,
> + 0x60, 0x8f, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00,
> + 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0xe0, 0x83, 0x0f, 0x3e, 0xf8, 0x00, 0x00,
> + },

Generally I see that these entries contain a high level of duplication.
Could you please extract the common part and a rate-dependent part.
Next, it would be best if instead of writing the register values via
the rate LUT, the driver could calculate those values.
This allows us to support other HDMI modes if the need arises at some point.

> + }, {
> + 24000000, {
> + 0x00, 0xd1, 0x50, 0xf0, 0x00, 0x00, 0x80, 0x00,
> + 0x4f, 0x30, 0x33, 0x65, 0x00, 0x01, 0x25, 0x80,
> + 0x6c, 0xf2, 0x67, 0x00, 0x10, 0x8f, 0x30, 0x32,
> + 0x60, 0x8f, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00,
> + 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0xe0, 0x83, 0x0f, 0x3e, 0xf8, 0x00, 0x00,
> + },

--
With best wishes
Dmitry