Re: [PATCH v7 1/3] mfd: ls2kbmc: Introduce Loongson-2K BMC core driver

[Lee Jones <lee@kernel.org>]

On Fri, 11 Jul 2025, Binbin Zhou wrote:

> Hi Lee:
> 
> Thanks for your review.
> 
> On Thu, Jul 10, 2025 at 5:56 PM Lee Jones <lee@kernel.org> wrote:
> >
> > On Fri, 04 Jul 2025, Binbin Zhou wrote:
> >
> > > The Loongson-2K Board Management Controller provides an PCIe interface
> > > to the host to access the feature implemented in the BMC.
> > >
> > > The BMC is assembled on a server similar to the server machine with
> > > Loongson-3 CPU. It supports multiple sub-devices like DRM and IPMI.
> > >
> > > Co-developed-by: Chong Qiao <qiaochong@loongson.cn>
> > > Signed-off-by: Chong Qiao <qiaochong@loongson.cn>
> > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > Acked-by: Corey Minyard <corey@minyard.net>
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---
> > >  MAINTAINERS                 |   6 ++
> > >  drivers/mfd/Kconfig         |  13 +++
> > >  drivers/mfd/Makefile        |   2 +
> > >  drivers/mfd/ls2k-bmc-core.c | 156 ++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 177 insertions(+)

[...]

> > > +static int ls2k_bmc_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > > +{
> > > +     struct simplefb_platform_data pd;
> > > +     resource_size_t base;
> > > +     int ret;
> > > +
> > > +     ret = pci_enable_device(dev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = ls2k_bmc_parse_mode(dev, &pd);
> > > +     if (ret)
> > > +             goto disable_pci;
> > > +
> > > +     ls2k_bmc_cells[0].platform_data = &pd;
> > > +     ls2k_bmc_cells[0].pdata_size = sizeof(pd);
> >
> > This is fragile.
> >
> > Please identify the elements in ls2k_bmc_cells and use it to index here.
> >
> > See: `static struct mfd_cell as3711_subdevs`
> 
> How about this:
> 
> enum {
>         LS2K_BMC_DISPLAY,
>         LS2k_BMC_IPMI0,
>         LS2k_BMC_IPMI1,
>         LS2k_BMC_IPMI2,
>         LS2k_BMC_IPMI3,
>         LS2k_BMC_IPMI4,
> };
> 
> static struct mfd_cell ls2k_bmc_cells[] = {
>         [LS2K_BMC_DISPLAY] = {
>                 .name = "simple-framebuffer",
>                 .num_resources = ARRAY_SIZE(ls2k_display_resources),
>                 .resources = ls2k_display_resources
>         },
>         [LS2k_BMC_IPMI0] = {
>                 .name = "ls2k-ipmi-si",
>                 .num_resources = ARRAY_SIZE(ls2k_ipmi0_resources),
>                 .resources = ls2k_ipmi0_resources
>         },
>         [LS2k_BMC_IPMI1] = {
>                 .name = "ls2k-ipmi-si",
>                 .num_resources = ARRAY_SIZE(ls2k_ipmi1_resources),
>                 .resources = ls2k_ipmi1_resources
>         },
>         [LS2k_BMC_IPMI2] = {
>                 .name = "ls2k-ipmi-si",
>                 .num_resources = ARRAY_SIZE(ls2k_ipmi2_resources),
>                 .resources = ls2k_ipmi2_resources
>         },
>         [LS2k_BMC_IPMI3] = {
>                 .name = "ls2k-ipmi-si",
>                 .num_resources = ARRAY_SIZE(ls2k_ipmi3_resources),
>                 .resources = ls2k_ipmi3_resources
>         },
>         [LS2k_BMC_IPMI4] = {
>                 .name = "ls2k-ipmi-si",
>                 .num_resources = ARRAY_SIZE(ls2k_ipmi4_resources),
>                 .resources = ls2k_ipmi4_resources
>         },
> };
> 
> and
> 
>         ls2k_bmc_cells[LS2K_BMC_DISPLAY].platform_data = &pd;
>         ls2k_bmc_cells[LS2K_BMC_DISPLAY].pdata_size = sizeof(pd);

Yes, that's it.

-- 
Lee Jones [李琼斯]