| Message ID | 20151023082054.GP1526@lahna.fi.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Oct 23, 2015 at 11:20:54AM +0300, Mika Westerberg wrote: > On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote: > > > The next issue is that the I2C-core isn't matching the device to the > > > "at24" driver, which has the alias "24c02". > > > > Here is what I found. > > i2c-core is creating the device with the ACPI name "24C02:00". > > The at24 driver uses "24c02" as the alias. > > i2c-code is matching devices to drivers using strcmp(). > > > > Result: no match. ("24c02" != "24C02:00"). > > > > If I modify acpi_i2c_add_device() to cut off the name at the ':' and > > covert to lowercase when populating info.type, it matches and works. > > I must be missing something here, because this would have never worked as-is. > > I'll ask on the I2C mailing list. > > You should either use proper _HID/_CID for the device or put "PRP0001" > to the _HID and let the match happen with DT .compatible strings. I've > attached a hack that I use locally. Alternatively you can use patch by Andy here: https://patchwork.ozlabs.org/patch/524923 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 23, 2015 at 3:20 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote: >> > The next issue is that the I2C-core isn't matching the device to the >> > "at24" driver, which has the alias "24c02". >> >> Here is what I found. >> i2c-core is creating the device with the ACPI name "24C02:00". >> The at24 driver uses "24c02" as the alias. >> i2c-code is matching devices to drivers using strcmp(). >> >> Result: no match. ("24c02" != "24C02:00"). >> >> If I modify acpi_i2c_add_device() to cut off the name at the ':' and >> covert to lowercase when populating info.type, it matches and works. >> I must be missing something here, because this would have never worked as-is. >> I'll ask on the I2C mailing list. > > You should either use proper _HID/_CID for the device or put "PRP0001" > to the _HID and let the match happen with DT .compatible strings. I've > attached a hack that I use locally. > > The corresponding ASL fragment would look like: > > > Device (AT24) > { > Name (_HID, "PRP0001") > > Method (_CRS, 0, Serialized) { > Name (UBUF, ResourceTemplate () { > I2cSerialBus (0x50, ControllerInitiated, 0x00061A80, > AddressingMode7Bit, "\\_SB.I2C6", > 0x00, ResourceConsumer) > }) > Return (UBUF) > } > > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () {"compatible", "atmel,24c02"}, > Package () {"size", 256}, > Package () {"pagesize", 32}, > Package () {"abs-value", 1}, > }, > }) > > Method (_STA, 0, NotSerialized) > { > Return (0xF) > } > } I like this approach, as one change to the driver will support all ACPI devices. I'll give it a try. Thanks, Ben -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 23, 2015 at 12:24 PM, Ben Gardner <gardner.ben@gmail.com> wrote: > On Fri, Oct 23, 2015 at 3:20 AM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: >> On Thu, Oct 22, 2015 at 12:17:58PM -0500, Ben Gardner wrote: >>> > The next issue is that the I2C-core isn't matching the device to the >>> > "at24" driver, which has the alias "24c02". >>> >>> Here is what I found. >>> i2c-core is creating the device with the ACPI name "24C02:00". >>> The at24 driver uses "24c02" as the alias. >>> i2c-code is matching devices to drivers using strcmp(). >>> >>> Result: no match. ("24c02" != "24C02:00"). >>> >>> If I modify acpi_i2c_add_device() to cut off the name at the ':' and >>> covert to lowercase when populating info.type, it matches and works. >>> I must be missing something here, because this would have never worked as-is. >>> I'll ask on the I2C mailing list. >> >> You should either use proper _HID/_CID for the device or put "PRP0001" >> to the _HID and let the match happen with DT .compatible strings. I've >> attached a hack that I use locally. >> >> The corresponding ASL fragment would look like: >> >> >> Device (AT24) >> { >> Name (_HID, "PRP0001") >> >> Method (_CRS, 0, Serialized) { >> Name (UBUF, ResourceTemplate () { >> I2cSerialBus (0x50, ControllerInitiated, 0x00061A80, >> AddressingMode7Bit, "\\_SB.I2C6", >> 0x00, ResourceConsumer) >> }) >> Return (UBUF) >> } >> >> Name (_DSD, Package () { >> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> Package () { >> Package () {"compatible", "atmel,24c02"}, >> Package () {"size", 256}, >> Package () {"pagesize", 32}, >> Package () {"abs-value", 1}, >> }, >> }) >> >> Method (_STA, 0, NotSerialized) >> { >> Return (0xF) >> } >> } > > I like this approach, as one change to the driver will support all ACPI devices. > I'll give it a try. This approach works well enough. Would you mind if I cleaned up the patch and submitted it for inclusion? Or is there a reason why this isn't upstream? Thanks, Ben -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 26, 2015 at 02:56:09PM -0500, Ben Gardner wrote: > This approach works well enough. Great. > Would you mind if I cleaned up the patch and submitted it for inclusion? Not at all. > Or is there a reason why this isn't upstream? I just haven't had time to clean it up ;-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On Fri Oct 23 11:20, Mika Westerberg wrote: > You should either use proper _HID/_CID for the device or put "PRP0001" > to the _HID and let the match happen with DT .compatible strings. I've > attached a hack that I use locally. I have a similar hack over here. I have a question though: > The corresponding ASL fragment would look like: > > > Device (AT24) > { > Name (_HID, "PRP0001") > > Method (_CRS, 0, Serialized) { > Name (UBUF, ResourceTemplate () { > I2cSerialBus (0x50, ControllerInitiated, 0x00061A80, > AddressingMode7Bit, "\\_SB.I2C6", > 0x00, ResourceConsumer) > }) > Return (UBUF) > } > > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () {"compatible", "atmel,24c02"}, The "c02" in 24c02 also indicates the size. I've always found it a little awkward when you could have a compatible string that disagrees with firmware properties. Should we do something about that? For example, is the more generic string, "atmel,at24" better? I'm not sure I like that approach in general, but it works well for the at24 devices. at25 does it the same way. > Package () {"size", 256}, > Package () {"pagesize", 32}, > Package () {"abs-value", 1}, > }, > }) > > Method (_STA, 0, NotSerialized) > { > Return (0xF) > } > } --Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 27, 2015 at 02:11:11PM -0700, Dustin Byford wrote: > Hi Mika, > > On Fri Oct 23 11:20, Mika Westerberg wrote: > > > You should either use proper _HID/_CID for the device or put "PRP0001" > > to the _HID and let the match happen with DT .compatible strings. I've > > attached a hack that I use locally. > > I have a similar hack over here. I have a question though: > > > The corresponding ASL fragment would look like: > > > > > > Device (AT24) > > { > > Name (_HID, "PRP0001") > > > > Method (_CRS, 0, Serialized) { > > Name (UBUF, ResourceTemplate () { > > I2cSerialBus (0x50, ControllerInitiated, 0x00061A80, > > AddressingMode7Bit, "\\_SB.I2C6", > > 0x00, ResourceConsumer) > > }) > > Return (UBUF) > > } > > > > Name (_DSD, Package () { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package () {"compatible", "atmel,24c02"}, > > The "c02" in 24c02 also indicates the size. I've always found it a > little awkward when you could have a compatible string that disagrees > with firmware properties. Should we do something about that? > > For example, is the more generic string, "atmel,at24" better? I'm not > sure I like that approach in general, but it works well for the at24 > devices. at25 does it the same way. I think DT version actually identifies this if the device node is called "at24". I'm not sure if we can do that with ACPI though. I agree that we could use "atmel,at24" here but that requires corresponding binding to be added to Documentation/devicetree/bindings/i2c/trivial-devices.txt and review from DT folks. Definitely worth doing. Alternatively we may use the same _HID "INT3499" as Andy is doing in his series with the below properties. > > Package () {"size", 256}, > > Package () {"pagesize", 32}, > > Package () {"abs-value", 1}, > > }, > > }) > > > > Method (_STA, 0, NotSerialized) > > { > > Return (0xF) > > } > > } > > --Dustin -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Oct 28, 2015 at 4:01 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Oct 27, 2015 at 02:11:11PM -0700, Dustin Byford wrote: >> Hi Mika, >> >> On Fri Oct 23 11:20, Mika Westerberg wrote: >> >> > You should either use proper _HID/_CID for the device or put "PRP0001" >> > to the _HID and let the match happen with DT .compatible strings. I've >> > attached a hack that I use locally. >> >> I have a similar hack over here. I have a question though: >> >> > The corresponding ASL fragment would look like: >> > >> > >> > Device (AT24) >> > { >> > Name (_HID, "PRP0001") >> > >> > Method (_CRS, 0, Serialized) { >> > Name (UBUF, ResourceTemplate () { >> > I2cSerialBus (0x50, ControllerInitiated, 0x00061A80, >> > AddressingMode7Bit, "\\_SB.I2C6", >> > 0x00, ResourceConsumer) >> > }) >> > Return (UBUF) >> > } >> > >> > Name (_DSD, Package () { >> > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> > Package () { >> > Package () {"compatible", "atmel,24c02"}, >> >> The "c02" in 24c02 also indicates the size. I've always found it a >> little awkward when you could have a compatible string that disagrees >> with firmware properties. Should we do something about that? >> >> For example, is the more generic string, "atmel,at24" better? I'm not >> sure I like that approach in general, but it works well for the at24 >> devices. at25 does it the same way. > > I think DT version actually identifies this if the device node is called > "at24". I'm not sure if we can do that with ACPI though. > > I agree that we could use "atmel,at24" here but that requires > corresponding binding to be added to > Documentation/devicetree/bindings/i2c/trivial-devices.txt and review > from DT folks. Definitely worth doing. It looks like you are supposed to have the first compatible entry be the exact manufacturer,model. http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property I am considering using a generic compatible entry ("linux,at24") to match the Linux at24 driver. In my case, I'm using a M24C02 from ST, so the compatible line would be: Package () {"compatible", Package () {"st,m24c02", "linux,at24"}}, Package () {"size", 256}, Package () {"pagesize", 16}, The "linux,at24" would require the other properties (size and pagesize) to be present. I've seen something similar done with a few other devices. > Alternatively we may use the same _HID "INT3499" as Andy is doing in his > series with the below properties. The only issue with reusing INT3499 is that it is configured to be a 24C08 in the at24 driver. Reusing the 24C08 HID for a 24C02 or other chip might be a bit confusing. Ben -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 30, 2015 at 11:51:01AM -0500, Ben Gardner wrote: > It looks like you are supposed to have the first compatible entry be > the exact manufacturer,model. > http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property > > I am considering using a generic compatible entry ("linux,at24") to > match the Linux at24 driver. > In my case, I'm using a M24C02 from ST, so the compatible line would be: > > Package () {"compatible", Package () {"st,m24c02", "linux,at24"}}, > Package () {"size", 256}, > Package () {"pagesize", 16}, > > The "linux,at24" would require the other properties (size and > pagesize) to be present. > I've seen something similar done with a few other devices. Using "linux,foo" is not a good idea and I'm pretty sure DT maintainers will not take such patches. The compatible (and other properties as well) should describe hardware not software. > > Alternatively we may use the same _HID "INT3499" as Andy is doing in his > > series with the below properties. > > The only issue with reusing INT3499 is that it is configured to be a > 24C08 in the at24 driver. > Reusing the 24C08 HID for a 24C02 or other chip might be a bit confusing. It could default to 24c08 but if it has properties the driver should use those to determine size etc. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index c6cb7f8f325e..d2963b5632d4 100644 --- a/drivers/misc/eeprom/at24.c +++ b/drivers/misc/eeprom/at24.c @@ -20,9 +20,9 @@ #include <linux/log2.h> #include <linux/bitops.h> #include <linux/jiffies.h> -#include <linux/of.h> #include <linux/i2c.h> #include <linux/platform_data/at24.h> +#include <linux/property.h> /* * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable. @@ -443,26 +443,24 @@ static ssize_t at24_macc_write(struct memory_accessor *macc, const char *buf, /*-------------------------------------------------------------------------*/ -#ifdef CONFIG_OF -static void at24_get_ofdata(struct i2c_client *client, +static int at24_get_fwdata(struct i2c_client *client, struct at24_platform_data *chip) { - const __be32 *val; - struct device_node *node = client->dev.of_node; - - if (node) { - if (of_get_property(node, "read-only", NULL)) - chip->flags |= AT24_FLAG_READONLY; - val = of_get_property(node, "pagesize", NULL); - if (val) - chip->page_size = be32_to_cpup(val); - } + if (device_property_present(&client->dev, "read-only")) + chip->flags |= AT24_FLAG_READONLY; + if (device_property_read_u16(&client->dev, "pagesize", &chip->page_size)) + return -ENODEV; + if (device_property_read_u32(&client->dev, "size", &chip->byte_len)) + return -ENODEV; + + return 0; } -#else -static void at24_get_ofdata(struct i2c_client *client, - struct at24_platform_data *chip) -{ } -#endif /* CONFIG_OF */ + +static const struct of_device_id at24_of_match[] = { + { .compatible = "atmel,24c02" }, + { }, +}; +MODULE_DEVICE_TABLE(of, at24_of_match); static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -477,7 +475,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) if (client->dev.platform_data) { chip = *(struct at24_platform_data *)client->dev.platform_data; - } else { + } else if (id) { if (!id->driver_data) return -ENODEV; @@ -493,10 +491,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) chip.page_size = 1; /* update chipdata if OF is present */ - at24_get_ofdata(client, &chip); + at24_get_fwdata(client, &chip); chip.setup = NULL; chip.context = NULL; + } else { + memset(&chip, 0, sizeof(chip)); + err = at24_get_fwdata(client, &chip); + if (err) + return err; } if (!is_power_of_2(chip.byte_len)) @@ -661,6 +664,7 @@ static int at24_remove(struct i2c_client *client) static struct i2c_driver at24_driver = { .driver = { .name = "at24", + .of_match_table = at24_of_match, }, .probe = at24_probe, .remove = at24_remove,