Re: [PATCH V2 4/5] drm/bridge: Add driver for GE B850v3 LVDS/DP++ Bridge

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Enric,
 
On Friday, June 10, 2016 09:39 CEST, Enric Balletbo Serra <eballetbo@xxxxxxxxx> wrote: 
 
> Hi Peter,
> 
> Only a few comments ;)

Thanks a lot for the review!

> 
> 2016-06-09 18:25 GMT+02:00 Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>:
> > Add a driver that create a drm_bridge and a drm_connector for the LVDS
> > to DP++ display bridge of the GE B850v3.
> >
> > There are two physical bridges on the video signal pipeline: a
> > STDP4028(LVDS to DP) and a STDP2690(DP to DP++).  The hardware and
> > firmware made it complicated for this binding to comprise two device
> > tree nodes, as the design goal is to configure both bridges based on
> > the LVDS signal, which leave the driver powerless to control the video
> > processing pipeline. The two bridges behaves as a single bridge, and
> > the driver is only needed for telling the host about EDID / HPD, and
> > for giving the host powers to ack interrupts. The video signal pipeline
> > is as follows:
> >
> >   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> >
> > Cc: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> > Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Fabio Estevam <fabio.estevam@xxxxxxx>
> > CC: David Airlie <airlied@xxxxxxxx>
> > CC: Thierry Reding <treding@xxxxxxxxxx>
> > CC: Thierry Reding <thierry.reding@xxxxxxxxx>
> > Signed-off-by: Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
> > ---
> > Changes from V1:
> >  - New commit message
> >  - Removed 3 empty entry points
> >  - Removed memory leak from ge_b850v3_lvds_dp_get_modes()
> >  - Added a lock for mode setting
> >  - Removed a few blank lines
> >  - Changed the order at Makefile and Kconfig
> >
> >  MAINTAINERS                                |   8 +
> >  drivers/gpu/drm/bridge/Kconfig             |  11 +
> >  drivers/gpu/drm/bridge/Makefile            |   1 +
> >  drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c | 392 +++++++++++++++++++++++++++++
> >  4 files changed, 412 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2ce5e91..2dd3d7f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -5010,6 +5010,14 @@ W:       https://linuxtv.org
> >  S:     Maintained
> >  F:     drivers/media/radio/radio-gemtek*
> >
> > +GENERAL ELECTRIC B850V3 LVDS/DP++ BRIDGE
> > +M:     Martin Donnelly <martin.donnelly@xxxxxx>
> > +M:     Peter Senna Tschudin <peter.senna@xxxxxxxxxxxxx>
> > +M:     Martyn Welch <martyn.welch@xxxxxxxxxxxxxxx>
> > +S:     Maintained
> > +F:     drivers/gpu/drm/bridge/ge_b850v3_dp2.c
> > +F:     Documentation/devicetree/bindings/ge/b850v3_dp2_bridge.txt
> > +
> >  GENERIC GPIO I2C DRIVER
> >  M:     Haavard Skinnemoen <hskinnemoen@xxxxxxxxx>
> >  S:     Supported
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 8f7423f..93dae5bd 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -32,6 +32,17 @@ config DRM_DW_HDMI_AHB_AUDIO
> >           Designware HDMI block.  This is used in conjunction with
> >           the i.MX6 HDMI driver.
> >
> > +config DRM_GE_B850V3_LVDS_DP
> > +       tristate "GE B850v3 LVDS to DP++ display bridge"
> > +       depends on OF
> > +       select DRM_KMS_HELPER
> > +       select DRM_PANEL
> > +       ---help---
> > +          This is a driver for the display bridge of
> > +          GE B850v3 that convert dual channel LVDS
> > +          to DP++. This is used with the i.MX6 imx-ldb
> > +          driver.
> > +
> >  config DRM_NXP_PTN3460
> >         tristate "NXP PTN3460 DP/LVDS bridge"
> >         depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 96b13b3..47ea6c1 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -3,6 +3,7 @@ ccflags-y := -Iinclude/drm
> >  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> >  obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
> >  obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
> > +obj-$(CONFIG_DRM_GE_B850V3_LVDS_DP) += ge_b850v3_lvds_dp.o
> >  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> >  obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> >  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> > diff --git a/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> > new file mode 100644
> > index 0000000..c73cd77
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/ge_b850v3_lvds_dp.c
> > @@ -0,0 +1,392 @@
> > +/*
> > + * Driver for GE B850v3 DP display bridge
> > +
> > + * Copyright (c) 2016, Collabora Ltd.
> > + * Copyright (c) 2016, General Electric Company
> > +
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > +
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > +
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > + * This driver creates a drm_bridge and a drm_connector for the LVDS to DP++
> > + * display bridge of the GE B850v3. There are two physical bridges on the video
> > + * signal pipeline: a STDP4028(LVDS to DP) and a STDP2690(DP to DP++). However
> > + * the physical bridges are automatically configured by the input video signal,
> > + * and the driver has no access to the video processing pipeline. The driver is
> > + * only needed to read EDID from the STDP2690 and to handle HPD events from the
> > + * STDP4028. The driver communicates with both bridges over i2c. The video
> > + * signal pipeline is as follows:
> > + *
> > + *   Host -> LVDS|--(STDP4028)--|DP -> DP|--(STDP2690)--|DP++ -> Video output
> > +
> > + *
> > + */
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include "drm_crtc.h"
> > +#include "drm_crtc_helper.h"
> > +#include "drm_edid.h"
> > +#include "drmP.h"
> > +
> > +#define EDID_EXT_BLOCK_CNT 0x7E
> > +
> > +#define STDP4028_IRQ_OUT_CONF_REG 0x02
> > +#define STDP4028_DPTX_IRQ_EN_REG 0x3C
> > +#define STDP4028_DPTX_IRQ_STS_REG 0x3D
> > +#define STDP4028_DPTX_STS_REG 0x3E
> > +
> > +#define STDP4028_DPTX_DP_IRQ_EN 0x1000
> > +
> > +#define STDP4028_DPTX_HOTPLUG_IRQ_EN 0x0400
> > +#define STDP4028_DPTX_LINK_CH_IRQ_EN 0x2000
> > +#define STDP4028_DPTX_IRQ_CONFIG \
> > +               (STDP4028_DPTX_LINK_CH_IRQ_EN | STDP4028_DPTX_HOTPLUG_IRQ_EN)
> > +
> > +#define STDP4028_DPTX_HOTPLUG_STS 0x0200
> > +#define STDP4028_DPTX_LINK_STS 0x1000
> > +#define STDP4028_CON_STATE_CONNECTED \
> > +               (STDP4028_DPTX_HOTPLUG_STS | STDP4028_DPTX_LINK_STS)
> > +
> > +#define STDP4028_DPTX_HOTPLUG_CH_STS 0x0400
> > +#define STDP4028_DPTX_LINK_CH_STS 0x2000
> > +#define STDP4028_DPTX_IRQ_CLEAR \
> > +               (STDP4028_DPTX_LINK_CH_STS | STDP4028_DPTX_HOTPLUG_CH_STS)
> > +
> > +struct ge_b850v3_lvds_dp {
> > +       struct drm_connector connector;
> > +       struct drm_bridge bridge;
> > +       struct i2c_client *ge_b850v3_lvds_dp_i2c;
> > +       struct i2c_client *edid_i2c;
> > +       struct edid *edid;
> > +       struct mutex lock;
> > +};
> > +
> > +static inline struct ge_b850v3_lvds_dp *
> > +               bridge_to_ge_b850v3_lvds_dp(struct drm_bridge *bridge)
> > +{
> > +       return container_of(bridge, struct ge_b850v3_lvds_dp, bridge);
> > +}
> > +
> > +static inline struct ge_b850v3_lvds_dp *
> > +               connector_to_ge_b850v3_lvds_dp(struct drm_connector *connector)
> > +{
> > +       return container_of(connector, struct ge_b850v3_lvds_dp, connector);
> > +}
> > +
> > +static void ge_b850v3_lvds_dp_enable(struct drm_bridge *bridge)
> > +{
> > +}
> > +
> 
> You can remove this function, see
> http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_bridge.c#L277

