[PATCH 0/4] Define i2c_designware in a header file

Florian Fainelli posted 4 patches 3 weeks ago
There is a newer version of this series
MAINTAINERS                                    | 1 +
drivers/i2c/busses/i2c-designware-pcidrv.c     | 5 +++--
drivers/i2c/busses/i2c-designware-platdrv.c    | 5 +++--
drivers/mfd/intel-lpss.c                       | 3 ++-
drivers/mfd/intel_quark_i2c_gpio.c             | 5 +++--
drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c | 7 ++++---
include/linux/i2c-designware.h                 | 7 +++++++
7 files changed, 23 insertions(+), 10 deletions(-)
create mode 100644 include/linux/i2c-designware.h
[PATCH 0/4] Define i2c_designware in a header file
Posted by Florian Fainelli 3 weeks ago
This patch series depends upon the following two patches being applied:

https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/

There is no reason why each driver should have to repeat the
"i2c_designware" string all over the place, because when that happens we
see the reverts like the above being necessary.

Florian Fainelli (4):
  i2c: designware: Create shared header hosting driver name
  mfd: intel-lpss: Utilize i2c-designware.h
  mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h
  net: txgbe: Utilize i2c-designware.h

 MAINTAINERS                                    | 1 +
 drivers/i2c/busses/i2c-designware-pcidrv.c     | 5 +++--
 drivers/i2c/busses/i2c-designware-platdrv.c    | 5 +++--
 drivers/mfd/intel-lpss.c                       | 3 ++-
 drivers/mfd/intel_quark_i2c_gpio.c             | 5 +++--
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c | 7 ++++---
 include/linux/i2c-designware.h                 | 7 +++++++
 7 files changed, 23 insertions(+), 10 deletions(-)
 create mode 100644 include/linux/i2c-designware.h

-- 
2.34.1
Re: [PATCH 0/4] Define i2c_designware in a header file
Posted by Andy Shevchenko 3 weeks ago
Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
> This patch series depends upon the following two patches being applied:
> 
> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
> https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/
> 
> There is no reason why each driver should have to repeat the
> "i2c_designware" string all over the place, because when that happens we
> see the reverts like the above being necessary.

Isn't that a part of ABI between drivers, i.e. whenever ones want to
request_module() or so they need to know what they are doing, no?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/4] Define i2c_designware in a header file
Posted by Florian Fainelli 3 weeks ago

On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
>> This patch series depends upon the following two patches being applied:
>>
>> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
>> https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/
>>
>> There is no reason why each driver should have to repeat the
>> "i2c_designware" string all over the place, because when that happens we
>> see the reverts like the above being necessary.
> 
> Isn't that a part of ABI between drivers, i.e. whenever ones want to
> request_module() or so they need to know what they are doing, no?

Yes, the drivers should know, but as evidenced by the two patches above, 
there was still room for error. If we have to abide by a certain 
contract, which is platform_driver::driver::name, then we might as well 
have a header defining it no?
-- 
Florian
Re: [PATCH 0/4] Define i2c_designware in a header file
Posted by Andy Shevchenko 2 weeks, 6 days ago
On Tue, Apr 23, 2024 at 06:21:21PM -0700, Florian Fainelli wrote:
> On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
> > Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
> > > This patch series depends upon the following two patches being applied:
> > > 
> > > https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
> > > https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/
> > > 
> > > There is no reason why each driver should have to repeat the
> > > "i2c_designware" string all over the place, because when that happens we
> > > see the reverts like the above being necessary.
> > 
> > Isn't that a part of ABI between drivers, i.e. whenever ones want to
> > request_module() or so they need to know what they are doing, no?
> 
> Yes, the drivers should know, but as evidenced by the two patches above,
> there was still room for error. If we have to abide by a certain contract,
> which is platform_driver::driver::name, then we might as well have a header
> defining it no?

Maybe, I simply don't like the manipulations with parts of the device instance
names / driver IDs / driver name, which all become mixed after this series.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH 0/4] Define i2c_designware in a header file
Posted by Florian Fainelli 2 weeks, 6 days ago
On 4/24/24 07:26, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 06:21:21PM -0700, Florian Fainelli wrote:
>> On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
>>> Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
>>>> This patch series depends upon the following two patches being applied:
>>>>
>>>> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
>>>> https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/
>>>>
>>>> There is no reason why each driver should have to repeat the
>>>> "i2c_designware" string all over the place, because when that happens we
>>>> see the reverts like the above being necessary.
>>>
>>> Isn't that a part of ABI between drivers, i.e. whenever ones want to
>>> request_module() or so they need to know what they are doing, no?
>>
>> Yes, the drivers should know, but as evidenced by the two patches above,
>> there was still room for error. If we have to abide by a certain contract,
>> which is platform_driver::driver::name, then we might as well have a header
>> defining it no?
> 
> Maybe, I simply don't like the manipulations with parts of the device instance
> names / driver IDs / driver name, which all become mixed after this series.
> 

That is fair enough, although there is definitively an expectation that 
the clock name will match the dev_name() of the i2c bus, which is why it 
changing or shortening the names as attempted by Duanqiang ended up not 
working.

This call in i2c-designware-platdrv.c is:

dev->clk = devm_clk_get_optional(&pdev->dev, NULL);

and because the name is NULL, we end-up searching for a clock named 
dev_name(), and if we have a mismatch, then we won't find it.

So the i2c_designware name is really something we must be sticking with, 
or change *globally* for a) device(s) binding to the driver and b) a 
successful clock search.
-- 
Florian