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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart