| Message ID | 4daf0c5dbed2c47c97003ab8de0a7dbd2a335dc3.1638307601.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: JZ4780 and CI20 HDMI | expand |
Hi Nikolaus, Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > From: Paul Boddie <paul@boddie.org.uk> > > A specialisation of the generic Synopsys HDMI driver is employed for > JZ4780 HDMI support. This requires a new driver, plus device tree and > configuration modifications. > > Here we add Kconfig DRM_INGENIC_DW_HDMI, Makefile and driver code. > > Signed-off-by: Paul Boddie <paul@boddie.org.uk> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > drivers/gpu/drm/ingenic/Kconfig | 9 ++ > drivers/gpu/drm/ingenic/Makefile | 1 + > drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 138 > ++++++++++++++++++++++ > 3 files changed, 148 insertions(+) > create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > > diff --git a/drivers/gpu/drm/ingenic/Kconfig > b/drivers/gpu/drm/ingenic/Kconfig > index 3b57f8be007c4..4efc709d77b0a 100644 > --- a/drivers/gpu/drm/ingenic/Kconfig > +++ b/drivers/gpu/drm/ingenic/Kconfig > @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU > > The Image Processing Unit (IPU) will appear as a second primary > plane. > > +config DRM_INGENIC_DW_HDMI > + tristate "Ingenic specific support for Synopsys DW HDMI" > + depends on MACH_JZ4780 > + select DRM_DW_HDMI > + help > + Choose this option to enable Synopsys DesignWare HDMI based > driver. > + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should > + select this option.. > + > endif > diff --git a/drivers/gpu/drm/ingenic/Makefile > b/drivers/gpu/drm/ingenic/Makefile > index d313326bdddbb..f10cc1c5a5f22 100644 > --- a/drivers/gpu/drm/ingenic/Makefile > +++ b/drivers/gpu/drm/ingenic/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o > ingenic-drm-y = ingenic-drm-drv.o > ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o > +obj-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o > diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > new file mode 100644 > index 0000000000000..199e39c227d29 > --- /dev/null > +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > @@ -0,0 +1,138 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. > + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk> > + * > + * Derived from dw_hdmi-imx.c with i.MX portions removed. > + * Probe and remove operations derived from rcar_dw_hdmi.c. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <drm/bridge/dw_hdmi.h> > +#include <drm/drm_of.h> > +#include <drm/drm_print.h> > + > +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { > + { 45250000, { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, > 0x0000 } } }, > + { 92500000, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, > 0x0005 } } }, > + { 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, > 0x000a } } }, > + { 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, > 0x000f } } }, > + { ~0UL, { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, > 0x0000 } } } > +}; > + > +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { > + /*pixelclk bpp8 bpp10 bpp12 */ > + { 54000000, { 0x091c, 0x091c, 0x06dc } }, > + { 58400000, { 0x091c, 0x06dc, 0x06dc } }, > + { 72000000, { 0x06dc, 0x06dc, 0x091c } }, > + { 74250000, { 0x06dc, 0x0b5c, 0x091c } }, > + { 118800000, { 0x091c, 0x091c, 0x06dc } }, > + { 216000000, { 0x06dc, 0x0b5c, 0x091c } }, > + { ~0UL, { 0x0000, 0x0000, 0x0000 } }, > +}; > + > +/* > + * Resistance term 133Ohm Cfg > + * PREEMP config 0.00 > + * TX/CK level 10 > + */ > +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { > + /*pixelclk symbol term vlev */ > + { 216000000, 0x800d, 0x0005, 0x01ad}, > + { ~0UL, 0x0000, 0x0000, 0x0000} > +}; > + > +static enum drm_mode_status > +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, > + const struct drm_display_info *info, > + const struct drm_display_mode *mode) > +{ > + if (mode->clock < 13500) > + return MODE_CLOCK_LOW; > + /* FIXME: Hardware is capable of 270MHz, but setup data is missing. > */ > + if (mode->clock > 216000) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = { > + .mpll_cfg = ingenic_mpll_cfg, > + .cur_ctr = ingenic_cur_ctr, > + .phy_config = ingenic_phy_config, > + .mode_valid = ingenic_dw_hdmi_mode_valid, > + .output_port = 1, > +}; > + > +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = { > + { .compatible = "ingenic,jz4780-dw-hdmi" }, > + { /* Sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids); > + > +static void ingenic_dw_hdmi_cleanup(void *data) > +{ > + struct dw_hdmi *hdmi = (struct dw_hdmi *)data; > + > + dw_hdmi_remove(hdmi); > +} > + > +static void ingenic_dw_hdmi_disable_regulator(void *data) > +{ > + struct regulator *regulator = (struct regulator *)data; > + > + regulator_disable(regulator); > +} > + > +static int ingenic_dw_hdmi_probe(struct platform_device *pdev) > +{ > + struct dw_hdmi *hdmi; > + struct regulator *regulator; > + int ret; > + > + hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); > + if (IS_ERR(hdmi)) > + return PTR_ERR(hdmi); > + > + ret = devm_add_action_or_reset(&pdev->dev, ingenic_dw_hdmi_cleanup, > hdmi); > + if (ret) > + return ret; > + > + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); > + if (IS_ERR(regulator)) { > + ret = PTR_ERR(regulator); > + > + DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n", > + "hdmi-5v", ret); > + > + return ret; > + } > + > + if (!regulator) > + return 0; Blank line here. But I can add it myself when applying. I'll just wait for Rob's ack first. -Paul > + ret = regulator_enable(regulator); > + if (ret) { > + DRM_DEV_ERROR(&pdev->dev, "Failed to enable hpd regulator: %d\n", > + ret); > + > + return ret; > + } > + > + return devm_add_action_or_reset(&pdev->dev, > ingenic_dw_hdmi_disable_regulator, > + regulator); > +} > + > +static struct platform_driver ingenic_dw_hdmi_driver = { > + .probe = ingenic_dw_hdmi_probe, > + .driver = { > + .name = "dw-hdmi-ingenic", > + .of_match_table = ingenic_dw_hdmi_dt_ids, > + }, > +}; > +module_platform_driver(ingenic_dw_hdmi_driver); > + > +MODULE_DESCRIPTION("JZ4780 Specific DW-HDMI Driver Extension"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:dwhdmi-ingenic"); > -- > 2.33.0 >
Hi Paul, > Am 01.12.2021 um 14:02 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> From: Paul Boddie <paul@boddie.org.uk> >> A specialisation of the generic Synopsys HDMI driver is employed for >> JZ4780 HDMI support. This requires a new driver, plus device tree and >> configuration modifications. >> Here we add Kconfig DRM_INGENIC_DW_HDMI, Makefile and driver code. >> Signed-off-by: Paul Boddie <paul@boddie.org.uk> >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> drivers/gpu/drm/ingenic/Kconfig | 9 ++ >> drivers/gpu/drm/ingenic/Makefile | 1 + >> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 138 ++++++++++++++++++++++ >> 3 files changed, 148 insertions(+) >> create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c >> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig >> index 3b57f8be007c4..4efc709d77b0a 100644 >> --- a/drivers/gpu/drm/ingenic/Kconfig >> +++ b/drivers/gpu/drm/ingenic/Kconfig >> >> + } >> + >> + if (!regulator) >> + return 0; > > Blank line here. It is one of these cases where checkpatch doesn't complain although it should be improved... > > But I can add it myself when applying. Yes, please. So that we are not wasting mailing list bandwidth... > I'll just wait for Rob's ack first. Indeed. Fortunately he had the right hint how to fix 3/8 quickly. BR and thanks, NIkolaus
On Wed, Dec 01, 2021 at 01:02:45PM +0000, Paul Cercueil wrote: > Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > > + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); > > + if (IS_ERR(regulator)) { > > + ret = PTR_ERR(regulator); Why is this using _optional()? This should only be done when the supply can be physically absent (in which case I'd expect to see special handling).
Hi, > Am 01.12.2021 um 14:39 schrieb Mark Brown <broonie@kernel.org>: > > On Wed, Dec 01, 2021 at 01:02:45PM +0000, Paul Cercueil wrote: >> Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller > >>> + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); >>> + if (IS_ERR(regulator)) { >>> + ret = PTR_ERR(regulator); > > Why is this using _optional()? This should only be done when the supply > can be physically absent There can be +5V for HDMI but without a regulator that is visible to or controllable by the driver. So hdmi-5v can be simply missing in DTS in which case the driver does not need to care about. The driver just can't turn it on or off. > (in which case I'd expect to see special > handling). The special case is to not enable/disable the regulator if it does not exist and assume that there is hardware providing it otherwise (the driver can't know that except by using get_optional). This is done by the code below >>> + if (IS_ERR(regulator)) { ... > + if (!regulator) > + return 0; > > + ret = regulator_enable(regulator); ... BR and thanks, Nikolaus
Hi Nikolaus, Mark, Le mer., déc. 1 2021 at 14:51:51 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Hi, > >> Am 01.12.2021 um 14:39 schrieb Mark Brown <broonie@kernel.org>: >> >> On Wed, Dec 01, 2021 at 01:02:45PM +0000, Paul Cercueil wrote: >>> Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller >> >>>> + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); >>>> + if (IS_ERR(regulator)) { >>>> + ret = PTR_ERR(regulator); >> >> Why is this using _optional()? This should only be done when the >> supply >> can be physically absent > > There can be +5V for HDMI but without a regulator that is visible to > or controllable > by the driver. There is always a power supply though. Either a controllable one (through e.g. a GPIO), or it's just connected to the mains +5V; the pin is never left floating. In the second case, in DTS the "hdmi-5v" would be connected to some 5v regulator, even if it's just a dummy VCC-5V regulator. So Mark has a point. > So hdmi-5v can be simply missing in DTS in which case the driver does > not need to > care about. The driver just can't turn it on or off. Please make it mandatory in DTS then, and use devm_regulator_get() in the driver. Cheers, -Paul >> (in which case I'd expect to see special >> handling). > > The special case is to not enable/disable the regulator if it does > not exist > and assume that there is hardware providing it otherwise (the driver > can't know > that except by using get_optional). This is done by the code below > >>>> + if (IS_ERR(regulator)) { > > ... > >> + if (!regulator) >> + return 0; >> >> + ret = regulator_enable(regulator); > > ... > > BR and thanks, > Nikolaus > > > >
Hi Paul, > Am 01.12.2021 um 15:03 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, Mark, > > Le mer., déc. 1 2021 at 14:51:51 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Hi, >>> Am 01.12.2021 um 14:39 schrieb Mark Brown <broonie@kernel.org>: >>> On Wed, Dec 01, 2021 at 01:02:45PM +0000, Paul Cercueil wrote: >>>> Le mar., nov. 30 2021 at 22:26:37 +0100, H. Nikolaus Schaller >>>>> + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); >>>>> + if (IS_ERR(regulator)) { >>>>> + ret = PTR_ERR(regulator); >>> Why is this using _optional()? This should only be done when the supply >>> can be physically absent >> There can be +5V for HDMI but without a regulator that is visible to or controllable >> by the driver. > > There is always a power supply though. Either a controllable one (through e.g. a GPIO), or it's just connected to the mains +5V; the pin is never left floating. In the second case, in DTS the "hdmi-5v" would be connected to some 5v regulator, even if it's just a dummy VCC-5V regulator. So Mark has a point. > >> So hdmi-5v can be simply missing in DTS in which case the driver does not need to >> care about. The driver just can't turn it on or off. > > Please make it mandatory in DTS then, and use devm_regulator_get() in the driver. Well, I just wonder why the elegant devm_regulator_get_optional() exists at all and seems to be used in ca. 80 places. And if it is not allowed, why some DTS should be forced to add not physically existing dummy-regulators. AFAIR drivers should implement functionality defined by DTS but not the other way round: enforce DTS style. BTW: there is no +5 mains dummy regulator defined in ci20.dts. What I fear is that if we always have to define the mains +5V (which is for example not defined in ci20.dts), which rules stops us from asking to add a dummy-regulator from 110/230V to +5V as well. In last consequence, it seems as if we have to describe all dummy regulators from the power plant to our hdmi-5v :) Since I always follow the KISS principle I tend to leave out what is not relevant... Of course adding a dummy regulator to the DTS allows to avoid the NULL pointer test in the driver code. Anyways, you are maintainers :) So should I spin a v11 for the series or just this patch or how should we do it? BR and thanks, Nikolaus > > Cheers, > -Paul > >>> (in which case I'd expect to see special >>> handling). >> The special case is to not enable/disable the regulator if it does not exist >> and assume that there is hardware providing it otherwise (the driver can't know >> that except by using get_optional). This is done by the code below >>>>> + if (IS_ERR(regulator)) { >> ... >>> + if (!regulator) >>> + return 0; >>> + ret = regulator_enable(regulator); >> ... >> BR and thanks, >> Nikolaus > >
On Wed, Dec 01, 2021 at 03:33:24PM +0100, H. Nikolaus Schaller wrote: > > Am 01.12.2021 um 15:03 schrieb Paul Cercueil <paul@crapouillou.net>: > > Please make it mandatory in DTS then, and use devm_regulator_get() in the driver. > Well, I just wonder why the elegant devm_regulator_get_optional() exists at all > and seems to be used in ca. 80 places. Frankly because half of them are broken usages like this since people seem determined to have the most fragile error handling they can :/ There are valid use cases for it, with things like SD cards where some supplies are genuinely optional and simply constrain what features are available if they're omitted from the design. You also see some devices with the ability to replace internal regulators with external ones. > And if it is not allowed, why some DTS should be forced to add not physically existing dummy-regulators. Again, if the supply can be physically absent that is a sensible use case but that means completely absent, not just not software controllable. We can represent fixed voltage regulators just fine. > AFAIR drivers should implement functionality defined by DTS but not the other way round: enforce DTS style. > BTW: there is no +5 mains dummy regulator defined in ci20.dts. It wouldn't be the first time a DTS were incomplete, and I'm sure it won't be the last. > What I fear is that if we always have to define the mains +5V (which is for example not > defined in ci20.dts), which rules stops us from asking to add a dummy-regulator from 110/230V to +5V as well. It is good practice to specify the full tree of supplies all the way to the main supply rail of the board, this ensures that if we need the information for something we've got it (even if that thing is just that we've got to the root of the tree). There's potential applications in battery supplied devices for managing very low power situations.
Hi Mark, > Am 01.12.2021 um 16:10 schrieb Mark Brown <broonie@kernel.org>: > > On Wed, Dec 01, 2021 at 03:33:24PM +0100, H. Nikolaus Schaller wrote: >>> Am 01.12.2021 um 15:03 schrieb Paul Cercueil <paul@crapouillou.net>: > >>> Please make it mandatory in DTS then, and use devm_regulator_get() in the driver. > >> Well, I just wonder why the elegant devm_regulator_get_optional() exists at all >> and seems to be used in ca. 80 places. > > Frankly because half of them are broken usages like this since people > seem determined to have the most fragile error handling they can :/ I see. I had made the mistake myself to not check for NULL pointer on regulator_disable here... > There are valid use cases for it, with things like SD cards where some > supplies are genuinely optional and simply constrain what features are > available if they're omitted from the design. You also see some devices > with the ability to replace internal regulators with external ones. > >> And if it is not allowed, why some DTS should be forced to add not physically existing dummy-regulators. > > Again, if the supply can be physically absent that is a sensible use > case but that means completely absent, not just not software > controllable. We can represent fixed voltage regulators just fine. The question may be how we can know for a more generic driver that there is always a regulator. In the present case we know the schematics but it is just one example. > >> AFAIR drivers should implement functionality defined by DTS but not the other way round: enforce DTS style. >> BTW: there is no +5 mains dummy regulator defined in ci20.dts. > > It wouldn't be the first time a DTS were incomplete, and I'm sure it > won't be the last. > >> What I fear is that if we always have to define the mains +5V (which is for example not >> defined in ci20.dts), which rules stops us from asking to add a dummy-regulator from 110/230V to +5V as well. > > It is good practice to specify the full tree of supplies all the way to > the main supply rail of the board, this ensures that if we need the > information for something we've got it (even if that thing is just that > we've got to the root of the tree). There's potential applications in > battery supplied devices for managing very low power situations. Indeed. So let's modify it as you have suggested. BR and thanks, Nikolaus
On Wed, Dec 01, 2021 at 05:53:03PM +0100, H. Nikolaus Schaller wrote: > > Am 01.12.2021 um 16:10 schrieb Mark Brown <broonie@kernel.org>: > > Again, if the supply can be physically absent that is a sensible use > > case but that means completely absent, not just not software > > controllable. We can represent fixed voltage regulators just fine. > The question may be how we can know for a more generic driver that there is always a regulator. > In the present case we know the schematics but it is just one example. The datasheet will generally explicitly call out if a supply can be disconnected. In general it is astonishingly rare for this to be the case, supporting that case will tend to make designing the chip harder (you have to cope with what happens where the power domains meet) and results in whatever functionality the supplies power not working. If not otherwise specified it's safer to assume that the supplies must be connected.
diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig index 3b57f8be007c4..4efc709d77b0a 100644 --- a/drivers/gpu/drm/ingenic/Kconfig +++ b/drivers/gpu/drm/ingenic/Kconfig @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU The Image Processing Unit (IPU) will appear as a second primary plane. +config DRM_INGENIC_DW_HDMI + tristate "Ingenic specific support for Synopsys DW HDMI" + depends on MACH_JZ4780 + select DRM_DW_HDMI + help + Choose this option to enable Synopsys DesignWare HDMI based driver. + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should + select this option.. + endif diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile index d313326bdddbb..f10cc1c5a5f22 100644 --- a/drivers/gpu/drm/ingenic/Makefile +++ b/drivers/gpu/drm/ingenic/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o ingenic-drm-y = ingenic-drm-drv.o ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o +obj-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file mode 100644 index 0000000000000..199e39c227d29 --- /dev/null +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. + * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk> + * + * Derived from dw_hdmi-imx.c with i.MX portions removed. + * Probe and remove operations derived from rcar_dw_hdmi.c. + */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include <drm/bridge/dw_hdmi.h> +#include <drm/drm_of.h> +#include <drm/drm_print.h> + +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { + { 45250000, { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } }, + { 92500000, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } }, + { 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } }, + { 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } }, + { ~0UL, { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } } +}; + +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { + /*pixelclk bpp8 bpp10 bpp12 */ + { 54000000, { 0x091c, 0x091c, 0x06dc } }, + { 58400000, { 0x091c, 0x06dc, 0x06dc } }, + { 72000000, { 0x06dc, 0x06dc, 0x091c } }, + { 74250000, { 0x06dc, 0x0b5c, 0x091c } }, + { 118800000, { 0x091c, 0x091c, 0x06dc } }, + { 216000000, { 0x06dc, 0x0b5c, 0x091c } }, + { ~0UL, { 0x0000, 0x0000, 0x0000 } }, +}; + +/* + * Resistance term 133Ohm Cfg + * PREEMP config 0.00 + * TX/CK level 10 + */ +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { + /*pixelclk symbol term vlev */ + { 216000000, 0x800d, 0x0005, 0x01ad}, + { ~0UL, 0x0000, 0x0000, 0x0000} +}; + +static enum drm_mode_status +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock < 13500) + return MODE_CLOCK_LOW; + /* FIXME: Hardware is capable of 270MHz, but setup data is missing. */ + if (mode->clock > 216000) + return MODE_CLOCK_HIGH; + + return MODE_OK; +} + +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = { + .mpll_cfg = ingenic_mpll_cfg, + .cur_ctr = ingenic_cur_ctr, + .phy_config = ingenic_phy_config, + .mode_valid = ingenic_dw_hdmi_mode_valid, + .output_port = 1, +}; + +static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = { + { .compatible = "ingenic,jz4780-dw-hdmi" }, + { /* Sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids); + +static void ingenic_dw_hdmi_cleanup(void *data) +{ + struct dw_hdmi *hdmi = (struct dw_hdmi *)data; + + dw_hdmi_remove(hdmi); +} + +static void ingenic_dw_hdmi_disable_regulator(void *data) +{ + struct regulator *regulator = (struct regulator *)data; + + regulator_disable(regulator); +} + +static int ingenic_dw_hdmi_probe(struct platform_device *pdev) +{ + struct dw_hdmi *hdmi; + struct regulator *regulator; + int ret; + + hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data); + if (IS_ERR(hdmi)) + return PTR_ERR(hdmi); + + ret = devm_add_action_or_reset(&pdev->dev, ingenic_dw_hdmi_cleanup, hdmi); + if (ret) + return ret; + + regulator = devm_regulator_get_optional(&pdev->dev, "hdmi-5v"); + if (IS_ERR(regulator)) { + ret = PTR_ERR(regulator); + + DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %s (%d)\n", + "hdmi-5v", ret); + + return ret; + } + + if (!regulator) + return 0; + ret = regulator_enable(regulator); + if (ret) { + DRM_DEV_ERROR(&pdev->dev, "Failed to enable hpd regulator: %d\n", + ret); + + return ret; + } + + return devm_add_action_or_reset(&pdev->dev, ingenic_dw_hdmi_disable_regulator, + regulator); +} + +static struct platform_driver ingenic_dw_hdmi_driver = { + .probe = ingenic_dw_hdmi_probe, + .driver = { + .name = "dw-hdmi-ingenic", + .of_match_table = ingenic_dw_hdmi_dt_ids, + }, +}; +module_platform_driver(ingenic_dw_hdmi_driver); + +MODULE_DESCRIPTION("JZ4780 Specific DW-HDMI Driver Extension"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:dwhdmi-ingenic");