[PATCHv2,1/3] phy: Add exynos-simple-phy driver

Commit Message

Tomasz Stanislawski April 8, 2014, 2:37 p.m. UTC

Add exynos-simple-phy driver to support a single register
PHY interfaces present on Exynos4 SoC.

Signed-off-by: Tomasz Stanislawski <[e-mail]>
---
 .../devicetree/bindings/phy/samsung-phy.txt        |   24 +++
 drivers/phy/Kconfig                                |    5 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/exynos-simple-phy.c                    |  154 ++++++++++++++++++++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/phy/exynos-simple-phy.c

Comments

Andrzej Hajda April 9, 2014, 8:37 a.m. UTC | #1

Hi Tomasz,

On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
> Add exynos-simple-phy driver to support a single register
> PHY interfaces present on Exynos4 SoC.
> 
> Signed-off-by: Tomasz Stanislawski <[e-mail]>
> ---
>  .../devicetree/bindings/phy/samsung-phy.txt        |   24 +++
>  drivers/phy/Kconfig                                |    5 +
>  drivers/phy/Makefile                               |    1 +
>  drivers/phy/exynos-simple-phy.c                    |  154 ++++++++++++++++++++
>  4 files changed, 184 insertions(+)
>  create mode 100644 drivers/phy/exynos-simple-phy.c
> 
> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> index b422e38..f97c4c3 100644
> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
> @@ -114,3 +114,27 @@ Example:
>  		compatible = "samsung,exynos-sataphy-i2c";
>  		reg = <0x38>;
>  	};
> +
> +Samsung S5P/EXYNOS SoC series SIMPLE PHY
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : should be one of the listed compatibles:
> +	- "samsung,exynos4210-simple-phy"
> +	- "samsung,exynos4412-simple-phy"
> +- reg : offset and length of the register set;
> +- #phy-cells : from the generic phy bindings, must be 1;
> +
> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and its meaning is as follows:
> +  0 - HDMI PHY,
> +  1 - DAC PHY,
> +  2 - ADC PHY,
> +  3 - PCIE PHY.
> +  4 - SATA PHY.
> +
> +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
> +the PHY specifier identifies the PHY and its meaning is as follows:
> +  0 - HDMI PHY,
> +  1 - ADC PHY,

What about using preprocessor macros?

> +
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 3bb05f1..65ab783 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -166,4 +166,9 @@ config PHY_XGENE
>  	help
>  	  This option enables support for APM X-Gene SoC multi-purpose PHY.
>  
> +config EXYNOS_SIMPLE_PHY
> +	tristate "Exynos Simple PHY driver"
> +	help
> +	  Support for 1-bit PHY controllers on SoCs from Exynos family.
> +
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 2faf78e..88c5b60 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS4X12_USB2)	+= phy-exynos4x12-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
> +obj-$(CONFIG_EXYNOS_SIMPLE_PHY)		+= exynos-simple-phy.o
> diff --git a/drivers/phy/exynos-simple-phy.c b/drivers/phy/exynos-simple-phy.c
> new file mode 100644
> index 0000000..57ad338
> --- /dev/null
> +++ b/drivers/phy/exynos-simple-phy.c
> @@ -0,0 +1,154 @@
> +/*
> + * Exynos Simple PHY driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Author: Tomasz Stanislawski <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +
> +#define EXYNOS_PHY_ENABLE	(1 << 0)
> +
> +static int exynos_phy_power_on(struct phy *phy)
> +{
> +	void __iomem *reg = phy_get_drvdata(phy);
> +	u32 val;
> +
> +	val = readl(reg);
> +	val |= EXYNOS_PHY_ENABLE;
> +	writel(val, reg);
> +
> +	return 0;
> +}
> +
> +static int exynos_phy_power_off(struct phy *phy)
> +{
> +	void __iomem *reg = phy_get_drvdata(phy);
> +	u32 val;
> +
> +	val = readl(reg);
> +	val &= ~EXYNOS_PHY_ENABLE;
> +	writel(val, reg);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops exynos_phy_ops = {
> +	.power_on	= exynos_phy_power_on,
> +	.power_off	= exynos_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const u32 exynos4210_offsets[] = {
> +	0x0700, /* HDMI_PHY */
> +	0x070C, /* DAC_PHY */
> +	0x0718, /* ADC_PHY */
> +	0x071C, /* PCIE_PHY */
> +	0x0720, /* SATA_PHY */
> +	~0, /* end mark */
> +};
> +
> +static const u32 exynos4412_offsets[] = {
> +	0x0700, /* HDMI_PHY */
> +	0x0718, /* ADC_PHY */
> +	~0, /* end mark */
> +};

Why have you selected only these registers?
According to specs Exynos 4210 has 9 and 4412 has 7 control registers
with 'phy-enable' functionality.
I guess MIPI would require little more work as it has also reset bits,
but it will be still better than separate driver.

