Messages in this thread | | | Date | Fri, 17 Nov 2023 01:18:49 +0800 | Subject | Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib | From | Sui Jingfeng <> |
| |
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?
| |