On Wed, Jan 22, 2020 at 12:34:54PM +0000, Lee Jones wrote:
> > +static int intel_pmc_probe(struct platform_device *pdev)
> > +{
> > + struct intel_scu_ipc_pdata pdata = {};
> > + struct intel_pmc_dev *pmc;
> > + int ret;
> > +
> > + pmc = devm_kzalloc(&pdev->dev, sizeof(*pmc), GFP_KERNEL);
> > + if (!pmc)
> > + return -ENOMEM;
> > +
> > + pmc->dev = &pdev->dev;
> > + spin_lock_init(&pmc->gcr_lock);
> > +
> > + ret = intel_pmc_get_resources(pdev, pmc, &pdata);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to request resources\n");
> > + return ret;
> > + }
> > +
> > + pmc->scu = devm_intel_scu_ipc_register(&pdev->dev, &pdata);
> > + if (IS_ERR(pmc->scu))
> > + return PTR_ERR(pmc->scu);
>
> *_register is better than *_probe. If it was called that (or maybe
> *_init) initially I may have missed the issue altogether ...
>
> However, I still think it the SCU IPC *device* needs to be a device
> driver and abide by the rules, ensuring it uses the device driver
> model/API. As such, it should be registered and probed as a device.
Which type of device you suggest here? And which bus it should be
registered to? I think we can make this create a platform_device but
then we would need to do that from the PCI driver as well which seems
unnecessary since we already have the struct pci_dev.
For instance in drivers/mfd/intel-lpss* we use similar approach (the
core part is library that gets called by probe drivers (ACPI, PCI). We
don't create any additional platform_devices.
There is another twist. Ideally we would like to see the SCU IPC probed
and intialized before the MFD children so that we know the SCU IPC is
ready by the time the children devices are created. I guess we could
work it around by returning -EPROBE_DEFER but that does not feel right
to be honest.
Thanks!