> +
> +static const struct of_device_id exynos_phy_of_match[] = {
> +	{ .compatible = "samsung,exynos4210-simple-phy",
> +	  .data = exynos4210_offsets},
> +	{ .compatible = "samsung,exynos4412-simple-phy",
> +	  .data = exynos4412_offsets},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
> +
> +static struct phy *exynos_phy_xlate(struct device *dev,
> +					struct of_phandle_args *args)
> +{
> +	struct phy **phys = dev_get_drvdata(dev);
> +	int index = args->args[0];
> +	int i;
> +
> +	/* verify if index is valid */
> +	for (i = 0; i <= index; ++i)
> +		if (!phys[i])
> +			return ERR_PTR(-ENODEV);
> +
> +	return phys[index];
> +}
> +
> +static int exynos_phy_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id = of_match_device(
> +		of_match_ptr(exynos_phy_of_match), &pdev->dev);
> +	const u32 *offsets = of_id->data;
> +	int count;
> +	struct device *dev = &pdev->dev;
> +	struct phy **phys;
> +	struct resource *res;
> +	void __iomem *regs;
> +	int i;
> +	struct phy_provider *phy_provider;
> +
> +	/* count number of phys to create */
> +	for (count = 0; offsets[count] != ~0; ++count)
> +		;

count = ARRAY_SIZE(offsets) - 1;

> +
> +	phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
> +	if (!phys)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, phys);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	regs = devm_ioremap(dev, res->start, res->end - res->start);
> +	if (!regs) {
> +		dev_err(dev, "failed to ioremap registers\n");
> +		return -EFAULT;
> +	}

Why not devm_ioremap_resource? If not, resource_size function calculates
length of resource correctly.

Anyway I like the idea of implementing multiple phys in one driver.
The only drawback I see is that some phys will be created even there are
no consumers for them. To avoid such situation you can try to use
lazy approach - create phy only if there is request for it,
exynos_phy_xlate callback should allow this.

Regards
Andrzej

