Re: [PATCH net-next v8 2/9] i2c: designware: Add driver support for Wangxun 10Gb NIC

[Jarkko Nikula <jarkko.nikula@linux.intel.com>]

On 5/17/23 12:44, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 12:26 PM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>> On Wednesday, May 17, 2023 4:49 PM, Jarkko Nikula wrote:
>>> On 5/15/23 12:24, andy.shevchenko@gmail.com wrote:
>>>> Mon, May 15, 2023 at 02:31:53PM +0800, Jiawen Wu kirjoitti:
>>>>>     dev->flags = (uintptr_t)device_get_match_data(&pdev->dev);
>>>>> +  if (device_property_present(&pdev->dev, "snps,i2c-platform"))
>>>>> +          dev->flags |= MODEL_WANGXUN_SP;
>>>>
>>>> What I meant here is to use device_property_present() _iff_ you have decided to
>>>> go with the _vendor-specific_ property name.
>>>>
>>>> Otherwise it should be handled differently, i.e. with reading the actual value
>>>> of that property. Hence it should correspond the model enum, which you need to
>>>> declare in the Device Tree bindings before use.
>>>>
>>>> So, either
>>>>
>>>>      if (device_property_present(&pdev->dev, "wx,..."))
>>>>              dev->flags |= MODEL_WANGXUN_SP;
>>>>
>>>> or
>>>>
>>>>      if ((dev->flags & MODEL_MASK) == MODEL_NONE) {
>>>>      // you now have to distinguish that there is no model set in driver data
>>>>              u32 model;
>>>>
>>>>              ret = device_property_read_u32(dev, "snps,i2c-platform");
>>>>              if (ret) {
>>>>                      ...handle error...
>>>>              }
>>>>              dev->flags |= model
>>>>
>>> I'm not a device tree expert but I wonder would it be possible somehow
>>> combine this and compatible properties in dw_i2c_of_match[]? They set
>>> model flag for MODEL_MSCC_OCELOT and MODEL_BAIKAL_BT1.
>>
>> Maybe the table could be changed to match device property, instead of relying
>> on DT only. Or device_get_match_data() could be also implemented in
>> software node case?
> 
> This has been discussed [1] and still no visible prototype. Perhaps
> you can collaborate with Vladimir on the matter.
> 
> [1]: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
> 
Ok, not possible at the moment. Perhaps for now setting model using 
device_property_read_u32() is good enough? I asked above out of 
curiosity rather than demanding perfection. They say "Perfect is the 
enemy of good" :-)