| 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: <61f1dc2f-84f8-4f04-8348-7a4470a1276c@linux.dev>
Date: Fri, 17 Nov 2023 01:18:49 +0800
From: Sui Jingfeng <sui.jingfeng@...ux.dev>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Phong LE <ple@...libre.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
Maxime Ripard <mripard@...nel.org>,
Sui Jingfeng <suijingfeng@...ngson.cn>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Thomas Zimmermann <tzimmermann@...e.de>
Subject: Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib
On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@...ux.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@...ux.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?
Powered by blists - more mailing lists