Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:19:02AM +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@xxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c7bef5c23927..58dc7492844f 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
>  
>  static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
>  	struct drm_bridge_timings *timings = &dvi->timings;
> -	struct device_node *ep;
> +	struct fwnode_handle *ep;
>  	u32 pclk_sample = 0;
>  	u32 bus_width = 24;
>  	u32 deskew = 0;
> @@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	 * and EDGE pins. They are specified in DT through endpoint properties
>  	 * and vendor-specific properties.
>  	 */
> -	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
>  	if (!ep)
>  		return -EINVAL;
>  
>  	/* Get the sampling edge from the endpoint. */
> -	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
> -	of_property_read_u32(ep, "bus-width", &bus_width);
> -	of_node_put(ep);
> +	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
> +	fwnode_property_read_u32(ep, "bus-width", &bus_width);
> +	fwnode_handle_put(ep);
>  
>  	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
>  
> @@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	}
>  
>  	/* Get the setup and hold time from vendor-specific properties. */
> -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
> +	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
>  	if (deskew > 7)
>  		return -EINVAL;
>  
> @@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  
>  static int tfp410_init(struct device *dev, bool i2c)
>  {
> -	struct device_node *node;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct tfp410 *dvi;
>  	int ret;
>  
> -	if (!dev->of_node) {
> -		dev_err(dev, "device-tree data is missing\n");
> +	if (!fwnode) {
> +		dev_err(dev, "firmware data is missing\n");
>  		return -ENXIO;
>  	}
>  
> @@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
>  	dvi->dev = dev;
>  	dev_set_drvdata(dev, dvi);
>  
> +	drm_bridge_set_node(&dvi->bridge, fwnode);
>  	dvi->bridge.funcs = &tfp410_bridge_funcs;
> -	dvi->bridge.of_node = dev->of_node;
>  	dvi->bridge.timings = &dvi->timings;
>  	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
>  
> @@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
>  		return ret;
>  
>  	/* Get the next bridge, connected to port@1. */
> -	node = of_graph_get_remote_node(dev->of_node, 1, -1);
> -	if (!node)
> -		return -ENODEV;
> -
> -	dvi->next_bridge = of_drm_find_bridge(node);
> -	of_node_put(node);
> -
> -	if (!dvi->next_bridge)
> +	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> +	if (IS_ERR(dvi->next_bridge)) {
> +		ret = PTR_ERR(dvi->next_bridge);
> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> +		return ret;

Same comment regarding dev_err_probe().

LGTM otherwise.

> +	} else if (!dvi->next_bridge) {
> +		dev_dbg(dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;

Looking at the bolerplate code, I think it would be better to make
drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.


> +	}
>  
>  	/* Get the powerdown GPIO. */
>  	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> @@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
>  /* There is currently no i2c functionality. */
>  static int tfp410_i2c_probe(struct i2c_client *client)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
>  	int reg;
>  
> -	if (!client->dev.of_node ||
> -	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
> +	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
>  		dev_err(&client->dev,
>  			"Can't get i2c reg property from device-tree\n");
>  		return -ENXIO;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry