phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

drivers/phy/phy-exynos-mipi-video.c new file +16 -0

+	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. */

drivers/phy/phy-exynos-mipi-video.c new file +17 -0

+}
+
+static struct phy_ops exynos_video_phy_ops = {
+	.power_on	= exynos_video_phy_power_on,
+	.power_off	= exynos_video_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);

drivers/phy/phy-exynos-mipi-video.c new file +9 -0

+	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",

drivers/phy/phy-exynos-mipi-video.c line 34

Quoted 6 lines · Sylwester Nawrocki

drivers/phy/phy-exynos-mipi-video.c new file +6 -0

+enum phy_id {
+	PHY_CSIS0,
+	PHY_DSIM0,
+	PHY_CSIS1,
+	PHY_DSIM1,
+	NUM_PHYS

please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_

drivers/phy/phy-exynos-mipi-video.c line 39

Quoted 3 lines

drivers/phy/phy-exynos-mipi-video.c new file +3 -0

+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[NUM_PHYS];

more than one phy ? This means you should instantiate driver multiple drivers. Each phy id should call probe again.

Quoted 4 lines

drivers/phy/phy-exynos-mipi-video.c new file +4 -0

+static int __set_phy_state(struct exynos_video_phy *state,
+				enum phy_id id, unsigned int on)
+{
+	void __iomem *addr;

drivers/phy/phy-exynos-mipi-video.c line 51

Quoted 5 lines

drivers/phy/phy-exynos-mipi-video.c new file +5 -0

+	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 :-)

Quoted 16 lines

drivers/phy/phy-exynos-mipi-video.c new file +16 -0

+	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. */

drivers/phy/phy-exynos-mipi-video.c line 78

Quoted 10 lines

drivers/phy/phy-exynos-mipi-video.c new file +10 -0

+	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);

use dev_dbg() instead.

drivers/phy/phy-exynos-mipi-video.c line 85

Quoted 7 lines

drivers/phy/phy-exynos-mipi-video.c new file +7 -0

+
+	return 0;
+}
+
+static int exynos_video_phy_power_on(struct phy *phy)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);

looks like we should have phy_get_drvdata() helper :-) Kishon ?

drivers/phy/phy-exynos-mipi-video.c line 101

Quoted 7 lines

drivers/phy/phy-exynos-mipi-video.c new file +7 -0

+static struct phy *exynos_video_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(dev);
+
+	if (WARN_ON(args->args[0] > NUM_PHYS))
+		return NULL;

please remove this check.

drivers/phy/phy-exynos-mipi-video.c line 103

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	return state->phys[args->args[0]];

and your xlate is 'wrong'.

Quoted 21 lines

drivers/phy/phy-exynos-mipi-video.c new file +21 -0

