| Message ID | 20230824181546.391796-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v3] drm/bridge/analogix/anx78xx: Drop ID table | expand |
Hi, On Thu, Aug 24, 2023 at 11:15 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > The driver has an ID table, but it uses the wrong API for retrieving match > data and that will lead to a crash, if it is instantiated by user space or > using ID. From this, there is no user for the ID table and let's drop it > from the driver as it saves some memory. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Updated commit header. > v1->v2: > * Dropped ID table support. > --- > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 7 ------- > 1 file changed, 7 deletions(-) Reviewed-by: Douglas Anderson <dianders@chromium.org> Unless there are objections, I'm happy to apply this late next week to drm-misc-next. -Doug
Hi Biju, Thank you for the patch. On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: > The driver has an ID table, but it uses the wrong API for retrieving match > data and that will lead to a crash, if it is instantiated by user space or > using ID. From this, there is no user for the ID table and let's drop it > from the driver as it saves some memory. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> I wonder, as the device can only be instantiated from OF, should we add depends on OF to Kconfig, and drop the #if IS_ENABLED(CONFIG_OF) from the driver ? > --- > v2->v3: > * Updated commit header. > v1->v2: > * Dropped ID table support. > --- > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > index 800555aef97f..6169db73d2fe 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > @@ -1367,12 +1367,6 @@ static void anx78xx_i2c_remove(struct i2c_client *client) > kfree(anx78xx->edid); > } > > -static const struct i2c_device_id anx78xx_id[] = { > - { "anx7814", 0 }, > - { /* sentinel */ } > -}; > -MODULE_DEVICE_TABLE(i2c, anx78xx_id); > - > static const struct of_device_id anx78xx_match_table[] = { > { .compatible = "analogix,anx7808", .data = anx7808_i2c_addresses }, > { .compatible = "analogix,anx7812", .data = anx781x_i2c_addresses }, > @@ -1389,7 +1383,6 @@ static struct i2c_driver anx78xx_driver = { > }, > .probe = anx78xx_i2c_probe, > .remove = anx78xx_i2c_remove, > - .id_table = anx78xx_id, > }; > module_i2c_driver(anx78xx_driver); >
Hi Laurent Pinchart, Thanks for the feedback. > Subject: Re: [PATCH v3] drm/bridge/analogix/anx78xx: Drop ID table > > Hi Biju, > > Thank you for the patch. > > On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: > > The driver has an ID table, but it uses the wrong API for retrieving > > match data and that will lead to a crash, if it is instantiated by > > user space or using ID. From this, there is no user for the ID table > > and let's drop it from the driver as it saves some memory. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > I wonder, as the device can only be instantiated from OF, should we add > > depends on OF > > to Kconfig, and drop the > > #if IS_ENABLED(CONFIG_OF) > > from the driver ? OK, will send a separate patch for this, if there is no objection. Cheers, Biju > > > --- > > v2->v3: > > * Updated commit header. > > v1->v2: > > * Dropped ID table support. > > --- > > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > index 800555aef97f..6169db73d2fe 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > @@ -1367,12 +1367,6 @@ static void anx78xx_i2c_remove(struct i2c_client > *client) > > kfree(anx78xx->edid); > > } > > > > -static const struct i2c_device_id anx78xx_id[] = { > > - { "anx7814", 0 }, > > - { /* sentinel */ } > > -}; > > -MODULE_DEVICE_TABLE(i2c, anx78xx_id); > > - > > static const struct of_device_id anx78xx_match_table[] = { > > { .compatible = "analogix,anx7808", .data = anx7808_i2c_addresses }, > > { .compatible = "analogix,anx7812", .data = anx781x_i2c_addresses }, > > @@ -1389,7 +1383,6 @@ static struct i2c_driver anx78xx_driver = { > > }, > > .probe = anx78xx_i2c_probe, > > .remove = anx78xx_i2c_remove, > > - .id_table = anx78xx_id, > > }; > > module_i2c_driver(anx78xx_driver); > > > > -- > Regards, > > Laurent Pinchart
Hi, On Thu, Aug 24, 2023 at 11:26 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Biju, > > Thank you for the patch. > > On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: > > The driver has an ID table, but it uses the wrong API for retrieving match > > data and that will lead to a crash, if it is instantiated by user space or > > using ID. From this, there is no user for the ID table and let's drop it > > from the driver as it saves some memory. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > I wonder, as the device can only be instantiated from OF, should we add > > depends on OF > > to Kconfig, and drop the > > #if IS_ENABLED(CONFIG_OF) > > from the driver ? In my opinion we shouldn't add the "depends on OF" since that will decrease the amount of compile testing. It's somewhat the opposite of adding "if COMPILE_TEST" to your driver. ;-) I think we could get rid of one of the "#if" statements in the driver anyway as of commit c9e358dfc4a8 ("driver-core: remove conditionals around devicetree pointers") from ~12 years ago. If we did something similar in "struct drm_bridge" we could drop both #ifs. -Doug
On Thu, Aug 24, 2023 at 01:51:59PM -0700, Doug Anderson wrote: > On Thu, Aug 24, 2023 at 11:26 AM Laurent Pinchart wrote: > > On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: > > > The driver has an ID table, but it uses the wrong API for retrieving match > > > data and that will lead to a crash, if it is instantiated by user space or > > > using ID. From this, there is no user for the ID table and let's drop it > > > from the driver as it saves some memory. > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > > I wonder, as the device can only be instantiated from OF, should we add > > > > depends on OF > > > > to Kconfig, and drop the > > > > #if IS_ENABLED(CONFIG_OF) > > > > from the driver ? > > In my opinion we shouldn't add the "depends on OF" since that will > decrease the amount of compile testing. It's somewhat the opposite of > adding "if COMPILE_TEST" to your driver. ;-) We could add a || COMPILE_TEST :-) > I think we could get rid of one of the "#if" statements in the driver > anyway as of commit c9e358dfc4a8 ("driver-core: remove conditionals > around devicetree pointers") from ~12 years ago. If we did something > similar in "struct drm_bridge" we could drop both #ifs. I'd be fine with that too.
On Thu, Aug 24, 2023 at 9:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: ... > I wonder, as the device can only be instantiated from OF, should we add > > depends on OF Generally speaking this is a bad idea. It prevents a component from being instantiated on ACPI based systems (even if there is no ACPI ID table, due to a gateway called PRP0001). > to Kconfig, and drop the > > #if IS_ENABLED(CONFIG_OF) > > from the driver ? Contrary this is an idea I fully support!
On Fri, Aug 25, 2023 at 06:48:45AM +0300, Andy Shevchenko wrote: > On Thu, Aug 24, 2023 at 9:26 PM Laurent Pinchart wrote: > > On Thu, Aug 24, 2023 at 07:15:46PM +0100, Biju Das wrote: > > ... > > > I wonder, as the device can only be instantiated from OF, should we add > > > > depends on OF > > Generally speaking this is a bad idea. It prevents a component from > being instantiated on ACPI based systems (even if there is no ACPI ID > table, due to a gateway called PRP0001). I'd be surprised if there were systems were that would work out of the box for this particular driver, so we could always drop the dependency when someone updates the driver to work on ACPI-based systems. I'm however not pushing to add the dependency though. > > to Kconfig, and drop the > > > > #if IS_ENABLED(CONFIG_OF) > > > > from the driver ? > > Contrary this is an idea I fully support! > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c index 800555aef97f..6169db73d2fe 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c @@ -1367,12 +1367,6 @@ static void anx78xx_i2c_remove(struct i2c_client *client) kfree(anx78xx->edid); } -static const struct i2c_device_id anx78xx_id[] = { - { "anx7814", 0 }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(i2c, anx78xx_id); - static const struct of_device_id anx78xx_match_table[] = { { .compatible = "analogix,anx7808", .data = anx7808_i2c_addresses }, { .compatible = "analogix,anx7812", .data = anx781x_i2c_addresses }, @@ -1389,7 +1383,6 @@ static struct i2c_driver anx78xx_driver = { }, .probe = anx78xx_i2c_probe, .remove = anx78xx_i2c_remove, - .id_table = anx78xx_id, }; module_i2c_driver(anx78xx_driver);
The driver has an ID table, but it uses the wrong API for retrieving match data and that will lead to a crash, if it is instantiated by user space or using ID. From this, there is no user for the ID table and let's drop it from the driver as it saves some memory. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Updated commit header. v1->v2: * Dropped ID table support. --- drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 7 ------- 1 file changed, 7 deletions(-)