You are right, I removed the empty callbacks, but  they returned due me doing something wrong when rebasing. V3 will be out soon.

> 
> > +static void ge_b850v3_lvds_dp_disable(struct drm_bridge *bridge)
> > +{
> > +}
> > +
> 
> And this one, see
> http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_bridge.c#L182

Same. This should not be here.

> 
> > +u8 *stdp2690_get_edid(struct i2c_client *client)
> > +{
> > +       struct i2c_adapter *adapter = client->adapter;
> > +       unsigned char start = 0x00;
> > +       unsigned int total_size;
> > +       u8 *block = kmalloc(EDID_LENGTH, GFP_KERNEL);
> > +
> > +       struct i2c_msg msgs[] = {
> > +               {
> > +                       .addr   = client->addr,
> > +                       .flags  = 0,
> > +                       .len    = 1,
> > +                       .buf    = &start,
> > +               }, {
> > +                       .addr   = client->addr,
> > +                       .flags  = I2C_M_RD,
> > +                       .len    = EDID_LENGTH,
> > +                       .buf    = block,
> > +               }
> > +       };
> > +
> > +       if (!block)
> > +               return NULL;
> > +
> > +       if (i2c_transfer(adapter, msgs, 2) != 2) {
> > +               DRM_ERROR("Unable to read EDID.\n");
> > +               goto err;
> > +       }
> > +
> > +       if (!drm_edid_block_valid(block, 0, false, NULL)) {
> > +               DRM_ERROR("Invalid EDID block\n");
> > +               goto err;
> > +       }
> > +
> > +       total_size = (block[EDID_EXT_BLOCK_CNT] + 1) * EDID_LENGTH;
> > +       if (total_size > EDID_LENGTH) {
> > +               kfree(block);
> > +               block = kmalloc(total_size, GFP_KERNEL);
> > +               if (!block)
> > +                       return NULL;
> > +
> > +               /* Yes, read the entire buffer, and do not skip the first
> > +                * EDID_LENGTH bytes.
> > +                */
> > +               start = 0x00;
> > +               msgs[1].len = total_size;
> > +               msgs[1].buf = block;
> > +
> > +               if (i2c_transfer(adapter, msgs, 2) != 2) {
> > +                       DRM_ERROR("Unable to read EDID extension blocks.\n");
> > +                       goto err;
> > +               }
> > +       }
> > +
> > +       return block;
> > +
> > +err:
> > +       kfree(block);
> > +       return NULL;
> > +}
> > +
> > +static int ge_b850v3_lvds_dp_get_modes(struct drm_connector *connector)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge;
> > +       struct i2c_client *client;
> > +       int num_modes = 0;
> > +
> > +       ptn_bridge = connector_to_ge_b850v3_lvds_dp(connector);
> > +       client = ptn_bridge->edid_i2c;
> > +
> > +       mutex_lock(&ptn_bridge->lock);
> > +
> > +       kfree(ptn_bridge->edid);
> > +       ptn_bridge->edid = (struct edid *) stdp2690_get_edid(client);
> > +
> > +       if (ptn_bridge->edid) {
> > +               drm_mode_connector_update_edid_property(connector,
> > +                               ptn_bridge->edid);
> > +               num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> > +       }
> > +
> > +       mutex_unlock(&ptn_bridge->lock);
> > +
> > +       return num_modes;
> > +}
> > +
> > +static struct drm_encoder
> > +*ge_b850v3_lvds_dp_best_encoder(struct drm_connector *connector)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge =
> > +               connector_to_ge_b850v3_lvds_dp(connector);
> > +
> > +       return ptn_bridge->bridge.encoder;
> > +}
> > +
> > +static const struct
> > +drm_connector_helper_funcs ge_b850v3_lvds_dp_connector_helper_funcs = {
> > +       .get_modes = ge_b850v3_lvds_dp_get_modes,
> > +       .best_encoder = ge_b850v3_lvds_dp_best_encoder,
> > +};
> > +
> > +static enum drm_connector_status ge_b850v3_lvds_dp_detect(
> > +               struct drm_connector *connector, bool force)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge =
> > +                       connector_to_ge_b850v3_lvds_dp(connector);
> > +       struct i2c_client *ge_b850v3_lvds_dp_i2c =
> > +                       ptn_bridge->ge_b850v3_lvds_dp_i2c;
> > +       s32 link_state;
> > +
> > +       link_state = i2c_smbus_read_word_data(ge_b850v3_lvds_dp_i2c,
> > +                       STDP4028_DPTX_STS_REG);
> > +
> > +       if (link_state == STDP4028_CON_STATE_CONNECTED)
> > +               return connector_status_connected;
> > +
> > +       if (link_state == 0)
> > +               return connector_status_disconnected;
> > +
> > +       return connector_status_unknown;
> > +}
> > +
> > +static void ge_b850v3_lvds_dp_connector_destroy(struct drm_connector *connector)
> > +{
> > +       drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs ge_b850v3_lvds_dp_connector_funcs = {
> > +       .dpms = drm_helper_connector_dpms,
> > +       .fill_modes = drm_helper_probe_single_connector_modes,
> > +       .detect = ge_b850v3_lvds_dp_detect,
> > +       .destroy = ge_b850v3_lvds_dp_connector_destroy,
> > +};
> > +
> > +static irqreturn_t ge_b850v3_lvds_dp_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge = dev_id;
> > +       struct i2c_client *ge_b850v3_lvds_dp_i2c
> > +                       = ptn_bridge->ge_b850v3_lvds_dp_i2c;
> > +
> > +       mutex_lock(&ptn_bridge->lock);
> > +
> > +       i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> > +                       STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> > +
> > +       mutex_unlock(&ptn_bridge->lock);
> > +
> > +       if (ptn_bridge->connector.dev)
> > +               drm_kms_helper_hotplug_event(ptn_bridge->connector.dev);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int ge_b850v3_lvds_dp_attach(struct drm_bridge *bridge)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge
> > +                       = bridge_to_ge_b850v3_lvds_dp(bridge);
> > +       struct drm_connector *connector = &ptn_bridge->connector;
> > +       struct i2c_client *ge_b850v3_lvds_dp_i2c
> > +                       = ptn_bridge->ge_b850v3_lvds_dp_i2c;
> > +       int ret;
> > +
> > +       if (!bridge->encoder) {
> > +               DRM_ERROR("Parent encoder object not found");
> > +               return -ENODEV;
> > +       }
> > +
> > +       connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > +       drm_connector_helper_add(connector,
> > +                       &ge_b850v3_lvds_dp_connector_helper_funcs);
> > +
> > +       ret = drm_connector_init(bridge->dev, connector,
> > +                       &ge_b850v3_lvds_dp_connector_funcs,
> > +                       DRM_MODE_CONNECTOR_DisplayPort);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to initialize connector with drm\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = drm_mode_connector_attach_encoder(connector, bridge->encoder);
> > +       if (ret)
> > +               return ret;
> > +
> > +       drm_bridge_enable(bridge);
> > +       if (ge_b850v3_lvds_dp_i2c->irq) {
> > +               drm_helper_hpd_irq_event(connector->dev);
> > +
> > +               ret = devm_request_threaded_irq(&ge_b850v3_lvds_dp_i2c->dev,
> > +                               ge_b850v3_lvds_dp_i2c->irq, NULL,
> > +                               ge_b850v3_lvds_dp_irq_handler,
> > +                               IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +                               "ge-b850v3-lvds-dp", ptn_bridge);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs ge_b850v3_lvds_dp_funcs = {
> > +       .enable = ge_b850v3_lvds_dp_enable,
> > +       .disable = ge_b850v3_lvds_dp_disable,
> 
> Remove the above empty callbacks.

Here too.

> 
> > +       .attach = ge_b850v3_lvds_dp_attach,
> > +};
> > +
> > +static int ge_b850v3_lvds_dp_probe(struct i2c_client *ge_b850v3_lvds_dp_i2c,
> > +                               const struct i2c_device_id *id)
> > +{
> > +       struct device *dev = &ge_b850v3_lvds_dp_i2c->dev;
> > +       struct ge_b850v3_lvds_dp *ptn_bridge;
> > +       int ret;
> > +       u32 edid_i2c_reg;
> > +
> > +       ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL);
> > +       if (!ptn_bridge)
> > +               return -ENOMEM;
> > +
> > +       mutex_init(&ptn_bridge->lock);
> > +
> > +       ptn_bridge->ge_b850v3_lvds_dp_i2c = ge_b850v3_lvds_dp_i2c;
> > +       ptn_bridge->bridge.driver_private = ptn_bridge;
> > +       i2c_set_clientdata(ge_b850v3_lvds_dp_i2c, ptn_bridge);
> > +
> > +       ret = of_property_read_u32(dev->of_node, "edid-reg", &edid_i2c_reg);
> > +       if (ret) {
> > +               dev_err(dev, "edid-reg not specified, aborting...\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       ptn_bridge->edid_i2c = devm_kzalloc(dev,
> > +                       sizeof(struct i2c_client), GFP_KERNEL);
> > +
> > +       if (!ptn_bridge->edid_i2c)
> > +               return -ENOMEM;
> > +
> > +       memcpy(ptn_bridge->edid_i2c, ge_b850v3_lvds_dp_i2c,
> > +                       sizeof(struct i2c_client));
> > +
> > +       ptn_bridge->edid_i2c->addr = (unsigned short) edid_i2c_reg;
> > +
> > +       /* Configures the bridge to re-enable interrupts after each ack */
> > +       i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> > +                       STDP4028_IRQ_OUT_CONF_REG, STDP4028_DPTX_DP_IRQ_EN);
> > +       i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> > +                       STDP4028_DPTX_IRQ_EN_REG, STDP4028_DPTX_IRQ_CONFIG);
> > +
> > +       /* Clear pending interrupts since power up. */
> > +       i2c_smbus_write_word_data(ge_b850v3_lvds_dp_i2c,
> > +                       STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
> > +
> > +       ptn_bridge->bridge.funcs = &ge_b850v3_lvds_dp_funcs;
> > +       ptn_bridge->bridge.of_node = dev->of_node;
> > +       ret = drm_bridge_add(&ptn_bridge->bridge);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to add bridge\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int ge_b850v3_lvds_dp_remove(struct i2c_client *ge_b850v3_lvds_dp_i2c)
> > +{
> > +       struct ge_b850v3_lvds_dp *ptn_bridge =
> > +               i2c_get_clientdata(ge_b850v3_lvds_dp_i2c);
> > +
> > +       drm_bridge_remove(&ptn_bridge->bridge);
> 
> Guess you need to free ptn_bridge->edid here.

Thanks a lot! I'll fix it when sending V3.

Thank you for the review!

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux