On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> 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@xxxxxxxxx> > >> 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. > > > > > >>> 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! >From my point of view: no. -- With best wishes Dmitry