Re: [RFC PATCH 4/9] phy: samsung: Add SEC DSIM DPHY driver

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 21/06/2021 09:24, Jagan Teki wrote:
> Samsung SEC MIPI DSIM DPHY controller is part of registers
> available in SEC MIPI DSIM bridge for NXP's i.MX8M Mini and
> Nano Processors.
> 
> Add phy driver for it.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  drivers/phy/samsung/Kconfig             |   9 +
>  drivers/phy/samsung/Makefile            |   1 +
>  drivers/phy/samsung/phy-sec-dsim-dphy.c | 236 ++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/phy/samsung/phy-sec-dsim-dphy.c

You add a driver for a Samsung's component, so please Cc respective
folks. They might help you with it or provide comments to avoid
duplication of drivers/code:

Around phy:
Marek Szyprowski <m.szyprowski@samsung.com>
Jaehoon Chung <jh80.chung@samsung.com>

Around MIPI/DRM:
Inki Dae <inki.dae@samsung.com>
Seung-Woo Kim <sw0312.kim@samsung.com>
Andrzej Hajda <a.hajda@samsung.com>

and:
linux-samsung-soc@vger.kernel.org

> 
> diff --git a/drivers/phy/samsung/Kconfig b/drivers/phy/samsung/Kconfig
> index e20d2fcc9fe7..e80d40d1278c 100644
> --- a/drivers/phy/samsung/Kconfig
> +++ b/drivers/phy/samsung/Kconfig
> @@ -103,3 +103,12 @@ config PHY_EXYNOS5250_SATA
>  	  Exynos5250 based SoCs.This SerDes/Phy supports SATA 1.5 Gb/s,
>  	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
>  	  port to accept one SATA device.
> +
> +config PHY_SEC_DSIM_DPHY

SEC is not a codename of a project/product/silicon. It is a company
name, so basically you called this "PHY_SAMSUNG_DSIM_DPHY" which is too
generic.

> +	tristate "Samsung SEC MIPI DSIM DPHY driver"

What is Samsung SEC? Either Samsung or SEC, not both.

> +	depends on OF && HAS_IOMEM
> +	select GENERIC_PHY
> +	select REGMAP_MMIO
> +	help
> +          Enable this to add support for the SEC MIPI DSIM DPHY as found
> +          on NXP's i.MX8M Mini and Nano family of SOCs.
> diff --git a/drivers/phy/samsung/Makefile b/drivers/phy/samsung/Makefile
> index 3959100fe8a2..4d46c7ec0072 100644
> --- a/drivers/phy/samsung/Makefile
> +++ b/drivers/phy/samsung/Makefile
> @@ -11,3 +11,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
>  phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2)	+= phy-s5pv210-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS5_USBDRD)	+= phy-exynos5-usbdrd.o
>  obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
> +obj-$(CONFIG_PHY_SEC_DSIM_DPHY)		+= phy-sec-dsim-dphy.o
> diff --git a/drivers/phy/samsung/phy-sec-dsim-dphy.c b/drivers/phy/samsung/phy-sec-dsim-dphy.c
> new file mode 100644
> index 000000000000..31de4a774b5f
> --- /dev/null
> +++ b/drivers/phy/samsung/phy-sec-dsim-dphy.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 NXP
> + * Copyright (C) 2021 Amarula Solutions(India)
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/regmap.h>
> +
> +#define DSI_PHYCTRL_B1			0x00
> +#define DSI_PHYCTRL_B2			0x04
> +#define DSI_PHYCTRL_M1			0x08
> +#define DSI_PHYCTRL_M2			0x0c
> +#define DSI_PHYTIMING			0x10
> +#define DSI_PHYTIMING1			0x14
> +#define DSI_PHYTIMING2			0x18
> +
> +/* phytiming */
> +#define M_TLPXCTL_MASK			GENMASK(15, 8)

What is the "M_" prefix for?

