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

[Binbin Zhou <zhoubinbin@loongson.cn>]

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)

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 ;-)