linux-kernel - Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFPB+Ydw4Ywo4dt5NB-Aed6gtt89h2kMucXtgJa42vwommZerw@mail.gmail.com>
Date:	Wed, 20 May 2015 16:07:00 +0300
From:	Robert Dolca <robert.dolca@...il.com>
To:	Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:	Robert Dolca <robert.dolca@...el.com>, linux-i2c@...r.kernel.org,
	linux-acpi@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Wolfram Sang <wsa@...-dreams.de>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <lenb@...nel.org>,
	Daniel Baluta <daniel.baluta@...el.com>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH RFC] i2c: Use ID table to detect ACPI I2C devices

On Wed, May 20, 2015 at 1:57 PM, Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> On Wed, May 20, 2015 at 01:49:02PM +0300, Robert Dolca wrote:
>> On Wed, May 20, 2015 at 12:48 PM, Mika Westerberg wrote:
>> > On Wed, May 20, 2015 at 12:39:22PM +0300, Robert Dolca wrote:
>> >> Currently, if the name used for DT (in dts) matches one of the names
>> >> specified in the id table you will have a match. Isn't that an
>> >> intended behavior?
>> >
>> > I thought one needs to put IDs to the driver .of_match_table. This is
>> > also what i2c_device_match() is expecting, if I read it right.
>>
>> If you put the DT id in of_match_table it will match here:
>>
>> i2c_device_match
>>         /* Attempt an OF style match */
>>         if (of_driver_match_device(dev, drv))
>>                 return 1;
>>
>> If you don't specify of_match_table and you put the same ID in
>> i2c_device_id table it wil match here:
>>
>> i2c_device_match
>>         driver = to_i2c_driver(drv);
>>         /* match on an id table if there is one */
>>         if (driver->id_table)
>>                 return i2c_match_id(driver->id_table, client) != NULL;
>>
>> This is happening because the name from dts is used for client->name.
>> i2c_match_id does the matching based on the client name.
>
> OK.
>
>> > BTW, how modules are supposed to be matched if we allow putting ACPI
>> > identifiers to i2c_device_id table?
>>
>> My aproach was like this: if the driver specifies .acpi_match table it
>> will work like before.
>>
>> i2c_device_match
>>         /* Then ACPI style match */
>>         if (acpi_driver_match_device(dev, drv))
>>                 return 1;
>>
>> If the driver does not specify .acpi_match table the i2c core will
>> atempt to match against the  i2c_match_id table (the same way it does
>> for DT). In the ACPI case the client->name has that :nn suffix and
>> what the patch does is to ignore that when i2c_match_id is called.
>>
>> i2c_device_match
>>         driver = to_i2c_driver(drv);
>>         /* match on an id table if there is one */
>>         if (driver->id_table)
>>                 return i2c_match_id(driver->id_table, client) != NULL;
>
> Yeah but when you have device with modalias of "acpi:FOO:" how
> udev/modprobe is supposed find the correct module?

The modalias file content exposed by the i2c device in sysfs will be the same.
Currently the alias exposed by the driver is based on the i2c_match_id
table and it does not have any aliases based on
acpi_device_id table. I am new to this are so please correct me if I am wrong.

>> The final goal is to simplify the driver and remove redundant code.
>
> IMHO mixing ACPI identifiers with I2C device identifiers does not
> simplify anything. And since you need to stick the ACPI ID somewhere
> anyway I don't get the point of removing redundant code either.

It will make the i2c_device_id be not NULL when the device is probed.
Here is an example:

static int probe(struct i2c_client *client, const struct i2c_device_id *id)
       /* Get device name from device table or ACPI */
        if (ACPI_HANDLE(dev)) {
                acpi_id = acpi_match_device(silead_ts_acpi_match, dev);
                if (!acpi_id)
                        return -ENODEV;

                sprintf(data->fw_name, "%s.fw", acpi_id->id);

                for (i = 0; i < strlen(data->fw_name); i++)
                        data->fw_name[i] = tolower(data->fw_name[i]);
        } else {
                sprintf(data->fw_name, "%s.fw", id->name);
        }

This will become:
        sprintf(data->fw_name, "%s.fw", id->name);

There are allot more cases like this already in the kernel.

Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/