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

[Binbin Zhou <zhoubinbin@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > ---
> >   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)

I think the judgment of "if..else.." is indispensable.

thanks.

Binbin

> > +
> >   static struct gpio_desc *i2c_gpio_get_desc(struct device *dev,
> >   					   const char *con_id,
> >   					   unsigned int index,
> > @@ -363,6 +382,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
> >   	struct device *dev = &pdev->dev;
> >   	struct device_node *np = dev->of_node;
> >   	enum gpiod_flags gflags;
> > +	acpi_status status;
> > +	unsigned long long id;
> >   	int ret;
> >   
> >   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > @@ -375,6 +396,8 @@ static int i2c_gpio_probe(struct platform_device *pdev)
> >   
> >   	if (np) {
> >   		of_i2c_gpio_get_props(np, pdata);
> > +	} else if (ACPI_COMPANION(dev)) {
> > +		acpi_i2c_gpio_get_props(dev, pdata);
> >   	} else {
> >   		/*
> >   		 * If all platform data settings are zero it is OK
> > @@ -445,7 +468,14 @@ static int i2c_gpio_probe(struct platform_device *pdev)
> >   	adap->dev.parent = dev;
> >   	adap->dev.of_node = np;
> >   
> > -	adap->nr = pdev->id;
> > +	if (ACPI_COMPANION(dev)) {
> > +		status = acpi_evaluate_integer(ACPI_HANDLE(dev),
> > +						"_UID", NULL, &id);
> > +		if (ACPI_SUCCESS(status) && (id >= 0))
> > +			adap->nr = id;
>
> Unrelated change? And if not then same comment about why you need the
> static number in the first place ;-)