@@ -0,0 +1,14 @@+Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY+-------------------------------------------------++Required properties:+- compatible : should be "samsung,s5pv210-mipi-video-phy";+- reg : offset and length of the MIPI DPHY register set;+- #phy-cells : from the generic phy bindings, must be 1;++For "samsung,s5pv210-mipi-video-phy" compatible PHYs the second cell in+the PHY specifier identifies the PHY and its meaning is as follows:+ 0 - MIPI CSIS 0,+ 1 - MIPI DSIM 0,+ 2 - MIPI CSIS 1,+ 3 - MIPI DSIM 1.
@@ -2,4 +2,5 @@# Makefile for the phy drivers.#-obj-$(CONFIG_GENERIC_PHY)+=phy-core.o+obj-$(CONFIG_GENERIC_PHY)+=phy-core.o+obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)+=phy-exynos-mipi-video.o
Use the generic PHY API instead of the platform callback to control
the MIPI DSIM DPHY. The 'phy_label' field is added to the platform
data structure to allow PHY lookup on non-dt platforms.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/video/exynos/exynos_mipi_dsi.c | 18 +++++++++---------
include/video/exynos_mipi_dsim.h | 6 ++++--
2 files changed, 13 insertions(+), 11 deletions(-)
@@ -156,8 +156,7 @@ static int exynos_mipi_dsi_blank_mode(struct mipi_dsim_device *dsim, int power)exynos_mipi_regulator_enable(dsim);/* enable MIPI-DSI PHY. */-if(dsim->pd->phy_enable)-dsim->pd->phy_enable(pdev,true);+phy_power_on(dsim->phy);clk_enable(dsim->clock);
@@ -373,6 +372,10 @@ static int exynos_mipi_dsi_probe(struct platform_device *pdev)returnret;}+dsim->phy=devm_phy_get(&pdev->dev,dsim_pd->phy_label);+if(IS_ERR(dsim->phy))+returnPTR_ERR(dsim->phy);+dsim->clock=devm_clk_get(&pdev->dev,"dsim0");if(IS_ERR(dsim->clock)){dev_err(&pdev->dev,"failed to get dsim clock source\n");
Use the generic PHY API instead of the platform callback to control
the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
data structure to allow PHY lookup on non-dt platforms
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/media/platform/exynos4-is/mipi-csis.c | 16 +++++++++++++---
include/linux/platform_data/mipi-csis.h | 11 ++---------
2 files changed, 15 insertions(+), 12 deletions(-)
Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
DPHYs so we can remove now unused code at arch/arm/plat-samsung.
In case there is any board file for S5PV210 platforms using MIPI
CSIS/DSIM (not any upstream currently) it should use the generic
PHY API to bind the PHYs to respective PHY consumer drivers.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes since v1:
- removed S5P_SETUP_MIPIPHY symbol completely from
arch/arm/plat-samsung/Kconfig
---
arch/arm/mach-exynos/include/mach/regs-pmu.h | 5 --
arch/arm/mach-s5pv210/include/mach/regs-clock.h | 4 --
arch/arm/plat-samsung/Kconfig | 5 --
arch/arm/plat-samsung/Makefile | 1 -
arch/arm/plat-samsung/setup-mipiphy.c | 60 -----------------------
5 files changed, 75 deletions(-)
delete mode 100644 arch/arm/plat-samsung/setup-mipiphy.c
you can use platform_set_drvdata(pdev, state);
same thing though, no strong feelings.
+ phy_provider = devm_of_phy_provider_register(dev,
+ exynos_video_phy_xlate);
+ if (IS_ERR(phy_provider))
+ return PTR_ERR(phy_provider);
+
+ for (i = 0; i < NUM_PHYS; i++) {
+ char label[8];
+
+ snprintf(label, sizeof(label), "%s.%d",
+ i == PHY_DSIM0 || i == PHY_DSIM1 ?
+ "dsim" : "csis", i / 2);
+
+ state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
+ label, state);
+ if (IS_ERR(state->phys[i])) {
+ dev_err(dev, "failed to create PHY %s\n", label);
+ return PTR_ERR(state->phys[i]);
+ }
+ }
this really doesn't look correct to me. It looks like you have multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.
and this should contain all PHY IDs:
{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
.data = (const void *) DSIM0, },
{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
.data = (const void *) DSIM1, },
{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
.data = (const void *) CSI0, },
{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
.data = (const void *) CSI1, },
then on your probe you can fetch that data field and use it as phy->id.
From: Felipe Balbi <hidden> Date: 2013-06-25 15:09:08
On Tue, Jun 25, 2013 at 04:21:49PM +0200, Sylwester Nawrocki wrote:
Use the generic PHY API instead of the platform callback to control
the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
data structure to allow PHY lookup on non-dt platforms
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
From: Felipe Balbi <hidden> Date: 2013-06-25 15:09:58
On Tue, Jun 25, 2013 at 04:21:50PM +0200, Sylwester Nawrocki wrote:
Generic PHY drivers are used to handle the MIPI CSIS and MIPI DSIM
DPHYs so we can remove now unused code at arch/arm/plat-samsung.
In case there is any board file for S5PV210 platforms using MIPI
CSIS/DSIM (not any upstream currently) it should use the generic
PHY API to bind the PHYs to respective PHY consumer drivers.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
pretty cool stuff
Acked-by: Felipe Balbi <redacted>
--
balbi
more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.
Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.
quoted
+static int __set_phy_state(struct exynos_video_phy *state,
+ enum phy_id id, unsigned int on)
+{
+ void __iomem *addr;
+ unsigned long flags;
+ u32 reg, reset;
+
+ if (WARN_ON(id > NUM_PHYS))
+ return -EINVAL;
you don't want to do this, actually. It'll bug you everytime you want to
add another phy ID :-)
If there is an SoC with more PHYs enum phy_id would be extended, and this
part would not need to be touched. OTOH @id cannot be normally greater
than NUM_PHYS. I think I'll drop that then.
quoted
+ addr = state->regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
+
+ if (id = PHY_DSIM0 || id = PHY_DSIM1)
+ reset = EXYNOS_MIPI_PHY_MRESETN;
+ else
+ reset = EXYNOS_MIPI_PHY_SRESETN;
+
+ spin_lock_irqsave(&state->slock, flags);
+ reg = readl(addr);
+ if (on)
+ reg |= reset;
+ else
+ reg &= ~reset;
+ writel(reg, addr);
+
+ /* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
+ if (on)
+ reg |= EXYNOS_MIPI_PHY_ENABLE;
+ else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
+ reg &= ~EXYNOS_MIPI_PHY_ENABLE;
+
+ writel(reg, addr);
+ spin_unlock_irqrestore(&state->slock, flags);
+
+ pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
+ __func__, id, on, (u32)addr, (u32)state->regs);
args->args[0] comes from DT as the PHY id and there is nothing
preventing it from being greater or equal to the state->phys[]
array length, unless I'm missing something. Actually it should
have been 'if (args->args[0] >= NUM_PHYS)'.
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
same thing though, no strong feelings.
quoted
+ phy_provider = devm_of_phy_provider_register(dev,
+ exynos_video_phy_xlate);
+ if (IS_ERR(phy_provider))
+ return PTR_ERR(phy_provider);
+
+ for (i = 0; i < NUM_PHYS; i++) {
+ char label[8];
+
+ snprintf(label, sizeof(label), "%s.%d",
+ i = PHY_DSIM0 || i = PHY_DSIM1 ?
+ "dsim" : "csis", i / 2);
+
+ state->phys[i] = devm_phy_create(dev, i, &exynos_video_phy_ops,
+ label, state);
+ if (IS_ERR(state->phys[i])) {
+ dev_err(dev, "failed to create PHY %s\n", label);
+ return PTR_ERR(state->phys[i]);
+ }
+ }
this really doesn't look correct to me. It looks like you have multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.
Yes, multiple PHY objects, but a single provider. There is no need
whatsoever for multiple PHY providers.
and this should contain all PHY IDs:
{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
.data = (const void *) DSIM0, },
{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
.data = (const void *) DSIM1, },
{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
.data = (const void *) CSI0, },
{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
.data = (const void *) CSI1, },
then on your probe you can fetch that data field and use it as phy->id.
This looks wrong to me, it doesn't look like a right usage of 'compatible'
property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
so one compatible property would need to be used for them. We don't use
different compatible strings for different instances of same device.
And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
handled by one provider, to synchronize accesses. That's one of the main
reasons I turned to the generic PHY framework for those devices.
you *must* provide a remove method. drivers with NULL remove are
non-removable :-)
Oops, my bad. I've forgotten to update this, after enabling build
as module. Will update and test that. It will be an empty callback
though.
Thanks,
Sylwester
more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.
Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.
IMHO one driver instance should represent one instance of IP block. Since
this is a single IP block containing multiple PHYs with shared control
interface, I think Sylwester did the right thing.
[snip]
args->args[0] comes from DT as the PHY id and there is nothing
preventing it from being greater or equal to the state->phys[]
array length, unless I'm missing something. Actually it should
have been 'if (args->args[0] >= NUM_PHYS)'.
The xlate() callback gets directly whatever parsed from device tree, so it
is possible for an out of range value to get here and so this check is
valid. However I think it should rather return an ERR_PTR, not NULL. See
of_phy_get().
quoted
quoted
+ return state->phys[args->args[0]];
and your xlate is 'wrong'.
What exactly is wrong here ?
Felipe, could you elaborate a bit more on this? I can't find any serious
problems with this code.
[snip]
quoted
quoted
+ phy_provider = devm_of_phy_provider_register(dev,
+ exynos_video_phy_xlate);
+ if (IS_ERR(phy_provider))
+ return PTR_ERR(phy_provider);
+
+ for (i = 0; i < NUM_PHYS; i++) {
+ char label[8];
+
+ snprintf(label, sizeof(label), "%s.%d",
+ i = PHY_DSIM0 || i = PHY_DSIM1 ?
+ "dsim" : "csis", i / 2);
+
+ state->phys[i] = devm_phy_create(dev, i,
&exynos_video_phy_ops,
quoted
quoted
+ label,
state);
quoted
quoted
+ if (IS_ERR(state->phys[i])) {
+ dev_err(dev, "failed to create PHY %s\n", label);
+ return PTR_ERR(state->phys[i]);
+ }
+ }
this really doesn't look correct to me. It looks like you have
multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.
Yes, multiple PHY objects, but a single provider. There is no need
whatsoever for multiple PHY providers.
The whole concept of whatever-provider is to allow managing multiple
objects by one parent object, like one clock provider for the whole clock
controller, one interrupt controller object for all interrupts of an
interrupt controller block, etc.
This is why a phandle has args, to allow addressing subobjects inside a
provider.
Best regards,
Tomasz
more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.
Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.
alright, I misunderstood your code then. When I looked over your id
usage I missed the "/2" part and assumed that you would have separate
EXYNOS_MIPI_PHY_CONTROL() register for each ;-)
My bad, you can disregard the other comments.
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.
and this should contain all PHY IDs:
{ .compatible = "samsung,s5pv210-mipi-video-dsim0-phy",
.data = (const void *) DSIM0, },
{ .compatible = "samsung,s5pv210-mipi-video-dsim1-phy",
.data = (const void *) DSIM1, },
{ .compatible = "samsung,s5pv210-mipi-video-csi0-phy"
.data = (const void *) CSI0, },
{ .compatible = "samsung,s5pv210-mipi-video-csi1-phy"
.data = (const void *) CSI1, },
then on your probe you can fetch that data field and use it as phy->id.
This looks wrong to me, it doesn't look like a right usage of 'compatible'
property. MIPI-CSIS0/MIPI-DSIM0, MIPI-CSIS1/MIPI-DSIM1 are identical pairs,
so one compatible property would need to be used for them. We don't use
different compatible strings for different instances of same device.
And MIPI DSIM and MIPI CSIS share one MMIO register, so they need to be
handled by one provider, to synchronize accesses. That's one of the main
reasons I turned to the generic PHY framework for those devices.
>
> you can use platform_set_drvdata(pdev, state);
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.
Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
to just drop the above call, since the pdev drvdata is currently not
referenced anywhere.
Thanks,
Sylwester
>
> you can use platform_set_drvdata(pdev, state);
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.
Indeed, I somehow confused phy->dev with with phy->dev.parent. I'm going
to just drop the above call, since the pdev drvdata is currently not
referenced anywhere.
more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.
Why ? This single PHY _provider_ can well handle multiple PHYs.
I don't see a good reason to further complicate this driver like
this. Please note that MIPI-CSIS 0 and MIPI DSIM 0 share MMIO
register, so does MIPI CSIS 1 and MIPI DSIM 1. There are only 2
registers for those 4 PHYs. I could have the involved object
multiplied, but it would have been just a waste of resources
with no difference to the PHY consumers.
alright, I misunderstood your code then. When I looked over your id
usage I missed the "/2" part and assumed that you would have separate
EXYNOS_MIPI_PHY_CONTROL() register for each ;-)
My bad, you can disregard the other comments.
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.
Thanks
Kishon
hmm, you do need to set the drvdata() to the phy object, but also to the
pdev object (should you need it on a suspend/resume callback, for
instance). Those are separate struct device instances.
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.
Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
be used and in a custom of_xlate dev_get_drvdata(dev) (assuming the phy
provider sets drvdata on its device beforehand).
I can't see any races from runtime PM with this approach.
Regards,
Sylwester
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.
Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
phy_get_drvdata(phy);
accessing the dev pointer will be done inside the helper :-)
--
balbi
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.
Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
be used and in a custom of_xlate dev_get_drvdata(dev) (assuming the phy
provider sets drvdata on its device beforehand).
thats correct. btw when you send the next version just have MODULE_LICENSE set
to GPL v2. Apart from that this patch looks good to me.
Thanks
Kishon
I had it in the previous version, but changed for symmetry with
dev_set_drvdata(). I guess those could be replaced with
phy_{get, set}_drvdata as you suggested.
right. currently I was setting dev_set_drvdata of phy (core) device
in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
which is little confusing.
So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
Felipe) to be used by phy provider drivers. So after creating the PHY, the
phy provider should use phy_set_drvdata and in phy_ops, it can use
phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
This also means _void *priv_ in phy_create is useless. So I'll be removing
_priv_ from phy_create.
Yeah, sounds good. Then in the phy ops phy_get_drvdata(&phy->dev) would
phy_get_drvdata(phy);
accessing the dev pointer will be done inside the helper :-)
you *must* provide a remove method. drivers with NULL remove are
non-removable :-)
Actually the remove() callback can be NULL, it's just missing module_exit
function that makes a module not unloadable.
look at the implementation of platform_drv_remove():
499 static int platform_drv_remove(struct device *_dev)
500 {
501 struct platform_driver *drv = to_platform_driver(_dev->driver);
502 struct platform_device *dev = to_platform_device(_dev);
503 int ret;
504
505 ret = drv->remove(dev);
506 if (ACPI_HANDLE(_dev))
507 acpi_dev_pm_detach(_dev, true);
508
509 return ret;
510 }
that's not a conditional call right :-)
It is conditional, just condition check is in different place:
int platform_driver_register(struct platform_driver *drv)
{
(...)
if (drv->remove)
drv->driver.remove = platform_drv_remove;
(...)
}
Regards
Andrzej
you *must* provide a remove method. drivers with NULL remove are
non-removable :-)
Actually the remove() callback can be NULL, it's just missing module_exit
function that makes a module not unloadable.
look at the implementation of platform_drv_remove():
499 static int platform_drv_remove(struct device *_dev)
500 {
501 struct platform_driver *drv = to_platform_driver(_dev->driver);
502 struct platform_device *dev = to_platform_device(_dev);
503 int ret;
504
505 ret = drv->remove(dev);
506 if (ACPI_HANDLE(_dev))
507 acpi_dev_pm_detach(_dev, true);
508
509 return ret;
510 }
that's not a conditional call right :-)
It is conditional, just condition check is in different place:
int platform_driver_register(struct platform_driver *drv)
{
(...)
if (drv->remove)
drv->driver.remove = platform_drv_remove;
(...)
}
good point :-) thanks. I'll go ack your driver now
--
balbi
you *must* provide a remove method. drivers with NULL remove are
non-removable :-)
Actually the remove() callback can be NULL, it's just missing module_exit
function that makes a module not unloadable.
look at the implementation of platform_drv_remove():
499 static int platform_drv_remove(struct device *_dev)
500 {
501 struct platform_driver *drv = to_platform_driver(_dev->driver);
502 struct platform_device *dev = to_platform_device(_dev);
503 int ret;
504
505 ret = drv->remove(dev);
506 if (ACPI_HANDLE(_dev))
507 acpi_dev_pm_detach(_dev, true);
508
509 return ret;
510 }
that's not a conditional call right :-)
Wrong.
if (drv->remove)
drv->driver.remove = platform_drv_remove;
The function you quote will only be used if drv->remove is non-NULL.
You do not need to provide a remove method.