On Wed, Jun 09, 2021 at 09:44:08AM -0500, Pierre-Louis Bossart wrote: > The model is exactly the same as what we have today with the platform > devices. We did not add ANY new fields or information, what is passed in > that structure is exactly the same as what we do upstream today with the > platform devices. > > To make my point, here is the structure in intel.h as of v5.13-rc1 > > struct sdw_intel_link_res { > struct platform_device *pdev; > void __iomem *mmio_base; /* not strictly needed, useful for debug */ > void __iomem *registers; > void __iomem *shim; > void __iomem *alh; > int irq; > const struct sdw_intel_ops *ops; > struct device *dev; > struct mutex *shim_lock; /* protect shared registers */ > u32 *shim_mask; > u32 clock_stop_quirks; > u32 link_mask; > struct sdw_cdns *cdns; > struct list_head list; > }; > > and here's what we suggested in this patch: > > struct sdw_intel_link_res { > void __iomem *mmio_base; /* not strictly needed, useful for debug */ > void __iomem *registers; > void __iomem *shim; > void __iomem *alh; > int irq; > const struct sdw_intel_ops *ops; > struct device *dev; > struct mutex *shim_lock; /* protect shared registers */ > u32 *shim_mask; > u32 clock_stop_quirks; > u32 link_mask; > struct sdw_cdns *cdns; > struct list_head list; > }; > > You will notice that we removed the platform_device *pdev, but embedded this > structure into a larger one to make use of container_of() > > struct sdw_intel_link_dev { > struct auxiliary_device auxdev; > struct sdw_intel_link_res link_res; > }; > > That's it. We did not change anything else, all the other fields are > identical. We are only changing the TYPE of device and the interfaces for > probe/remove but using the same information and the same device hierarchy. And this is the correct thing to do, you have done it properly here, nice work. greg k-h