Received-SPF: pass (google.com: domain of linux-kernel+bounces-79849-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sat, 24 Feb 2024 17:22:39 -0300
From: "Ricardo B. Marliere" <ricardo@marliere.net>
To: Alex Elder <elder@ieee.org>
Cc: Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, greybus-dev@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] greybus: constify the struct device_type usage
Message-ID: <ids3525jm5zkwkgizt2xtg3do37kqqtfdy5gc2cptdduexoe7r@cr4yxh3tykkh>
References: <20240219-device_cleanup-greybus-v1-1-babb3f65e8cc@marliere.net>
 <cf5b2696-d7ca-40fc-856a-52f352cd270c@ieee.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cf5b2696-d7ca-40fc-856a-52f352cd270c@ieee.org>

On 24 Feb 09:43, Alex Elder wrote:
> On 2/19/24 6:40 AM, Ricardo B. Marliere wrote:
> > Since commit aed65af1cc2f ("drivers: make device_type const"), the driver
> > core can properly handle constant struct device_type. Move the
> > greybus_hd_type, greybus_module_type, greybus_interface_type,
> > greybus_control_type, greybus_bundle_type and greybus_svc_type variables to
> > be constant structures as well, placing it into read-only memory which can
> > not be modified at runtime.
> > 
> > Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net>
> 
> This looks good to me.  Assuming it compiles cleanly:
> 
> Reviewed-by: Alex Elder <elder@linaro.org>

Hi Alex!

Thanks for reviewing.

> 
> On another subject:
> 
> Johan might disagree, but I think it would be nice to make
> the definitions of the Greybus device types as static (private)
> and make the is_gb_host_device() etc. functions real functions
> rather than static inlines in <linux/greybus.h>.
> 
> It turns out that all of the is_gb_*() functions are called only
> from drivers/greybus/core.c; they could all be moved there rather
> than advertising them in <linux/greybus.h>.

I guess it depends whether they would be used somewhere else in the
future. Perhaps it was left there with that intention when it was first
being developed? I agree, though. Will happily send a patch with this if
desired.

Best regards,
-	Ricardo.


> 
> 					-Alex
> 
> 
> > ---
> >   drivers/greybus/bundle.c    |  2 +-
> >   drivers/greybus/control.c   |  2 +-
> >   drivers/greybus/hd.c        |  2 +-
> >   drivers/greybus/interface.c |  2 +-
> >   drivers/greybus/module.c    |  2 +-
> >   drivers/greybus/svc.c       |  2 +-
> >   include/linux/greybus.h     | 12 ++++++------
> >   7 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c
> > index 84660729538b..a6e1cca06172 100644
> > --- a/drivers/greybus/bundle.c
> > +++ b/drivers/greybus/bundle.c
> > @@ -166,7 +166,7 @@ static const struct dev_pm_ops gb_bundle_pm_ops = {
> >   	SET_RUNTIME_PM_OPS(gb_bundle_suspend, gb_bundle_resume, gb_bundle_idle)
> >   };
> >   
> > -struct device_type greybus_bundle_type = {
> > +const struct device_type greybus_bundle_type = {
> >   	.name =		"greybus_bundle",
> >   	.release =	gb_bundle_release,
> >   	.pm =		&gb_bundle_pm_ops,
> > diff --git a/drivers/greybus/control.c b/drivers/greybus/control.c
> > index 359a25841973..b5cf49d09df2 100644
> > --- a/drivers/greybus/control.c
> > +++ b/drivers/greybus/control.c
> > @@ -436,7 +436,7 @@ static void gb_control_release(struct device *dev)
> >   	kfree(control);
> >   }
> >   
> > -struct device_type greybus_control_type = {
> > +const struct device_type greybus_control_type = {
> >   	.name =		"greybus_control",
> >   	.release =	gb_control_release,
> >   };
> > diff --git a/drivers/greybus/hd.c b/drivers/greybus/hd.c
> > index 72b21bf2d7d3..e2f3496bddc3 100644
> > --- a/drivers/greybus/hd.c
> > +++ b/drivers/greybus/hd.c
> > @@ -116,7 +116,7 @@ static void gb_hd_release(struct device *dev)
> >   	kfree(hd);
> >   }
> >   
> > -struct device_type greybus_hd_type = {
> > +const struct device_type greybus_hd_type = {
> >   	.name		= "greybus_host_device",
> >   	.release	= gb_hd_release,
> >   };
> > diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c
> > index 9ec949a438ef..a88dc701289c 100644
> > --- a/drivers/greybus/interface.c
> > +++ b/drivers/greybus/interface.c
> > @@ -765,7 +765,7 @@ static const struct dev_pm_ops gb_interface_pm_ops = {
> >   			   gb_interface_runtime_idle)
> >   };
> >   
> > -struct device_type greybus_interface_type = {
> > +const struct device_type greybus_interface_type = {
> >   	.name =		"greybus_interface",
> >   	.release =	gb_interface_release,
> >   	.pm =		&gb_interface_pm_ops,
> > diff --git a/drivers/greybus/module.c b/drivers/greybus/module.c
> > index 36f77f9e1d74..7f7153a1dd60 100644
> > --- a/drivers/greybus/module.c
> > +++ b/drivers/greybus/module.c
> > @@ -81,7 +81,7 @@ static void gb_module_release(struct device *dev)
> >   	kfree(module);
> >   }
> >   
> > -struct device_type greybus_module_type = {
> > +const struct device_type greybus_module_type = {
> >   	.name		= "greybus_module",
> >   	.release	= gb_module_release,
> >   };
> > diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c
> > index 0d7e749174a4..4256467fcd35 100644
> > --- a/drivers/greybus/svc.c
> > +++ b/drivers/greybus/svc.c
> > @@ -1305,7 +1305,7 @@ static void gb_svc_release(struct device *dev)
> >   	kfree(svc);
> >   }
> >   
> > -struct device_type greybus_svc_type = {
> > +const struct device_type greybus_svc_type = {
> >   	.name		= "greybus_svc",
> >   	.release	= gb_svc_release,
> >   };
> > diff --git a/include/linux/greybus.h b/include/linux/greybus.h
> > index 18c0fb958b74..5f9791fae3c0 100644
> > --- a/include/linux/greybus.h
> > +++ b/include/linux/greybus.h
> > @@ -106,12 +106,12 @@ struct dentry *gb_debugfs_get(void);
> >   
> >   extern struct bus_type greybus_bus_type;
> >   
> > -extern struct device_type greybus_hd_type;
> > -extern struct device_type greybus_module_type;
> > -extern struct device_type greybus_interface_type;
> > -extern struct device_type greybus_control_type;
> > -extern struct device_type greybus_bundle_type;
> > -extern struct device_type greybus_svc_type;
> > +extern const struct device_type greybus_hd_type;
> > +extern const struct device_type greybus_module_type;
> > +extern const struct device_type greybus_interface_type;
> > +extern const struct device_type greybus_control_type;
> > +extern const struct device_type greybus_bundle_type;
> > +extern const struct device_type greybus_svc_type;
> >   
> >   static inline int is_gb_host_device(const struct device *dev)
> >   {
> > 
> > ---
> > base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
> > change-id: 20240219-device_cleanup-greybus-c97c1ef52458
> > 
> > Best regards,
>