diff mbox

[RESEND,v5,00/16] eeprom: at24: Add OF device ID table

Message ID 20170830174228.sx5jgtxxy56zq47t@ninjato (mailing list archive)
State Accepted
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Wolfram Sang Aug. 30, 2017, 5:42 p.m. UTC
On Wed, Aug 30, 2017 at 06:19:02PM +0200, Javier Martinez Canillas wrote:
> Hello Wolfram,
> 
> On Tue, Aug 29, 2017 at 10:48 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> >> I don't have a DT based system at hand now, but I'll test it again and
> >> let you know probably tomorrow.
> >
> > I will try again today, too. Thanks!
> >
> 
> Ok, I had some time to do some tests again. I used an ARM Chromebook
> (Exynos Peach Pi) that has an I2C touchpad (Atmel maXTouch).

I tried again as well and it still fails for me.

> Tested the following cases:

I think we should talk about the same case: Let me repeat what I did:

1) I added your patch "eeprom: at24: Add OF device ID table"
2) I added an EEPROM node to an I2C

+       eeprom@50 {
+               compatible = "renesas,24c01";
+               reg = <0x50>;
+       };

-> no at24 binding to the device

3) I revert your patch

-> at24 binding to the device

I think you should be able to test this DTS snipplet even without a real
eeprom. Especially after applying this to the at24 driver.



Can you check this?

Thanks,

   Wolfram

Comments

Javier Martinez Canillas Aug. 30, 2017, 7:57 p.m. UTC | #1
>
> I think we should talk about the same case: Let me repeat what I did:
>
> 1) I added your patch "eeprom: at24: Add OF device ID table"
> 2) I added an EEPROM node to an I2C
>
> +       eeprom@50 {
> +               compatible = "renesas,24c01";
> +               reg = <0x50>;
> +       };
>
> -> no at24 binding to the device
>
> 3) I revert your patch
>
> -> at24 binding to the device
>

I've tested this and you are right, it fails...

The problem is that the patch also changes how the driver obtains the
EEPROM parameters (the magic value in the entry's data field).

So even when module autoload and device / driver matching works, the
driver probe function fails because if (client->dev.of_node) the
driver attempts to get the entry data using
of_device_get_match_data(), which is obviously wrong since the
compatible string in the dev node isn't present in the OF table.

The id->driver_data from the I2C table should be used instead since
that's the table that matches in this case.

One option is to fallback to id->driver_data if
of_device_get_match_data() fails, but that's just an (ugly)
workaround. So I agree with you that the best option is to wait for
the DTS patches to land first.

It worked for me on my previous tests because the tested drivers
didn't use a table entry data, I'm so sorry for missing this :(

Best regards,
Javier
Geert Uytterhoeven Aug. 30, 2017, 8:15 p.m. UTC | #2
Hi Javier,

On Wed, Aug 30, 2017 at 9:57 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:
>> I think we should talk about the same case: Let me repeat what I did:
>>
>> 1) I added your patch "eeprom: at24: Add OF device ID table"
>> 2) I added an EEPROM node to an I2C
>>
>> +       eeprom@50 {
>> +               compatible = "renesas,24c01";
>> +               reg = <0x50>;
>> +       };
>>
>> -> no at24 binding to the device
>>
>> 3) I revert your patch
>>
>> -> at24 binding to the device
>>
>
> I've tested this and you are right, it fails...
>
> The problem is that the patch also changes how the driver obtains the
> EEPROM parameters (the magic value in the entry's data field).
>
> So even when module autoload and device / driver matching works, the
> driver probe function fails because if (client->dev.of_node) the
> driver attempts to get the entry data using
> of_device_get_match_data(), which is obviously wrong since the
> compatible string in the dev node isn't present in the OF table.
>
> The id->driver_data from the I2C table should be used instead since
> that's the table that matches in this case.
>
> One option is to fallback to id->driver_data if
> of_device_get_match_data() fails, but that's just an (ugly)
> workaround. So I agree with you that the best option is to wait for
> the DTS patches to land first.

Which means new kernels won't work with old DTBs. Oops...
I'm afraid that needs to be fixed.  People care about DTB backward
compatibility on many platforms.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Javier Martinez Canillas Aug. 30, 2017, 8:59 p.m. UTC | #3
Hello Geert,

On Wed, Aug 30, 2017 at 10:15 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Javier,
>
> On Wed, Aug 30, 2017 at 9:57 PM, Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
>>> I think we should talk about the same case: Let me repeat what I did:
>>>
>>> 1) I added your patch "eeprom: at24: Add OF device ID table"
>>> 2) I added an EEPROM node to an I2C
>>>
>>> +       eeprom@50 {
>>> +               compatible = "renesas,24c01";
>>> +               reg = <0x50>;
>>> +       };
>>>
>>> -> no at24 binding to the device
>>>
>>> 3) I revert your patch
>>>
>>> -> at24 binding to the device
>>>
>>
>> I've tested this and you are right, it fails...
>>
>> The problem is that the patch also changes how the driver obtains the
>> EEPROM parameters (the magic value in the entry's data field).
>>
>> So even when module autoload and device / driver matching works, the
>> driver probe function fails because if (client->dev.of_node) the
>> driver attempts to get the entry data using
>> of_device_get_match_data(), which is obviously wrong since the
>> compatible string in the dev node isn't present in the OF table.
>>
>> The id->driver_data from the I2C table should be used instead since
>> that's the table that matches in this case.
>>
>> One option is to fallback to id->driver_data if
>> of_device_get_match_data() fails, but that's just an (ugly)
>> workaround. So I agree with you that the best option is to wait for
>> the DTS patches to land first.
>
> Which means new kernels won't work with old DTBs. Oops...
> I'm afraid that needs to be fixed.  People care about DTB backward
> compatibility on many platforms.
>

Right, I've yet to find one of those mythical platforms that ship old
DTBs with new kernels, but I agree with you since people seem to care
about backward compatibility (at least on theory).

So I see two options then:

1) Use the workaround I mentioned and lookup the I2C device ID table
entry data if of_device_get_match_data() fails

2) Only call of_device_get_match_data() if (dev->of_node &&
of_match_device(dev->driver->of_match_table, dev))

Not sure what's the preferred idiom for these cases.

To good thing about keeping backward compatibility is that Wolfram
would be able to pick the driver patch even before the DTS patches
land.

Best regards,
Javier
diff mbox

Patch

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 79c5c39be29cac..f9f547680c53db 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -805,11 +805,6 @@  static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	 * Perform a one-byte test read to verify that the
 	 * chip is functional.
 	 */
-	err = at24_read(at24, 0, &test_byte, 1);
-	if (err) {
-		err = -ENODEV;
-		goto err_clients;
-	}
 
 	at24->nvmem_config.name = dev_name(&client->dev);
 	at24->nvmem_config.dev = &client->dev;