Received-SPF: pass (google.com: domain of linux-kernel+bounces-96109-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240307172334.1753343-1-sui.jingfeng@linux.dev> <20240307172334.1753343-5-sui.jingfeng@linux.dev>
In-Reply-To: <20240307172334.1753343-5-sui.jingfeng@linux.dev>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Thu, 7 Mar 2024 21:31:06 +0200
Message-ID: <CAA8EJpqtPRfe1VL_ACYEOSq=iNMkZ03-fwVv3XdVrpTObZFu1w@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] drm-bridge: it66121: Use fwnode API to acquire
 device properties
To: Sui Jingfeng <sui.jingfeng@linux.dev>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>, Neil Armstrong <neil.armstrong@linaro.org>, 
	Robert Foss <rfoss@kernel.org>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, 
	Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@gmail.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
	Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, 
	Phong LE <ple@baylibre.com>, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Thu, 7 Mar 2024 at 19:24, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>
> Make this driver less DT-dependent by calling the freshly created helpers,
> should be no functional changes for DT based systems. But open the door for
> otherwise use cases. Even though there is no user emerged yet, this still
> do no harms. In fact, we reduce some boilerplate across drm bridge drivers.
>
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 63 ++++++++++++++++------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 1c3433b5e366..a2cf2be86065 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -15,7 +15,6 @@
>  #include <linux/bitfield.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> -#include <linux/of_graph.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/regulator/consumer.h>
> @@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
>
>         dev_dbg(dev, "%s\n", __func__);
>
> -       if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
> +       if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
>                 dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
>                 return 0;
>         }
> @@ -1503,13 +1502,37 @@ static const char * const it66121_supplies[] = {
>         "vcn33", "vcn18", "vrf12"
>  };
>
> +static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 port,
> +                                 u32 *bus_width)
> +{
> +       struct fwnode_handle *endpoint;
> +       u32 val;
> +       int ret;
> +
> +       endpoint = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
> +       if (!endpoint)
> +               return -EINVAL;
> +
> +       ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
> +       fwnode_handle_put(endpoint);
> +       if (ret)
> +               return ret;
> +
> +       if (val != 12 && val != 24)
> +               return -EINVAL;
> +
> +       *bus_width = val;
> +
> +       return 0;
> +}
> +
>  static int it66121_probe(struct i2c_client *client)
>  {
>         u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> -       struct device_node *ep;
>         int ret;
>         struct it66121_ctx *ctx;
>         struct device *dev = &client->dev;
> +       struct fwnode_handle *fwnode = dev_fwnode(dev);
>
>         if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>                 dev_err(dev, "I2C check functionality failed.\n");
> @@ -1520,37 +1543,23 @@ static int it66121_probe(struct i2c_client *client)
>         if (!ctx)
>                 return -ENOMEM;
>
> -       ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> -       if (!ep)
> -               return -EINVAL;
> -
>         ctx->dev = dev;
>         ctx->client = client;
>         ctx->info = i2c_get_match_data(client);
>
> -       of_property_read_u32(ep, "bus-width", &ctx->bus_width);
> -       of_node_put(ep);
> -
> -       if (ctx->bus_width != 12 && ctx->bus_width != 24)
> -               return -EINVAL;
> -
> -       ep = of_graph_get_remote_node(dev->of_node, 1, -1);
> -       if (!ep) {
> -               dev_err(ctx->dev, "The endpoint is unconnected\n");
> -               return -EINVAL;
> -       }
> -
> -       if (!of_device_is_available(ep)) {
> -               of_node_put(ep);
> -               dev_err(ctx->dev, "The remote device is disabled\n");
> -               return -ENODEV;
> -       }
> +       /* Endpoint of port@0 contains the bus-width property */
> +       ret = it66121_read_bus_width(fwnode, 0, &ctx->bus_width);

There is no need to pass port as an argument to that function.


> +       if (ret)
> +               return ret;
>
> -       ctx->next_bridge = of_drm_find_bridge(ep);
> -       of_node_put(ep);
> +       ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>         if (!ctx->next_bridge) {
>                 dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
>                 return -EPROBE_DEFER;
> +       } else if (IS_ERR(ctx->next_bridge)) {
> +               ret = PTR_ERR(ctx->next_bridge);
> +               dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> +               return ret;

Nit: I'd usually expect this part to be in a different order: first
check for error, then check for absence. But that's a minor thing.

>         }
>
>         i2c_set_clientdata(client, ctx);
> @@ -1584,9 +1593,9 @@ static int it66121_probe(struct i2c_client *client)
>         }
>
>         ctx->bridge.funcs = &it66121_bridge_funcs;
> -       ctx->bridge.of_node = dev->of_node;
>         ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>         ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
> +       drm_bridge_set_node(&ctx->bridge, fwnode);
>
>         ret = devm_request_threaded_irq(dev, client->irq, NULL, it66121_irq_threaded_handler,
>                                         IRQF_ONESHOT, dev_name(dev), ctx);
> --
> 2.34.1
>


--
With best wishes
Dmitry