| Message ID | 20170824134110.63335-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 24, 2017 at 04:41:10PM +0300, Andy Shevchenko wrote: > There is no platform code that uses i2c module table. > Remove it altogether and adjust ->probe() to be ->probe_new(). What is the value in doing this? Removing the ability to instantiate via board files is just inconveniencing people doing that, it's not a goal in itself.
On Thu, 24 Aug 2017 15:49:24 +0200, Mark Brown wrote: > > On Thu, Aug 24, 2017 at 04:41:10PM +0300, Andy Shevchenko wrote: > > > There is no platform code that uses i2c module table. > > Remove it altogether and adjust ->probe() to be ->probe_new(). > > What is the value in doing this? Removing the ability to instantiate > via board files is just inconveniencing people doing that, it's not a > goal in itself. But the id table is already empty...? Takashi
On Thu, Aug 24, 2017 at 04:33:29PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > On Thu, Aug 24, 2017 at 04:41:10PM +0300, Andy Shevchenko wrote: > > > > > There is no platform code that uses i2c module table. > > > Remove it altogether and adjust ->probe() to be ->probe_new(). > > What is the value in doing this? Removing the ability to instantiate > > via board files is just inconveniencing people doing that, it's not a > > goal in itself. > But the id table is already empty...? Right, that seems like a bug though.
On Thu, 24 Aug 2017 17:49:47 +0200, Mark Brown wrote: > > On Thu, Aug 24, 2017 at 04:33:29PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > On Thu, Aug 24, 2017 at 04:41:10PM +0300, Andy Shevchenko wrote: > > > > > > > There is no platform code that uses i2c module table. > > > > Remove it altogether and adjust ->probe() to be ->probe_new(). > > > > What is the value in doing this? Removing the ability to instantiate > > > via board files is just inconveniencing people doing that, it's not a > > > goal in itself. > > > But the id table is already empty...? > > Right, that seems like a bug though. Well, that's what the commit ddc9e69b9dc2 does after all: it cleans up the legacy usage, and moves all either OF or ACPI matching. If we want to take back old i2c ids, better to put the explicit i2c_match_id() check in rt5677_i2c_probe() instead of sticking with the old i2c probe callback. Takashi
On Thu, 2017-08-24 at 18:11 +0200, Takashi Iwai wrote: > On Thu, 24 Aug 2017 17:49:47 +0200, > Mark Brown wrote: > > > > On Thu, Aug 24, 2017 at 04:33:29PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > On Thu, Aug 24, 2017 at 04:41:10PM +0300, Andy Shevchenko wrote: > > > > > > > > > There is no platform code that uses i2c module table. > > > > > Remove it altogether and adjust ->probe() to be ->probe_new(). > > > > What is the value in doing this? Removing the ability to > > > > instantiate > > > > via board files is just inconveniencing people doing that, it's > > > > not a > > > > goal in itself. > > > But the id table is already empty...? > > > > Right, that seems like a bug though. > > Well, that's what the commit ddc9e69b9dc2 does after all: it cleans up > the legacy usage, and moves all either OF or ACPI matching. > > If we want to take back old i2c ids, better to put the explicit > i2c_match_id() check in rt5677_i2c_probe() instead of sticking with > the old i2c probe callback. Well, there was never user for rt5676, thus, platform code can use driver name to register / create platform device to be matched.
On Thu, Aug 24, 2017 at 06:11:12PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > > But the id table is already empty...? > > Right, that seems like a bug though. > Well, that's what the commit ddc9e69b9dc2 does after all: it cleans up > the legacy usage, and moves all either OF or ACPI matching. Which is broken since not all the world is ACPI or DT. > If we want to take back old i2c ids, better to put the explicit > i2c_match_id() check in rt5677_i2c_probe() instead of sticking with > the old i2c probe callback. That's what I said, yes.
On Fri, 2017-08-25 at 14:11 +0100, Mark Brown wrote: > On Thu, Aug 24, 2017 at 06:11:12PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > But the id table is already empty...? > > > Right, that seems like a bug though. > > Well, that's what the commit ddc9e69b9dc2 does after all: it cleans > > up > > the legacy usage, and moves all either OF or ACPI matching. > > Which is broken since not all the world is ACPI or DT. While I agree on the second part, I don't think it's broken per se, there never was a single user of that, and looking how ASoC board files are organized it is quite unlikely there will be one... > > If we want to take back old i2c ids, better to put the explicit > > i2c_match_id() check in rt5677_i2c_probe() instead of sticking with > > the old i2c probe callback. > > That's what I said, yes. ...though we can (re)introduce this dead code back for a potential user in the future.
On Thu, Aug 24, 2017 at 08:12:12PM +0300, Andy Shevchenko wrote: > On Thu, 2017-08-24 at 18:11 +0200, Takashi Iwai wrote: > > If we want to take back old i2c ids, better to put the explicit > > i2c_match_id() check in rt5677_i2c_probe() instead of sticking with > > the old i2c probe callback. > Well, there was never user for rt5676, thus, platform code can use > driver name to register / create platform device to be matched. The driver handle both rt5676 and rt5677 using different IDs so it can't just rely on the driver table.
On Fri, Aug 25, 2017 at 04:33:08PM +0300, Andy Shevchenko wrote: > On Fri, 2017-08-25 at 14:11 +0100, Mark Brown wrote: > > On Thu, Aug 24, 2017 at 06:11:12PM +0200, Takashi Iwai wrote: > > > Well, that's what the commit ddc9e69b9dc2 does after all: it cleans > > > up > > > the legacy usage, and moves all either OF or ACPI matching. > > Which is broken since not all the world is ACPI or DT. > While I agree on the second part, I don't think it's broken per se, > there never was a single user of that, and looking how ASoC board files > are organized it is quite unlikely there will be one... I have no idea why you say the ASoC machine drivers would prevent someone adding a platform based user, obviously ASoC predates the use of both ACPI and DT quite considerably. > > That's what I said, yes. > ...though we can (re)introduce this dead code back for a potential user > in the future. The IDs are back as a result of the merge resolution I did.
On Fri, 2017-08-25 at 15:21 +0100, Mark Brown wrote: > On Fri, Aug 25, 2017 at 04:33:08PM +0300, Andy Shevchenko wrote: > > On Fri, 2017-08-25 at 14:11 +0100, Mark Brown wrote: > > > On Thu, Aug 24, 2017 at 06:11:12PM +0200, Takashi Iwai wrote: > > > > Well, that's what the commit ddc9e69b9dc2 does after all: it > > > > cleans > > > > up > > > > the legacy usage, and moves all either OF or ACPI matching. > > > Which is broken since not all the world is ACPI or DT. > > While I agree on the second part, I don't think it's broken per se, > > there never was a single user of that, and looking how ASoC board > > files > > are organized it is quite unlikely there will be one... > > I have no idea why you say the ASoC machine drivers would prevent > someone adding a platform based user, For example, looking into sound/soc/mediatek/mt8173/mt8173-rt5650-rt5676.c I see a direct use of codec driver without any platform driver involvement. Is it exception from the rule, or a common use? > obviously ASoC predates the use of > both ACPI and DT quite considerably. Yep, the current users of driver in question either DT or ACPI, would it be pure platform in the future? > > > > That's what I said, yes. > > ...though we can (re)introduce this dead code back for a potential > > user > > in the future. > > The IDs are back as a result of the merge resolution I did. I'm fine as long as it doesn't include broken 'ACPI instance as I2C ID'. Thanks!
On Fri, Aug 25, 2017 at 06:09:52PM +0300, Andy Shevchenko wrote: > On Fri, 2017-08-25 at 15:21 +0100, Mark Brown wrote: > > I have no idea why you say the ASoC machine drivers would prevent > > someone adding a platform based user, > For example, looking into > sound/soc/mediatek/mt8173/mt8173-rt5650-rt5676.c > I see a direct use of codec driver without any platform driver > involvement. > Is it exception from the rule, or a common use? That's an entirely standard driver using normally registered devices? > > obviously ASoC predates the use of > > both ACPI and DT quite considerably. > Yep, the current users of driver in question either DT or ACPI, would it > be pure platform in the future? It could be potentially (well, I2C but registered using a board file). Most architectures still use board files and neither ACPI nor DT has a sensible story for instantiating plugin modules yet.
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c index c735590c5a25..6448b7a00203 100644 --- a/sound/soc/codecs/rt5677.c +++ b/sound/soc/codecs/rt5677.c @@ -5009,11 +5009,6 @@ static const struct regmap_config rt5677_regmap = { .num_ranges = ARRAY_SIZE(rt5677_ranges), }; -static const struct i2c_device_id rt5677_i2c_id[] = { - { } -}; -MODULE_DEVICE_TABLE(i2c, rt5677_i2c_id); - static const struct of_device_id rt5677_of_match[] = { { .compatible = "realtek,rt5677", RT5677 }, { } @@ -5131,8 +5126,7 @@ static void rt5677_free_irq(struct i2c_client *i2c) regmap_del_irq_chip(i2c->irq, rt5677->irq_data); } -static int rt5677_i2c_probe(struct i2c_client *i2c, - const struct i2c_device_id *id) +static int rt5677_i2c_probe(struct i2c_client *i2c) { struct rt5677_priv *rt5677; int ret; @@ -5279,9 +5273,8 @@ static struct i2c_driver rt5677_i2c_driver = { .of_match_table = rt5677_of_match, .acpi_match_table = ACPI_PTR(rt5677_acpi_match), }, - .probe = rt5677_i2c_probe, + .probe_new = rt5677_i2c_probe, .remove = rt5677_i2c_remove, - .id_table = rt5677_i2c_id, }; module_i2c_driver(rt5677_i2c_driver);
There is no platform code that uses i2c module table. Remove it altogether and adjust ->probe() to be ->probe_new(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- sound/soc/codecs/rt5677.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)