> +
> +	/* NOTE: last entry in phys[] is NULL */
> +	for (i = 0; i < count; ++i) {
> +		phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
> +		if (IS_ERR(phys[i])) {
> +			dev_err(dev, "failed to create PHY %d\n", i);
> +			return PTR_ERR(phys[i]);
> +		}
> +		phy_set_drvdata(phys[i], regs + offsets[i]);
> +	}
> +
> +	phy_provider = devm_of_phy_provider_register(dev, exynos_phy_xlate);
> +	if (IS_ERR(phy_provider)) {
> +		dev_err(dev, "failed to register PHY provider\n");
> +		return PTR_ERR(phy_provider);
> +	}
> +
> +	dev_info(dev, "added %d phys\n", count);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver exynos_phy_driver = {
> +	.probe	= exynos_phy_probe,
> +	.driver = {
> +		.of_match_table	= exynos_phy_of_match,
> +		.name  = "exynos-simple-phy",
> +		.owner = THIS_MODULE,
> +	}
> +};
> +module_platform_driver(exynos_phy_driver);
> +
> +MODULE_DESCRIPTION("Exynos Simple PHY driver");
> +MODULE_AUTHOR("Tomasz Stanislawski <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
>

Rahul Sharma April 9, 2014, 9:12 a.m. UTC | #2

Hi Tomasz,

On 9 April 2014 14:07, Andrzej Hajda <[e-mail]> wrote:
> Hi Tomasz,
>
> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>> Add exynos-simple-phy driver to support a single register
>> PHY interfaces present on Exynos4 SoC.
>>
>> Signed-off-by: Tomasz Stanislawski <[e-mail]>
>> ---
>>  .../devicetree/bindings/phy/samsung-phy.txt        |   24 +++
>>  drivers/phy/Kconfig                                |    5 +
>>  drivers/phy/Makefile                               |    1 +
>>  drivers/phy/exynos-simple-phy.c                    |  154 ++++++++++++++++++++
>>  4 files changed, 184 insertions(+)
>>  create mode 100644 drivers/phy/exynos-simple-phy.c
>>
>> diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> index b422e38..f97c4c3 100644
>> --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
>> @@ -114,3 +114,27 @@ Example:
>>               compatible = "samsung,exynos-sataphy-i2c";
>>               reg = <0x38>;
>>       };
>> +
>> +Samsung S5P/EXYNOS SoC series SIMPLE PHY
>> +-------------------------------------------------
>> +
>> +Required properties:
>> +- compatible : should be one of the listed compatibles:
>> +     - "samsung,exynos4210-simple-phy"
>> +     - "samsung,exynos4412-simple-phy"
>> +- reg : offset and length of the register set;
>> +- #phy-cells : from the generic phy bindings, must be 1;
>> +
>> +For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
>> +the PHY specifier identifies the PHY and its meaning is as follows:
>> +  0 - HDMI PHY,
>> +  1 - DAC PHY,
>> +  2 - ADC PHY,
>> +  3 - PCIE PHY.
>> +  4 - SATA PHY.
>> +
>> +For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
>> +the PHY specifier identifies the PHY and its meaning is as follows:
>> +  0 - HDMI PHY,
>> +  1 - ADC PHY,
>
> What about using preprocessor macros?
>
>> +
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 3bb05f1..65ab783 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -166,4 +166,9 @@ config PHY_XGENE
>>       help
>>         This option enables support for APM X-Gene SoC multi-purpose PHY.
>>
>> +config EXYNOS_SIMPLE_PHY
>> +     tristate "Exynos Simple PHY driver"
>> +     help
>> +       Support for 1-bit PHY controllers on SoCs from Exynos family.
>> +
>>  endmenu
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 2faf78e..88c5b60 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_PHY_EXYNOS4210_USB2)   += phy-exynos4210-usb2.o
>>  obj-$(CONFIG_PHY_EXYNOS4X12_USB2)    += phy-exynos4x12-usb2.o
>>  obj-$(CONFIG_PHY_EXYNOS5250_USB2)    += phy-exynos5250-usb2.o
>>  obj-$(CONFIG_PHY_XGENE)                      += phy-xgene.o
>> +obj-$(CONFIG_EXYNOS_SIMPLE_PHY)              += exynos-simple-phy.o
>> diff --git a/drivers/phy/exynos-simple-phy.c b/drivers/phy/exynos-simple-phy.c
>> new file mode 100644
>> index 0000000..57ad338
>> --- /dev/null
>> +++ b/drivers/phy/exynos-simple-phy.c
>> @@ -0,0 +1,154 @@
>> +/*
>> + * Exynos Simple PHY driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Author: Tomasz Stanislawski <[e-mail]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +
>> +#define EXYNOS_PHY_ENABLE    (1 << 0)
>> +
>> +static int exynos_phy_power_on(struct phy *phy)
>> +{
>> +     void __iomem *reg = phy_get_drvdata(phy);
>> +     u32 val;
>> +
>> +     val = readl(reg);
>> +     val |= EXYNOS_PHY_ENABLE;
>> +     writel(val, reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static int exynos_phy_power_off(struct phy *phy)
>> +{
>> +     void __iomem *reg = phy_get_drvdata(phy);
>> +     u32 val;
>> +
>> +     val = readl(reg);
>> +     val &= ~EXYNOS_PHY_ENABLE;
>> +     writel(val, reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct phy_ops exynos_phy_ops = {
>> +     .power_on       = exynos_phy_power_on,
>> +     .power_off      = exynos_phy_power_off,
>> +     .owner          = THIS_MODULE,
>> +};
>> +
>> +static const u32 exynos4210_offsets[] = {
>> +     0x0700, /* HDMI_PHY */
>> +     0x070C, /* DAC_PHY */
>> +     0x0718, /* ADC_PHY */
>> +     0x071C, /* PCIE_PHY */
>> +     0x0720, /* SATA_PHY */
>> +     ~0, /* end mark */
>> +};
>> +
>> +static const u32 exynos4412_offsets[] = {
>> +     0x0700, /* HDMI_PHY */
>> +     0x0718, /* ADC_PHY */
>> +     ~0, /* end mark */
>> +};
>
> Why have you selected only these registers?
> According to specs Exynos 4210 has 9 and 4412 has 7 control registers
> with 'phy-enable' functionality.
> I guess MIPI would require little more work as it has also reset bits,
> but it will be still better than separate driver.
>
>> +
>> +static const struct of_device_id exynos_phy_of_match[] = {
>> +     { .compatible = "samsung,exynos4210-simple-phy",
>> +       .data = exynos4210_offsets},
>> +     { .compatible = "samsung,exynos4412-simple-phy",
>> +       .data = exynos4412_offsets},
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
>> +
>> +static struct phy *exynos_phy_xlate(struct device *dev,
>> +                                     struct of_phandle_args *args)
>> +{
>> +     struct phy **phys = dev_get_drvdata(dev);
>> +     int index = args->args[0];
>> +     int i;
>> +
>> +     /* verify if index is valid */
>> +     for (i = 0; i <= index; ++i)
>> +             if (!phys[i])
>> +                     return ERR_PTR(-ENODEV);
>> +
>> +     return phys[index];
>> +}
>> +
>> +static int exynos_phy_probe(struct platform_device *pdev)
>> +{
>> +     const struct of_device_id *of_id = of_match_device(
>> +             of_match_ptr(exynos_phy_of_match), &pdev->dev);
>> +     const u32 *offsets = of_id->data;
>> +     int count;
>> +     struct device *dev = &pdev->dev;
>> +     struct phy **phys;
>> +     struct resource *res;
>> +     void __iomem *regs;
>> +     int i;
>> +     struct phy_provider *phy_provider;
>> +
>> +     /* count number of phys to create */
>> +     for (count = 0; offsets[count] != ~0; ++count)
>> +             ;
>
> count = ARRAY_SIZE(offsets) - 1;
>
>> +
>> +     phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
>> +     if (!phys)
>> +             return -ENOMEM;
>> +
>> +     dev_set_drvdata(dev, phys);
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +     regs = devm_ioremap(dev, res->start, res->end - res->start);
>> +     if (!regs) {
>> +             dev_err(dev, "failed to ioremap registers\n");
>> +             return -EFAULT;
>> +     }
>
> Why not devm_ioremap_resource? If not, resource_size function calculates
> length of resource correctly.
>
> Anyway I like the idea of implementing multiple phys in one driver.
> The only drawback I see is that some phys will be created even there are
> no consumers for them. To avoid such situation you can try to use
> lazy approach - create phy only if there is request for it,
> exynos_phy_xlate callback should allow this.
>
> Regards
> Andrzej
>

Idea looks good. How about keeping compatible which is independent
of SoC, something like "samsung,exynos-simple-phy" and provide Reg
and Bit through phy provider node. This way we can avoid SoC specific
hardcoding in phy driver and don't need to look into dt bindings for
each new SoC.

We can use syscon interface to access PMU bits like USB phy.
PMU is already registered as system controller

Regards,
Rahul Sharma.

>> +
>> +     /* NOTE: last entry in phys[] is NULL */
>> +     for (i = 0; i < count; ++i) {
>> +             phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
>> +             if (IS_ERR(phys[i])) {
>> +                     dev_err(dev, "failed to create PHY %d\n", i);
>> +                     return PTR_ERR(phys[i]);
>> +             }
>> +             phy_set_drvdata(phys[i], regs + offsets[i]);
>> +     }
>> +
>> +     phy_provider = devm_of_phy_provider_register(dev, exynos_phy_xlate);
>> +     if (IS_ERR(phy_provider)) {
>> +             dev_err(dev, "failed to register PHY provider\n");
>> +             return PTR_ERR(phy_provider);
>> +     }
>> +
>> +     dev_info(dev, "added %d phys\n", count);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct platform_driver exynos_phy_driver = {
>> +     .probe  = exynos_phy_probe,
>> +     .driver = {
>> +             .of_match_table = exynos_phy_of_match,
>> +             .name  = "exynos-simple-phy",
>> +             .owner = THIS_MODULE,
>> +     }
>> +};
>> +module_platform_driver(exynos_phy_driver);
>> +
>> +MODULE_DESCRIPTION("Exynos Simple PHY driver");
>> +MODULE_AUTHOR("Tomasz Stanislawski <[e-mail]>");
>> +MODULE_LICENSE("GPL v2");
>>
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics April 9, 2014, 10:01 a.m. UTC | #3

Hi,

On 09/04/14 11:12, Rahul Sharma wrote:
> Idea looks good. How about keeping compatible which is independent
> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
> and Bit through phy provider node. This way we can avoid SoC specific
> hardcoding in phy driver and don't need to look into dt bindings for
> each new SoC.

I believe it is a not recommended approach.

> We can use syscon interface to access PMU bits like USB phy.
> PMU is already registered as system controller

Yes, that sounds good. This way we could avoid overlapping memory
mapped register regions specified in 'reg' properties in the device
tree.

Tomasz Stanislawski April 9, 2014, 11:14 a.m. UTC | #4

Hi Rahul,

On 04/09/2014 11:12 AM, Rahul Sharma wrote:
> Hi Tomasz,
> 
> On 9 April 2014 14:07, Andrzej Hajda <[e-mail]> wrote:
>> Hi Tomasz,
>>
>> On 04/08/2014 04:37 PM, Tomasz Stanislawski wrote:
>>> Add exynos-simple-phy driver to support a single register
>>> PHY interfaces present on Exynos4 SoC.
>>>
>>> Signed-off-by: Tomasz Stanislawski <[e-mail]>

[snip]

>>> +
>>> +     regs = devm_ioremap(dev, res->start, res->end - res->start);
>>> +     if (!regs) {
>>> +             dev_err(dev, "failed to ioremap registers\n");
>>> +             return -EFAULT;
>>> +     }
>>
>> Why not devm_ioremap_resource? If not, resource_size function calculates
>> length of resource correctly.
>>
>> Anyway I like the idea of implementing multiple phys in one driver.
>> The only drawback I see is that some phys will be created even there are
>> no consumers for them. To avoid such situation you can try to use
>> lazy approach - create phy only if there is request for it,
>> exynos_phy_xlate callback should allow this.
>>
>> Regards
>> Andrzej
>>
> 
> Idea looks good. How about keeping compatible which is independent
> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
> and Bit through phy provider node. This way we can avoid SoC specific
> hardcoding in phy driver and don't need to look into dt bindings for
> each new SoC.

A very nice idea BUT there is a very strong pressure from DT guys
to avoid adding any bit fields/offsets/masks in DT nodes.

Hopefully, as long as driver name starts with "exynos-" prefix
one can hide SoCs specific tricks deep inside driver code.

The idea behind this driver was not to create a generic phy for 1-bit
devices but rather to hide SoC-specific issues from client drivers
like DRM-HDMI.

> 
> We can use syscon interface to access PMU bits like USB phy.
> PMU is already registered as system controller
> 

Ok. I will try to use it in PATCHv3.

> Regards,
> Rahul Sharma.
> 

Regards,
Tomasz Stanislawski

Andreas Oberritter April 9, 2014, 11:47 a.m. UTC | #5

Hello Andrzej,

On 09.04.2014 10:37, Andrzej Hajda wrote:
>> +static int exynos_phy_probe(struct platform_device *pdev)
>> +{
>> +	const struct of_device_id *of_id = of_match_device(
>> +		of_match_ptr(exynos_phy_of_match), &pdev->dev);
>> +	const u32 *offsets = of_id->data;
>> +	int count;
>> +	struct device *dev = &pdev->dev;
>> +	struct phy **phys;
>> +	struct resource *res;
>> +	void __iomem *regs;
>> +	int i;
>> +	struct phy_provider *phy_provider;
>> +
>> +	/* count number of phys to create */
>> +	for (count = 0; offsets[count] != ~0; ++count)
>> +		;
> 
> count = ARRAY_SIZE(offsets) - 1;

u32 *offsets is not an array.

Regards,
Andreas

Rahul Sharma April 30, 2014, 6:37 a.m. UTC | #6

Hi Tomasz,

I have tested your patches for exynos5250 and 5420. Works fine. Are
you planning to post v3? If you want I can share hand with you for v3.

Regards,
Rahul Sharma

On 9 April 2014 17:17, Andreas Oberritter <[e-mail]> wrote:
> Hello Andrzej,
>
> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>> +static int exynos_phy_probe(struct platform_device *pdev)
>>> +{
>>> +    const struct of_device_id *of_id = of_match_device(
>>> +            of_match_ptr(exynos_phy_of_match), &pdev->dev);
>>> +    const u32 *offsets = of_id->data;
>>> +    int count;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct phy **phys;
>>> +    struct resource *res;
>>> +    void __iomem *regs;
>>> +    int i;
>>> +    struct phy_provider *phy_provider;
>>> +
>>> +    /* count number of phys to create */
>>> +    for (count = 0; offsets[count] != ~0; ++count)
>>> +            ;
>>
>> count = ARRAY_SIZE(offsets) - 1;
>
> u32 *offsets is not an array.
>
> Regards,
> Andreas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tomasz Stanislawski April 30, 2014, 8:32 a.m. UTC | #7

Hi Rahul,
I will prepare we v3 version.
Do you want me to add your patches for exynos5?50 to the patchset?
Regards,
Tomasz Stanislawski

On 04/30/2014 08:37 AM, Rahul Sharma wrote:
> Hi Tomasz,
> 
> I have tested your patches for exynos5250 and 5420. Works fine. Are
> you planning to post v3? If you want I can share hand with you for v3.
> 
> Regards,
> Rahul Sharma
> 
> On 9 April 2014 17:17, Andreas Oberritter <[e-mail]> wrote:
>> Hello Andrzej,
>>
>> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>>> +static int exynos_phy_probe(struct platform_device *pdev)
>>>> +{
>>>> +    const struct of_device_id *of_id = of_match_device(
>>>> +            of_match_ptr(exynos_phy_of_match), &pdev->dev);
>>>> +    const u32 *offsets = of_id->data;
>>>> +    int count;
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct phy **phys;
>>>> +    struct resource *res;
>>>> +    void __iomem *regs;
>>>> +    int i;
>>>> +    struct phy_provider *phy_provider;
>>>> +
>>>> +    /* count number of phys to create */
>>>> +    for (count = 0; offsets[count] != ~0; ++count)
>>>> +            ;
>>>
>>> count = ARRAY_SIZE(offsets) - 1;
>>
>> u32 *offsets is not an array.
>>
>> Regards,
>> Andreas
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Rahul Sharma April 30, 2014, 8:43 a.m. UTC | #8

Sure (5250, 5420). I will wait for the same to update DT patches, if any.

Regards,
Rahul Sharma.

On 30 April 2014 14:02, Tomasz Stanislawski <[e-mail]> wrote:
> Hi Rahul,
> I will prepare we v3 version.
> Do you want me to add your patches for exynos5?50 to the patchset?
> Regards,
> Tomasz Stanislawski
>
> On 04/30/2014 08:37 AM, Rahul Sharma wrote:
>> Hi Tomasz,
>>
>> I have tested your patches for exynos5250 and 5420. Works fine. Are
>> you planning to post v3? If you want I can share hand with you for v3.
>>
>> Regards,
>> Rahul Sharma
>>
>> On 9 April 2014 17:17, Andreas Oberritter <[e-mail]> wrote:
>>> Hello Andrzej,
>>>
>>> On 09.04.2014 10:37, Andrzej Hajda wrote:
>>>>> +static int exynos_phy_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +    const struct of_device_id *of_id = of_match_device(
>>>>> +            of_match_ptr(exynos_phy_of_match), &pdev->dev);
>>>>> +    const u32 *offsets = of_id->data;
>>>>> +    int count;
>>>>> +    struct device *dev = &pdev->dev;
>>>>> +    struct phy **phys;
>>>>> +    struct resource *res;
>>>>> +    void __iomem *regs;
>>>>> +    int i;
>>>>> +    struct phy_provider *phy_provider;
>>>>> +
>>>>> +    /* count number of phys to create */
>>>>> +    for (count = 0; offsets[count] != ~0; ++count)
>>>>> +            ;
>>>>
>>>> count = ARRAY_SIZE(offsets) - 1;
>>>
>>> u32 *offsets is not an array.
>>>
>>> Regards,
>>> Andreas
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to [e-mail]
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Kishon Vijay Abraham I May 5, 2014, 9:44 a.m. UTC | #9

Hi,

On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
> Hi,
> 
> On 09/04/14 11:12, Rahul Sharma wrote:
>> Idea looks good. How about keeping compatible which is independent
>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>> and Bit through phy provider node. This way we can avoid SoC specific
>> hardcoding in phy driver and don't need to look into dt bindings for
>> each new SoC.
> 
> I believe it is a not recommended approach.

Why not? We should try to avoid hard coding in the driver code. Moreover by
avoiding hardcoding we can make it a generic driver for single bit PHYs.

Cheers
Kishon

Rahul Sharma May 7, 2014, 10:38 a.m. UTC | #10

On 5 May 2014 15:14, Kishon Vijay Abraham I <[e-mail]> wrote:
> Hi,
>
> On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
>> Hi,
>>
>> On 09/04/14 11:12, Rahul Sharma wrote:
>>> Idea looks good. How about keeping compatible which is independent
>>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>>> and Bit through phy provider node. This way we can avoid SoC specific
>>> hardcoding in phy driver and don't need to look into dt bindings for
>>> each new SoC.
>>
>> I believe it is a not recommended approach.
>
> Why not? We should try to avoid hard coding in the driver code. Moreover by
> avoiding hardcoding we can make it a generic driver for single bit PHYs.
>

+1.

@Tomasz, any plans to consider this approach for simple phy driver?

Regards,
Rahul Sharma.

> Cheers
> Kishon

Tomasz Stanislawski May 7, 2014, 1:36 p.m. UTC | #11

On 05/07/2014 12:38 PM, Rahul Sharma wrote:
> On 5 May 2014 15:14, Kishon Vijay Abraham I <[e-mail]> wrote:
>> Hi,
>>
>> On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
>>> Hi,
>>>
>>> On 09/04/14 11:12, Rahul Sharma wrote:
>>>> Idea looks good. How about keeping compatible which is independent
>>>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>>>> and Bit through phy provider node. This way we can avoid SoC specific
>>>> hardcoding in phy driver and don't need to look into dt bindings for
>>>> each new SoC.
>>>
>>> I believe it is a not recommended approach.
>>
>> Why not? We should try to avoid hard coding in the driver code. Moreover by
>> avoiding hardcoding we can make it a generic driver for single bit PHYs.
>>
> 
> +1.
> 
> @Tomasz, any plans to consider this approach for simple phy driver?
> 
> Regards,
> Rahul Sharma.
> 

Hi Rahul,
Initially, I wanted to make a very generic driver and to add bit and
register (or its offset) attribute to the PHY node.
However, there was a very strong opposition from DT maintainers
to adding any bit related configuration to DT.
The current solution was designed to be a trade-off between
being generic and being accepted :).

Regards,
Tomasz Stanislawski



>> Cheers
>> Kishon
>

Rahul Sharma May 7, 2014, 2:19 p.m. UTC | #12

On 7 May 2014 19:06, Tomasz Stanislawski <[e-mail]> wrote:
> On 05/07/2014 12:38 PM, Rahul Sharma wrote:
>> On 5 May 2014 15:14, Kishon Vijay Abraham I <[e-mail]> wrote:
>>> Hi,
>>>
>>> On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
>>>> Hi,
>>>>
>>>> On 09/04/14 11:12, Rahul Sharma wrote:
>>>>> Idea looks good. How about keeping compatible which is independent
>>>>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>>>>> and Bit through phy provider node. This way we can avoid SoC specific
>>>>> hardcoding in phy driver and don't need to look into dt bindings for
>>>>> each new SoC.
>>>>
>>>> I believe it is a not recommended approach.
>>>
>>> Why not? We should try to avoid hard coding in the driver code. Moreover by
>>> avoiding hardcoding we can make it a generic driver for single bit PHYs.
>>>
>>
>> +1.
>>
>> @Tomasz, any plans to consider this approach for simple phy driver?
>>
>> Regards,
>> Rahul Sharma.
>>
>
> Hi Rahul,
> Initially, I wanted to make a very generic driver and to add bit and
> register (or its offset) attribute to the PHY node.
> However, there was a very strong opposition from DT maintainers
> to adding any bit related configuration to DT.
> The current solution was designed to be a trade-off between
> being generic and being accepted :).
>

Thanks Tomasz,
Ok got it. lets discuss it again and conclude it.

@Kishon, DT-folks,

The original RFC patch from Tomasz (at https://lkml.org/lkml/2013/10/21/313)
added simple phy driver as "Generic-simple-phy" with these properties:

+ of_property_read_u32(dev->of_node, "mask", &sphy->mask);
+ of_property_read_u32(dev->of_node, "on-value", &sphy->on_value);
+ of_property_read_u32(dev->of_node, "off-value", &sphy->off_value);

Shall we consider the same solution again for generic simple phy
driver which just expose on/off control through register bit.

Regards,
Rahul Sharma

> Regards,
> Tomasz Stanislawski
>
>
>
>>> Cheers
>>> Kishon
>>
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Tomasz Figa May 7, 2014, 3:33 p.m. UTC | #13

[CCing more DT-folks :)]

On 07.05.2014 16:19, Rahul Sharma wrote:
> On 7 May 2014 19:06, Tomasz Stanislawski <[e-mail]> wrote:
>> On 05/07/2014 12:38 PM, Rahul Sharma wrote:
>>> On 5 May 2014 15:14, Kishon Vijay Abraham I <[e-mail]> wrote:
>>>> Hi,
>>>>
>>>> On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
>>>>> Hi,
>>>>>
>>>>> On 09/04/14 11:12, Rahul Sharma wrote:
>>>>>> Idea looks good. How about keeping compatible which is independent
>>>>>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>>>>>> and Bit through phy provider node. This way we can avoid SoC specific
>>>>>> hardcoding in phy driver and don't need to look into dt bindings for
>>>>>> each new SoC.
>>>>>
>>>>> I believe it is a not recommended approach.
>>>>
>>>> Why not? We should try to avoid hard coding in the driver code. Moreover by
>>>> avoiding hardcoding we can make it a generic driver for single bit PHYs.
>>>>
>>>
>>> +1.
>>>
>>> @Tomasz, any plans to consider this approach for simple phy driver?
>>>
>>> Regards,
>>> Rahul Sharma.
>>>
>>
>> Hi Rahul,
>> Initially, I wanted to make a very generic driver and to add bit and
>> register (or its offset) attribute to the PHY node.
>> However, there was a very strong opposition from DT maintainers
>> to adding any bit related configuration to DT.
>> The current solution was designed to be a trade-off between
>> being generic and being accepted :).
>>
> 
> Thanks Tomasz,
> Ok got it. lets discuss it again and conclude it.
> 
> @Kishon, DT-folks,
> 
> The original RFC patch from Tomasz (at https://lkml.org/lkml/2013/10/21/313)
> added simple phy driver as "Generic-simple-phy" with these properties:
> 
> + of_property_read_u32(dev->of_node, "mask", &sphy->mask);
> + of_property_read_u32(dev->of_node, "on-value", &sphy->on_value);
> + of_property_read_u32(dev->of_node, "off-value", &sphy->off_value);
> 
> Shall we consider the same solution again for generic simple phy
> driver which just expose on/off control through register bit.
> 
> Regards,
> Rahul Sharma
> 
>> Regards,
>> Tomasz Stanislawski
>>
>>
>>
>>>> Cheers
>>>> Kishon
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Best regards,
Tomasz

Rahul Sharma May 13, 2014, 11:20 a.m. UTC | #14

<Gentle PING>

On 7 May 2014 21:03, Tomasz Figa <[e-mail]> wrote:
> [CCing more DT-folks :)]
>
> On 07.05.2014 16:19, Rahul Sharma wrote:
>> On 7 May 2014 19:06, Tomasz Stanislawski <[e-mail]> wrote:
>>> On 05/07/2014 12:38 PM, Rahul Sharma wrote:
>>>> On 5 May 2014 15:14, Kishon Vijay Abraham I <[e-mail]> wrote:
>>>>> Hi,
>>>>>
>>>>> On Wednesday 09 April 2014 03:31 PM, Sylwester Nawrocki wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 09/04/14 11:12, Rahul Sharma wrote:
>>>>>>> Idea looks good. How about keeping compatible which is independent
>>>>>>> of SoC, something like "samsung,exynos-simple-phy" and provide Reg
>>>>>>> and Bit through phy provider node. This way we can avoid SoC specific
>>>>>>> hardcoding in phy driver and don't need to look into dt bindings for
>>>>>>> each new SoC.
>>>>>>
>>>>>> I believe it is a not recommended approach.
>>>>>
>>>>> Why not? We should try to avoid hard coding in the driver code. Moreover by
>>>>> avoiding hardcoding we can make it a generic driver for single bit PHYs.
>>>>>
>>>>
>>>> +1.
>>>>
>>>> @Tomasz, any plans to consider this approach for simple phy driver?
>>>>
>>>> Regards,
>>>> Rahul Sharma.
>>>>
>>>
>>> Hi Rahul,
>>> Initially, I wanted to make a very generic driver and to add bit and
>>> register (or its offset) attribute to the PHY node.
>>> However, there was a very strong opposition from DT maintainers
>>> to adding any bit related configuration to DT.
>>> The current solution was designed to be a trade-off between
>>> being generic and being accepted :).
>>>
>>
>> Thanks Tomasz,
>> Ok got it. lets discuss it again and conclude it.
>>
>> @Kishon, DT-folks,
>>
>> The original RFC patch from Tomasz (at https://lkml.org/lkml/2013/10/21/313)
>> added simple phy driver as "Generic-simple-phy" with these properties:
>>
>> + of_property_read_u32(dev->of_node, "mask", &sphy->mask);
>> + of_property_read_u32(dev->of_node, "on-value", &sphy->on_value);
>> + of_property_read_u32(dev->of_node, "off-value", &sphy->off_value);
>>
>> Shall we consider the same solution again for generic simple phy
>> driver which just expose on/off control through register bit.
>>
>> Regards,
>> Rahul Sharma
>>
>>> Regards,
>>> Tomasz Stanislawski
>>>
>>>
>>>
>>>>> Cheers
>>>>> Kishon
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> [e-mail]
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index b422e38..f97c4c3 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -114,3 +114,27 @@  Example:
 		compatible = "samsung,exynos-sataphy-i2c";
 		reg = <0x38>;
 	};
+
+Samsung S5P/EXYNOS SoC series SIMPLE PHY
+-------------------------------------------------
+
+Required properties:
+- compatible : should be one of the listed compatibles:
+	- "samsung,exynos4210-simple-phy"
+	- "samsung,exynos4412-simple-phy"
+- reg : offset and length of the register set;
+- #phy-cells : from the generic phy bindings, must be 1;
+
+For "samsung,exynos4210-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and its meaning is as follows:
+  0 - HDMI PHY,
+  1 - DAC PHY,
+  2 - ADC PHY,
+  3 - PCIE PHY.
+  4 - SATA PHY.
+
+For "samsung,exynos4412-simple-phy" compatible PHYs the second cell in
+the PHY specifier identifies the PHY and its meaning is as follows:
+  0 - HDMI PHY,
+  1 - ADC PHY,
+
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 3bb05f1..65ab783 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -166,4 +166,9 @@  config PHY_XGENE
 	help
 	  This option enables support for APM X-Gene SoC multi-purpose PHY.
 
+config EXYNOS_SIMPLE_PHY
+	tristate "Exynos Simple PHY driver"
+	help
+	  Support for 1-bit PHY controllers on SoCs from Exynos family.
+
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 2faf78e..88c5b60 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -18,3 +18,4 @@  obj-$(CONFIG_PHY_EXYNOS4210_USB2)	+= phy-exynos4210-usb2.o
 obj-$(CONFIG_PHY_EXYNOS4X12_USB2)	+= phy-exynos4x12-usb2.o
 obj-$(CONFIG_PHY_EXYNOS5250_USB2)	+= phy-exynos5250-usb2.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
+obj-$(CONFIG_EXYNOS_SIMPLE_PHY)		+= exynos-simple-phy.o
diff --git a/drivers/phy/exynos-simple-phy.c b/drivers/phy/exynos-simple-phy.c
new file mode 100644
index 0000000..57ad338
--- /dev/null
+++ b/drivers/phy/exynos-simple-phy.c
@@ -0,0 +1,154 @@ 
+/*
+ * Exynos Simple PHY driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Tomasz Stanislawski <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+
+#define EXYNOS_PHY_ENABLE	(1 << 0)
+
+static int exynos_phy_power_on(struct phy *phy)
+{
+	void __iomem *reg = phy_get_drvdata(phy);
+	u32 val;
+
+	val = readl(reg);
+	val |= EXYNOS_PHY_ENABLE;
+	writel(val, reg);
+
+	return 0;
+}
+
+static int exynos_phy_power_off(struct phy *phy)
+{
+	void __iomem *reg = phy_get_drvdata(phy);
+	u32 val;
+
+	val = readl(reg);
+	val &= ~EXYNOS_PHY_ENABLE;
+	writel(val, reg);
+
+	return 0;
+}
+
+static struct phy_ops exynos_phy_ops = {
+	.power_on	= exynos_phy_power_on,
+	.power_off	= exynos_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const u32 exynos4210_offsets[] = {
+	0x0700, /* HDMI_PHY */
+	0x070C, /* DAC_PHY */
+	0x0718, /* ADC_PHY */
+	0x071C, /* PCIE_PHY */
+	0x0720, /* SATA_PHY */
+	~0, /* end mark */
+};
+
+static const u32 exynos4412_offsets[] = {
+	0x0700, /* HDMI_PHY */
+	0x0718, /* ADC_PHY */
+	~0, /* end mark */
+};
+
+static const struct of_device_id exynos_phy_of_match[] = {
+	{ .compatible = "samsung,exynos4210-simple-phy",
+	  .data = exynos4210_offsets},
+	{ .compatible = "samsung,exynos4412-simple-phy",
+	  .data = exynos4412_offsets},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, exynos_phy_of_match);
+
+static struct phy *exynos_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct phy **phys = dev_get_drvdata(dev);
+	int index = args->args[0];
+	int i;
+
+	/* verify if index is valid */
+	for (i = 0; i <= index; ++i)
+		if (!phys[i])
+			return ERR_PTR(-ENODEV);
+
+	return phys[index];
+}
+
+static int exynos_phy_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id = of_match_device(
+		of_match_ptr(exynos_phy_of_match), &pdev->dev);
+	const u32 *offsets = of_id->data;
+	int count;
+	struct device *dev = &pdev->dev;
+	struct phy **phys;
+	struct resource *res;
+	void __iomem *regs;
+	int i;
+	struct phy_provider *phy_provider;
+
+	/* count number of phys to create */
+	for (count = 0; offsets[count] != ~0; ++count)
+		;
+
+	phys = devm_kzalloc(dev, (count + 1) * sizeof(phys[0]), GFP_KERNEL);
+	if (!phys)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, phys);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	regs = devm_ioremap(dev, res->start, res->end - res->start);
+	if (!regs) {
+		dev_err(dev, "failed to ioremap registers\n");
+		return -EFAULT;
+	}
+
+	/* NOTE: last entry in phys[] is NULL */
+	for (i = 0; i < count; ++i) {
+		phys[i] = devm_phy_create(dev, &exynos_phy_ops, NULL);
+		if (IS_ERR(phys[i])) {
+			dev_err(dev, "failed to create PHY %d\n", i);
+			return PTR_ERR(phys[i]);
+		}
+		phy_set_drvdata(phys[i], regs + offsets[i]);
+	}
+
+	phy_provider = devm_of_phy_provider_register(dev, exynos_phy_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "failed to register PHY provider\n");
+		return PTR_ERR(phy_provider);
+	}
+
+	dev_info(dev, "added %d phys\n", count);
+
+	return 0;
+}
+
+static struct platform_driver exynos_phy_driver = {
+	.probe	= exynos_phy_probe,
+	.driver = {
+		.of_match_table	= exynos_phy_of_match,
+		.name  = "exynos-simple-phy",
+		.owner = THIS_MODULE,
+	}
+};
+module_platform_driver(exynos_phy_driver);
+
+MODULE_DESCRIPTION("Exynos Simple PHY driver");
+MODULE_AUTHOR("Tomasz Stanislawski <[e-mail]>");
+MODULE_LICENSE("GPL v2");