diff mbox series

[v3] drm/bridge/analogix/anx78xx: Drop ID table

Message ID 20230824181546.391796-1-biju.das.jz@bp.renesas.com (mailing list archive)
State New, archived
Headers show
Series [v3] drm/bridge/analogix/anx78xx: Drop ID table | expand

Commit Message

Biju Das Aug. 24, 2023, 6:15 p.m. UTC
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(-)

Comments

Doug Anderson Aug. 24, 2023, 6:17 p.m. UTC | #1
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
Laurent Pinchart Aug. 24, 2023, 6:26 p.m. UTC | #2
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);
>
Biju Das Aug. 24, 2023, 8:50 p.m. UTC | #3
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
Doug Anderson Aug. 24, 2023, 8:51 p.m. UTC | #4
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
Laurent Pinchart Aug. 24, 2023, 10:01 p.m. UTC | #5
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.
Andy Shevchenko Aug. 25, 2023, 3:48 a.m. UTC | #6
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!
Laurent Pinchart Aug. 25, 2023, 10:57 a.m. UTC | #7
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 mbox series

Patch

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);