Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
HI Andrezej, On Thu, Mar 15, 2018 at 10:44:42AM +0100, Andrzej Hajda wrote: > On 14.03.2018 11:09, jacopo mondi wrote: > > Hi Andrzej, > > thanks for review > > > > On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote: > >> On 13.03.2018 15:30, Jacopo Mondi wrote: > >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > >>> output decoder. > >> IMO converter suits here better, but it is just suggestion. > >> > >>> Signed-off-by: Jacopo Mondi > >>> --- > >>> drivers/gpu/drm/bridge/Kconfig| 7 + > >>> drivers/gpu/drm/bridge/Makefile | 1 + > >>> drivers/gpu/drm/bridge/thc63lvd1024.c | 241 > >>> ++ > >>> 3 files changed, 249 insertions(+) > >>> create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c > >>> > >>> diff --git a/drivers/gpu/drm/bridge/Kconfig > >>> b/drivers/gpu/drm/bridge/Kconfig > >>> index 3b99d5a..facf6bd 100644 > >>> --- a/drivers/gpu/drm/bridge/Kconfig > >>> +++ b/drivers/gpu/drm/bridge/Kconfig > >>> @@ -92,6 +92,13 @@ config DRM_SII9234 > >>> It is an I2C driver, that detects connection of MHL bridge > >>> and starts encapsulation of HDMI signal. > >>> > >>> +config DRM_THINE_THC63LVD1024 > >>> + tristate "Thine THC63LVD1024 LVDS decoder bridge" > >>> + depends on OF > >>> + select DRM_PANEL_BRIDGE > >> You don't use PANEL_BRIDGE, it should be possible to drop the select. > >> > > Ack > > > >>> + ---help--- > >>> + Thine THC63LVD1024 LVDS decoder bridge driver. > >> Decoder to what? > >> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB > >> converter or bridge. > > "LVDS to digital CMOS/TTL parallel data converter" as the manual > > describes the chip? > > Should work, unless we want some consistency in interface names, we have > already: parallel / DPI / RGB. I am little bit confused about relations > between them so I do not want to enforce any. config DRM_THINE_THC63LVD1024 tristate "Thine THC63LVD1024 LVDS decoder bridge" depends on OF ---help--- Thine THC63LVD1024 LVDS/parallel converter driver. I guess this is consistent with other symbol descriptions I found > [snip] > > >>> + > >>> +static int thc63_gpio_init(struct thc63_dev *thc63) > >>> +{ > >>> + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn", > >>> + GPIOD_OUT_LOW); > >> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled? > > No, the PWDN gpio is active low, it puts the chip in power down mode > > when the physical line level is set to 0. > > > > That's another confusing thing, when requesting the GPIO, the actual > > physical line value has to be provided, while when "set_value()" the > > logical one is requested, iirc. > > I think it is opposite, only *raw* functions uses physical level. Other > uses logical level. > > > > > Being the GPIO defined as active low, in power enable we set it to > > "logical inactive" == "physical 1", while during power disable it has > > to be set to "logical active" == "physical 0" > > > > Not the right place to complain here, but imo that's not consistent. > > Or have I misinterpreted the APIs? I cannot do much test, as in my setup > > the PWDN pin is hardwired to +vcc (through a physical switch, not > > controlled through a GPIO though). > > devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which > calls gpiod_direction_output which uses logical levels. I clearly mis-interpreted the APIs. I'll fix and I'm sending v4 out shortly. Thanks j > > Regards > Andrzej > > > > > Thanks > >j > > > >> Regards > >> Andrzej > >>> + if (IS_ERR(thc63->pwdn)) { > >>> + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n"); > >>> + return PTR_ERR(thc63->pwdn); > >>> + } > >>> + > >>> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); > >>> + if (IS_ERR(thc63->oe)) { > >>> + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n"); > >>> + return PTR_ERR(thc63->oe); > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int thc63_regulator_init(struct thc63_dev *thc63) > >>> +{ > >>> + struct regulator **reg; > >>> + int i; > >>> + > >>> + reg = &thc63->vccs[0]; > >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) { > >>> + *reg = devm_regulator_get_optional(thc63->dev, > >>> +thc63_reg_names[i]); > >>> + if (IS_ERR(*reg)) { > >>> + if (PTR_ERR(*reg) == -EPROBE_DEFER) > >>> + return -EPROBE_DEFER; > >>> + *reg = NULL; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int thc63_probe(struct platform_device *pdev) > >>> +{ > >>> + struct thc63_dev *thc63; > >>> + int ret; > >>> + > >>> + thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL); > >>> + if (!thc63) > >>> + return -ENOMEM; > >>> + > >>> + thc63->dev = &pdev->dev; > >>> + platform_set_drvdata(pd
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
On 14.03.2018 11:09, jacopo mondi wrote: > Hi Andrzej, > thanks for review > > On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote: >> On 13.03.2018 15:30, Jacopo Mondi wrote: >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel >>> output decoder. >> IMO converter suits here better, but it is just suggestion. >> >>> Signed-off-by: Jacopo Mondi >>> --- >>> drivers/gpu/drm/bridge/Kconfig| 7 + >>> drivers/gpu/drm/bridge/Makefile | 1 + >>> drivers/gpu/drm/bridge/thc63lvd1024.c | 241 >>> ++ >>> 3 files changed, 249 insertions(+) >>> create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c >>> >>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >>> index 3b99d5a..facf6bd 100644 >>> --- a/drivers/gpu/drm/bridge/Kconfig >>> +++ b/drivers/gpu/drm/bridge/Kconfig >>> @@ -92,6 +92,13 @@ config DRM_SII9234 >>> It is an I2C driver, that detects connection of MHL bridge >>> and starts encapsulation of HDMI signal. >>> >>> +config DRM_THINE_THC63LVD1024 >>> + tristate "Thine THC63LVD1024 LVDS decoder bridge" >>> + depends on OF >>> + select DRM_PANEL_BRIDGE >> You don't use PANEL_BRIDGE, it should be possible to drop the select. >> > Ack > >>> + ---help--- >>> + Thine THC63LVD1024 LVDS decoder bridge driver. >> Decoder to what? >> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB >> converter or bridge. > "LVDS to digital CMOS/TTL parallel data converter" as the manual > describes the chip? Should work, unless we want some consistency in interface names, we have already: parallel / DPI / RGB. I am little bit confused about relations between them so I do not want to enforce any. > >>> + >>> config DRM_TOSHIBA_TC358767 >>> tristate "Toshiba TC358767 eDP bridge" >>> depends on OF >>> diff --git a/drivers/gpu/drm/bridge/Makefile >>> b/drivers/gpu/drm/bridge/Makefile >>> index 373eb28..fd90b16 100644 >>> --- a/drivers/gpu/drm/bridge/Makefile >>> +++ b/drivers/gpu/drm/bridge/Makefile >>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >>> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o >>> obj-$(CONFIG_DRM_SII902X) += sii902x.o >>> obj-$(CONFIG_DRM_SII9234) += sii9234.o >>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o >>> obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o >>> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ >>> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ >>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c >>> b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> new file mode 100644 >>> index 000..4b059c0 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> @@ -0,0 +1,241 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver. >>> + * >>> + * Copyright (C) 2018 Jacopo Mondi >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> + >>> +static const char * const thc63_reg_names[] = { >>> + "vcc", "lvcc", "pvcc", "cvcc", }; >>> + >>> +struct thc63_dev { >>> + struct device *dev; >>> + >>> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; >>> + >>> + struct gpio_desc *pwdn; >>> + struct gpio_desc *oe; >>> + >>> + struct drm_bridge bridge; >>> + struct drm_bridge *next; >>> +}; >>> + >>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) >>> +{ >>> + return container_of(bridge, struct thc63_dev, bridge); >>> +} >>> + >>> +static int thc63_attach(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + >>> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); >>> +} >>> + >>> +static void thc63_enable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + unsigned int i; >>> + int ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (vcc) { >>> + ret = regulator_enable(vcc); >>> + if (ret) >>> + goto error_vcc_enable; >>> + } >>> + } >>> + >>> + if (thc63->pwdn) >>> + gpiod_set_value(thc63->pwdn, 0); >>> + >>> + if (thc63->oe) >>> + gpiod_set_value(thc63->oe, 1); >>> + >>> + return; >>> + >>> +error_vcc_enable: >>> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); >>> +} >>> + >>> +static void thc63_disable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + unsigned int i; >>> + int ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (vcc) { >>> + ret = regulator_disable(vcc); >>> + if (ret) >>> + goto error_vcc_disable; >> I think in disable path
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
On 03/14/2018 09:04 PM, jacopo mondi wrote: >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel >>> output decoder. >>> >>> Signed-off-by: Jacopo Mondi >> [...] >>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c >>> b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> new file mode 100644 >>> index 000..4b059c0 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c >>> @@ -0,0 +1,241 @@ [...] >>> +static void thc63_enable(struct drm_bridge *bridge) >>> +{ >>> + struct thc63_dev *thc63 = to_thc63(bridge); >>> + struct regulator *vcc; >>> + unsigned int i; >>> + int ret; >>> + >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { >>> + vcc = thc63->vccs[i]; >>> + if (vcc) { >>> + ret = regulator_enable(vcc); >>> + if (ret) >> >>You hardly need this variable, could do a call right in this *if*. >> >> [...] >>> +error_vcc_enable: >>> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); >>> +} >>> + >> >>Why not do this instead of *goto* before? > > Well, goto breaks the loop, if I only print out the error message, the > enable sequence will go on and enable the other regulators. > I can print out and break, but I don't see that much benefit Sorry, I meant that you should *return* there instead of *goto*. > One thing I could do instead, is not only print out the error message, > but disable the already enabled regulators if one fails to start. Yeah, probably... [...] >>> +static int thc63_gpio_init(struct thc63_dev *thc63) >>> +{ >>> + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn", >>> + GPIOD_OUT_LOW); >>> + if (IS_ERR(thc63->pwdn)) { >>> + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n"); >> >>"pwdn-gpios" maybe? >> >>> + return PTR_ERR(thc63->pwdn); >>> + } >>> + >>> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); >>> + if (IS_ERR(thc63->oe)) { >>> + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n"); >> >>"oe-gpios" maybe? > > Are you referring to the error message? Yes, seems more clear what to look for this way, IMHO... [...] MBR, Sergei
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
Hi Sergei, thanks for review On Wed, Mar 14, 2018 at 08:09:52PM +0300, Sergei Shtylyov wrote: > On 03/13/2018 05:30 PM, Jacopo Mondi wrote: > > > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > > output decoder. > > > > Signed-off-by: Jacopo Mondi > [...] > > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c > > b/drivers/gpu/drm/bridge/thc63lvd1024.c > > new file mode 100644 > > index 000..4b059c0 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > > @@ -0,0 +1,241 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > > + * > > + * Copyright (C) 2018 Jacopo Mondi > > + */ > > + > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +static const char * const thc63_reg_names[] = { > > + "vcc", "lvcc", "pvcc", "cvcc", }; > >Your bracing style is pretty strange -- neither here nor there. Please > place }; > on the next line... Yeah, I had doubt about this.. The most common style I found around is static const char * const foo[] = { "bar", "baz", "...", }; But seems really too many lines for a bunch of 4 character strings... > > [...] > > +static void thc63_enable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > > + vcc = thc63->vccs[i]; > > + if (vcc) { > > + ret = regulator_enable(vcc); > > + if (ret) > >You hardly need this variable, could do a call right in this *if*. > > [...] > > +error_vcc_enable: > > + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); > > +} > > + > >Why not do this instead of *goto* before? Well, goto breaks the loop, if I only print out the error message, the enable sequence will go on and enable the other regulators. I can print out and break, but I don't see that much benefit One thing I could do instead, is not only print out the error message, but disable the already enabled regulators if one fails to start. > > > +static void thc63_disable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > > + vcc = thc63->vccs[i]; > > + if (vcc) { > > + ret = regulator_disable(vcc); > > + if (ret) > >Again, no need for 'ret' whatsoever... > > > + goto error_vcc_disable; > > + } > > + } > > + > > + if (thc63->pwdn) > > + gpiod_set_value(thc63->pwdn, 1); > > + > > + if (thc63->oe) > > + gpiod_set_value(thc63->oe, 0); > > + > > + return; > > + > > +error_vcc_disable: > > + dev_err(thc63->dev, "Failed to disable regulator %u\n", i); > >Again, why not do it instead of *goto*? ditto > > [...] > > +static int thc63_gpio_init(struct thc63_dev *thc63) > > +{ > > + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(thc63->pwdn)) { > > + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n"); > >"pwdn-gpios" maybe? > > > + return PTR_ERR(thc63->pwdn); > > + } > > + > > + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); > > + if (IS_ERR(thc63->oe)) { > > + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n"); > >"oe-gpios" maybe? Are you referring to the error message? I can change this, but again, I see no standards around. Thanks j > > [...] > > MBR, Sergei signature.asc Description: PGP signature
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
On 03/13/2018 05:30 PM, Jacopo Mondi wrote: > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > output decoder. > > Signed-off-by: Jacopo Mondi [...] > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c > b/drivers/gpu/drm/bridge/thc63lvd1024.c > new file mode 100644 > index 000..4b059c0 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > @@ -0,0 +1,241 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > + * > + * Copyright (C) 2018 Jacopo Mondi > + */ > + > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +static const char * const thc63_reg_names[] = { > + "vcc", "lvcc", "pvcc", "cvcc", }; Your bracing style is pretty strange -- neither here nor there. Please place }; on the next line... [...] > +static void thc63_enable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + unsigned int i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (vcc) { > + ret = regulator_enable(vcc); > + if (ret) You hardly need this variable, could do a call right in this *if*. [...] > +error_vcc_enable: > + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); > +} > + Why not do this instead of *goto* before? > +static void thc63_disable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + unsigned int i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (vcc) { > + ret = regulator_disable(vcc); > + if (ret) Again, no need for 'ret' whatsoever... > + goto error_vcc_disable; > + } > + } > + > + if (thc63->pwdn) > + gpiod_set_value(thc63->pwdn, 1); > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 0); > + > + return; > + > +error_vcc_disable: > + dev_err(thc63->dev, "Failed to disable regulator %u\n", i); Again, why not do it instead of *goto*? [...] > +static int thc63_gpio_init(struct thc63_dev *thc63) > +{ > + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn", > + GPIOD_OUT_LOW); > + if (IS_ERR(thc63->pwdn)) { > + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n"); "pwdn-gpios" maybe? > + return PTR_ERR(thc63->pwdn); > + } > + > + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW); > + if (IS_ERR(thc63->oe)) { > + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n"); "oe-gpios" maybe? [...] MBR, Sergei
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
Hi Andrzej, thanks for review On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote: > On 13.03.2018 15:30, Jacopo Mondi wrote: > > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > > output decoder. > > IMO converter suits here better, but it is just suggestion. > > > > > Signed-off-by: Jacopo Mondi > > --- > > drivers/gpu/drm/bridge/Kconfig| 7 + > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/thc63lvd1024.c | 241 > > ++ > > 3 files changed, 249 insertions(+) > > create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > > index 3b99d5a..facf6bd 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -92,6 +92,13 @@ config DRM_SII9234 > > It is an I2C driver, that detects connection of MHL bridge > > and starts encapsulation of HDMI signal. > > > > +config DRM_THINE_THC63LVD1024 > > + tristate "Thine THC63LVD1024 LVDS decoder bridge" > > + depends on OF > > + select DRM_PANEL_BRIDGE > > You don't use PANEL_BRIDGE, it should be possible to drop the select. > Ack > > + ---help--- > > + Thine THC63LVD1024 LVDS decoder bridge driver. > > Decoder to what? > Maybe it would be better to say it is LVDS/parallel or LVDS/RGB > converter or bridge. "LVDS to digital CMOS/TTL parallel data converter" as the manual describes the chip? > > > + > > config DRM_TOSHIBA_TC358767 > > tristate "Toshiba TC358767 eDP bridge" > > depends on OF > > diff --git a/drivers/gpu/drm/bridge/Makefile > > b/drivers/gpu/drm/bridge/Makefile > > index 373eb28..fd90b16 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > > obj-$(CONFIG_DRM_SII902X) += sii902x.o > > obj-$(CONFIG_DRM_SII9234) += sii9234.o > > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c > > b/drivers/gpu/drm/bridge/thc63lvd1024.c > > new file mode 100644 > > index 000..4b059c0 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > > @@ -0,0 +1,241 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > > + * > > + * Copyright (C) 2018 Jacopo Mondi > > + */ > > + > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +static const char * const thc63_reg_names[] = { > > + "vcc", "lvcc", "pvcc", "cvcc", }; > > + > > +struct thc63_dev { > > + struct device *dev; > > + > > + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; > > + > > + struct gpio_desc *pwdn; > > + struct gpio_desc *oe; > > + > > + struct drm_bridge bridge; > > + struct drm_bridge *next; > > +}; > > + > > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct thc63_dev, bridge); > > +} > > + > > +static int thc63_attach(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + > > + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); > > +} > > + > > +static void thc63_enable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > > + vcc = thc63->vccs[i]; > > + if (vcc) { > > + ret = regulator_enable(vcc); > > + if (ret) > > + goto error_vcc_enable; > > + } > > + } > > + > > + if (thc63->pwdn) > > + gpiod_set_value(thc63->pwdn, 0); > > + > > + if (thc63->oe) > > + gpiod_set_value(thc63->oe, 1); > > + > > + return; > > + > > +error_vcc_enable: > > + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); > > +} > > + > > +static void thc63_disable(struct drm_bridge *bridge) > > +{ > > + struct thc63_dev *thc63 = to_thc63(bridge); > > + struct regulator *vcc; > > + unsigned int i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > > + vcc = thc63->vccs[i]; > > + if (vcc) { > > + ret = regulator_disable(vcc); > > + if (ret) > > + goto error_vcc_disable; > > I think in disable path you can report an error and continue - should be > safer. > Ack > > + } > > + } > > + > > + if (thc63->pwdn) > > + gpiod_set_value(thc63->pwdn, 1); > > + > > + if (thc63->oe) > > + gpiod_set_value(thc63->o
Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
On 13.03.2018 15:30, Jacopo Mondi wrote: > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel > output decoder. IMO converter suits here better, but it is just suggestion. > > Signed-off-by: Jacopo Mondi > --- > drivers/gpu/drm/bridge/Kconfig| 7 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/thc63lvd1024.c | 241 > ++ > 3 files changed, 249 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 3b99d5a..facf6bd 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -92,6 +92,13 @@ config DRM_SII9234 > It is an I2C driver, that detects connection of MHL bridge > and starts encapsulation of HDMI signal. > > +config DRM_THINE_THC63LVD1024 > + tristate "Thine THC63LVD1024 LVDS decoder bridge" > + depends on OF > + select DRM_PANEL_BRIDGE You don't use PANEL_BRIDGE, it should be possible to drop the select. > + ---help--- > + Thine THC63LVD1024 LVDS decoder bridge driver. Decoder to what? Maybe it would be better to say it is LVDS/parallel or LVDS/RGB converter or bridge. > + > config DRM_TOSHIBA_TC358767 > tristate "Toshiba TC358767 eDP bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 373eb28..fd90b16 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > obj-$(CONFIG_DRM_SII902X) += sii902x.o > obj-$(CONFIG_DRM_SII9234) += sii9234.o > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c > b/drivers/gpu/drm/bridge/thc63lvd1024.c > new file mode 100644 > index 000..4b059c0 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c > @@ -0,0 +1,241 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * THC63LVD1024 LVDS to parallel data DRM bridge driver. > + * > + * Copyright (C) 2018 Jacopo Mondi > + */ > + > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +static const char * const thc63_reg_names[] = { > + "vcc", "lvcc", "pvcc", "cvcc", }; > + > +struct thc63_dev { > + struct device *dev; > + > + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; > + > + struct gpio_desc *pwdn; > + struct gpio_desc *oe; > + > + struct drm_bridge bridge; > + struct drm_bridge *next; > +}; > + > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct thc63_dev, bridge); > +} > + > +static int thc63_attach(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + > + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); > +} > + > +static void thc63_enable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + unsigned int i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (vcc) { > + ret = regulator_enable(vcc); > + if (ret) > + goto error_vcc_enable; > + } > + } > + > + if (thc63->pwdn) > + gpiod_set_value(thc63->pwdn, 0); > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 1); > + > + return; > + > +error_vcc_enable: > + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); > +} > + > +static void thc63_disable(struct drm_bridge *bridge) > +{ > + struct thc63_dev *thc63 = to_thc63(bridge); > + struct regulator *vcc; > + unsigned int i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { > + vcc = thc63->vccs[i]; > + if (vcc) { > + ret = regulator_disable(vcc); > + if (ret) > + goto error_vcc_disable; I think in disable path you can report an error and continue - should be safer. > + } > + } > + > + if (thc63->pwdn) > + gpiod_set_value(thc63->pwdn, 1); > + > + if (thc63->oe) > + gpiod_set_value(thc63->oe, 0); Usually disable uses reverse order. Ie first disable oe, then, pwdn, then regulators, also in reverse order. Looks more reasonable. > + > + return; > + > +error_vcc_disable: > + dev_err(thc63->dev, "Failed to disable regulator %u\n", i); > +} > + > +struct drm_bridge_funcs thc63_bridge_func = { > + .attach = thc63_attach, > + .enable = thc63_enable, > + .disable = th
[PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver
Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel output decoder. Signed-off-by: Jacopo Mondi --- drivers/gpu/drm/bridge/Kconfig| 7 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++ 3 files changed, 249 insertions(+) create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 3b99d5a..facf6bd 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -92,6 +92,13 @@ config DRM_SII9234 It is an I2C driver, that detects connection of MHL bridge and starts encapsulation of HDMI signal. +config DRM_THINE_THC63LVD1024 + tristate "Thine THC63LVD1024 LVDS decoder bridge" + depends on OF + select DRM_PANEL_BRIDGE + ---help--- + Thine THC63LVD1024 LVDS decoder bridge driver. + config DRM_TOSHIBA_TC358767 tristate "Toshiba TC358767 eDP bridge" depends on OF diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 373eb28..fd90b16 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o obj-$(CONFIG_DRM_SII902X) += sii902x.o obj-$(CONFIG_DRM_SII9234) += sii9234.o +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c new file mode 100644 index 000..4b059c0 --- /dev/null +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * THC63LVD1024 LVDS to parallel data DRM bridge driver. + * + * Copyright (C) 2018 Jacopo Mondi + */ + +#include +#include +#include + +#include +#include +#include + +static const char * const thc63_reg_names[] = { + "vcc", "lvcc", "pvcc", "cvcc", }; + +struct thc63_dev { + struct device *dev; + + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)]; + + struct gpio_desc *pwdn; + struct gpio_desc *oe; + + struct drm_bridge bridge; + struct drm_bridge *next; +}; + +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge) +{ + return container_of(bridge, struct thc63_dev, bridge); +} + +static int thc63_attach(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + + return drm_bridge_attach(bridge->encoder, thc63->next, bridge); +} + +static void thc63_enable(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + struct regulator *vcc; + unsigned int i; + int ret; + + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { + vcc = thc63->vccs[i]; + if (vcc) { + ret = regulator_enable(vcc); + if (ret) + goto error_vcc_enable; + } + } + + if (thc63->pwdn) + gpiod_set_value(thc63->pwdn, 0); + + if (thc63->oe) + gpiod_set_value(thc63->oe, 1); + + return; + +error_vcc_enable: + dev_err(thc63->dev, "Failed to enable regulator %u\n", i); +} + +static void thc63_disable(struct drm_bridge *bridge) +{ + struct thc63_dev *thc63 = to_thc63(bridge); + struct regulator *vcc; + unsigned int i; + int ret; + + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) { + vcc = thc63->vccs[i]; + if (vcc) { + ret = regulator_disable(vcc); + if (ret) + goto error_vcc_disable; + } + } + + if (thc63->pwdn) + gpiod_set_value(thc63->pwdn, 1); + + if (thc63->oe) + gpiod_set_value(thc63->oe, 0); + + return; + +error_vcc_disable: + dev_err(thc63->dev, "Failed to disable regulator %u\n", i); +} + +struct drm_bridge_funcs thc63_bridge_func = { + .attach = thc63_attach, + .enable = thc63_enable, + .disable = thc63_disable, +}; + +static int thc63_parse_dt(struct thc63_dev *thc63) +{ + struct device_node *thc63_out; + struct device_node *remote; + int ret = 0; + + thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1); + if (!thc63_out) { + dev_err(thc63->dev, "Missing endpoint in Port@2\n"); + return -ENODEV; + } + + remote = of_graph_get_remote_port_parent(thc63_out); + if (!remote) { + dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n"); + ret = -ENODEV; + goto error_put_out_node; + } + + if (!of_device_is_available(remote)) { +