diff mbox series

[v3,3/5] mc: Provide a helper for setting bus_info field

Message ID 20220309163112.11708-4-sakari.ailus@linux.intel.com (mailing list archive)
State Under Review
Delegated to: Sakari Ailus
Headers
Series Set bus_info field in framework |

Commit Message

Sakari Ailus March 9, 2022, 4:31 p.m. UTC 
  The bus_info or a similar field exists in a lot of structs, yet drivers
tend to set the value of that field by themselves in a determinable way.
Thus provide a helper for doing this. To be used in subsequent patches.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-device.h | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart March 16, 2022, 9:18 a.m. UTC | #1 
Hi Sakari,

Thank you for the patch.

On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote:
> The bus_info or a similar field exists in a lot of structs, yet drivers
> tend to set the value of that field by themselves in a determinable way.
> Thus provide a helper for doing this. To be used in subsequent patches.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/media-device.h | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index bc015d2cf541..2122d15bde4e 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -13,12 +13,13 @@
>  
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
>  
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
>  
>  struct ida;
> -struct device;
>  struct media_device;
>  
>  /**
> @@ -181,8 +182,7 @@ struct media_device {
>  	atomic_t request_id;
>  };
>  
> -/* We don't need to include pci.h or usb.h here */
> -struct pci_dev;
> +/* We don't need to include usb.h here */
>  struct usb_device;
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
> @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev,
>  #define media_device_usb_init(mdev, udev, name) \
>  	__media_device_usb_init(mdev, udev, name, KBUILD_MODNAME)
>  
> +
> +/**
> + * media_set_bus_info() - Set bus_info field
> + *
> + * @bus_info:		Variable where to write the bus info (char array)
> + * @bus_info_size:	Length of the bus_info
> + * @dev:		Related struct device
> + *
> + * Sets bus information based on &dev. This is currently done for PCI and
> + * platform devices. dev is required to be non-NULL for this to happen.
> + *
> + * This function is not meant to be called from drivers.
> + */
> +static inline void
> +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
> +{
> +	if (!dev)
> +		strscpy(bus_info, "no bus info", bus_info_size);
> +	else if (dev_is_platform(dev))
> +		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
> +	else if (dev_is_pci(dev))
> +		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
> +}

I wouldn't inline this, as it's not used in any hot path, and will
generate quite a bit of code. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  #endif
Laurent Pinchart March 16, 2022, 9:24 a.m. UTC | #2 
On Wed, Mar 16, 2022 at 11:18:13AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote:
> > The bus_info or a similar field exists in a lot of structs, yet drivers
> > tend to set the value of that field by themselves in a determinable way.
> > Thus provide a helper for doing this. To be used in subsequent patches.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/media-device.h | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index bc015d2cf541..2122d15bde4e 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -13,12 +13,13 @@
> >  
> >  #include <linux/list.h>
> >  #include <linux/mutex.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> >  
> >  #include <media/media-devnode.h>
> >  #include <media/media-entity.h>
> >  
> >  struct ida;
> > -struct device;
> >  struct media_device;
> >  
> >  /**
> > @@ -181,8 +182,7 @@ struct media_device {
> >  	atomic_t request_id;
> >  };
> >  
> > -/* We don't need to include pci.h or usb.h here */
> > -struct pci_dev;
> > +/* We don't need to include usb.h here */
> >  struct usb_device;
> >  
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> > @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev,
> >  #define media_device_usb_init(mdev, udev, name) \
> >  	__media_device_usb_init(mdev, udev, name, KBUILD_MODNAME)
> >  
> > +
> > +/**
> > + * media_set_bus_info() - Set bus_info field
> > + *
> > + * @bus_info:		Variable where to write the bus info (char array)
> > + * @bus_info_size:	Length of the bus_info
> > + * @dev:		Related struct device
> > + *
> > + * Sets bus information based on &dev. This is currently done for PCI and
> > + * platform devices. dev is required to be non-NULL for this to happen.
> > + *
> > + * This function is not meant to be called from drivers.
> > + */
> > +static inline void
> > +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
> > +{
> > +	if (!dev)
> > +		strscpy(bus_info, "no bus info", bus_info_size);
> > +	else if (dev_is_platform(dev))
> > +		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
> > +	else if (dev_is_pci(dev))
> > +		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
> > +}
> 
> I wouldn't inline this, as it's not used in any hot path, and will
> generate quite a bit of code. Apart from that,

But the function is only called in two places, and we'd have to export
it, and handle the case where MC is disabled. Possibly not worth it,
although it would be nice to not inline it if possible. If there's a
suitable location to make that easy let's do so, otherwise you can keep
it inline.

