lkml.org 
[lkml]   [2018]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 <jacopo+renesas@jmondi.org>
    > >>> ---
    > >>> 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(pdev, thc63);
    > >>> +
    > >>> + ret = thc63_regulator_init(thc63);
    > >>> + if (ret)
    > >>> + return ret;
    > >>> +
    > >>> + ret = thc63_gpio_init(thc63);
    > >>> + if (ret)
    > >>> + return ret;
    > >>> +
    > >>> + ret = thc63_parse_dt(thc63);
    > >>> + if (ret)
    > >>> + return ret;
    > >>> +
    > >>> + thc63->bridge.driver_private = thc63;
    > >>> + thc63->bridge.of_node = pdev->dev.of_node;
    > >>> + thc63->bridge.funcs = &thc63_bridge_func;
    > >>> +
    > >>> + drm_bridge_add(&thc63->bridge);
    > >>> +
    > >>> + return 0;
    > >>> +}
    > >>> +
    > >>> +static int thc63_remove(struct platform_device *pdev)
    > >>> +{
    > >>> + struct thc63_dev *thc63 = platform_get_drvdata(pdev);
    > >>> +
    > >>> + drm_bridge_remove(&thc63->bridge);
    > >>> +
    > >>> + return 0;
    > >>> +}
    > >>> +
    > >>> +#ifdef CONFIG_OF
    > >>> +static const struct of_device_id thc63_match[] = {
    > >>> + { .compatible = "thine,thc63lvd1024", },
    > >>> + { },
    > >>> +};
    > >>> +MODULE_DEVICE_TABLE(of, thc63_match);
    > >>> +#endif
    > >>> +
    > >>> +static struct platform_driver thc63_driver = {
    > >>> + .probe = thc63_probe,
    > >>> + .remove = thc63_remove,
    > >>> + .driver = {
    > >>> + .name = "thc63lvd1024",
    > >>> + .of_match_table = thc63_match,
    > >>> + },
    > >>> +};
    > >>> +module_platform_driver(thc63_driver);
    > >>> +
    > >>> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
    > >>> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
    > >>> +MODULE_LICENSE("GPL v2");
    > >>
    >
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-03-15 11:49    [W:7.040 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site