Re: [PATCH 2/5] i2c: gpio: Add support on ACPI-based system

From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Binbin Zhou <zhoubinbin@loongson.cn>
Cc: Wolfram Sang <wsa@kernel.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	linux-i2c@vger.kernel.org, loongarch@lists.linux.dev,
	linux-acpi@vger.kernel.org, WANG Xuerui <kernel@xen0n.name>,
	Jianmin Lv <lvjianmin@loongson.cn>,
	Huacai Chen <chenhuacai@loongson.cn>
Subject: Re: [PATCH 2/5] i2c: gpio: Add support on ACPI-based system
Date: Fri, 23 Sep 2022 13:15:32 +0300	[thread overview]
Message-ID: <Yy2HRPxWPCvda8Hz@black.fi.intel.com> (raw)
In-Reply-To: <73a11791-997b-c2d7-fa0c-3d0431293dae@loongson.cn>

On Fri, Sep 23, 2022 at 06:01:30PM +0800, Binbin Zhou wrote:
> Hi Mika：
> 
> 在 2022/9/22 20:26, Mika Westerberg 写道:
> > Hi,
> > 
> > On Thu, Sep 22, 2022 at 07:39:55PM +0800, Binbin Zhou wrote:
> > > Add support for the ACPI-based device registration so that the driver
> > > can be also enabled through ACPI table.
> > > 
> > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---
> > >   drivers/i2c/busses/i2c-gpio.c | 41 ++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 40 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> > > index b1985c1667e1..ccea37e755e6 100644
> > > --- a/drivers/i2c/busses/i2c-gpio.c
> > > +++ b/drivers/i2c/busses/i2c-gpio.c
> > > @@ -13,6 +13,7 @@
> > >   #include <linux/init.h>
> > >   #include <linux/interrupt.h>
> > >   #include <linux/module.h>
> > > +#include <linux/acpi.h>
> > >   #include <linux/of.h>
> > >   #include <linux/platform_data/i2c-gpio.h>
> > >   #include <linux/platform_device.h>
> > > @@ -318,6 +319,24 @@ static void of_i2c_gpio_get_props(struct device_node *np,
> > >   		of_property_read_bool(np, "i2c-gpio,scl-output-only");
> > >   }
> > > +static void acpi_i2c_gpio_get_props(struct device *dev,
> > > +				  struct i2c_gpio_platform_data *pdata)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	device_property_read_u32(dev, "delay-us", &pdata->udelay);
> > > +
> > > +	if (!device_property_read_u32(dev, "timeout-ms", &reg))
> > > +		pdata->timeout = msecs_to_jiffies(reg);
> > > +
> > > +	pdata->sda_is_open_drain =
> > > +		device_property_read_bool(dev, "sda-open-drain");
> > > +	pdata->scl_is_open_drain =
> > > +		device_property_read_bool(dev, "scl-open-drain");
> > > +	pdata->scl_is_output_only =
> > > +		device_property_read_bool(dev, "scl-output-only");
> > > +}
> > 
> > I think this would work with the DT description too as it is using
> > device_property_xxx() so I wonder if you can just do:
> > 
> > 	i2c_gpio_get_props(dev, pdata);
> > 
> > instead of
> > 
> >   	if (np) {
> >   		of_i2c_gpio_get_props(np, pdata);
> > 	} else if (ACPI_COMPANION(dev)) {
> > 		acpi_i2c_gpio_get_props(dev, pdata);
> > 
> Sorry, I don't quite understand how to do a unified api.
> 
> We get the corresponding value by matching the propname, but obviously the
> propnames related in the two ways are different.
> 
> e.g. "delay-us"(ACPI) vs "i2c-gpio, delay-us"(FDT)

Oh, we have different bindings for these? :( That's unfortunate - they
should really have the same. That's the whole purpose of device
properties API in the first place.