| Message ID | 20230813085137.74608-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm/bridge/analogix/anx78xx: Extend match data support for ID table | expand |
Hi, On Sun, Aug 13, 2023 at 1:51 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > The driver has ID table, but still it uses device_get_match_data() > for retrieving match data. Replace device_get_match_data-> > i2c_get_match_data() for retrieving OF/ACPI/I2C match data by adding > match data for ID table similar to OF table. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > This patch is only compile tested > --- > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) It seems like this is a sign that nobody is actually using the i2c match table. It was probably added because the original author just copy/pasted from something else, but obviously it hasn't been kept up to date and isn't working. While your patch would make it work for "anx7814", it wouldn't make it work for any of the other similar parts. ...and yes, you could add support for those parts in your patch too, but IMO it makes more sense to just delete the i2c table and when someone has an actual need then they can re-add it. Sound OK? -Doug
Hi Doug Anderson, > Subject: Re: [PATCH] drm/bridge/analogix/anx78xx: Extend match data support > for ID table > > Hi, > > On Sun, Aug 13, 2023 at 1:51 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > The driver has ID table, but still it uses device_get_match_data() > > for retrieving match data. Replace device_get_match_data-> > > i2c_get_match_data() for retrieving OF/ACPI/I2C match data by adding > > match data for ID table similar to OF table. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > This patch is only compile tested > > --- > > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > It seems like this is a sign that nobody is actually using the i2c match > table. It was probably added because the original author just copy/pasted > from something else, but obviously it hasn't been kept up to date and isn't > working. While your patch would make it work for "anx7814", it wouldn't > make it work for any of the other similar parts. ...and yes, you could add > support for those parts in your patch too, but IMO it makes more sense to > just delete the i2c table and when someone has an actual need then they can > re-add it. > > Sound OK? Yes, it make sense, as it saves some memory. Cheers, Biju
On Wed, Aug 23, 2023 at 5:36 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > On Sun, Aug 13, 2023 at 1:51 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: ... > > It seems like this is a sign that nobody is actually using the i2c match > > table. You can't know. The I2C ID table allows to instantiate a device from user space by supplying it's address and a name, that has to be matched with the one in ID table. > > It was probably added because the original author just copy/pasted > > from something else, but obviously it hasn't been kept up to date and isn't > > working. How can you be so sure? > > While your patch would make it work for "anx7814", it wouldn't > > make it work for any of the other similar parts. ...and yes, you could add > > support for those parts in your patch too, but IMO it makes more sense to > > just delete the i2c table and when someone has an actual need then they can > > re-add it. > > > > Sound OK? No. Please, do not remove the I2C ID table. It had already been discussed a few years ago. > Yes, it make sense, as it saves some memory.
On Wed, Aug 23, 2023 at 7:52 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 23, 2023 at 5:36 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > On Sun, Aug 13, 2023 at 1:51 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: ... > > > It seems like this is a sign that nobody is actually using the i2c match > > > table. > > You can't know. The I2C ID table allows to instantiate a device from > user space by supplying it's address and a name, that has to be > matched with the one in ID table. > > > > It was probably added because the original author just copy/pasted > > > from something else, but obviously it hasn't been kept up to date and isn't > > > working. > > How can you be so sure? > > > > While your patch would make it work for "anx7814", it wouldn't > > > make it work for any of the other similar parts. ...and yes, you could add > > > support for those parts in your patch too, but IMO it makes more sense to > > > just delete the i2c table and when someone has an actual need then they can > > > re-add it. > > > > > > Sound OK? > > No. Please, do not remove the I2C ID table. It had already been > discussed a few years ago. > > > Yes, it make sense, as it saves some memory Okay, reading code a bit, it seems that it won't work with purely i2c ID matching. So the question here is "Do we want to allow enumeration via sysfs or not?"
Hi, On Wed, Aug 23, 2023 at 9:53 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Aug 23, 2023 at 5:36 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > On Sun, Aug 13, 2023 at 1:51 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: > > ... > > > > It seems like this is a sign that nobody is actually using the i2c match > > > table. > > You can't know. The I2C ID table allows to instantiate a device from > user space by supplying it's address and a name, that has to be > matched with the one in ID table. In general, right, you can't know. ...and in general, I wouldn't have suggested removing the table. However, in this specific case I think we have a very good idea that nobody is using it. Specifically, if you take a look at Biju's patch you can see that if anyone had been trying to use the I2C table then they would have been getting a NULL pointer dereference at probe time for the last ~5 years. Specifically, I think that as of commit 025910db8057 ("drm/bridge: analogix-anx78xx: add support for 7808 addresses") that if anyone were using the I2C ID table: 1. In anx78xx_i2c_probe(), device_get_match_data() would have returned NULL 2. We would have tried to dereference that NULL in the loop. > > > It was probably added because the original author just copy/pasted > > > from something else, but obviously it hasn't been kept up to date and isn't > > > working. > > How can you be so sure? Unless I misunderstood the code, they'd be crashing. > > > While your patch would make it work for "anx7814", it wouldn't > > > make it work for any of the other similar parts. ...and yes, you could add > > > support for those parts in your patch too, but IMO it makes more sense to > > > just delete the i2c table and when someone has an actual need then they can > > > re-add it. > > > > > > Sound OK? > > No. Please, do not remove the I2C ID table. It had already been > discussed a few years ago. If you really want the table kept then it's no skin off my teeth. I just happened to see that nobody was responding to the patch and I was trying to be helpful. My analysis above showed that the I2C table must not be used, but if you feel strongly that we need to add code then feel free to provide a Reviewed-by tag to Biju's patch! :-) -Doug
On Wed, Aug 23, 2023 at 8:14 PM Doug Anderson <dianders@chromium.org> wrote: > On Wed, Aug 23, 2023 at 9:53 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Aug 23, 2023 at 5:36 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: ... > > No. Please, do not remove the I2C ID table. It had already been > > discussed a few years ago. > > If you really want the table kept then it's no skin off my teeth. I > just happened to see that nobody was responding to the patch and I was > trying to be helpful. My analysis above showed that the I2C table must > not be used, but if you feel strongly that we need to add code then > feel free to provide a Reviewed-by tag to Biju's patch! :-) Have you seen my reply to my reply? I agree with your above analysis.
Hi, On Wed, Aug 23, 2023 at 10:10 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > No. Please, do not remove the I2C ID table. It had already been > > discussed a few years ago. > > > > > Yes, it make sense, as it saves some memory > > Okay, reading code a bit, it seems that it won't work with purely i2c > ID matching. OK, so you are in agreement that it would be OK to drop the I2C ID table? > So the question here is "Do we want to allow enumeration via sysfs or not?" Is there some pressing need for it? If not, I guess I'd tend to wait until someone needs this support before adding it. -Doug
On Wed, Aug 23, 2023 at 9:39 PM Doug Anderson <dianders@chromium.org> wrote: > On Wed, Aug 23, 2023 at 10:10 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > > No. Please, do not remove the I2C ID table. It had already been > > > discussed a few years ago. > > > > > > > Yes, it make sense, as it saves some memory > > > > Okay, reading code a bit, it seems that it won't work with purely i2c > > ID matching. > > OK, so you are in agreement that it would be OK to drop the I2C ID table? Yes. > > So the question here is "Do we want to allow enumeration via sysfs or not?" > > Is there some pressing need for it? If not, I guess I'd tend to wait > until someone needs this support before adding it. Depends. Is this device anyhow useful IRL as standalone if instantiated via sysfs? I think it may be not, so it's unlikely we want to have sysfs option for it.
Hi Andy Shevchenko, > Subject: Re: [PATCH] drm/bridge/analogix/anx78xx: Extend match data support > for ID table > > On Wed, Aug 23, 2023 at 9:39 PM Doug Anderson <dianders@chromium.org> > wrote: > > On Wed, Aug 23, 2023 at 10:10 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > > No. Please, do not remove the I2C ID table. It had already been > > > > discussed a few years ago. > > > > > > > > > Yes, it make sense, as it saves some memory > > > > > > Okay, reading code a bit, it seems that it won't work with purely > > > i2c ID matching. > > > > OK, so you are in agreement that it would be OK to drop the I2C ID table? > > Yes. > > > > So the question here is "Do we want to allow enumeration via sysfs or > not?" > > > > Is there some pressing need for it? If not, I guess I'd tend to wait > > until someone needs this support before adding it. > > Depends. Is this device anyhow useful IRL as standalone if instantiated via > sysfs? I think it may be not, so it's unlikely we want to have sysfs option > for it. Sysfs option will match against ID table or OF table? ID table, means it will crash. OF table means it works ok as it has proper matching support. Cheers, Biju
On Thu, 24 Aug 2023 12:59:19 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Aug 23, 2023 at 9:39 PM Doug Anderson <dianders@chromium.org> wrote: > > On Wed, Aug 23, 2023 at 10:10 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > > No. Please, do not remove the I2C ID table. It had already been > > > > discussed a few years ago. > > > > > > > > > Yes, it make sense, as it saves some memory > > > > > > Okay, reading code a bit, it seems that it won't work with purely i2c > > > ID matching. > > > > OK, so you are in agreement that it would be OK to drop the I2C ID table? > > Yes. > > > > So the question here is "Do we want to allow enumeration via sysfs or not?" > > > > Is there some pressing need for it? If not, I guess I'd tend to wait > > until someone needs this support before adding it. > > Depends. Is this device anyhow useful IRL as standalone if > instantiated via sysfs? I think it may be not, so it's unlikely we > want to have sysfs option for it. > So this is what the id table is about :D I guess I should remove it for leds-turris-omnia :D
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c index 800555aef97f..f56a46b993a7 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c @@ -1259,7 +1259,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client) } /* Map slave addresses of ANX7814 */ - i2c_addresses = device_get_match_data(&client->dev); + i2c_addresses = i2c_get_match_data(client); for (i = 0; i < I2C_NUM_ADDRESSES; i++) { struct i2c_client *i2c_dummy; @@ -1368,7 +1368,7 @@ static void anx78xx_i2c_remove(struct i2c_client *client) } static const struct i2c_device_id anx78xx_id[] = { - { "anx7814", 0 }, + { "anx7814", (kernel_ulong_t)anx781x_i2c_addresses }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(i2c, anx78xx_id);
The driver has ID table, but still it uses device_get_match_data() for retrieving match data. Replace device_get_match_data-> i2c_get_match_data() for retrieving OF/ACPI/I2C match data by adding match data for ID table similar to OF table. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- This patch is only compile tested --- drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)