| Message ID | 82c6f53cfa03f9bc7c0adfc423ae65fc986a1d25.1530599660.git.nikolaus.voss@loewensteinmedical.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > struct i2c_device_id argument of probe() is not used, so use probe_new() > instead. > This makes... > MODULE_DEVICE_TABLE(i2c, st_accel_id_table); ...this table obsolete IIUC. At least that's what I did when switched to ->probe_new() in some drivers. If I'm mistaken (again? :-) ) I would hear from someone to point me how it can be used after a switch. > > -static int st_accel_i2c_probe(struct i2c_client *client, > - const struct i2c_device_id *id) > +static int st_accel_i2c_probe(struct i2c_client *client) > { > struct iio_dev *indio_dev; > struct st_sensor_data *adata; > @@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = { > .of_match_table = of_match_ptr(st_accel_of_match), > .acpi_match_table = ACPI_PTR(st_accel_acpi_match), > }, > - .probe = st_accel_i2c_probe, > + .probe_new = st_accel_i2c_probe, > .remove = st_accel_i2c_remove, > .id_table = st_accel_id_table, > }; > -- > 2.17.1 >
On Wed, 4 Jul 2018, Andy Shevchenko wrote: > On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: >> struct i2c_device_id argument of probe() is not used, so use probe_new() >> instead. >> > > This makes... > >> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); > > ...this table obsolete IIUC. At least that's what I did when switched > to ->probe_new() in some drivers. > > If I'm mistaken (again? :-) ) I would hear from someone to point me > how it can be used after a switch. It is still used by the i2c-core in i2c_device_match() if DT and ACPI matching fails. And it is used to create the corresponding modaliases for driver loading. So it is necessary for non-DT/ non-ACPI systems and used for fallback matching if no match is found in of_device_ids. > >> >> -static int st_accel_i2c_probe(struct i2c_client *client, >> - const struct i2c_device_id *id) >> +static int st_accel_i2c_probe(struct i2c_client *client) >> { >> struct iio_dev *indio_dev; >> struct st_sensor_data *adata; >> @@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = { >> .of_match_table = of_match_ptr(st_accel_of_match), >> .acpi_match_table = ACPI_PTR(st_accel_acpi_match), >> }, >> - .probe = st_accel_i2c_probe, >> + .probe_new = st_accel_i2c_probe, >> .remove = st_accel_i2c_remove, >> .id_table = st_accel_id_table, >> }; >> -- >> 2.17.1 >> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > On Wed, 4 Jul 2018, Andy Shevchenko wrote: >> >> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss >> <nikolaus.voss@loewensteinmedical.de> wrote: >>> >>> struct i2c_device_id argument of probe() is not used, so use probe_new() >>> instead. >>> >> >> This makes... >> >>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); >> >> >> ...this table obsolete IIUC. At least that's what I did when switched >> to ->probe_new() in some drivers. >> >> If I'm mistaken (again? :-) ) I would hear from someone to point me >> how it can be used after a switch. > > > It is still used by the i2c-core in i2c_device_match() if DT and ACPI > matching fails. > And it is used to create the corresponding modaliases for > driver loading. My question is "How?!" I don't really see any points to match against it after switching to ->probe_new(). Could you point me to the code path in i2c (or OF?) core for that? > So it is necessary for non-DT/ non-ACPI systems and used for > fallback matching if no match is found in of_device_ids.
On Wed, 4 Jul 2018, Andy Shevchenko wrote: > On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: >> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>> >>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss >>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>> >>>> struct i2c_device_id argument of probe() is not used, so use probe_new() >>>> instead. >>>> >>> >>> This makes... >>> >>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); >>> >>> >>> ...this table obsolete IIUC. At least that's what I did when switched >>> to ->probe_new() in some drivers. >>> >>> If I'm mistaken (again? :-) ) I would hear from someone to point me >>> how it can be used after a switch. >> >> >> It is still used by the i2c-core in i2c_device_match() if DT and ACPI >> matching fails. > >> And it is used to create the corresponding modaliases for >> driver loading. > > My question is "How?!" > I don't really see any points to match against it after switching to > ->probe_new(). > > Could you point me to the code path in i2c (or OF?) core for that? As written above in i2c-core-base.c: i2c_device_match() -> i2c_match_id(driver->id_table,... This is used for driver matching before probe() or probe_new() of the device driver can be called. probe_new() actually is a function signature change only. Niko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > On Wed, 4 Jul 2018, Andy Shevchenko wrote: >> >> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss >> <nikolaus.voss@loewensteinmedical.de> wrote: >>> >>> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>>> >>>> >>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss >>>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>>> >>>>> >>>>> struct i2c_device_id argument of probe() is not used, so use >>>>> probe_new() >>>>> instead. >>>>> >>>> >>>> This makes... >>>> >>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); >>>> >>>> >>>> >>>> ...this table obsolete IIUC. At least that's what I did when switched >>>> to ->probe_new() in some drivers. >>>> >>>> If I'm mistaken (again? :-) ) I would hear from someone to point me >>>> how it can be used after a switch. >>> >>> >>> >>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI >>> matching fails. >> >> >>> And it is used to create the corresponding modaliases for >>> driver loading. >> >> >> My question is "How?!" >> I don't really see any points to match against it after switching to >> ->probe_new(). >> >> Could you point me to the code path in i2c (or OF?) core for that? > > > As written above in i2c-core-base.c: i2c_device_match() -> > i2c_match_id(driver->id_table,... > > This is used for driver matching before probe() or probe_new() of the device > driver can be called. probe_new() actually is a function signature change > only. Okay, IIUC we got a match. What should we do with it? The table is not used in ->probe_new() (in i2c core), so, you can't say which line matched there.
On Wed, 4 Jul 2018, Andy Shevchenko wrote: > On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: >> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>> >>> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss >>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>> >>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>>>> >>>>> >>>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss >>>>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>>>> >>>>>> >>>>>> struct i2c_device_id argument of probe() is not used, so use >>>>>> probe_new() >>>>>> instead. >>>>>> >>>>> >>>>> This makes... >>>>> >>>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); >>>>> >>>>> >>>>> >>>>> ...this table obsolete IIUC. At least that's what I did when switched >>>>> to ->probe_new() in some drivers. >>>>> >>>>> If I'm mistaken (again? :-) ) I would hear from someone to point me >>>>> how it can be used after a switch. >>>> >>>> >>>> >>>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI >>>> matching fails. >>> >>> >>>> And it is used to create the corresponding modaliases for >>>> driver loading. >>> >>> >>> My question is "How?!" >>> I don't really see any points to match against it after switching to >>> ->probe_new(). >>> >>> Could you point me to the code path in i2c (or OF?) core for that? >> >> >> As written above in i2c-core-base.c: i2c_device_match() -> >> i2c_match_id(driver->id_table,... >> >> This is used for driver matching before probe() or probe_new() of the device >> driver can be called. probe_new() actually is a function signature change >> only. > > Okay, IIUC we got a match. What should we do with it? The table is not > used in ->probe_new() (in i2c core), so, you can't say which line > matched there. The table is not used by the driver, but is necessary to a) bind an i2c device declared via i2c_board_info with type field set to one of the names of the i2c_device_id table to this driver b) bind an i2c device declared via DT or ACPI but with no match in of_id/ acpi_id table but an i2c_device_id table match to this driver (fallback matching) c) create the right modaliases at compile time for this driver to make module auto-loading work in case of a) and b) Niko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Summon Javier to the discussion. For my opinion he is an expert in this topic. On Wed, Jul 4, 2018 at 12:42 PM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > On Wed, 4 Jul 2018, Andy Shevchenko wrote: >> On Wed, Jul 4, 2018 at 12:09 PM, Nikolaus Voss >> <nikolaus.voss@loewensteinmedical.de> wrote: >>> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>>> On Wed, Jul 4, 2018 at 9:37 AM, Nikolaus Voss >>>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>>> On Wed, 4 Jul 2018, Andy Shevchenko wrote: >>>>>> On Tue, Jul 3, 2018 at 9:06 AM, Nikolaus Voss >>>>>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>>>>> struct i2c_device_id argument of probe() is not used, so use >>>>>>> probe_new() >>>>>>> instead. >>>>>> This makes... >>>>>> >>>>>>> MODULE_DEVICE_TABLE(i2c, st_accel_id_table); >>>>>> ...this table obsolete IIUC. At least that's what I did when switched >>>>>> to ->probe_new() in some drivers. >>>>>> >>>>>> If I'm mistaken (again? :-) ) I would hear from someone to point me >>>>>> how it can be used after a switch. >>>>> It is still used by the i2c-core in i2c_device_match() if DT and ACPI >>>>> matching fails. >>>>> And it is used to create the corresponding modaliases for >>>>> driver loading. >>>> My question is "How?!" >>>> I don't really see any points to match against it after switching to >>>> ->probe_new(). >>>> >>>> Could you point me to the code path in i2c (or OF?) core for that? >>> As written above in i2c-core-base.c: i2c_device_match() -> >>> i2c_match_id(driver->id_table,... >>> >>> This is used for driver matching before probe() or probe_new() of the >>> device >>> driver can be called. probe_new() actually is a function signature change >>> only. >> Okay, IIUC we got a match. What should we do with it? The table is not >> used in ->probe_new() (in i2c core), so, you can't say which line >> matched there. > The table is not used by the driver, but is necessary to > > a) bind an i2c device declared via i2c_board_info with type field set > to one of the names of the i2c_device_id table to this driver > b) bind an i2c device declared via DT or ACPI but with no match in of_id/ > acpi_id table but an i2c_device_id table match to this driver (fallback > matching) > c) create the right modaliases at compile time for this driver to make > module auto-loading work in case of a) and b) Javier, just a summary of the above. Nikolaus switched one driver to use ->probe_new() hook and left i2c ID table at the same time. My understanding that this table is not anymore in use. But I have to admit I didn't see entire picture of this. Can you shed a light?
Hi Andy, On 07/04/2018 12:19 PM, Andy Shevchenko wrote: > Summon Javier to the discussion. > For my opinion he is an expert in this topic. I wouldn't call myself an expert in anything to be honest :) [snip] > >> The table is not used by the driver, but is necessary to >> >> a) bind an i2c device declared via i2c_board_info with type field set >> to one of the names of the i2c_device_id table to this driver >> b) bind an i2c device declared via DT or ACPI but with no match in of_id/ >> acpi_id table but an i2c_device_id table match to this driver (fallback >> matching) >> c) create the right modaliases at compile time for this driver to make >> module auto-loading work in case of a) and b) > I think Nikolaus is correct, assuming that the driver can be used on legacy platforms that register the I2C devices using board files / platform data. In that case you still need a I2C device ID table for (a) and (c) as he said. I don't buy on (b) though, that's a bug in my opinion. If you register an I2C device via DT then you must have a OF device ID entry that matches the device and the same for ACPI. You can't rely on the I2C device table to do the match. I would also remove the struct i2c_device_id .driver_data fields from the I2C device ID table, since are not used and just makes reading the code confusing (only the struct i2c_device_id .name is used as far as I can see). > Javier, just a summary of the above. Nikolaus switched one driver to > use ->probe_new() hook and left i2c ID table at the same time. > My understanding that this table is not anymore in use. > > But I have to admit I didn't see entire picture of this. Can you shed a light? > So to shed some light, in the past even {OF,ACPI}-only drivers needed an I2C ID table because: 1) the .probe callback had a struct i2c_device_id * parameter and 2) the I2C core always reported a modalias of the form i2c:<foo> even for devices registered via OF. The .probe_new callbacks solves (1) and the I2C core now reports of:N*T*Cfoo* solving (2). So the I2C device table isn't required anymore for {OF,ACPI}-only drivers, but it's still required for drivers that support legacy board files that calls i2c_register_board_info() directly. Same for the old .probe callback, it's needed if struct i2c_device_id .driver_data is used in the probe function. Best regards,
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: > Hi Andy, > > On 07/04/2018 12:19 PM, Andy Shevchenko wrote: >> Summon Javier to the discussion. >> For my opinion he is an expert in this topic. > > I wouldn't call myself an expert in anything to be honest :) > > [snip] > >> >>> The table is not used by the driver, but is necessary to >>> >>> a) bind an i2c device declared via i2c_board_info with type field set >>> to one of the names of the i2c_device_id table to this driver >>> b) bind an i2c device declared via DT or ACPI but with no match in of_id/ >>> acpi_id table but an i2c_device_id table match to this driver (fallback >>> matching) >>> c) create the right modaliases at compile time for this driver to make >>> module auto-loading work in case of a) and b) >> > > I think Nikolaus is correct, assuming that the driver can be used on legacy > platforms that register the I2C devices using board files / platform data. > In that case you still need a I2C device ID table for (a) and (c) as he said. > > I don't buy on (b) though, that's a bug in my opinion. If you register an I2C > device via DT then you must have a OF device ID entry that matches the device > and the same for ACPI. You can't rely on the I2C device table to do the match. Ok, in my opinion it is an elegant way of not bloating the driver when no explicit handling (e.g. reading DT properties) is needed. Just adding an of_device_id doesn't change any driver functionality then but only maps to an already existing i2c_table_id. > > I would also remove the struct i2c_device_id .driver_data fields from the I2C > device ID table, since are not used and just makes reading the code confusing > (only the struct i2c_device_id .name is used as far as I can see). Valid point, thanks. I will change that. > >> Javier, just a summary of the above. Nikolaus switched one driver to >> use ->probe_new() hook and left i2c ID table at the same time. >> My understanding that this table is not anymore in use. >> >> But I have to admit I didn't see entire picture of this. Can you shed a light? >> > > So to shed some light, in the past even {OF,ACPI}-only drivers needed an I2C ID > table because: 1) the .probe callback had a struct i2c_device_id * parameter > and 2) the I2C core always reported a modalias of the form i2c:<foo> even for > devices registered via OF. It could have been a null pointer and device driver binding (and auto-loading) done just via driver.name. Niko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nikolaus, On Wed, Jul 4, 2018 at 1:15 PM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: [snip] >> I think Nikolaus is correct, assuming that the driver can be used on >> legacy >> platforms that register the I2C devices using board files / platform data. >> In that case you still need a I2C device ID table for (a) and (c) as he >> said. >> >> I don't buy on (b) though, that's a bug in my opinion. If you register an >> I2C >> device via DT then you must have a OF device ID entry that matches the >> device >> and the same for ACPI. You can't rely on the I2C device table to do the >> match. > > > Ok, in my opinion it is an elegant way of not bloating the driver when no > explicit handling (e.g. reading DT properties) is needed. Just adding an > of_device_id doesn't change any driver functionality then but only maps to > an already existing i2c_table_id. > I disagree, in the case of OF for example a compatible string is composed of both a vendor a device, the complete tuple is what should be matched. But with the fallback, only the device portion would be used so both <foo,bar> and <baz,bar> will match against the i2c device with id "bar". It may or may not be correct but the vendor portion is very important to disambiguate. >> >> I would also remove the struct i2c_device_id .driver_data fields from the >> I2C >> device ID table, since are not used and just makes reading the code >> confusing >> (only the struct i2c_device_id .name is used as far as I can see). > > > Valid point, thanks. I will change that. > >> >>> Javier, just a summary of the above. Nikolaus switched one driver to >>> use ->probe_new() hook and left i2c ID table at the same time. >>> My understanding that this table is not anymore in use. >>> >>> But I have to admit I didn't see entire picture of this. Can you shed a >>> light? >>> >> >> So to shed some light, in the past even {OF,ACPI}-only drivers needed an >> I2C ID >> table because: 1) the .probe callback had a struct i2c_device_id * >> parameter >> and 2) the I2C core always reported a modalias of the form i2c:<foo> even >> for >> devices registered via OF. > > > It could have been a null pointer and device driver binding (and > auto-loading) done just via driver.name. > I'm not sure I understood this comment. > Niko > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 4, 2018 at 1:26 PM, Javier Martinez Canillas <javier@dowhile0.org> wrote: [snip] >> Ok, in my opinion it is an elegant way of not bloating the driver when no >> explicit handling (e.g. reading DT properties) is needed. Just adding an >> of_device_id doesn't change any driver functionality then but only maps to >> an already existing i2c_table_id. >> > > I disagree, in the case of OF for example a compatible string is > composed of both a vendor a device, the complete tuple is what should > be matched. > > But with the fallback, only the device portion would be used so both > <foo,bar> and <baz,bar> will match against the i2c device with id > "bar". It may or may not be correct but the vendor portion is very > important to disambiguate. > And having a compatible = "bar" is also wrong since the compatible string would lack the vendor prefix. The fact that the I2C (and SPI) core allowed this was what caused the problem in the first place IMO. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Javier, On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: > Hi Nikolaus, > > On Wed, Jul 4, 2018 at 1:15 PM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: >> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: > > [snip] > >>> I think Nikolaus is correct, assuming that the driver can be used on >>> legacy >>> platforms that register the I2C devices using board files / platform data. >>> In that case you still need a I2C device ID table for (a) and (c) as he >>> said. >>> >>> I don't buy on (b) though, that's a bug in my opinion. If you register an >>> I2C >>> device via DT then you must have a OF device ID entry that matches the >>> device >>> and the same for ACPI. You can't rely on the I2C device table to do the >>> match. >> >> >> Ok, in my opinion it is an elegant way of not bloating the driver when no >> explicit handling (e.g. reading DT properties) is needed. Just adding an >> of_device_id doesn't change any driver functionality then but only maps to >> an already existing i2c_table_id. >> > > I disagree, in the case of OF for example a compatible string is > composed of both a vendor a device, the complete tuple is what should > be matched. > > But with the fallback, only the device portion would be used so both > <foo,bar> and <baz,bar> will match against the i2c device with id > "bar". It may or may not be correct but the vendor portion is very > important to disambiguate. Disambiguation via DT implies there is already a name collision in i2c modalias name space, so adding an of_id would work around this collision for DT enumeration. I2c_board_info driver binding would still be broken. The right fix would be to remove the name collision. >>> >>> I would also remove the struct i2c_device_id .driver_data fields from the >>> I2C >>> device ID table, since are not used and just makes reading the code >>> confusing >>> (only the struct i2c_device_id .name is used as far as I can see). >> >> >> Valid point, thanks. I will change that. >> >>> >>>> Javier, just a summary of the above. Nikolaus switched one driver to >>>> use ->probe_new() hook and left i2c ID table at the same time. >>>> My understanding that this table is not anymore in use. >>>> >>>> But I have to admit I didn't see entire picture of this. Can you shed a >>>> light? >>>> >>> >>> So to shed some light, in the past even {OF,ACPI}-only drivers needed an >>> I2C ID >>> table because: 1) the .probe callback had a struct i2c_device_id * >>> parameter >>> and 2) the I2C core always reported a modalias of the form i2c:<foo> even >>> for >>> devices registered via OF. >> >> >> It could have been a null pointer and device driver binding (and >> auto-loading) done just via driver.name. >> > > I'm not sure I understood this comment. What I was trying to say is that if the i2c_device_id table information was not needed (i.d. only one single id), even the old probe() could be used without defining an i2c_device_id table, the i2c_device_id* argument to probe() being a nullptr. Niko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: [snip] >>> >>> Ok, in my opinion it is an elegant way of not bloating the driver when no >>> explicit handling (e.g. reading DT properties) is needed. Just adding an >>> of_device_id doesn't change any driver functionality then but only maps >>> to >>> an already existing i2c_table_id. >>> >> >> I disagree, in the case of OF for example a compatible string is >> composed of both a vendor a device, the complete tuple is what should >> be matched. >> >> But with the fallback, only the device portion would be used so both >> <foo,bar> and <baz,bar> will match against the i2c device with id >> "bar". It may or may not be correct but the vendor portion is very >> important to disambiguate. > > > Disambiguation via DT implies there is already a name collision in i2c > modalias name space, so adding an of_id would work around this collision for There wouldn't be a name collision between OF modaliases in that case since the vendor would be different (of:N*T*Cfoo,bar and of:N*T*Cbaz,bar). So it wouldn't be a problem for DT-only drivers. > DT enumeration. I2c_board_info driver binding would still be broken. The > right fix would be to remove the name collision. > Yes, for legacy drivers using board files it would be an issue. But that's unrelated to what I'm saying. What I don't think is correct is to ignore the vendor prefix since that's part of the compatible string semantics. In general, I think that there should be consistency between the firmware interface used to register the device, how the module alias uevent is reported to auto-load a driver and how the driver is matched with the device. So drivers supporting DT should have a n OF table (and export it with MODULE_DEVICE_TABLE), drivers supporting ACPI should have a ACPI table and so on. But this discussion isn't really related to your patch. I think is correct but just said that (b) wasn't a justification to leave the I2C table, points (a) and (c) are though. I won't really be convinced that the fallback is the correct thing to do or even a good idea. [snip] >>> >>> It could have been a null pointer and device driver binding (and >>> auto-loading) done just via driver.name. >>> >> >> I'm not sure I understood this comment. > > > What I was trying to say is that if the i2c_device_id table information was > not needed (i.d. only one single id), even the old probe() could be used > without defining an i2c_device_id table, the i2c_device_id* argument to > probe() being a nullptr. > I see, yes that would work too. I didn't authored the .probe_new callback so I don't know if other options were discussed. I also can't remember if the I2C core attempted to probe even without an I2C device ID table, I thought it didn't but I can be wrong. > Niko Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: > On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: > [snip] > But this discussion isn't really related to your patch. I think is > correct but just said that (b) wasn't a justification to leave the I2C > table, points (a) and (c) are though. I won't really be convinced that > the fallback is the correct thing to do or even a good idea. I didn't want to annoy you, I just wanted to understand why you think fallback is such a bad thing that you call it a bug. And I see, it has its drawbacks ;-). Anyway, thanks for taking the time to clarify this, Niko [snip] -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> wrote: > On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >> >> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss >> <nikolaus.voss@loewensteinmedical.de> wrote: >> > > [snip] > >> But this discussion isn't really related to your patch. I think is >> correct but just said that (b) wasn't a justification to leave the I2C >> table, points (a) and (c) are though. I won't really be convinced that >> the fallback is the correct thing to do or even a good idea. > > > I didn't want to annoy you, I just wanted to understand why you think > fallback is such a bad thing that you call it a bug. And I see, it has its > drawbacks ;-). Anyway, thanks for taking the time to clarify this, > Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say is that I've a strong opinion on this and won't be convinced otherwise :) So for me is a bug because that would mean that either an entry is missing in an OF device table or a DTS has a node with a compatible string without a vendor prefix. > Niko > > [snip] > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: > On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss > <nikolaus.voss@loewensteinmedical.de> wrote: >> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >>> >>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss >>> <nikolaus.voss@loewensteinmedical.de> wrote: >>> >> >> [snip] >> >>> But this discussion isn't really related to your patch. I think is >>> correct but just said that (b) wasn't a justification to leave the I2C >>> table, points (a) and (c) are though. I won't really be convinced that >>> the fallback is the correct thing to do or even a good idea. >> >> >> I didn't want to annoy you, I just wanted to understand why you think >> fallback is such a bad thing that you call it a bug. And I see, it has its >> drawbacks ;-). Anyway, thanks for taking the time to clarify this, >> > > Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say > is that I've a strong opinion on this and won't be convinced otherwise > :) > > So for me is a bug because that would mean that either an entry is > missing in an OF device table or a DTS has a node with a compatible > string without a vendor prefix. Yes, I see your point (and your strong opinion :-)), but AFAIK vendor prefix is not mandatory... At least for vendor-agnostic drivers like "regulator-fixed" (very popular in dts files). My point is not bloating drivers with large redundant (from a driver-functional view) tables when one table could be enough for a properly working driver. Having three different names for exactly the same isn't very beautiful IMO. I hope you're still not annoyed... Niko -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/04/2018 03:24 PM, Nikolaus Voss wrote: > On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >> On Wed, Jul 4, 2018 at 2:31 PM, Nikolaus Voss >> <nikolaus.voss@loewensteinmedical.de> wrote: >>> On Wed, 4 Jul 2018, Javier Martinez Canillas wrote: >>>> >>>> On Wed, Jul 4, 2018 at 1:46 PM, Nikolaus Voss >>>> <nikolaus.voss@loewensteinmedical.de> wrote: >>>> >>> >>> [snip] >>> >>>> But this discussion isn't really related to your patch. I think is >>>> correct but just said that (b) wasn't a justification to leave the I2C >>>> table, points (a) and (c) are though. I won't really be convinced that >>>> the fallback is the correct thing to do or even a good idea. >>> >>> >>> I didn't want to annoy you, I just wanted to understand why you think >>> fallback is such a bad thing that you call it a bug. And I see, it has its >>> drawbacks ;-). Anyway, thanks for taking the time to clarify this, >>> >> >> Oh, I'm not annoyed, sorry if I sounded that way. What I tried to say >> is that I've a strong opinion on this and won't be convinced otherwise >> :) >> >> So for me is a bug because that would mean that either an entry is >> missing in an OF device table or a DTS has a node with a compatible >> string without a vendor prefix. > > Yes, I see your point (and your strong opinion :-)), but AFAIK vendor > prefix is not mandatory... At least for vendor-agnostic drivers like The latest Device Tree specification [0] says about the compatible string: "The recommended format is 'manufacturer,model', where manufacturer is a string describing the name of the manufacturer (such as a stock ticker symbol), and model specifies the model number". > "regulator-fixed" (very popular in dts files). My point is not bloating I don't think the "regulator-fixed" is a good example. Since the Device Tree should describe the hardware. The "regulator-fixed" is a convenient way to describe a fixed voltage regulator but I think is more of an exception. I'm pretty sure that the DT maintainers wouldn't ack a DT binding with a compatible string that doesn't have a manufacture prefix nowadays. > drivers with large redundant (from a driver-functional view) tables when > one table could be enough for a properly working driver. Having three You need the OF table anyways for module autoload since the I2C core will report a OF module alias. You can only do the I2C fallback trick if your driver can't be build as a module. But even in that case you would be ignoring the vendor. > different names for exactly the same isn't very beautiful IMO. > I agree with you on that. But abusing a table used by another firmware interface isn't beautiful either. So I think the best is to have consistency and always use the same table for the same firmware interface. > I hope you're still not annoyed... > Don't worry for that, it's very hard to get my annoyed :) > Niko > [0]: https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2 Best regards,
On Wed, Jul 4, 2018 at 4:44 PM, Javier Martinez Canillas <javierm@redhat.com> wrote: > On 07/04/2018 03:24 PM, Nikolaus Voss wrote: >> I hope you're still not annoyed... > Don't worry for that, it's very hard to get my annoyed :) Javier, thanks for your patience and nice explanation!
Hi Andy, On 07/04/2018 06:11 PM, Andy Shevchenko wrote: > On Wed, Jul 4, 2018 at 4:44 PM, Javier Martinez Canillas > <javierm@redhat.com> wrote: >> On 07/04/2018 03:24 PM, Nikolaus Voss wrote: > >>> I hope you're still not annoyed... >> Don't worry for that, it's very hard to get my annoyed :) > > Javier, thanks for your patience and nice explanation! > You are welcome. I'm glad you found it useful! Best regards,
diff --git a/drivers/iio/accel/st_accel_i2c.c b/drivers/iio/accel/st_accel_i2c.c index 163f7ec189b0..2bc54a5a2c96 100644 --- a/drivers/iio/accel/st_accel_i2c.c +++ b/drivers/iio/accel/st_accel_i2c.c @@ -138,8 +138,7 @@ static const struct i2c_device_id st_accel_id_table[] = { }; MODULE_DEVICE_TABLE(i2c, st_accel_id_table); -static int st_accel_i2c_probe(struct i2c_client *client, - const struct i2c_device_id *id) +static int st_accel_i2c_probe(struct i2c_client *client) { struct iio_dev *indio_dev; struct st_sensor_data *adata; @@ -182,7 +181,7 @@ static struct i2c_driver st_accel_driver = { .of_match_table = of_match_ptr(st_accel_of_match), .acpi_match_table = ACPI_PTR(st_accel_acpi_match), }, - .probe = st_accel_i2c_probe, + .probe_new = st_accel_i2c_probe, .remove = st_accel_i2c_remove, .id_table = st_accel_id_table, };
struct i2c_device_id argument of probe() is not used, so use probe_new() instead. Signed-off-by: Nikolaus Voss <nikolaus.voss@loewensteinmedical.de> --- drivers/iio/accel/st_accel_i2c.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)