Re: [PATCH v2 4/4] drm-bridge: it66121: Use fwnode API to acquire device properties

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,


On 2024/3/8 03:31, Dmitry Baryshkov wrote:
> 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.
>
>
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.