> +#define M_TLPXCTL(x)			FIELD_PREP(M_TLPXCTL_MASK, (x))
> +#define M_THSEXITCTL_MASK		GENMASK(7, 0)
> +#define M_THSEXITCTL(x)			FIELD_PREP(M_THSEXITCTL_MASK, (x))
> +
> +/* phytiming1 */
> +#define M_TCLKPRPRCTL_MASK		GENMASK(31, 24)
> +#define M_TCLKPRPRCTL(x)		FIELD_PREP(M_TCLKPRPRCTL_MASK, (x))
> +#define M_TCLKZEROCTL_MASK		GENMASK(23, 16)
> +#define M_TCLKZEROCTL(x)		FIELD_PREP(M_TCLKZEROCTL_MASK, (x))
> +#define M_TCLKPOSTCTL_MASK		GENMASK(15, 8)
> +#define M_TCLKPOSTCTL(x)		FIELD_PREP(M_TCLKPOSTCTL_MASK, (x))
> +#define M_TCLKTRAILCTL_MASK		GENMASK(7, 0)
> +#define M_TCLKTRAILCTL(x)		FIELD_PREP(M_TCLKTRAILCTL_MASK, (x))
> +
> +/* phytiming2 */
> +#define M_THSPRPRCTL_MASK		GENMASK(23, 16)
> +#define M_THSPRPRCTL(x)			FIELD_PREP(M_THSPRPRCTL_MASK, (x))
> +#define M_THSZEROCTL_MASK		GENMASK(15, 8)
> +#define M_THSZEROCTL(x)			FIELD_PREP(M_THSZEROCTL_MASK, (x))
> +#define M_THSTRAILCTL_MASK		GENMASK(7, 0)
> +#define M_THSTRAILCTL(x)		FIELD_PREP(M_THSTRAILCTL_MASK, (x))
> +
> +struct dsim_dphy_plat_data {

Remove the m_ prefix. Please document all fields.

> +	unsigned int m_tlpxctl;
> +	unsigned int m_thsexitctl;
> +	unsigned int m_tclkprprctl;
> +	unsigned int m_tclkzeroctl;
> +	unsigned int m_tclkpostctl;
> +	unsigned int m_tclktrailctl;
> +	unsigned int m_thsprprctl;
> +	unsigned int m_thszeroctl;
> +	unsigned int m_thstrailctl;
> +};
> +
> +struct dsim_dphy {
> +	struct regmap *regmap;
> +	struct clk *phy_ref_clk;
> +	const struct dsim_dphy_plat_data *pdata;
> +};
> +
> +static const struct regmap_config dsim_dphy_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = DSI_PHYTIMING2,
> +	.name = "mipi-dphy",
> +};
> +
> +static int dsim_dphy_init(struct phy *phy)
> +{
> +	struct dsim_dphy *dphy = phy_get_drvdata(phy);
> +	const struct dsim_dphy_plat_data *pdata = dphy->pdata;
> +	u32 reg;
> +
> +	/* phytiming */
> +	regmap_read(dphy->regmap, DSI_PHYTIMING, &reg);
> +
> +	reg &= ~M_TLPXCTL_MASK;
> +	reg |= M_TLPXCTL(pdata->m_tlpxctl);
> +	reg &= ~M_THSEXITCTL_MASK;
> +	reg |= M_THSEXITCTL(pdata->m_thsexitctl);
> +	regmap_write(dphy->regmap, DSI_PHYTIMING, reg);
> +
> +	/* phytiming1 */
> +	regmap_read(dphy->regmap, DSI_PHYTIMING1, &reg);
> +
> +	reg &= ~M_TCLKPRPRCTL_MASK;
> +	reg |= M_TCLKPRPRCTL(pdata->m_tclkprprctl);
> +	reg &= ~M_TCLKZEROCTL_MASK;
> +	reg |= M_TCLKZEROCTL(pdata->m_tclkzeroctl);
> +	reg &= ~M_TCLKPOSTCTL_MASK;
> +	reg |= M_TCLKPOSTCTL(pdata->m_tclkpostctl);
> +	reg &= ~M_TCLKTRAILCTL_MASK;
> +	reg |= M_TCLKTRAILCTL(pdata->m_tclktrailctl);
> +	regmap_write(dphy->regmap, DSI_PHYTIMING1, reg);
> +
> +	/* phytiming2 */
> +	regmap_read(dphy->regmap, DSI_PHYTIMING2, &reg);
> +
> +	reg &= ~M_THSPRPRCTL_MASK;
> +	reg |= M_THSPRPRCTL(pdata->m_thsprprctl);
> +	reg &= ~M_THSZEROCTL_MASK;
> +	reg |= M_THSZEROCTL(pdata->m_thszeroctl);
> +	reg &= ~M_THSTRAILCTL_MASK;
> +	reg |= M_THSTRAILCTL(pdata->m_thstrailctl);
> +	regmap_write(dphy->regmap, DSI_PHYTIMING2, reg);
> +
> +	return 0;
> +}
> +
> +static int dsim_dphy_exit(struct phy *phy)
> +{
> +	return 0;
> +}
> +
> +static int dsim_dphy_power_on(struct phy *phy)
> +{
> +	struct dsim_dphy *dphy = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = clk_prepare_enable(dphy->phy_ref_clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;
> +}
> +
> +static int dsim_dphy_power_off(struct phy *phy)
> +{
> +	struct dsim_dphy *dphy = phy_get_drvdata(phy);
> +
> +	clk_disable_unprepare(dphy->phy_ref_clk);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops dsim_dphy_phy_ops = {
> +	.init = dsim_dphy_init,
> +	.exit = dsim_dphy_exit,
> +	.power_on = dsim_dphy_power_on,
> +	.power_off = dsim_dphy_power_off,
> +	.owner = THIS_MODULE,
> +};
> +
> +static const struct dsim_dphy_plat_data imx8mm_dphy_plat_data = {
> +	/* phytiming */
> +	.m_tlpxctl	= 0x06,
> +	.m_thsexitctl	= 0x0b,
> +	/* phytiming1 */
> +	.m_tclkprprctl	= 0x07,
> +	.m_tclkzeroctl	= 0x26,
> +	.m_tclkpostctl	= 0x0d,
> +	.m_tclktrailctl	= 0x08,
> +	/* phytimings2 */
> +	.m_thsprprctl	= 0x08,
> +	.m_thszeroctl	= 0x0d,
> +	.m_thstrailctl	= 0x0b,
> +};
> +
> +static const struct of_device_id dsim_dphy_of_match[] = {
> +	{
> +		.compatible = "fsl,imx8mm-sec-dsim-dphy",
> +		.data = &imx8mm_dphy_plat_data,
> +	},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, dsim_dphy_of_match);
> +
> +static int dsim_dphy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct phy_provider *phy_provider;
> +	struct dsim_dphy *dphy;
> +	struct phy *phy;
> +	void __iomem *base;
> +
> +	if (!np)
> +		return -ENODEV;

How can this driver bind without 'np'? How is it possible?

> +
> +	dphy = devm_kzalloc(dev, sizeof(*dphy), GFP_KERNEL);
> +	if (!dphy)
> +		return -ENOMEM;
> +
> +	dphy->pdata = of_device_get_match_data(&pdev->dev);
> +	if (!dphy->pdata)
> +		return -EINVAL;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	dphy->regmap = devm_regmap_init_mmio(&pdev->dev, base,
> +					     &dsim_dphy_regmap_config);
> +	if (IS_ERR(dphy->regmap)) {
> +		dev_err(dev, "failed create the DPHY regmap\n");
> +		return PTR_ERR(dphy->regmap);
> +	}
> +
> +	dphy->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref");
> +	if (IS_ERR(dphy->phy_ref_clk)) {
> +		dev_err(dev, "failed to get phy_ref clock\n");
> +		return PTR_ERR(dphy->phy_ref_clk);
> +	}
> +
> +	dev_set_drvdata(dev, dphy);
> +
> +	phy = devm_phy_create(dev, np, &dsim_dphy_phy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "failed to create phy %ld\n", PTR_ERR(phy));
> +		return PTR_ERR(phy);
> +	}
> +	phy_set_drvdata(phy, dphy);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver dsim_dphy_driver = {
> +	.probe	= dsim_dphy_probe,
> +	.driver = {
> +		.name = "sec-dsim-dphy",

"sec" it's too generic, what if next time we have another one from
Samsung? sec2? Please choose a name matching the actual component.

> +		.of_match_table	= dsim_dphy_of_match,
> +	}
> +};
> +module_platform_driver(dsim_dphy_driver);
> +
> +MODULE_AUTHOR("Jagan Teki <jagan@amarulasolutions.com>");
> +MODULE_DESCRIPTION("Samsung SEC MIPI DSIM DPHY driver");> +MODULE_LICENSE("GPL");
> 


Best regards,
Krzysztof