| 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: <b9eacd91-8d6f-4265-931e-bc31cadd54d4@linux.dev>
Date: Thu, 16 Nov 2023 20:07:59 +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 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.
>
>
>>> so I still think It is best to make this drivers functional as a
>>> static lib, but I want
>>> to hear you to say more. Why it would be a *better* alternative to
>>> turn OF calls into
>>> fwnode calls? what are the potential benefits?
>> Because then you can populate device properties from your root device.
>> Because it allows the platform to specify the bus width instead of
>> hardcoding 24 bits (which might work in your case, but might not be
>> applicable to another user next week).
>
>
> No, this problem can be easily solved. Simply add another argument.
>
> ```
>
> int it66121_create_bridge(struct i2c_client *client, bool of_support,
> bool hpd_support, bool audio_support, u32
> bus_width,
> struct drm_bridge **bridge);
> ```
>
>
>> Anyway, even without fwnode, I'd strongly suggest you to drop the
>> it66121_create_bridge() as it is now and start by populating the i2c
>> bus from your root device.
>
> This will force all non-DT users to add the similar code patter at the
> display controller side,
> which is another kind of duplication. The monitor is also as I2C slave
> device, can be abstract
> as a identify drm bridges in theory, I guess.
>
'identify' -> 'identity'
>
>> Then you will need some way (fwnode?) to
>> discover the bridge chain. And at the last point you will get into the
>> device data and/or properties business.
>>
> No, leave that chance to a more better programmer and forgive me please,
> too difficult, I'm afraid of not able to solve. Thanks a lot for the
> trust!
>
>
Powered by blists - more mailing lists