| Message ID | 1372170110-12993-1-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote: > +enum phy_id { > + PHY_CSIS0, > + PHY_DSIM0, > + PHY_CSIS1, > + PHY_DSIM1, > + NUM_PHYS please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_ > +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. > +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 :-) > + 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); use dev_dbg() instead. > + > + 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 ? > +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. > + return state->phys[args->args[0]]; and your xlate is 'wrong'. > +} > + > +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); > + > + 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. > + 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. > +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. > +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. On 06/25/2013 05:06 PM, Felipe Balbi wrote: > On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote: >> +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. >> +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. >> +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. >> + 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); > > use dev_dbg() instead. > >> + >> + 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 ? Indeed, that might be useful. >> +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)'. >> + return state->phys[args->args[0]]; > > and your xlate is 'wrong'. What exactly is wrong here ? >> +} >> + >> +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); >> + >> + 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. > 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. Yes, multiple PHY objects, but a single provider. There is no need whatsoever for multiple PHY providers. >> +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. 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. >> +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. Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, Felipe, On Tuesday 25 of June 2013 19:44:52 Sylwester Nawrocki wrote: > Hi Felipe, > > Thanks for the review. > > On 06/25/2013 05:06 PM, Felipe Balbi wrote: > > On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote: > >> +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. > > >> +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. 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] > >> +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)'. 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(). > >> + 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] > >> + 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Jun 25, 2013 at 07:44:52PM +0200, Sylwester Nawrocki wrote: > >> +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. 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. > >> +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); > >> + > >> + 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. > >> +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. > > 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. understood :-) > >> +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. right, because of devm.
Hi, On 06/25/2013 10:54 PM, Felipe Balbi wrote: >>>> +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); >>>> > >> + >>>> > >> + 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 25, 2013 at 11:47:13PM +0200, Sylwester Nawrocki wrote: > Hi, > > On 06/25/2013 10:54 PM, Felipe Balbi wrote: > >>>>+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); > >>>>> >> + > >>>>> >> + 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. > > 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. ok cool :-)
Hi, On Wednesday 26 June 2013 02:24 AM, Felipe Balbi wrote: > Hi, > > On Tue, Jun 25, 2013 at 07:44:52PM +0200, Sylwester Nawrocki wrote: >>>> +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. > > 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. > >>>> +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); >>>> + >>>> + 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote: >>>>> +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); >>>>> >>>> + >>>>> >>>> + 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 26, 2013 at 02:03:42PM +0200, Sylwester Nawrocki wrote: > On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote: > >>>>> +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); > >>>>> >>>> + > >>>>> >>>> + 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. > > > > 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 :-)
On Wednesday 26 June 2013 05:33 PM, Sylwester Nawrocki wrote: > On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote: >>>>>> +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); >>>>>>>>>> + >>>>>>>>>> + 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. >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 26 June 2013 05:52 PM, Felipe Balbi wrote: > On Wed, Jun 26, 2013 at 02:03:42PM +0200, Sylwester Nawrocki wrote: >> On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote: >>>>>>> +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); >>>>>>>>>>> + >>>>>>>>>>> + 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. >>> >>> 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 :-) right :-) -Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06/25/2013 05:06 PM, Felipe Balbi wrote: >> +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 :-) Actually the remove() callback can be NULL, it's just missing module_exit function that makes a module not unloadable. -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote: > Hi, > > On 06/25/2013 05:06 PM, Felipe Balbi wrote: > >> +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 :-) > > 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 :-)
Hi Felipe, On 06/27/2013 08:17 AM, Felipe Balbi wrote: > On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote: >> Hi, >> >> On 06/25/2013 05:06 PM, Felipe Balbi wrote: >>>> +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 :-) >> >> 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 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 27, 2013 at 09:47:47AM +0200, Andrzej Hajda wrote: > Hi Felipe, > > On 06/27/2013 08:17 AM, Felipe Balbi wrote: > > On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote: > >> Hi, > >> > >> On 06/25/2013 05:06 PM, Felipe Balbi wrote: > >>>> +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 :-) > >> > >> 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
On Thu, Jun 27, 2013 at 09:17:13AM +0300, Felipe Balbi wrote: > On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote: > > Hi, > > > > On 06/25/2013 05:06 PM, Felipe Balbi wrote: > > >> +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 :-) > > > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt b/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt new file mode 100644 index 0000000..5ff208c --- /dev/null +++ b/Documentation/devicetree/bindings/phy/samsung,s5pv210-mipi-video-phy.txt @@ -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. diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index 5f85909..6f446d0 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -11,3 +11,12 @@ menuconfig GENERIC_PHY devices present in the kernel. This layer will have the generic API by which phy drivers can create PHY using the phy framework and phy users can obtain reference to the PHY. + +if GENERIC_PHY + +config PHY_EXYNOS_MIPI_VIDEO + tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver" + help + Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung + S5P and EXYNOS SoCs. +endif diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index 9e9560f..71d8841 100644 --- a/drivers/phy/Makefile +++ b/drivers/phy/Makefile @@ -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 diff --git a/drivers/phy/phy-exynos-mipi-video.c b/drivers/phy/phy-exynos-mipi-video.c new file mode 100644 index 0000000..074a623 --- /dev/null +++ b/drivers/phy/phy-exynos-mipi-video.c @@ -0,0 +1,173 @@ +/* + * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver + * + * Copyright (C) 2013 Samsung Electronics Co., Ltd. + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/phy/phy.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +/* MIPI_PHYn_CONTROL register offset: n = 0..1 */ +#define EXYNOS_MIPI_PHY_CONTROL(n) ((n) * 4) +#define EXYNOS_MIPI_PHY_ENABLE (1 << 0) +#define EXYNOS_MIPI_PHY_SRESETN (1 << 1) +#define EXYNOS_MIPI_PHY_MRESETN (1 << 2) +#define EXYNOS_MIPI_PHY_RESET_MASK (3 << 1) + +enum phy_id { + PHY_CSIS0, + PHY_DSIM0, + PHY_CSIS1, + PHY_DSIM1, + NUM_PHYS +}; + +struct exynos_video_phy { + spinlock_t slock; + struct phy *phys[NUM_PHYS]; + void __iomem *regs; +}; + +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; + + 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); + + return 0; +} + +static int exynos_video_phy_power_on(struct phy *phy) +{ + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev); + return __set_phy_state(state, phy->id, 1); +} + +static int exynos_video_phy_power_off(struct phy *phy) +{ + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev); + return __set_phy_state(state, phy->id, 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; + + return state->phys[args->args[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); + + state->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(state->regs)) + return PTR_ERR(state->regs); + + dev_set_drvdata(dev, state); + + 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]); + } + } + + return 0; +} + +static const struct of_device_id exynos_video_phy_of_match[] = { + { .compatible = "samsung,s5pv210-mipi-video-phy" }, + { }, +}; +MODULE_DEVICE_TABLE(of, exynos_video_phy_of_match); + +static struct platform_driver exynos_video_phy_driver = { + .probe = exynos_video_phy_probe, + .driver = { + .of_match_table = exynos_video_phy_of_match, + .name = "exynos-mipi-video-phy", + .owner = THIS_MODULE, + } +}; +module_platform_driver(exynos_video_phy_driver); + +MODULE_DESCRIPTION("Samsung S5P/EXYNOS SoC MIPI CSI-2/DSI PHY driver"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");