| Message ID | 20230826071901.29420-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Drop ID table and conditionals around of_node pointers for anx78xx driver | expand |
On Sat, Aug 26, 2023 at 08:19:01AM +0100, Biju Das wrote: > Having conditional around the of_node pointers turns out to make driver > code use ugly #ifdef and #if blocks. So drop the conditionals. ... > -#ifdef CONFIG_OF > /** @of_node: device node pointer to the bridge */ > struct device_node *of_node; > -#endif This simply has to be struct fwnode_handle to begin with. Can you convert the driver to use it?
Hi Biju, Thank you for the patch. On Sat, Aug 26, 2023 at 08:19:01AM +0100, Biju Das wrote: > Having conditional around the of_node pointers turns out to make driver > code use ugly #ifdef and #if blocks. So drop the conditionals. > > Suggested-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v4: > * New patch > --- > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 2 -- > include/drm/drm_bridge.h | 2 -- This should be split in two patches, one addressing drm_bridge.h (and likely referencing the commit that dropped the conditionals in struct device, as mentioned by Doug), and one for the drivers. > 2 files changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > index 6169db73d2fe..ad8241758896 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > @@ -1231,9 +1231,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client) > > mutex_init(&anx78xx->lock); > > -#if IS_ENABLED(CONFIG_OF) > anx78xx->bridge.of_node = client->dev.of_node; > -#endif > > anx78xx->client = client; > i2c_set_clientdata(client, anx78xx); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index c339fc85fd07..d49d5c03df3e 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -716,10 +716,8 @@ struct drm_bridge { > struct drm_encoder *encoder; > /** @chain_node: used to form a bridge chain */ > struct list_head chain_node; > -#ifdef CONFIG_OF > /** @of_node: device node pointer to the bridge */ > struct device_node *of_node; > -#endif > /** @list: to keep track of all added bridges */ > struct list_head list; > /**
On Mon, Aug 28, 2023 at 02:17:02PM +0300, Andy Shevchenko wrote: > On Sat, Aug 26, 2023 at 08:19:01AM +0100, Biju Das wrote: > > Having conditional around the of_node pointers turns out to make driver > > code use ugly #ifdef and #if blocks. So drop the conditionals. > > ... > > > -#ifdef CONFIG_OF > > /** @of_node: device node pointer to the bridge */ > > struct device_node *of_node; > > -#endif > > This simply has to be struct fwnode_handle to begin with. > Can you convert the driver to use it? While that's possibly a good idea, it will require touching all the bridge drivers that set this field, so I think it could be done separately.
On Mon, Aug 28, 2023 at 02:29:21PM +0300, Laurent Pinchart wrote: > On Mon, Aug 28, 2023 at 02:17:02PM +0300, Andy Shevchenko wrote: > > On Sat, Aug 26, 2023 at 08:19:01AM +0100, Biju Das wrote: > > > Having conditional around the of_node pointers turns out to make driver > > > code use ugly #ifdef and #if blocks. So drop the conditionals. ... > > > -#ifdef CONFIG_OF > > > /** @of_node: device node pointer to the bridge */ > > > struct device_node *of_node; > > > -#endif > > > > This simply has to be struct fwnode_handle to begin with. > > Can you convert the driver to use it? > > While that's possibly a good idea, it will require touching all the > bridge drivers that set this field, so I think it could be done > separately. Okay.
Hello! Thanks for the patch. On Saturday, August 26, 2023 04:19 -03, Biju Das <biju.das.jz@bp.renesas.com> wrote: > Having conditional around the of_node pointers turns out to make driver > code use ugly #ifdef and #if blocks. So drop the conditionals. It would be nice to explain why those ifdev/if conditionals are not required anymore (besides the cosmetic part). Regards, Helen > > Suggested-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v4: > * New patch > --- > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 2 -- > include/drm/drm_bridge.h | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > index 6169db73d2fe..ad8241758896 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > @@ -1231,9 +1231,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client) > > mutex_init(&anx78xx->lock); > > -#if IS_ENABLED(CONFIG_OF) > anx78xx->bridge.of_node = client->dev.of_node; > -#endif > > anx78xx->client = client; > i2c_set_clientdata(client, anx78xx); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index c339fc85fd07..d49d5c03df3e 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -716,10 +716,8 @@ struct drm_bridge { > struct drm_encoder *encoder; > /** @chain_node: used to form a bridge chain */ > struct list_head chain_node; > -#ifdef CONFIG_OF > /** @of_node: device node pointer to the bridge */ > struct device_node *of_node; > -#endif > /** @list: to keep track of all added bridges */ > struct list_head list; > /** > -- > 2.25.1 >
Hi Helen Mae Koike Fornazier, > Subject: Re: [PATCH v4 2/2] drm/bridge: Drop conditionals around of_node > pointers > > Hello! > > Thanks for the patch. > > On Saturday, August 26, 2023 04:19 -03, Biju Das > <biju.das.jz@bp.renesas.com> wrote: > > > Having conditional around the of_node pointers turns out to make > > driver code use ugly #ifdef and #if blocks. So drop the conditionals. > > It would be nice to explain why those ifdev/if conditionals are not > required anymore (besides the cosmetic part). OK ,will explain as if we include the header mod_devicetable.h then we don't need conditionals. Cheers, Biju > > Regards, > Helen > > > > > Suggested-by: Douglas Anderson <dianders@chromium.org> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v4: > > * New patch > > --- > > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 2 -- > > include/drm/drm_bridge.h | 2 -- > > 2 files changed, 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > index 6169db73d2fe..ad8241758896 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > @@ -1231,9 +1231,7 @@ static int anx78xx_i2c_probe(struct i2c_client > > *client) > > > > mutex_init(&anx78xx->lock); > > > > -#if IS_ENABLED(CONFIG_OF) > > anx78xx->bridge.of_node = client->dev.of_node; -#endif > > > > anx78xx->client = client; > > i2c_set_clientdata(client, anx78xx); diff --git > > a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index > > c339fc85fd07..d49d5c03df3e 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -716,10 +716,8 @@ struct drm_bridge { > > struct drm_encoder *encoder; > > /** @chain_node: used to form a bridge chain */ > > struct list_head chain_node; > > -#ifdef CONFIG_OF > > /** @of_node: device node pointer to the bridge */ > > struct device_node *of_node; > > -#endif > > /** @list: to keep track of all added bridges */ > > struct list_head list; > > /** > > -- > > 2.25.1 > >
Hi Laurent Pinchart, > Subject: Re: [PATCH v4 2/2] drm/bridge: Drop conditionals around of_node > pointers > > Hi Biju, > > Thank you for the patch. > > On Sat, Aug 26, 2023 at 08:19:01AM +0100, Biju Das wrote: > > Having conditional around the of_node pointers turns out to make > > driver code use ugly #ifdef and #if blocks. So drop the conditionals. > > > > Suggested-by: Douglas Anderson <dianders@chromium.org> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v4: > > * New patch > > --- > > drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 2 -- > > include/drm/drm_bridge.h | 2 -- > > This should be split in two patches, one addressing drm_bridge.h (and > likely referencing the commit that dropped the conditionals in struct > device, as mentioned by Doug), and one for the drivers. OK, will create 2 patches. Cheers, Biju > > > 2 files changed, 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > index 6169db73d2fe..ad8241758896 100644 > > --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > > @@ -1231,9 +1231,7 @@ static int anx78xx_i2c_probe(struct i2c_client > > *client) > > > > mutex_init(&anx78xx->lock); > > > > -#if IS_ENABLED(CONFIG_OF) > > anx78xx->bridge.of_node = client->dev.of_node; -#endif > > > > anx78xx->client = client; > > i2c_set_clientdata(client, anx78xx); diff --git > > a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index > > c339fc85fd07..d49d5c03df3e 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -716,10 +716,8 @@ struct drm_bridge { > > struct drm_encoder *encoder; > > /** @chain_node: used to form a bridge chain */ > > struct list_head chain_node; > > -#ifdef CONFIG_OF > > /** @of_node: device node pointer to the bridge */ > > struct device_node *of_node; > > -#endif > > /** @list: to keep track of all added bridges */ > > struct list_head list; > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c index 6169db73d2fe..ad8241758896 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c @@ -1231,9 +1231,7 @@ static int anx78xx_i2c_probe(struct i2c_client *client) mutex_init(&anx78xx->lock); -#if IS_ENABLED(CONFIG_OF) anx78xx->bridge.of_node = client->dev.of_node; -#endif anx78xx->client = client; i2c_set_clientdata(client, anx78xx); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index c339fc85fd07..d49d5c03df3e 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -716,10 +716,8 @@ struct drm_bridge { struct drm_encoder *encoder; /** @chain_node: used to form a bridge chain */ struct list_head chain_node; -#ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; -#endif /** @list: to keep track of all added bridges */ struct list_head list; /**
Having conditional around the of_node pointers turns out to make driver code use ugly #ifdef and #if blocks. So drop the conditionals. Suggested-by: Douglas Anderson <dianders@chromium.org> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v4: * New patch --- drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 2 -- include/drm/drm_bridge.h | 2 -- 2 files changed, 4 deletions(-)