Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP
On Tue, Jul 07, 2020 at 03:01:25PM +0800, Nicolas Boichat wrote:
Hi Nicolas, thanks for the replay.
> On Tue, Jun 9, 2020 at 3:20 PM Xin Ji wrote:
> >
> > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
> >
> > Signed-off-by: Xin Ji
> > ---
> > drivers/gpu/drm/bridge/analogix/Kconfig |9 +
> > drivers/gpu/drm/bridge/analogix/Makefile |1 +
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 1999
> > +
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++
> > 4 files changed, 2406 insertions(+)
> > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
> >
> > [snip]
> > +static int anx7625_parse_dt(struct device *dev,
> > + struct anx7625_platform_data *pdata)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct device_node *panel_node, *out_ep;
> > +
> > + pdata->node.mipi_dsi_host_node = of_graph_get_remote_node(np, 0, 0);
> > + if (!pdata->node.mipi_dsi_host_node) {
> > + DRM_DEV_ERROR(dev, "fail to get internal panel.\n");
> > + return -EPROBE_DEFER;
>
> This does not look correct. I don't think of_graph_get_remote_node
> will ever return NULL if the device tree is configured properly, and
> it's useless to retry later (EPROBE_DEFER). You should just fail (e.g.
> return EINVAL).
OK
>
> > + }
> > +
> > + of_node_put(pdata->node.mipi_dsi_host_node);
>
> You are using pdata->node.mipi_dsi_host_node in other places in the
> code, so I don't think it's ok to call of_node_put?
I'll move the related code to here.
>
> > + DRM_DEV_DEBUG_DRIVER(dev, "found dsi host node.\n");
> > +
> > + pdata->node.panel_node = of_graph_get_port_by_id(np, 1);
> > + if (!pdata->node.panel_node) {
> > + DRM_DEV_ERROR(dev, "fail to get panel node.\n");
> > + return -EPROBE_DEFER;
>
> -EINVAL.
OK
>
> > + }
> > +
> > + of_node_put(pdata->node.panel_node);
> > + out_ep = of_get_child_by_name(pdata->node.panel_node,
> > + "endpoint");
> > + if (!out_ep) {
> > + DRM_DEV_DEBUG_DRIVER(dev, "cannot get endpoint.\n");
>
> DRM_DEV_ERROR seems more appropriate
OK, also I'll remove drm_panel based on Sam comment.
>
> > + return -EPROBE_DEFER;
>
> -EINVAL
OK
>
> > + }
> > +
> > + panel_node = of_graph_get_remote_port_parent(out_ep);
> > + of_node_put(out_ep);
> > + pdata->panel = of_drm_find_panel(panel_node);
> > + DRM_DEV_DEBUG_DRIVER(dev, "get panel node.\n");
> > +
> > + of_node_put(panel_node);
> > + if (IS_ERR_OR_NULL(pdata->panel))
> > + return -EPROBE_DEFER;
>
> of_drm_find_panel cannot return NULL, so, do this instead:
>
> if (IS_ERR(pdata->panel))
>return PTR_ERR(pdata->panel);
>
> (which actually _may_ return EPROBE_DEFER)
I'll remove drm_panel, use panel_bridge.
>
> > +
> > + return 0;
> > +}
> > [snip]
> > +static int anx7625_i2c_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> > + struct anx7625_data *platform;
> > + struct anx7625_platform_data *pdata;
> > + int ret = 0;
> > + struct device *dev = &client->dev;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > +I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > + DRM_DEV_ERROR(dev, "anx7625's i2c bus doesn't support\n");
> > + return -ENODEV;
> > + }
> > +
> > + platform = kzalloc(sizeof(*platform), GFP_KERNEL);
> > + if (!platform) {
> > + DRM_DEV_ERROR(dev, "fail to allocate driver data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pdata = &platform->pdata;
> > +
> > + ret = anx7625_parse_dt(dev, pdata);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "fail to parse devicetree.\n");
>
> Please do not print this error (or at least not if err == -EPROBE_DEFER).
OK
>
> > + goto free_platform;
> > + }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP
On Tue, Jun 9, 2020 at 3:20 PM Xin Ji wrote:
>
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
>
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/Kconfig |9 +
> drivers/gpu/drm/bridge/analogix/Makefile |1 +
> drivers/gpu/drm/bridge/analogix/anx7625.c | 1999
> +
> drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++
> 4 files changed, 2406 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
>
> [snip]
> +static int anx7625_parse_dt(struct device *dev,
> + struct anx7625_platform_data *pdata)
> +{
> + struct device_node *np = dev->of_node;
> + struct device_node *panel_node, *out_ep;
> +
> + pdata->node.mipi_dsi_host_node = of_graph_get_remote_node(np, 0, 0);
> + if (!pdata->node.mipi_dsi_host_node) {
> + DRM_DEV_ERROR(dev, "fail to get internal panel.\n");
> + return -EPROBE_DEFER;
This does not look correct. I don't think of_graph_get_remote_node
will ever return NULL if the device tree is configured properly, and
it's useless to retry later (EPROBE_DEFER). You should just fail (e.g.
return EINVAL).
> + }
> +
> + of_node_put(pdata->node.mipi_dsi_host_node);
You are using pdata->node.mipi_dsi_host_node in other places in the
code, so I don't think it's ok to call of_node_put?
> + DRM_DEV_DEBUG_DRIVER(dev, "found dsi host node.\n");
> +
> + pdata->node.panel_node = of_graph_get_port_by_id(np, 1);
> + if (!pdata->node.panel_node) {
> + DRM_DEV_ERROR(dev, "fail to get panel node.\n");
> + return -EPROBE_DEFER;
-EINVAL.
> + }
> +
> + of_node_put(pdata->node.panel_node);
> + out_ep = of_get_child_by_name(pdata->node.panel_node,
> + "endpoint");
> + if (!out_ep) {
> + DRM_DEV_DEBUG_DRIVER(dev, "cannot get endpoint.\n");
DRM_DEV_ERROR seems more appropriate
> + return -EPROBE_DEFER;
-EINVAL
> + }
> +
> + panel_node = of_graph_get_remote_port_parent(out_ep);
> + of_node_put(out_ep);
> + pdata->panel = of_drm_find_panel(panel_node);
> + DRM_DEV_DEBUG_DRIVER(dev, "get panel node.\n");
> +
> + of_node_put(panel_node);
> + if (IS_ERR_OR_NULL(pdata->panel))
> + return -EPROBE_DEFER;
of_drm_find_panel cannot return NULL, so, do this instead:
if (IS_ERR(pdata->panel))
return PTR_ERR(pdata->panel);
(which actually _may_ return EPROBE_DEFER)
> +
> + return 0;
> +}
> [snip]
> +static int anx7625_i2c_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{
> + struct anx7625_data *platform;
> + struct anx7625_platform_data *pdata;
> + int ret = 0;
> + struct device *dev = &client->dev;
> +
> + if (!i2c_check_functionality(client->adapter,
> +I2C_FUNC_SMBUS_I2C_BLOCK)) {
> + DRM_DEV_ERROR(dev, "anx7625's i2c bus doesn't support\n");
> + return -ENODEV;
> + }
> +
> + platform = kzalloc(sizeof(*platform), GFP_KERNEL);
> + if (!platform) {
> + DRM_DEV_ERROR(dev, "fail to allocate driver data\n");
> + return -ENOMEM;
> + }
> +
> + pdata = &platform->pdata;
> +
> + ret = anx7625_parse_dt(dev, pdata);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "fail to parse devicetree.\n");
Please do not print this error (or at least not if err == -EPROBE_DEFER).
> + goto free_platform;
> + }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP
On Sun, Jun 21, 2020 at 10:09:25AM +0200, Sam Ravnborg wrote:
> Hi Xin.
>
>
> On Tue, Jun 09, 2020 at 03:19:50PM +0800, Xin Ji wrote:
> > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
> >
> > Signed-off-by: Xin Ji
>
> The bridge driver now implements the bridge functions for get_edid +
> detect which is good.
> But the bridge driver also needs to check the flags argument
> of anx7625_bridge_attach() so the connector creation is optional.
> This is required to prepare the bridge driver to fully support a
> chain of bridges and is mandatory for new bridge drivers.
>
> Futhermore the bridge driver needs to be converted to use the bridge
> panel. See what other bridge drivers do.
> In short - the drm_panel shall be replaced by a bridge in your
> platform data structure.
> Usual this end up in less code after the conversion.
>
> Sam
>
Hi Sam, thanks for your comments.
1. I'll add flags argument check in anx7625_bridge_attach().
2. Do you mean panel bridge, related interface is
"devm_drm_panel_bridge_add(...)"? If it is, do I still need call
drm_panel_prepare()/drm_panel_unprepare() to enable/disable panel power?
Thanks,
Xin
> > ---
> > drivers/gpu/drm/bridge/analogix/Kconfig |9 +
> > drivers/gpu/drm/bridge/analogix/Makefile |1 +
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 1999
> > +
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++
> > 4 files changed, 2406 insertions(+)
> > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> > create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
> > b/drivers/gpu/drm/bridge/analogix/Kconfig
> > index e1fa7d8..024ea2a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> > +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> > @@ -25,3 +25,12 @@ config DRM_ANALOGIX_ANX78XX
> > config DRM_ANALOGIX_DP
> > tristate
> > depends on DRM
> > +
> > +config DRM_ANALOGIX_ANX7625
> > + tristate "Analogix Anx7625 MIPI to DP interface support"
> > + depends on DRM
> > + depends on OF
> > + help
> > + ANX7625 is an ultra-low power 4K mobile HD transmitter
> > + designed for portable devices. It converts MIPI/DPI to
> > + DisplayPort1.3 4K.
> > diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
> > b/drivers/gpu/drm/bridge/analogix/Makefile
> > index 97669b3..44da392 100644
> > --- a/drivers/gpu/drm/bridge/analogix/Makefile
> > +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> > @@ -1,5 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o
> > analogix-i2c-dptx.o
> > obj-$(CONFIG_DRM_ANALOGIX_ANX6345) += analogix-anx6345.o
> > +obj-$(CONFIG_DRM_ANALOGIX_ANX7625) += anx7625.o
> > obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > new file mode 100644
> > index 000..db37f68
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -0,0 +1,1999 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright(c) 2020, Analogix Semiconductor. All rights reserved.
> > + *
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include "anx7625.h"
> > +
> > +/*
> > + * There is a sync issue while access I2C register between AP(CPU) and
> > + * internal firmware(OCM), to avoid the race condition, AP should access
> > + * the reserved slave address before slave address occurs changes.
> > + */
> > +static int i2c_access_workaround(struct anx7625_data *ctx,
> > +struct i2c_client *client)
> > +{
> > + u8 offset;
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + if (client == ctx->last_client)
> > + return 0;
> > +
> > + ctx->last_client = client;
> > +
> > + if (client == ctx->i2c.tcpc_client)
> > + offset = RSVD_00_ADDR;
> > + else if (client == ctx->i2c.tx_p0_client)
> > + offset = RSVD_D1_ADDR;
> > + else if (client == ctx->i2c.tx_p1_client)
> > + offset = RSVD_60_ADDR;
> > + else if (client == ctx->i2c.rx_p0_client)
> > + offset = RSVD_39_ADDR;
> > + else if (client == ctx->i2c.rx_p1_client)
> > + offset = RSVD_7F_ADDR;
> > + else
> > + offset = RSVD_00_ADDR;
> > +
> > + ret = i2c_smbus_write_byte_data(client, offset, 0x00);
> > + if (ret < 0)
> > + DRM_DEV_ERROR(dev,
>
Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP
Hi Xin.
On Tue, Jun 09, 2020 at 03:19:50PM +0800, Xin Ji wrote:
> The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
>
> Signed-off-by: Xin Ji
The bridge driver now implements the bridge functions for get_edid +
detect which is good.
But the bridge driver also needs to check the flags argument
of anx7625_bridge_attach() so the connector creation is optional.
This is required to prepare the bridge driver to fully support a
chain of bridges and is mandatory for new bridge drivers.
Futhermore the bridge driver needs to be converted to use the bridge
panel. See what other bridge drivers do.
In short - the drm_panel shall be replaced by a bridge in your
platform data structure.
Usual this end up in less code after the conversion.
Sam
> ---
> drivers/gpu/drm/bridge/analogix/Kconfig |9 +
> drivers/gpu/drm/bridge/analogix/Makefile |1 +
> drivers/gpu/drm/bridge/analogix/anx7625.c | 1999
> +
> drivers/gpu/drm/bridge/analogix/anx7625.h | 397 ++
> 4 files changed, 2406 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
>
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
> b/drivers/gpu/drm/bridge/analogix/Kconfig
> index e1fa7d8..024ea2a 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -25,3 +25,12 @@ config DRM_ANALOGIX_ANX78XX
> config DRM_ANALOGIX_DP
> tristate
> depends on DRM
> +
> +config DRM_ANALOGIX_ANX7625
> + tristate "Analogix Anx7625 MIPI to DP interface support"
> + depends on DRM
> + depends on OF
> + help
> + ANX7625 is an ultra-low power 4K mobile HD transmitter
> + designed for portable devices. It converts MIPI/DPI to
> + DisplayPort1.3 4K.
> diff --git a/drivers/gpu/drm/bridge/analogix/Makefile
> b/drivers/gpu/drm/bridge/analogix/Makefile
> index 97669b3..44da392 100644
> --- a/drivers/gpu/drm/bridge/analogix/Makefile
> +++ b/drivers/gpu/drm/bridge/analogix/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o analogix-i2c-dptx.o
> obj-$(CONFIG_DRM_ANALOGIX_ANX6345) += analogix-anx6345.o
> +obj-$(CONFIG_DRM_ANALOGIX_ANX7625) += anx7625.o
> obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> new file mode 100644
> index 000..db37f68
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -0,0 +1,1999 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright(c) 2020, Analogix Semiconductor. All rights reserved.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include "anx7625.h"
> +
> +/*
> + * There is a sync issue while access I2C register between AP(CPU) and
> + * internal firmware(OCM), to avoid the race condition, AP should access
> + * the reserved slave address before slave address occurs changes.
> + */
> +static int i2c_access_workaround(struct anx7625_data *ctx,
> + struct i2c_client *client)
> +{
> + u8 offset;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + if (client == ctx->last_client)
> + return 0;
> +
> + ctx->last_client = client;
> +
> + if (client == ctx->i2c.tcpc_client)
> + offset = RSVD_00_ADDR;
> + else if (client == ctx->i2c.tx_p0_client)
> + offset = RSVD_D1_ADDR;
> + else if (client == ctx->i2c.tx_p1_client)
> + offset = RSVD_60_ADDR;
> + else if (client == ctx->i2c.rx_p0_client)
> + offset = RSVD_39_ADDR;
> + else if (client == ctx->i2c.rx_p1_client)
> + offset = RSVD_7F_ADDR;
> + else
> + offset = RSVD_00_ADDR;
> +
> + ret = i2c_smbus_write_byte_data(client, offset, 0x00);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev,
> + "fail to access i2c id=%x\n:%x",
> + client->addr, offset);
> +
> + return ret;
> +}
> +
> +static int anx7625_reg_read(struct anx7625_data *ctx,
> + struct i2c_client *client, u8 reg_addr)
> +{
> + int ret;
> + struct device *dev = &client->dev;
> +
> + i2c_access_workaround(ctx, client);
> +
> + ret = i2c_smbus_read_byte_data(client, reg_addr);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev, "read i2c fail id=%x:%x\n",
> + client->addr, reg
