| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9edaecbc-e6be-4518-b110-a6a574df967a@linux.dev>
Date: Fri, 8 Mar 2024 03:39:43 +0800
From: Sui Jingfeng <sui.jingfeng@...ux.dev>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Phong LE <ple@...libre.com>, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] drm-bridge: it66121: Use fwnode API to acquire
device properties
Hi,
On 2024/3/8 03:31, Dmitry Baryshkov wrote:
> On Thu, 7 Mar 2024 at 19:24, Sui Jingfeng <sui.jingfeng@...ux.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@...ux.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.
>
>
Yeah, extremely correct. Because the bus width property should always
located at the endpoint of the input port(port@0 for it66121).
>> + 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.
OK, fine, will be fixed at the next version if no other objects.
Powered by blists - more mailing lists