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. 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.
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?
On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <[email protected]> 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.
> 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).
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. 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.
--
With best wishes
Dmitry
Hi,
On 2023/11/16 19:29, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 13:18, Sui Jingfeng <[email protected]> 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.
> 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!
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 <[email protected]>
>> 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!
>
>
On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <[email protected]> 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 <[email protected]>
> >> 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
On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <[email protected]> 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 <[email protected]>
>>>> 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?
Hi,
On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>> 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.
I respect the fact that the community prefer generic mechanisms.
If our approach is not what the community want, can I switch back
to my previous solution? I can reduce the duplication of our
localized it66121 driver to a minimal, rewrite it until it meets
the community's requirement. I know our device looks weird and
our approach is not elegant. But at the very least, we could not
mess the community's design up by localize. Otherwise, I don't know
what is the better approach to solve such a problem.
Can I switch back or any other ideas?
On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> >>>> 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.
>
>
> I respect the fact that the community prefer generic mechanisms.
> If our approach is not what the community want, can I switch back
> to my previous solution? I can reduce the duplication of our
> localized it66121 driver to a minimal, rewrite it until it meets
> the community's requirement. I know our device looks weird and
> our approach is not elegant. But at the very least, we could not
> mess the community's design up by localize. Otherwise, I don't know
> what is the better approach to solve such a problem.
>
> Can I switch back or any other ideas?
I keep on repeating: create the i2c device from your root device
driver, which parses BIOS data.
--
With best wishes
Dmitry
On Fri, Nov 17, 2023 at 01:18:49AM +0800, Sui Jingfeng wrote:
>
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 14:08, Sui Jingfeng <[email protected]> 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 <[email protected]>
> > > > > 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).
To expand further on that, I guess one of the key difference is that you
don't really expect to interact with the EEPROM, you'll only read it,
which is fairly different from your bridge.
And if someone wanted to instatiate nvmem devices for the various
EEPROMs in the monitor, I would very much welcome that change.
Maxime
On Fri, Nov 17, 2023 at 12:24:22PM +0800, Sui Jingfeng wrote:
> Hi,
>
> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> > > > > 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.
>
> I respect the fact that the community prefer generic mechanisms.
> If our approach is not what the community want, can I switch back
> to my previous solution?
By your previous solution, you mean rolling your own bridge driver? If
so, then no, it's not acceptable either.
> I can reduce the duplication of our localized it66121 driver to a
> minimal, rewrite it until it meets the community's requirement. I know
> our device looks weird and our approach is not elegant.
I'm glad we agree then :)
> But at the very least, we could not mess the community's design up by
> localize. Otherwise, I don't know what is the better approach to solve
> such a problem.
I think there's a gap between what we want from you and what you want
from us.
What we really care about is maintenance. In other words, it's mostly
about two things:
- Once you and/or your company have moved on to other things, how easy
it will be for us to keep that driver in good shape, and how much it
will hold back any future development.
- If we want to do a big rework, how much your driver will stand in
the way.
That's pretty much all that we care about, and we will very much prefer
not to merge a driver in the first place than to have to maintain it for
10y while it stands in our way and we don't have any real documentation
or help.
So by making it "not weird" or "elegant" or whatever we can call it, you
effectively remove any concern we might have about merging your driver,
and there's only an upside (more hardware support and company
involvement is good!). So you're making it easy for you too.
Maxime
Hi,
On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>> 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.
>>
>> I respect the fact that the community prefer generic mechanisms.
>> If our approach is not what the community want, can I switch back
>> to my previous solution? I can reduce the duplication of our
>> localized it66121 driver to a minimal, rewrite it until it meets
>> the community's requirement. I know our device looks weird and
>> our approach is not elegant. But at the very least, we could not
>> mess the community's design up by localize. Otherwise, I don't know
>> what is the better approach to solve such a problem.
>>
>> Can I switch back or any other ideas?
> I keep on repeating: create the i2c device from your root device
> driver, which parses BIOS data.
>
This is not my own problems, currently it66121 (but not only) display bridge driver
don't works on X86 either. What we are trying to do is to provide a generic, non-platform
dependent solution. It is not only relevant to my driver. In fact, this series made
no assumption which hardware/display controller will be the user.
I have investigated before respin this patch, there are other hardwares which
ship the it66121 display bridge. For example, the Fresco Logic FL2000dx USB 3.0
to VGA display adapter[1][2]. Even the windows have a driver.
[1] https://github.com/FrescoLogic/FL2000
[2] https://oemdrivers.com/graphics-fresco-logic-fl2000
Hi,
On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <[email protected]> wrote:
>> Hi,
>>
>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>> 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.
>>
>> I respect the fact that the community prefer generic mechanisms.
>> If our approach is not what the community want, can I switch back
>> to my previous solution? I can reduce the duplication of our
>> localized it66121 driver to a minimal, rewrite it until it meets
>> the community's requirement. I know our device looks weird and
>> our approach is not elegant. But at the very least, we could not
>> mess the community's design up by localize. Otherwise, I don't know
>> what is the better approach to solve such a problem.
>>
>> Can I switch back or any other ideas?
> I keep on repeating: create the i2c device from your root device
> driver, which parses BIOS data.
You didn't focus on solve the problem, You are focus on solving me.
How does the method that parsing BIOS data can be generic and applied
universally?
On 17/11/2023 18:14, Sui Jingfeng wrote:
> Hi,
>
> On 2023/11/17 17:03, Dmitry Baryshkov wrote:
>> On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <[email protected]> wrote:
>>> Hi,
>>>
>>> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
>>>>>>> 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.
>>>
>>> I respect the fact that the community prefer generic mechanisms.
>>> If our approach is not what the community want, can I switch back
>>> to my previous solution? I can reduce the duplication of our
>>> localized it66121 driver to a minimal, rewrite it until it meets
>>> the community's requirement. I know our device looks weird and
>>> our approach is not elegant. But at the very least, we could not
>>> mess the community's design up by localize. Otherwise, I don't know
>>> what is the better approach to solve such a problem.
>>>
>>> Can I switch back or any other ideas?
>> I keep on repeating: create the i2c device from your root device
>> driver, which parses BIOS data.
>>
> This is not my own problems, currently it66121 (but not only) display bridge driver
> don't works on X86 either. What we are trying to do is to provide a generic, non-platform
> dependent solution. It is not only relevant to my driver. In fact, this series made
> no assumption which hardware/display controller will be the user.
>
> I have investigated before respin this patch, there are other hardwares which
> ship the it66121 display bridge. For example, the Fresco Logic FL2000dx USB 3.0
> to VGA display adapter[1][2]. Even the windows have a driver.
>
> [1] https://github.com/FrescoLogic/FL2000
> [2] https://oemdrivers.com/graphics-fresco-logic-fl2000
Switching to fwnodes, registering an i2c bus and generating fwnode data matching the
interconnect architecture is the way.
DRM Bridge transition to fwnode only should be done first, this will open bridge
to any architecture and device description (DT or ACPI).
Neil
>
On Fri, 17 Nov 2023 at 19:36, Sui Jingfeng <[email protected]> wrote:
>
> Hi,
>
>
> On 2023/11/17 17:03, Dmitry Baryshkov wrote:
> > On Fri, 17 Nov 2023 at 06:24, Sui Jingfeng <[email protected]> wrote:
> >> Hi,
> >>
> >> On 2023/11/16 23:23, Dmitry Baryshkov wrote:
> >>>>>> 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.
> >>
> >> I respect the fact that the community prefer generic mechanisms.
> >> If our approach is not what the community want, can I switch back
> >> to my previous solution? I can reduce the duplication of our
> >> localized it66121 driver to a minimal, rewrite it until it meets
> >> the community's requirement. I know our device looks weird and
> >> our approach is not elegant. But at the very least, we could not
> >> mess the community's design up by localize. Otherwise, I don't know
> >> what is the better approach to solve such a problem.
> >>
> >> Can I switch back or any other ideas?
> > I keep on repeating: create the i2c device from your root device
> > driver, which parses BIOS data.
>
>
> You didn't focus on solve the problem, You are focus on solving me.
> How does the method that parsing BIOS data can be generic and applied
> universally?
Parsing BIOS data is unique to your platform (as well as your BIOS
tables). However using and extending (instead of replacing it just for
your platform) is a generic item.
--
With best wishes
Dmitry