(By the way, at some point we may want to not make MC optional)

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  #endif
Sakari Ailus March 16, 2022, 9:50 a.m. UTC | #3 
On Wed, Mar 16, 2022 at 11:24:32AM +0200, Laurent Pinchart wrote:
> On Wed, Mar 16, 2022 at 11:18:13AM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote:
> > > The bus_info or a similar field exists in a lot of structs, yet drivers
> > > tend to set the value of that field by themselves in a determinable way.
> > > Thus provide a helper for doing this. To be used in subsequent patches.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  include/media/media-device.h | 30 +++++++++++++++++++++++++++---
> > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > > index bc015d2cf541..2122d15bde4e 100644
> > > --- a/include/media/media-device.h
> > > +++ b/include/media/media-device.h
> > > @@ -13,12 +13,13 @@
> > >  
> > >  #include <linux/list.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/platform_device.h>
> > >  
> > >  #include <media/media-devnode.h>
> > >  #include <media/media-entity.h>
> > >  
> > >  struct ida;
> > > -struct device;
> > >  struct media_device;
> > >  
> > >  /**
> > > @@ -181,8 +182,7 @@ struct media_device {
> > >  	atomic_t request_id;
> > >  };
> > >  
> > > -/* We don't need to include pci.h or usb.h here */
> > > -struct pci_dev;
> > > +/* We don't need to include usb.h here */
> > >  struct usb_device;
> > >  
> > >  #ifdef CONFIG_MEDIA_CONTROLLER
> > > @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev,
> > >  #define media_device_usb_init(mdev, udev, name) \
> > >  	__media_device_usb_init(mdev, udev, name, KBUILD_MODNAME)
> > >  
> > > +
> > > +/**
> > > + * media_set_bus_info() - Set bus_info field
> > > + *
> > > + * @bus_info:		Variable where to write the bus info (char array)
> > > + * @bus_info_size:	Length of the bus_info
> > > + * @dev:		Related struct device
> > > + *
> > > + * Sets bus information based on &dev. This is currently done for PCI and
> > > + * platform devices. dev is required to be non-NULL for this to happen.
> > > + *
> > > + * This function is not meant to be called from drivers.
> > > + */
> > > +static inline void
> > > +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
> > > +{
> > > +	if (!dev)
> > > +		strscpy(bus_info, "no bus info", bus_info_size);
> > > +	else if (dev_is_platform(dev))
> > > +		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
> > > +	else if (dev_is_pci(dev))
> > > +		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
> > > +}
> > 
> > I wouldn't inline this, as it's not used in any hot path, and will
> > generate quite a bit of code. Apart from that,
> 
> But the function is only called in two places, and we'd have to export
> it, and handle the case where MC is disabled. Possibly not worth it,
> although it would be nice to not inline it if possible. If there's a
> suitable location to make that easy let's do so, otherwise you can keep
> it inline.

There's no such location currently. If one will be added, this should be
moved there. But it's not really worth a new kernel module at the moment.

> 
> (By the way, at some point we may want to not make MC optional)

Yes.

> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!
diff mbox series

Patch

diff --git a/include/media/media-device.h b/include/media/media-device.h
index bc015d2cf541..2122d15bde4e 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -13,12 +13,13 @@ 
 
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
 
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
 
 struct ida;
-struct device;
 struct media_device;
 
 /**
@@ -181,8 +182,7 @@  struct media_device {
 	atomic_t request_id;
 };
 
-/* We don't need to include pci.h or usb.h here */
-struct pci_dev;
+/* We don't need to include usb.h here */
 struct usb_device;
 
 #ifdef CONFIG_MEDIA_CONTROLLER
@@ -502,4 +502,28 @@  static inline void __media_device_usb_init(struct media_device *mdev,
 #define media_device_usb_init(mdev, udev, name) \
 	__media_device_usb_init(mdev, udev, name, KBUILD_MODNAME)
 
+
+/**
+ * media_set_bus_info() - Set bus_info field
+ *
+ * @bus_info:		Variable where to write the bus info (char array)
+ * @bus_info_size:	Length of the bus_info
+ * @dev:		Related struct device
+ *
+ * Sets bus information based on &dev. This is currently done for PCI and
+ * platform devices. dev is required to be non-NULL for this to happen.
+ *
+ * This function is not meant to be called from drivers.
+ */
+static inline void
+media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
+{
+	if (!dev)
+		strscpy(bus_info, "no bus info", bus_info_size);
+	else if (dev_is_platform(dev))
+		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
+	else if (dev_is_pci(dev))
+		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
+}
+
 #endif