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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxx>
Thanks!
--
Sakari Ailus
