Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16 May 2014 20:19, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 16:30, Rahul Sharma wrote:
On 16 May 2014 16:20, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys
with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
I agree. Actually the role of generic phy framework for these 'simple' phys
is
only that much.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need that
much functionality from the phy provider.
Well, this is what several drivers already do, like USB PHY (dedicated
IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
too) or will do, like I2C (for configuration of I2C mux on Exynos5).
At least this would be consistent with them and wouldn't be an API
abuse, so I'd be inclined to go this way more than introducing
abstractions like this patch does.
Ok. I had already posted a patch for this at
http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
I will revive that thread.
@Tomasz Stanislawski, Do you have different opinion here?
Regards,
Rahul Sharma.
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 19.05.2014 09:10, Rahul Sharma wrote:
On 16 May 2014 20:19, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 16:30, Rahul Sharma wrote:
On 16 May 2014 16:20, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider,
e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these
phys with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
I agree. Actually the role of generic phy framework for these 'simple' phys
is
only that much.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we
will
not be having any common phy provider for all these independent PHYs and
it
would be clean to add each of these phy nodes in DT. Please see my
original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need
that
much functionality from the phy provider.
Well, this is what several drivers already do, like USB PHY (dedicated
IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
too) or will do, like I2C (for configuration of I2C mux on Exynos5).
At least this would be consistent with them and wouldn't be an API
abuse, so I'd be inclined to go this way more than introducing
abstractions like this patch does.
Ok. I had already posted a patch for this at
http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
I will revive that thread.
Looks good to me.
@Tomasz Stanislawski, Do you have different opinion here?
I'm afraid Tomasz might not be very responsive during next few days, as
he is on a business trip. You might be able to reach him on our internal
communicator, though.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 19 May 2014 16:24, Tomasz Figa tomasz.f...@gmail.com wrote:
On 19.05.2014 09:10, Rahul Sharma wrote:
On 16 May 2014 20:19, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 16:30, Rahul Sharma wrote:
On 16 May 2014 16:20, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider,
e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree
we
must design things carefully, so it's better to take a bit more time
and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these
phys with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
I agree. Actually the role of generic phy framework for these 'simple'
phys is
only that much.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we
will
not be having any common phy provider for all these independent PHYs and
it
would be clean to add each of these phy nodes in DT. Please see my
original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need
that
much functionality from the phy provider.
Well, this is what several drivers already do, like USB PHY (dedicated
IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
too) or will do, like I2C (for configuration of I2C mux on Exynos5).
At least this would be consistent with them and wouldn't be an API
abuse, so I'd be inclined to go this way more than introducing
abstractions like this patch does.
Ok. I had already posted a patch for this at
http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
I will revive that thread.
Looks good to me.
@Tomasz Stanislawski, Do you have different opinion here?
I'm afraid Tomasz might not be very responsive during next few days, as
he is on a business trip. You might be able to reach him on our internal
communicator, though.
Thanks Tomasz,
I will contact him over communicator.
Regards.
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
Thanks Tomasz,
On 15 May 2014 01:31, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Rahul, Tomasz,
[snip]
+ simplephys: simple-phys@1004 {
+ compatible = samsung,exynos5250-simple-phy;
Missing reg property or unnecessary @unit-address suffix in node name.
I should have removed @unit-address. I will change this.
+ samsung,pmu-syscon = pmu_system_controller;
+ #phy-cells = 1;
+ };
In general, the idea is quite good, but I think this should rather bind
to the main PMU node, since this is just a part of the PMU, not another
device in the system. This also means that the PMU node itself should be
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
Now, there is already ongoing effort to convert current freestanding PMU
configuration code in mach-exynos into a full-fledged PMU driver, but
not exactly in the same direction as I stated above. I'll try to
cooperate with Pankaj, who is responsible for this work to make this go
into the right track.
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys with
the PMU is 'enable/disable control'. Controlling this bit using regmap interface
still looks better to me.
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
Regards,
Rahul Sharma
Now, there is already ongoing effort to convert current freestanding PMU
configuration code in mach-exynos into a full-fledged PMU driver, but
not exactly in the same direction as I stated above. I'll try to
cooperate with Pankaj, who is responsible for this work to make this go
into the right track.
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys
with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16 May 2014 16:20, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys
with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
I agree. Actually the role of generic phy framework for these 'simple' phys is
only that much.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need that
much functionality from the phy provider.
Regards,
Rahul Sharma
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 16.05.2014 16:30, Rahul Sharma wrote:
On 16 May 2014 16:20, Tomasz Figa t.f...@samsung.com wrote:
On 16.05.2014 12:35, Rahul Sharma wrote:
On 16 May 2014 15:12, Rahul Sharma rahul.sha...@samsung.com wrote:
On 16 May 2014 03:14, Tomasz Figa tomasz.f...@gmail.com wrote:
On 15.05.2014 06:01, Rahul Sharma wrote:
[snip]
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Hi Tomasz,
These PHYs are not part of PMU as such. I am not sure if it is correct to
probe them as phy provider for all these phys. Only relation of these phys
with
the PMU is 'enable/disable control'.
Well, in reality what is implemented by this driver is not even a PHY,
just some kind of power controllers, which are contained entirely in the
PMU.
I agree. Actually the role of generic phy framework for these 'simple' phys is
only that much.
Controlling this bit using regmap interface
still looks better to me.
Well, when there is a choice between using regmap and not using regmap,
I'd rather choose the latter. Why would you want to introduce additional
abstraction layer if there is no need for such?
IMHO Ideal method would be probing these PHYs independently and resolving
the necessary dependencies like syscon handle, clocks etc. This way we will
not be having any common phy provider for all these independent PHYs and it
would be clean to add each of these phy nodes in DT. Please see my original
comment below.
http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
With the solution I proposed, you don't need any kind of dependencies
for those simple power controllers. They are just single bits that don't
need anything special to operate, except PMU clock running.
In that case we can further trim it down and let the drivers use the regmap
interface to control this bit. Many drivers including HDMI, DP just need that
much functionality from the phy provider.
Well, this is what several drivers already do, like USB PHY (dedicated
IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
too) or will do, like I2C (for configuration of I2C mux on Exynos5).
At least this would be consistent with them and wouldn't be an API
abuse, so I'd be inclined to go this way more than introducing
abstractions like this patch does.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: Hi Thierry, On 15 May 2014 03:44, Thierry Reding thierry.red...@gmail.com wrote: On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: [...] +#define HDMI_PHY 0 +#define DAC_PHY 1 +#define ADC_PHY 2 +#define PCIE_PHY 3 +#define SATA_PHY 4 Perhaps these should be namespaced somehow to avoid potential conflicts with other PHY providers? How about XXX_SIMPLE_PHY? The indices are specific to the Exynos PHY device, so why not PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)? +#define PHY_NR 5 I'm not sure that this belongs here either. It's not a value that will ever appear in a DT source file. I want it to grow along with new additions in the phy list else catastrophic. This will look unrelated in driver. But this is in no way growing automatically as it is. Whoever adds a new type of PHY will need to manually increment this define. Furthermore the driver will need to be updated to cope with this anyway. I think this is even a reason to have this only in the driver. Otherwise you could have a situation where somebody upgrades the binding (and this file along with it) but not update the driver with the necessary support. But the driver will still pick up the PHY_NR change and will accept the new PHY type as valid, even though it has no code to handle it. If you have this in the driver, however, then as long as the driver has not yet been updated it will reject requests for the new PHY type. And that's exactly what it should be doing since it doesn't know how to handle it. Thierry pgpQbsvs8_Iog.pgp Description: PGP signature
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 15 May 2014 13:12, Thierry Reding thierry.red...@gmail.com wrote: On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: Hi Thierry, On 15 May 2014 03:44, Thierry Reding thierry.red...@gmail.com wrote: On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: [...] +#define HDMI_PHY 0 +#define DAC_PHY 1 +#define ADC_PHY 2 +#define PCIE_PHY 3 +#define SATA_PHY 4 Perhaps these should be namespaced somehow to avoid potential conflicts with other PHY providers? How about XXX_SIMPLE_PHY? The indices are specific to the Exynos PHY device, so why not PHY_EXYNOS_SIMPLE_XXX (or any variation thereof)? This looks ok. I will use that. +#define PHY_NR 5 I'm not sure that this belongs here either. It's not a value that will ever appear in a DT source file. I want it to grow along with new additions in the phy list else catastrophic. This will look unrelated in driver. But this is in no way growing automatically as it is. Whoever adds a new type of PHY will need to manually increment this define. Furthermore the driver will need to be updated to cope with this anyway. Not automatically. What I meant was If keeping it at end of the list, it is not possible that somebody skip the updation of PHY_NR when adding a new phy type. If I leave a comment at the end of the list to update PHY_NR (after moving it to driver), that also serves the purpose. I think this is even a reason to have this only in the driver. Otherwise you could have a situation where somebody upgrades the binding (and this file along with it) but not update the driver with the necessary support. But the driver will still pick up the PHY_NR change and will accept the new PHY type as valid, even though it has no code to handle it. If you have this in the driver, however, then as long as the driver has not yet been updated it will reject requests for the new PHY type. And that's exactly what it should be doing since it doesn't know how to handle it. hmn...Ok. Regards, Rahul Sharma Thierry -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On Thu, May 15, 2014 at 01:47:33PM +0530, Rahul Sharma wrote: On 15 May 2014 13:12, Thierry Reding thierry.red...@gmail.com wrote: On Thu, May 15, 2014 at 10:49:37AM +0530, Rahul Sharma wrote: On 15 May 2014 03:44, Thierry Reding thierry.red...@gmail.com wrote: On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote: [...] +#define PHY_NR 5 I'm not sure that this belongs here either. It's not a value that will ever appear in a DT source file. I want it to grow along with new additions in the phy list else catastrophic. This will look unrelated in driver. But this is in no way growing automatically as it is. Whoever adds a new type of PHY will need to manually increment this define. Furthermore the driver will need to be updated to cope with this anyway. Not automatically. What I meant was If keeping it at end of the list, it is not possible that somebody skip the updation of PHY_NR when adding a new phy type. It's perhaps not as likely, but still possible. If I leave a comment at the end of the list to update PHY_NR (after moving it to driver), that also serves the purpose. I don't think this is needed either. Like I said earlier, since the driver has an internal maximum number of PHYs that it supports the maximum that can be specified in the DTS is irrelevant. If it doesn't support a new one, then it will simply return an error. And I would assume that if somebody added support for a new PHY type then they probably wouldn't forget to update the driver since they're modifying it anyway and testing will fail if they don't. Thierry pgpK8tcno6Din.pgp Description: PGP signature
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
Hi, On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: From: Tomasz Stanislawski t.stanisl...@samsung.com Add exynos-simple-phy driver to support a single register PHY interfaces present on Exynos4 SoC. Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- .../devicetree/bindings/phy/samsung-phy.txt| 56 ++ drivers/phy/Kconfig|5 + drivers/phy/Makefile |1 + drivers/phy/exynos-simple-phy.c| 189 include/dt-bindings/phy/exynos-simple-phy.h| 27 +++ 5 files changed, 278 insertions(+) create mode 100644 drivers/phy/exynos-simple-phy.c create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h [...] diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f06..2a13e0d 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -178,4 +178,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 Please limit this driver to EXYNOS platforms with: depends on ARCH_EXYNOS or (optionally) depends on ARCH_EXYNOS || COMPILE_TEST Moreover the generic PHY dependency is missing. It can be fixed with: select GENERIC_PHY + help + Support for 1-bit PHY controllers on SoCs from Exynos family. + endmenu Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
Hi, On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: Hi, On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: From: Tomasz Stanislawski t.stanisl...@samsung.com Add exynos-simple-phy driver to support a single register PHY interfaces present on Exynos4 SoC. Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- .../devicetree/bindings/phy/samsung-phy.txt| 56 ++ drivers/phy/Kconfig|5 + drivers/phy/Makefile |1 + drivers/phy/exynos-simple-phy.c| 189 include/dt-bindings/phy/exynos-simple-phy.h| 27 +++ 5 files changed, 278 insertions(+) create mode 100644 drivers/phy/exynos-simple-phy.c create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h [...] diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f06..2a13e0d 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -178,4 +178,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 Please limit this driver to EXYNOS platforms with: depends on ARCH_EXYNOS or (optionally) depends on ARCH_EXYNOS || COMPILE_TEST Moreover the generic PHY dependency is missing. It can be fixed with: select GENERIC_PHY Yea, I will fix this part. Regards, Rahul Sharma. + help + Support for 1-bit PHY controllers on SoCs from Exynos family. + endmenu Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote: Hi, On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: Hi, On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: From: Tomasz Stanislawski t.stanisl...@samsung.com Add exynos-simple-phy driver to support a single register PHY interfaces present on Exynos4 SoC. Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- .../devicetree/bindings/phy/samsung-phy.txt| 56 ++ drivers/phy/Kconfig|5 + drivers/phy/Makefile |1 + drivers/phy/exynos-simple-phy.c| 189 include/dt-bindings/phy/exynos-simple-phy.h| 27 +++ 5 files changed, 278 insertions(+) create mode 100644 drivers/phy/exynos-simple-phy.c create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h [...] diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f06..2a13e0d 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -178,4 +178,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 Please limit this driver to EXYNOS platforms with: depends on ARCH_EXYNOS or (optionally) depends on ARCH_EXYNOS || COMPILE_TEST Moreover the generic PHY dependency is missing. It can be fixed with: select GENERIC_PHY Yea, I will fix this part. While at that, change the header of the driver file to *2014* Thanks Kishon Regards, Rahul Sharma. + help + Support for 1-bit PHY controllers on SoCs from Exynos family. + endmenu Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 15 May 2014 19:11, Kishon Vijay Abraham I kis...@ti.com wrote: On Thursday 15 May 2014 07:05 PM, Rahul Sharma wrote: Hi, On 15 May 2014 19:01, Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com wrote: Hi, On Thursday, May 15, 2014 12:47:21 AM Rahul Sharma wrote: From: Tomasz Stanislawski t.stanisl...@samsung.com Add exynos-simple-phy driver to support a single register PHY interfaces present on Exynos4 SoC. Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com Signed-off-by: Rahul Sharma rahul.sha...@samsung.com --- .../devicetree/bindings/phy/samsung-phy.txt| 56 ++ drivers/phy/Kconfig|5 + drivers/phy/Makefile |1 + drivers/phy/exynos-simple-phy.c| 189 include/dt-bindings/phy/exynos-simple-phy.h| 27 +++ 5 files changed, 278 insertions(+) create mode 100644 drivers/phy/exynos-simple-phy.c create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h [...] diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 16a2f06..2a13e0d 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -178,4 +178,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 Please limit this driver to EXYNOS platforms with: depends on ARCH_EXYNOS or (optionally) depends on ARCH_EXYNOS || COMPILE_TEST Moreover the generic PHY dependency is missing. It can be fixed with: select GENERIC_PHY Yea, I will fix this part. While at that, change the header of the driver file to *2014* Sure. Thanks, Rahul Sharma. Thanks Kishon Regards, Rahul Sharma. + help + Support for 1-bit PHY controllers on SoCs from Exynos family. + endmenu Best regards, -- Bartlomiej Zolnierkiewicz Samsung RD Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On 15.05.2014 06:01, Rahul Sharma wrote:
Thanks Tomasz,
On 15 May 2014 01:31, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Rahul, Tomasz,
[snip]
+ simplephys: simple-phys@1004 {
+ compatible = samsung,exynos5250-simple-phy;
Missing reg property or unnecessary @unit-address suffix in node name.
I should have removed @unit-address. I will change this.
+ samsung,pmu-syscon = pmu_system_controller;
+ #phy-cells = 1;
+ };
In general, the idea is quite good, but I think this should rather bind
to the main PMU node, since this is just a part of the PMU, not another
device in the system. This also means that the PMU node itself should be
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Not exactly.
What I meant is that the PMU node itself should be the PHY provider, e.g.
pmu_system_controller: system-controller@1004 {
/* ... */
#phy-cells = 1;
};
and then the PMU node should instantiate the Exynos simple PHY driver,
as this is a driver for a facility existing entirely inside of the PMU.
Moreover, the driver should be rather called Exynos PMU PHY.
I know this isn't really possible at the moment, but with device tree we
must design things carefully, so it's better to take a bit more time and
do things properly.
So my opinion on this is that there should be a central Exynos PMU
driver that claims the IO region and instantiates necessary subdrivers,
such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
driver and more, similar to what is being done in drivers/mfd.
Now, there is already ongoing effort to convert current freestanding PMU
configuration code in mach-exynos into a full-fledged PMU driver, but
not exactly in the same direction as I stated above. I'll try to
cooperate with Pankaj, who is responsible for this work to make this go
into the right track.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
Hi Rahul, Tomasz,
On 14.05.2014 21:17, Rahul Sharma wrote:
From: Tomasz Stanislawski t.stanisl...@samsung.com
Add exynos-simple-phy driver to support a single register
PHY interfaces present on Exynos4 SoC.
Signed-off-by: Tomasz Stanislawski t.stanisl...@samsung.com
Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
---
.../devicetree/bindings/phy/samsung-phy.txt| 56 ++
drivers/phy/Kconfig|5 +
drivers/phy/Makefile |1 +
drivers/phy/exynos-simple-phy.c| 189
include/dt-bindings/phy/exynos-simple-phy.h| 27 +++
5 files changed, 278 insertions(+)
create mode 100644 drivers/phy/exynos-simple-phy.c
create mode 100644 include/dt-bindings/phy/exynos-simple-phy.h
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt
b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index 2049261..12ad9cf 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -161,3 +161,59 @@ Example:
usbdrdphy0 = usb3_phy0;
usbdrdphy1 = usb3_phy1;
};
+
+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
+ - samsung,exynos5250-simple-phy
+ - samsung,exynos5420-simple-phy
+- samsung,pmureg-phandle - handle to syscon to control PMU registers
+- #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 the supported phys for exynos4210
+are:
+ HDMI_PHY,
+ DAC_PHY,
+ ADC_PHY,
+ PCIE_PHY,
+ SATA_PHY.
+
+For samsung,exynos4412-simple-phy compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos4412
+are:
+ HDMI_PHY,
+ ADC_PHY.
+
+For samsung,exynos5250-simple-phy compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos5250
+are:
+ HDMI_PHY,
+ ADC_PHY,
+ SATA_PHY.
+
+For samsung,exynos5420-simple-phy compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos5420
+are:
+ HDMI_PHY,
+ ADC_PHY.
+
+Example:
+Simple PHY provider node:
+
+ simplephys: simple-phys@1004 {
+ compatible = samsung,exynos5250-simple-phy;
Missing reg property or unnecessary @unit-address suffix in node name.
+ samsung,pmu-syscon = pmu_system_controller;
+ #phy-cells = 1;
+ };
In general, the idea is quite good, but I think this should rather bind
to the main PMU node, since this is just a part of the PMU, not another
device in the system. This also means that the PMU node itself should be
the PHY provider.
Otherwise looks good.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt
b/Documentation/devicetree/bindings/phy/samsung-phy.txt
[...]
+For samsung,exynos4210-simple-phy compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos4210
I think the specifier is only the part after the phandle, so this should
probably be ... compatible PHYs the single cell specifier ... or
something equivalent.
+are:
+ HDMI_PHY,
+ DAC_PHY,
+ ADC_PHY,
+ PCIE_PHY,
+ SATA_PHY.
I think you need to specify the literal values here as well, since the
binding must be fully self-contained. That is you can't rely on the DT
binding to be bundled with the exynos-simple-phy.h header.
@@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2) +=
phy-exynos4x12-usb2.o
phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)+= phy-exynos5250-usb2.o
obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
+obj-$(CONFIG_EXYNOS_SIMPLE_PHY) += exynos-simple-phy.o
Perhaps this should be named phy-exynos-simple for consistency? Also it
may be a good idea to sort this alphabetically to reduce the potential
for conflicts.
+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);
Why does this need of_match_ptr()?
+ dev_info(dev, probe success\n);
If at all this should be dev_dbg(). But in general the driver core will
already complain if the driver fails to probe, so there's in general no
need to mention when it probes successfully.
diff --git a/include/dt-bindings/phy/exynos-simple-phy.h
b/include/dt-bindings/phy/exynos-simple-phy.h
[...]
+/* simeple phys */
s/simeple phys/simple PHYs/
Although on second thought that comment probably shouldn't be there in
the first place.
+#define INVALID (~1)
This doesn't belong in this header. The value should never be used by a
DT source file, should it?
+#define HDMI_PHY 0
+#define DAC_PHY 1
+#define ADC_PHY 2
+#define PCIE_PHY 3
+#define SATA_PHY 4
Perhaps these should be namespaced somehow to avoid potential conflicts
with other PHY providers?
+#define PHY_NR 5
I'm not sure that this belongs here either. It's not a value that will
ever appear in a DT source file.
Thierry
pgp5LSgujCJfH.pgp
Description: PGP signature
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
Thanks Tomasz,
On 15 May 2014 01:31, Tomasz Figa tomasz.f...@gmail.com wrote:
Hi Rahul, Tomasz,
[snip]
+ simplephys: simple-phys@1004 {
+ compatible = samsung,exynos5250-simple-phy;
Missing reg property or unnecessary @unit-address suffix in node name.
I should have removed @unit-address. I will change this.
+ samsung,pmu-syscon = pmu_system_controller;
+ #phy-cells = 1;
+ };
In general, the idea is quite good, but I think this should rather bind
to the main PMU node, since this is just a part of the PMU, not another
device in the system. This also means that the PMU node itself should be
the PHY provider.
Please correct me if I got you wrong. You want somthing like this:
pmu_system_controller: system-controller@1004 {
...
simple_phys: simple-phys {
compatible = samsung,exynos5420-simple-phy;
...
};
};
Regards,
Rahul Sharma.
Otherwise looks good.
Best regards,
Tomasz
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver
Hi Thierry,
On 15 May 2014 03:44, Thierry Reding thierry.red...@gmail.com wrote:
On Thu, May 15, 2014 at 12:47:21AM +0530, Rahul Sharma wrote:
[...]
diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt
b/Documentation/devicetree/bindings/phy/samsung-phy.txt
[...]
+For samsung,exynos4210-simple-phy compatible PHYs the second cell in
+the PHY specifier identifies the PHY and the supported phys for exynos4210
I think the specifier is only the part after the phandle, so this should
probably be ... compatible PHYs the single cell specifier ... or
something equivalent.
ok. I will rephrase this line.
+are:
+ HDMI_PHY,
+ DAC_PHY,
+ ADC_PHY,
+ PCIE_PHY,
+ SATA_PHY.
I think you need to specify the literal values here as well, since the
binding must be fully self-contained. That is you can't rely on the DT
binding to be bundled with the exynos-simple-phy.h header.
Ok. I will add that.
@@ -20,3 +20,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS4X12_USB2) +=
phy-exynos4x12-usb2.o
phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2)+=
phy-exynos5250-usb2.o
obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
obj-$(CONFIG_PHY_XGENE) += phy-xgene.o
+obj-$(CONFIG_EXYNOS_SIMPLE_PHY) += exynos-simple-phy.o
Perhaps this should be named phy-exynos-simple for consistency? Also it
may be a good idea to sort this alphabetically to reduce the potential
for conflicts.
yea correct. I will use phy-exynos-simple.
I will place this addition in alphabetical order in makefile.
+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);
Why does this need of_match_ptr()?
yea correct. Not required.
+ dev_info(dev, probe success\n);
If at all this should be dev_dbg(). But in general the driver core will
already complain if the driver fails to probe, so there's in general no
need to mention when it probes successfully.
ok.
diff --git a/include/dt-bindings/phy/exynos-simple-phy.h
b/include/dt-bindings/phy/exynos-simple-phy.h
[...]
+/* simeple phys */
s/simeple phys/simple PHYs/
Although on second thought that comment probably shouldn't be there in
the first place.
+#define INVALID (~1)
This doesn't belong in this header. The value should never be used by a
DT source file, should it?
I will move this to driver file.
+#define HDMI_PHY 0
+#define DAC_PHY 1
+#define ADC_PHY 2
+#define PCIE_PHY 3
+#define SATA_PHY 4
Perhaps these should be namespaced somehow to avoid potential conflicts
with other PHY providers?
How about XXX_SIMPLE_PHY?
+#define PHY_NR 5
I'm not sure that this belongs here either. It's not a value that will
ever appear in a DT source file.
I want it to grow along with new additions in the phy list else
catastrophic. This will look unrelated in driver.
Regards,
Rahul Sharma.
Thierry
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
