| Message ID | 20230818075600.24277-6-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | OF/ACPI/ID Match table improvements for ak8975 driver | expand |
On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > Sort OF table alphabetically by compatibles. > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Wrong, I haven't suggested that. See comment to the previous patch. And this is definitely wrong as Geert explained already why. You need to fix the code that handles the ID table first.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > Sort OF table alphabetically by compatibles. > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Wrong, I haven't suggested that. See comment to the previous patch. > > And this is definitely wrong as Geert explained already why. > You need to fix the code that handles the ID table first. That rule applicable only for fallback. I checked bindings and there are no fallbacks. Cheers, Biju
Hi Andy, On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > Sort OF table alphabetically by compatibles. > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Wrong, I haven't suggested that. See comment to the previous patch. > > And this is definitely wrong as Geert explained already why. > You need to fix the code that handles the ID table first. I retracted my own comment: https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com Upon a second read, I agree my reply Seems like it is, cfr. the scoring system in drivers/of/base.c was confusing, as it was not super clear if it was a response to the first or the second line of your comment: You mean the OF ID list must be specifically ordered?! What a nice minefield! This has to be fixed somewhere else, surely. Conclusion: there is no issue, the scoring system handles primary vs. fallback compatible values, irrespective of ordering. Gr{oetje,eeting}s, Geert
On Fri, Aug 18, 2023 at 11:39:03AM +0000, Biju Das wrote: > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > Sort OF table alphabetically by compatibles. > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > And this is definitely wrong as Geert explained already why. > > You need to fix the code that handles the ID table first. > > That rule applicable only for fallback. I checked bindings and there are no fallbacks. You can't check the _whole_ world, so you checked only bindings that are in tree. But it doesn't matter as a user somewhere may use something you have no access to.
Hi Andy Shevchenko, > Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table > > On Fri, Aug 18, 2023 at 11:39:03AM +0000, Biju Das wrote: > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > > Sort OF table alphabetically by compatibles. > > > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > > > And this is definitely wrong as Geert explained already why. > > > You need to fix the code that handles the ID table first. > > > > That rule applicable only for fallback. I checked bindings and there are > no fallbacks. > > You can't check the _whole_ world, so you checked only bindings that are in > tree. > But it doesn't matter as a user somewhere may use something you have no > access to. Yes true, for those not using tree can patch bindings and modify the driver and dts accordingly. If they are using tree, they must go with binding docs and update driver/bindings accordingly. + device_tree. Cheers, Biju
On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote: > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > Sort OF table alphabetically by compatibles. > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > And this is definitely wrong as Geert explained already why. > > You need to fix the code that handles the ID table first. > > I retracted my own comment: > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com > > Upon a second read, I agree my reply > > Seems like it is, cfr. the scoring system in drivers/of/base.c > > was confusing, as it was not super clear if it was a response to the > first or the second line of your comment: > > You mean the OF ID list must be specifically ordered?! What a nice > minefield! > This has to be fixed somewhere else, surely. > > Conclusion: there is no issue, the scoring system handles primary > vs. fallback compatible values, irrespective of ordering. Now I'm totally confused. Previously you mentioned a couple of different APIs — one in OF, one in SoC. AFAIU the second one still needs to be fixed to follow the logic that OF does. My previous understanding was that OF code — no issue SoC code — the ordering is required to be correct Can you confirm that there is no issue in that second case? And if there is none, why did you mention it?
Hi Andy, On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote: > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > > Sort OF table alphabetically by compatibles. > > > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > > > And this is definitely wrong as Geert explained already why. > > > You need to fix the code that handles the ID table first. > > > > I retracted my own comment: > > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com > > > > Upon a second read, I agree my reply > > > > Seems like it is, cfr. the scoring system in drivers/of/base.c > > > > was confusing, as it was not super clear if it was a response to the > > first or the second line of your comment: > > > > You mean the OF ID list must be specifically ordered?! What a nice > > minefield! > > This has to be fixed somewhere else, surely. > > > > Conclusion: there is no issue, the scoring system handles primary > > vs. fallback compatible values, irrespective of ordering. > > Now I'm totally confused. Previously you mentioned a couple of > different APIs — one in OF, one in SoC. AFAIU the second one > still needs to be fixed to follow the logic that OF does. > > My previous understanding was that > OF code — no issue > SoC code — the ordering is required to be correct Correct. > Can you confirm that there is no issue in that second case? > And if there is none, why did you mention it? There is still an issue (read: you have to be careful) in the second case, which does not matter here, as this driver does not use soc_device_match(). I mentioned soc_device_match() because it is the second popular way to match on OF platforms, but behaves slightly different than of_match_node(). Gr{oetje,eeting}s, Geert
On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote: > On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote: > > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > > > Sort OF table alphabetically by compatibles. > > > > > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > > > > > And this is definitely wrong as Geert explained already why. > > > > You need to fix the code that handles the ID table first. > > > > > > I retracted my own comment: > > > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com > > > > > > Upon a second read, I agree my reply > > > > > > Seems like it is, cfr. the scoring system in drivers/of/base.c > > > > > > was confusing, as it was not super clear if it was a response to the > > > first or the second line of your comment: > > > > > > You mean the OF ID list must be specifically ordered?! What a nice > > > minefield! > > > This has to be fixed somewhere else, surely. > > > > > > Conclusion: there is no issue, the scoring system handles primary > > > vs. fallback compatible values, irrespective of ordering. > > > > Now I'm totally confused. Previously you mentioned a couple of > > different APIs — one in OF, one in SoC. AFAIU the second one > > still needs to be fixed to follow the logic that OF does. > > > > My previous understanding was that > > OF code — no issue > > SoC code — the ordering is required to be correct > > Correct. > > > Can you confirm that there is no issue in that second case? > > And if there is none, why did you mention it? > > There is still an issue (read: you have to be careful) in the second > case, which does not matter here, as this driver does not use > soc_device_match(). > I mentioned soc_device_match() because it is the second popular way > to match on OF platforms, but behaves slightly different than > of_match_node(). Now it's clear, thanks. Biju, please add that to the commit message.
Hi Andy, Thanks for the feedback. > Subject: Re: [PATCH v2 5/5] iio: magnetometer: ak8975: Sort OF table > > On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote: > > On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote: > > > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > > > > Sort OF table alphabetically by compatibles. > > > > > > > > > > > Suggested-by: Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> > > > > > > > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > > > > > > > And this is definitely wrong as Geert explained already why. > > > > > You need to fix the code that handles the ID table first. > > > > > > > > I retracted my own comment: > > > > > > > > Upon a second read, I agree my reply > > > > > > > > Seems like it is, cfr. the scoring system in drivers/of/base.c > > > > > > > > was confusing, as it was not super clear if it was a response to > > > > the first or the second line of your comment: > > > > > > > > You mean the OF ID list must be specifically ordered?! What a > > > > nice minefield! > > > > This has to be fixed somewhere else, surely. > > > > > > > > Conclusion: there is no issue, the scoring system handles primary > > > > vs. fallback compatible values, irrespective of ordering. > > > > > > Now I'm totally confused. Previously you mentioned a couple of > > > different APIs — one in OF, one in SoC. AFAIU the second one still > > > needs to be fixed to follow the logic that OF does. > > > > > > My previous understanding was that > > > OF code — no issue > > > SoC code — the ordering is required to be correct > > > > Correct. > > > > > Can you confirm that there is no issue in that second case? > > > And if there is none, why did you mention it? > > > > There is still an issue (read: you have to be careful) in the second > > case, which does not matter here, as this driver does not use > > soc_device_match(). > > I mentioned soc_device_match() because it is the second popular way to > > match on OF platforms, but behaves slightly different than > > of_match_node(). > > Now it's clear, thanks. > Biju, please add that to the commit message. OK, will mention in the commit message about "soc_device_match". Cheers, Biju
Hi Andy, On Fri, Aug 18, 2023 at 7:04 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Aug 18, 2023 at 05:43:15PM +0200, Geert Uytterhoeven wrote: > > On Fri, Aug 18, 2023 at 5:35 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Aug 18, 2023 at 04:55:18PM +0200, Geert Uytterhoeven wrote: > > > > On Fri, Aug 18, 2023 at 1:30 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Aug 18, 2023 at 08:56:00AM +0100, Biju Das wrote: > > > > > > Sort OF table alphabetically by compatibles. > > > > > > > > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > > > > > Wrong, I haven't suggested that. See comment to the previous patch. > > > > > > > > > > And this is definitely wrong as Geert explained already why. > > > > > You need to fix the code that handles the ID table first. > > > > > > > > I retracted my own comment: > > > > https://lore.kernel.org/r/CAMuHMdUVCS_D0SBtDBrLQbAkdt0ZUbMOca+ukdwUtnGqzUr+cA@mail.gmail.com > > > > > > > > Upon a second read, I agree my reply > > > > > > > > Seems like it is, cfr. the scoring system in drivers/of/base.c > > > > > > > > was confusing, as it was not super clear if it was a response to the > > > > first or the second line of your comment: > > > > > > > > You mean the OF ID list must be specifically ordered?! What a nice > > > > minefield! > > > > This has to be fixed somewhere else, surely. > > > > > > > > Conclusion: there is no issue, the scoring system handles primary > > > > vs. fallback compatible values, irrespective of ordering. > > > > > > Now I'm totally confused. Previously you mentioned a couple of > > > different APIs — one in OF, one in SoC. AFAIU the second one > > > still needs to be fixed to follow the logic that OF does. > > > > > > My previous understanding was that > > > OF code — no issue > > > SoC code — the ordering is required to be correct > > > > Correct. > > > > > Can you confirm that there is no issue in that second case? > > > And if there is none, why did you mention it? > > > > There is still an issue (read: you have to be careful) in the second > > case, which does not matter here, as this driver does not use > > soc_device_match(). > > I mentioned soc_device_match() because it is the second popular way > > to match on OF platforms, but behaves slightly different than > > of_match_node(). > > Now it's clear, thanks. > Biju, please add that to the commit message. All of that? The only thing that matters is that OF match tables use scoring, so order shouldn't matter. soc_device_match() uses different tables, and is irrelevant here. Gr{oetje,eeting}s, Geert
On Fri, 18 Aug 2023 08:56:00 +0100 Biju Das <biju.das.jz@bp.renesas.com> wrote: > Sort OF table alphabetically by compatibles. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> I adjusted this for not taking patch 4 and applied. Thanks, Jonathan > --- > v2: > * New patch > --- > drivers/iio/magnetometer/ak8975.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c > index 295b7be5e36d..7cc443a86995 100644 > --- a/drivers/iio/magnetometer/ak8975.c > +++ b/drivers/iio/magnetometer/ak8975.c > @@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = { > MODULE_DEVICE_TABLE(i2c, ak8975_id); > > static const struct of_device_id ak8975_of_match[] = { > - { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, > { .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] }, > + { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, > { .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] }, > { .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] }, > { .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
On Mon, 28 Aug 2023 15:27:12 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 18 Aug 2023 08:56:00 +0100 > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Sort OF table alphabetically by compatibles. > > > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > I adjusted this for not taking patch 4 and applied. Dropped this one again as I just saw the suggestion to group the prefixed an on prefixed version which is a good idea. Jonathan > > Thanks, > > Jonathan > > > --- > > v2: > > * New patch > > --- > > drivers/iio/magnetometer/ak8975.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c > > index 295b7be5e36d..7cc443a86995 100644 > > --- a/drivers/iio/magnetometer/ak8975.c > > +++ b/drivers/iio/magnetometer/ak8975.c > > @@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = { > > MODULE_DEVICE_TABLE(i2c, ak8975_id); > > > > static const struct of_device_id ak8975_of_match[] = { > > - { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, > > { .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] }, > > + { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, > > { .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] }, > > { .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] }, > > { .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] }, >
diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c index 295b7be5e36d..7cc443a86995 100644 --- a/drivers/iio/magnetometer/ak8975.c +++ b/drivers/iio/magnetometer/ak8975.c @@ -1073,8 +1073,8 @@ static const struct i2c_device_id ak8975_id[] = { MODULE_DEVICE_TABLE(i2c, ak8975_id); static const struct of_device_id ak8975_of_match[] = { - { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, { .compatible = "asahi-kasei,ak8963", .data = &ak_def_array[AK8963] }, + { .compatible = "asahi-kasei,ak8975", .data = &ak_def_array[AK8975] }, { .compatible = "asahi-kasei,ak09911", .data = &ak_def_array[AK09911] }, { .compatible = "asahi-kasei,ak09912", .data = &ak_def_array[AK09912] }, { .compatible = "asahi-kasei,ak09916", .data = &ak_def_array[AK09916] },
Sort OF table alphabetically by compatibles. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2: * New patch --- drivers/iio/magnetometer/ak8975.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)