+}
+
+static struct phy_ops exynos_video_phy_ops = {
+	.power_on	= exynos_video_phy_power_on,
+	.power_off	= exynos_video_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c new file +6 -0

+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

same thing though, no strong feelings.

Quoted 15 lines

drivers/phy/phy-exynos-mipi-video.c new file +15 -0

+	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])) {

drivers/phy/phy-exynos-mipi-video.c line 150

Quoted 4 lines

drivers/phy/phy-exynos-mipi-video.c new file +4 -0

+			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.

drivers/phy/phy-exynos-mipi-video.c line 156

Quoted 2 lines

drivers/phy/phy-exynos-mipi-video.c new file +2 -0

+static const struct of_device_id exynos_video_phy_of_match[] = {
+	{ .compatible = "samsung,s5pv210-mipi-video-phy" },

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.

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 2 lines

drivers/phy/phy-exynos-mipi-video.c new file +2 -0

+static struct platform_driver exynos_video_phy_driver = {
+	.probe	= exynos_video_phy_probe,

you *must* provide a remove method. drivers with NULL remove are non-removable :-)

Hi Felipe,

Thanks for the review.

Quoted 9 lines · Felipe Balbi

drivers/phy/phy-exynos-mipi-video.c line 34

Quoted 6 lines · Sylwester Nawrocki

drivers/phy/phy-exynos-mipi-video.c new file +6 -0

+enum phy_id {
+	PHY_CSIS0,
+	PHY_DSIM0,
+	PHY_CSIS1,
+	PHY_DSIM1,
+	NUM_PHYS

please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_

OK, will fix that.

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c line 39

Quoted 3 lines

drivers/phy/phy-exynos-mipi-video.c new file +3 -0

+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[NUM_PHYS];

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 9 lines

drivers/phy/phy-exynos-mipi-video.c line 101

Quoted 7 lines

drivers/phy/phy-exynos-mipi-video.c new file +7 -0

+static struct phy *exynos_video_phy_xlate(struct device *dev,
+					struct of_phandle_args *args)
+{
+	struct exynos_video_phy *state = dev_get_drvdata(dev);
+
+	if (WARN_ON(args->args[0] > NUM_PHYS))
+		return NULL;

please remove this check.

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)'.

Quoted 3 lines

drivers/phy/phy-exynos-mipi-video.c line 103

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	return state->phys[args->args[0]];

and your xlate is 'wrong'.

What exactly is wrong here ?

Quoted 5 lines

Quoted 5 lines

drivers/phy/phy-exynos-mipi-video.c new file +5 -0

+	phy_provider = devm_of_phy_provider_register(dev,
+					exynos_video_phy_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c new file +8 -0

+	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, 

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +1 -0

+								label, 

Quoted 10 lines

drivers/phy/phy-exynos-mipi-video.c line 150

Quoted 5 lines

drivers/phy/phy-exynos-mipi-video.c new file +5 -0

+		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.

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c line 39

Quoted 3 lines

drivers/phy/phy-exynos-mipi-video.c new file +3 -0

+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[NUM_PHYS];

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 13 lines

Quoted 13 lines

drivers/phy/phy-exynos-mipi-video.c new file +13 -0

+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c new file +6 -0

+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

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.

Quoted 14 lines

drivers/phy/phy-exynos-mipi-video.c line 156

Quoted 2 lines

drivers/phy/phy-exynos-mipi-video.c new file +2 -0

+static const struct of_device_id exynos_video_phy_of_match[] = {
+	{ .compatible = "samsung,s5pv210-mipi-video-phy" },

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, },

Quoted 1 line

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.

Quoted 5 lines

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 2 lines

drivers/phy/phy-exynos-mipi-video.c new file +2 -0

+static struct platform_driver exynos_video_phy_driver = {
+	.probe	= exynos_video_phy_probe,

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.

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

+{
+ struct exynos_video_phy *state;
+ struct device *dev =&pdev->dev;
+ struct resource *res;
+ struct phy_provider *phy_provider;
+ int i;
+
+ state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ state->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(state->regs))
+ return PTR_ERR(state->regs);
+
+ dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

Quoted 3 lines

Quoted 3 lines

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.

Hi,

Quoted 26 lines · Felipe Balbi

Quoted 25 lines

Quoted 25 lines

Quoted 21 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

+{
+ struct exynos_video_phy *state;
+ struct device *dev =&pdev->dev;
+ struct resource *res;
+ struct phy_provider *phy_provider;
+ int i;
+
+ state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ state->regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(state->regs))
+ return PTR_ERR(state->regs);
+
+ dev_set_drvdata(dev, state);

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.

Quoted 3 lines

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.

Hi,

Quoted 7 lines · Sylwester Nawrocki

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c line 39

Quoted 3 lines

drivers/phy/phy-exynos-mipi-video.c new file +3 -0

+struct exynos_video_phy {
+	spinlock_t slock;
+	struct phy *phys[NUM_PHYS];

more than one phy ? This means you should instantiate driver multiple drivers. Each phy id should call probe again.

Quoted 7 lines

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.

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

drivers/phy/phy-exynos-mipi-video.c new file +13 -0

+static int exynos_video_phy_probe(struct platform_device *pdev)
+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 6 lines

drivers/phy/phy-exynos-mipi-video.c new file +6 -0

+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

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.

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

drivers/phy/phy-exynos-mipi-video.c new file +10 -0

+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c new file +8 -0

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 34 lines · Kishon Vijay Abraham I

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

drivers/phy/phy-exynos-mipi-video.c new file +10 -0

+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c new file +8 -0

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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).

Quoted 2 lines

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

Quoted 35 lines · Sylwester Nawrocki

Quoted 34 lines · Kishon Vijay Abraham I

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +10 -0

+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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).

Quoted 4 lines

Quoted 2 lines

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 :-)

Quoted 34 lines · Kishon Vijay Abraham I

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

+static int exynos_video_phy_probe(struct platform_device *pdev)

Quoted 20 lines

Quoted 20 lines

Quoted 10 lines

Quoted 10 lines

drivers/phy/phy-exynos-mipi-video.c new file +10 -0

+{
+	struct exynos_video_phy *state;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy_provider *phy_provider;
+	int i;
+
+	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c line 130

Quoted 8 lines

drivers/phy/phy-exynos-mipi-video.c new file +8 -0

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	state->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(state->regs))
+		return PTR_ERR(state->regs);
+
+	dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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).

Quoted 2 lines

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).

Quoted 2 lines

+static struct platform_driver exynos_video_phy_driver = {

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	.probe	= exynos_video_phy_probe,

you *must* provide a remove method. drivers with NULL remove are non-removable :-)

Hi,

Quoted 5 lines · Felipe Balbi

Quoted 2 lines

+static struct platform_driver exynos_video_phy_driver = {

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	.probe	= exynos_video_phy_probe,

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.

Quoted 11 lines · Sylwester Nawrocki

Hi,

Quoted 5 lines · Felipe Balbi

Quoted 2 lines

+static struct platform_driver exynos_video_phy_driver = {

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	.probe	= exynos_video_phy_probe,

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 :-)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi Felipe,

Quoted 28 lines · Felipe Balbi

Quoted 11 lines · Sylwester Nawrocki

Hi,

Quoted 5 lines · Felipe Balbi

Quoted 2 lines

+static struct platform_driver exynos_video_phy_driver = {

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	.probe	= exynos_video_phy_probe,

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 }

Quoted 1 line

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;
	(...)
}

Quoted 11 lines · Sylwester Nawrocki

Hi,

Quoted 5 lines · Felipe Balbi

Quoted 2 lines

+static struct platform_driver exynos_video_phy_driver = {

drivers/phy/phy-exynos-mipi-video.c line 162

Quoted 1 line

drivers/phy/phy-exynos-mipi-video.c new file +1 -0

+	.probe	= exynos_video_phy_probe,

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 :-)