On Sun, 2019-02-10 at 22:12 +0100, Alexandre Belloni wrote: > +}; > > +MODULE_DEVICE_TABLE(i2c, isl1208_id); > > + > > +static const struct of_device_id isl1208_of_match[] = { > > + { .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] }, > > + { .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] }, > > This compatible needs to be documented. I see isil,isl1219 escaped documentation when it was added too. I'll add both to rtc.txt. > > > > return handled ? IRQ_HANDLED : IRQ_NONE; > > @@ -743,6 +778,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id) > > return -ENOMEM; > > i2c_set_clientdata(client, isl1208); > > > > + /* Determine which chip we have */ > > + if (client->dev.of_node) { > > + const struct of_device_id *match = > > + of_match_node(isl1208_of_match, client->dev.of_node); > > + if (!match) > > This will never happen, else the driver would not be probed. If there was a driver_override ability with i2c, like PCI and SPI, then it would be possible. > > > + return -EINVAL; > > + isl1208->config = match->data; > > Maybe you should use of_device_get_match_data anyway. Yes, of_device_get_match_data() would be better. Not sure why I didn't find that. Interestingly, I see could have just depended on id->driver_data being set even in the OF case, as the i2c-core code always sets it. But it look to be deprecated and i2c clients should provide a probe_new method that does not provide an i2c_device_id. I'm not sure if that's entirely better, as every i2c client that cares will need to re- implement the code to look up by device name and/or OF node and/or ACPI. > > > + } else { > > + if (id->driver_data >= ISL_LAST_ID) > > + return -EINVAL; > > This should be -ENODEV. There's quite a few drivers that return EINVAL when the driver_data appears to be incorrect. But some also return ENODEV.