Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

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

On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@linux.dev> wrote:
>>
>> On 2023/11/16 19:53, Sui Jingfeng wrote:
>>> Hi,
>>>
>>>
>>> On 2023/11/16 19:29, Dmitry Baryshkov wrote:
>>>> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <sui.jingfeng@linux.dev>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
>>>>>>> +
>>>>>>> +               ctx->connector = connector;
>>>>>>> +       }
>>>>>>>
>>>>>>>            if (ctx->info->id == ID_IT66121) {
>>>>>>>                    ret = regmap_write_bits(ctx->regmap,
>>>>>>> IT66121_CLK_BANK_REG,
>>>>>>> @@ -1632,16 +1651,13 @@ static const char * const
>>>>>>> it66121_supplies[] = {
>>>>>>>            "vcn33", "vcn18", "vrf12"
>>>>>>>     };
>>>>>>>
>>>>>>> -static int it66121_probe(struct i2c_client *client)
>>>>>>> +int it66121_create_bridge(struct i2c_client *client, bool
>>>>>>> of_support,
>>>>>>> +                         bool hpd_support, bool audio_support,
>>>>>>> +                         struct drm_bridge **bridge)
>>>>>>>     {
>>>>>>> +       struct device *dev = &client->dev;
>>>>>>>            int ret;
>>>>>>>            struct it66121_ctx *ctx;
>>>>>>> -       struct device *dev = &client->dev;
>>>>>>> -
>>>>>>> -       if (!i2c_check_functionality(client->adapter,
>>>>>>> I2C_FUNC_I2C)) {
>>>>>>> -               dev_err(dev, "I2C check functionality failed.\n");
>>>>>>> -               return -ENXIO;
>>>>>>> -       }
>>>>>>>
>>>>>>>            ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>>>>>>            if (!ctx)
>>>>>>> @@ -1649,24 +1665,19 @@ static int it66121_probe(struct i2c_client
>>>>>>> *client)
>>>>>>>
>>>>>>>            ctx->dev = dev;
>>>>>>>            ctx->client = client;
>>>>>>> -       ctx->info = i2c_get_match_data(client);
>>>>>>> -
>>>>>>> -       ret = it66121_of_read_bus_width(dev, &ctx->bus_width);
>>>>>>> -       if (ret)
>>>>>>> -               return ret;
>>>>>>> -
>>>>>>> -       ret = it66121_of_get_next_bridge(dev, &ctx->next_bridge);
>>>>>>> -       if (ret)
>>>>>>> -               return ret;
>>>>>>> -
>>>>>>> -       i2c_set_clientdata(client, ctx);
>>>>>>>            mutex_init(&ctx->lock);
>>>>>>>
>>>>>>> -       ret = devm_regulator_bulk_get_enable(dev,
>>>>>>> ARRAY_SIZE(it66121_supplies),
>>>>>>> - it66121_supplies);
>>>>>>> -       if (ret) {
>>>>>>> -               dev_err(dev, "Failed to enable power supplies\n");
>>>>>>> -               return ret;
>>>>>>> +       if (of_support) {
>>>>>>> +               ret = it66121_of_read_bus_width(dev,
>>>>>>> &ctx->bus_width);
>>>>>>> +               if (ret)
>>>>>>> +                       return ret;
>>>>>>> +
>>>>>>> +               ret = it66121_of_get_next_bridge(dev,
>>>>>>> &ctx->next_bridge);
>>>>>>> +               if (ret)
>>>>>>> +                       return ret;
>>>>>>> +       } else {
>>>>>>> +               ctx->bus_width = 24;
>>>>>>> +               ctx->next_bridge = NULL;
>>>>>>>            }
>>>>>> A better alternative would be to turn OF calls into fwnode calls and
>>>>>> to populate the fwnode properties. See
>>>>>> drivers/platform/x86/intel/chtwc_int33fe.c for example.
>>>>> Honestly, I don't want to leave any scratch(breadcrumbs).
>>>>> I'm worries about that turn OF calls into fwnode calls will leave
>>>>> something unwanted.
>>>>>
>>>>> Because I am not sure if fwnode calls will make sense in the DT
>>>>> world, while my patch
>>>>> *still* be useful in the DT world.
>>>> fwnode calls work for both DT and non-DT cases. In the DT case they
>>>> work with DT nodes and properties. In the non-DT case, they work with
>>>> manually populated properties.
>>>>
>>>>> Because the newly introduced it66121_create_bridge()
>>>>> function is a core. I think It's better leave this task to a more
>>>>> advance programmer.
>>>>> if there have use case. It can be introduced at a latter time,
>>>>> probably parallel with
>>>>> the DT.
>>>>>
>>>>> I think DT and/or ACPI is best for integrated devices, but it66121
>>>>> display bridges is
>>>>> a i2c slave device. Personally, I think slave device shouldn't be
>>>>> standalone. I'm more
>>>>> prefer to turn this driver to support hot-plug, even remove the
>>>>> device on the run time
>>>>> freely when detach and allow reattach. Like the I2C EEPROM device in
>>>>> the monitor (which
>>>>> contains the EDID, with I2C slave address 0x50). The I2C EEPROM
>>>>> device *also* don't has
>>>>> a corresponding struct device representation in linux kernel.
>>>> It has. See i2c_client::dev.
>>> No, what I mean is that there don't have a device driver for
>>> monitor(display) hardware entity.
>>> And the drm_do_probe_ddc_edid() is the static linked driver, which is
>>> similar with the idea
>>> this series want to express.
> Because the monitor is not a part of the display pipeline.
>
I think the monitor *is definitely* part of the display pipeline, and it
is the most important part of the entire display pipeline.

1)

DPMS, self-refreshing, display timings, resolutions supported, HDR, DSC,
gsync and freesync etc can be part of whole mode-set. Please consider
what the various ->mode_valid() and -> the atomic_check() are for?

2)

If the monitor is not a part of the display pipeline, then the various
display panels hardware should also not be part of the display pipeline.
Because they are all belong to display category.
  
the monitor = panel + panel drive IC(such as RTD2281CL, HT1622, ssd130x).

There are panel bridges which abstract the panel + connector as a drm bridge,
why the bare panel can be part of the display pipeline, while the more complex
monitor can't be?