Re: [PATCH v5 4/6] soc: mfd: add ASPEED AST2600 PCIe BMC device driver

> > +
> > +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev,
> > +                                          struct aspeed_pci_bmc_dev *pci_bmc_dev)
> > +{
> > +     resource_size_t bar = pci_resource_start(pdev, 1);
> > +     struct plat_serial8250_port *port;
> > +     u16 vuart_ioport;
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < ASPEED_BMC_VUART; i++) {
> > +             port = &pci_bmc_dev->uart[i];
> > +
> > +             /* ASPEED BMC device shift addresses by 2 to the left */
> > +             vuart_ioport = vuart_port_addr[i] << 2;
> > +
> > +             port->mapbase = bar + vuart_ioport;
> > +             port->uartclk = 115200 * 16;
> > +             port->irq = pci_irq_vector(pdev, vuart_msi_index[i]);
> > +             port->iotype = UPIO_MEM32;
> > +             port->type = PORT_16550A;
> > +             port->flags |= (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE);
> > +             port->regshift = 2;
> > +     }
> > +
> > +     pci_bmc_dev->cell = (struct mfd_cell) {
> > +             .name           = "serial8250",
> > +             .platform_data  = pci_bmc_dev->uart,
> > +             .pdata_size     = sizeof(pci_bmc_dev->uart),
> > +     };
>
> In the commit message you're describing that the MFD driver will be used for
> setting up MSI interrupts:
> > It doesn't bind the whole PCIe resource to allow each sub-driver to bind the
> > needed addresses.
>
> But you're adding the UART devices here which now means that the core driver is
> doing more than MSI interrupt init.

I might have misunderstood but this is creating the UART device by
using the 'serial8250' driver.

> the core driver is doing more than MSI interrupt init.

Yes, as the core driver should add the devices with the right drivers.
I will add this in the commit message.

> Suggestion from my side would be to have an array of `mfd_cell` where you define
> which devices are part of the MFD,

There is already such an array, it is a one element array as only the
VUART is done.
'struct mfd_cell cell; ' will become an real array when KCS will be
added afterwards.

This is not an 2 cell array for the 2 VUART as the 'serial8250'
manages the two VUART on its own.
From what I have researched, this was the way to add UART devices from
an MFD driver.

> and your VUART lives in another driver.
> See `ls2k-bmc-core` for example.

The UART already lives in another driver: the 'serial8250' driver.

Regards,
Grégoire