diff mbox series

[v4,7/9] drm-bridge: it66121: Use fwnode API to acquire device properties

Message ID 20240422191903.255642-8-sui.jingfeng@linux.dev (mailing list archive)
State New, archived
Headers show
Series drm/bridge: Allow using fwnode API to get the next bridge | expand

Commit Message

Sui Jingfeng April 22, 2024, 7:19 p.m. UTC 
Make this driver DT-independent by calling the freshly created helpers,
which reduce boilerplate and open the door for otherwise use cases. No
functional changes for DT based systems.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 57 +++++++++++++++++-----------
 1 file changed, 35 insertions(+), 22 deletions(-)

Comments

Dmitry Baryshkov April 22, 2024, 8:06 p.m. UTC | #1 
On Tue, Apr 23, 2024 at 03:19:01AM +0800, Sui Jingfeng wrote:
> Make this driver DT-independent by calling the freshly created helpers,
> which reduce boilerplate and open the door for otherwise use cases. No
> functional changes for DT based systems.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 57 +++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 925e42f46cd8..688dc1830654 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,36 @@ static const char * const it66121_supplies[] = {
>  	"vcn33", "vcn18", "vrf12"
>  };
>  
> +static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 *bus_width)
> +{
> +	struct fwnode_handle *endpoint;
> +	u32 val;
> +	int ret;
> +
> +	endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 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;
> +}

Ideally this should come as two patches. First patch extracts the
function, second patch changes the driver to use fwnode / property API.

> +
>  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,29 +1542,20 @@ 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;
> -	}
> +	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
> +	if (ret)
> +		return ret;
>  
> -	ctx->next_bridge = of_drm_find_bridge(ep);
> -	of_node_put(ep);
> -	if (!ctx->next_bridge) {
> +	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> +	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;

return dev_err_probe(dev, ret, "msg"), if your function doesn't do this.
If it does, just return ret.

> +	} else if (!ctx->next_bridge) {
>  		dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;
>  	}
> @@ -1577,8 +1590,8 @@ static int it66121_probe(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> +	drm_bridge_set_node(&ctx->bridge, fwnode);
>  	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;
>  	if (client->irq > 0) {
> -- 
> 2.34.1
>
Sui Jingfeng April 27, 2024, 1:11 p.m. UTC | #2 
Hi,


On 2024/4/23 04:06, Dmitry Baryshkov wrote:
>> +
>>   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,29 +1542,20 @@ 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;
>> -	}
>> +	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
>> +	if (ret)
>> +		return ret;
>>   
>> -	ctx->next_bridge = of_drm_find_bridge(ep);
>> -	of_node_put(ep);
>> -	if (!ctx->next_bridge) {
>> +	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>> +	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;
> return dev_err_probe(dev, ret, "msg"), if your function doesn't do this.
> If it does, just return ret.


My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER
this is known for sure. As a//prior(priori) knowledge. Calling the dev_err_probe()
just introduce extra overhead. It is useless to use dev_err_probe() here.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 925e42f46cd8..688dc1830654 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,36 @@  static const char * const it66121_supplies[] = {
 	"vcn33", "vcn18", "vrf12"
 };
 
+static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 *bus_width)
+{
+	struct fwnode_handle *endpoint;
+	u32 val;
+	int ret;
+
+	endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 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,29 +1542,20 @@  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;
-	}
+	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
+	if (ret)
+		return ret;
 
-	ctx->next_bridge = of_drm_find_bridge(ep);
-	of_node_put(ep);
-	if (!ctx->next_bridge) {
+	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+	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;
+	} else if (!ctx->next_bridge) {
 		dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
@@ -1577,8 +1590,8 @@  static int it66121_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	drm_bridge_set_node(&ctx->bridge, fwnode);
 	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;
 	if (client->irq > 0) {