Re: [PATCH v5 5/6] mfd: motorola-cpcap: diverge configuration per-board

[Lee Jones <lee@kernel.org>]

On Wed, 20 May 2026, Svyatoslav Ryhel wrote:

> ср, 20 трав. 2026 р. о 18:08 Lee Jones <lee@kernel.org> пише:
> >
> > On Sun, 10 May 2026, Svyatoslav Ryhel wrote:
> >
> > > MFD have rigid subdevice structure which does not allow flexible dynamic
> > > subdevice linking. Address this by diverging CPCAP subdevice composition
> > > to take into account board specific configuration.
> > >
> > > Create a common default subdevice composition, rename existing subdevice
> > > composition into cpcap_mapphone_mfd_devices since it targets mainly
> > > Mapphone board.
> > >
> > > Removed st,6556002 as it is no longer applicable to all cases and
> > > duplicates motorola,cpcap, which is used as the default composition.
> > >
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/mfd/motorola-cpcap.c       | 142 ++++++++++++++++-------------
> > >  include/linux/mfd/motorola-cpcap.h |   6 ++
> > >  2 files changed, 87 insertions(+), 61 deletions(-)
> >
> > Looking much better, thanks.
> >
> > Nit: A patch-level changelog really is much more helpful to reviewers.
> >
> 
> Noted, but I will not guarantee that I will do patch-level changelogs, sorry.

That's fine.  All I can do is ask.

Note that, helping out reviewers usually ends up helping you too.

[...]

> > > +static const struct mfd_cell cpcap_default_mfd_devices[] = {
> > > +     MFD_CELL_OF("cpcap_adc", NULL, NULL, 0, 0, "motorola,cpcap-adc"),
> > > +     MFD_CELL_OF("cpcap_battery", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-battery"),
> > > +     MFD_CELL_OF("cpcap-regulator", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-regulator"),
> > > +     MFD_CELL_OF("cpcap-rtc", NULL, NULL, 0, 0, "motorola,cpcap-rtc"),
> > > +     MFD_CELL_OF("cpcap-pwrbutton", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-pwrbutton"),
> > > +     MFD_CELL_OF("cpcap-usb-phy", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-usb-phy"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 0, "motorola,cpcap-led-red"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 1, "motorola,cpcap-led-green"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 2, "motorola,cpcap-led-blue"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 3, "motorola,cpcap-led-adl"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 4, "motorola,cpcap-led-cp"),
> > > +     MFD_CELL_NAME("cpcap-codec"),
> > > +};
> >
> > Nit: I wouldn't complain if you wanted to have all of these on a single
> > line for neatness.
> >
> 
> Noted
> 
> > > +static const struct mfd_cell cpcap_mapphone_mfd_devices[] = {
> > > +     MFD_CELL_OF("cpcap_adc", NULL, NULL, 0, 0,
> > > +                 "motorola,mapphone-cpcap-adc"),
> > > +     MFD_CELL_OF("cpcap_battery", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-battery"),
> > > +     MFD_CELL_OF("cpcap-charger", NULL, NULL, 0, 0,
> > > +                 "motorola,mapphone-cpcap-charger"),
> > > +     MFD_CELL_OF("cpcap-regulator", NULL, NULL, 0, 0,
> > > +                 "motorola,mapphone-cpcap-regulator"),
> > > +     MFD_CELL_OF("cpcap-rtc", NULL, NULL, 0, 0, "motorola,cpcap-rtc"),
> > > +     MFD_CELL_OF("cpcap-pwrbutton", NULL, NULL, 0, 0,
> > > +                 "motorola,cpcap-pwrbutton"),
> > > +     MFD_CELL_OF("cpcap-usb-phy", NULL, NULL, 0, 0,
> > > +                 "motorola,mapphone-cpcap-usb-phy"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 0, "motorola,cpcap-led-red"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 1, "motorola,cpcap-led-green"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 2, "motorola,cpcap-led-blue"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 3, "motorola,cpcap-led-adl"),
> > > +     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 4, "motorola,cpcap-led-cp"),
> > > +     MFD_CELL_NAME("cpcap-codec"),
> > >  };
> >
> > A lot of these are duplicated, right?
> >
> > I would have a comment set, then the differences in separate containers.
> 
> It may be impossible to predict a generic setup since some devices may
> require unique compatibles, other may not have LEDs, third may be
> partially incompatible with existing cells. In other mfd cases
> creating a generic bundle might be good, but in this case I would
> suggest better to keep these separate entirely per-device. They will
> not take much space, nor add confusion with these macros.

I'm not sure we're understanding each other.  Let me give you an example:

static const struct mfd_cell cpcap_common_devices[] = {
     MFD_CELL_OF("cpcap_battery", NULL, NULL, 0, 0, "motorola,cpcap-battery"),
     MFD_CELL_OF("cpcap-rtc", NULL, NULL, 0, 0, "motorola,cpcap-rtc"),
     MFD_CELL_OF("cpcap-pwrbutton", NULL, NULL, 0, 0, "motorola,cpcap-pwrbutton"),
     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 0, "motorola,cpcap-led-red"),
     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 1, "motorola,cpcap-led-green"),
     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 2, "motorola,cpcap-led-blue"),
     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 3, "motorola,cpcap-led-adl"),
     MFD_CELL_OF("cpcap-led", NULL, NULL, 0, 4, "motorola,cpcap-led-cp"),
     MFD_CELL_NAME("cpcap-codec"),
};

static const struct mfd_cell cpcap_default_devices[] = {
     MFD_CELL_OF("cpcap_adc", NULL, NULL, 0, 0, "motorola,cpcap-adc"),
     MFD_CELL_OF("cpcap-regulator", NULL, NULL, 0, 0, "motorola,cpcap-regulator"),
     MFD_CELL_OF("cpcap-usb-phy", NULL, NULL, 0, 0, "motorola,cpcap-usb-phy"),
};

static const struct mfd_cell cpcap_mapphone_devices[] = {
     MFD_CELL_OF("cpcap_adc", NULL, NULL, 0, 0, "motorola,mapphone-cpcap-adc"),
     MFD_CELL_OF("cpcap-charger", NULL, NULL, 0, 0, "motorola,mapphone-cpcap-charger"),
     MFD_CELL_OF("cpcap-regulator", NULL, NULL, 0, 0, "motorola,mapphone-cpcap-regulator"),
     MFD_CELL_OF("cpcap-usb-phy", NULL, NULL, 0, 0, "motorola,mapphone-cpcap-usb-phy"),
};

This way, it's super easy to read / maintain the common and unique devices.

The only potential drawback would be 2 calls to mfd_add_devices() but
that's common practice.

Also notice that I droped the "_mfd" parts, which you should too.

> > >  static int cpcap_probe(struct spi_device *spi)
> > >  {
> > >       struct cpcap_ddata *cpcap;
> > > +     const struct mfd_cell *cells;
> > > +     unsigned int num_cells;
> > >       int ret;
> > >
> > >       cpcap = devm_kzalloc(&spi->dev, sizeof(*cpcap), GFP_KERNEL);
> > >       if (!cpcap)
> > >               return -ENOMEM;
> > >
> > > +     cpcap->variant = (enum cpcap_variant)spi_get_device_match_data(spi);
> > > +     if (!cpcap->variant)
> > > +             return -ENODEV;
> >
> > Isn't this covered in the 'default' below?
> >
> 
> This is for case cpcap->variant = 0, it should never happen, but check
> will not cause harm

The 'default' branch in the switch below will pick that up too.  This
check is superfluous.

> > > +     switch (cpcap->variant) {
> > > +     case CPCAP_DEFAULT:
> > > +             cells = cpcap_default_mfd_devices;
> > > +             num_cells = ARRAY_SIZE(cpcap_default_mfd_devices);
> > > +             break;
> > > +     case CPCAP_MAPPHONE:
> > > +             cells = cpcap_mapphone_mfd_devices;
> > > +             num_cells = ARRAY_SIZE(cpcap_mapphone_mfd_devices);
> > > +             break;
> > > +     default:
> > > +             return dev_err_probe(&spi->dev, -EINVAL,
> > > +                                  "Unknown device %d\n", cpcap->variant);
> >
> > This should be -ENODEV.
> >
> 
> hm, match is ENODEV cause it looks for device, here driver checks
> id/variant, so shouldn't it be EINVAL? I assume error message should
> be "Unknown device version" or "Unknown device ID"

All of your supported devices are represented in this switch statement.
Any other request, regardless of the reason should results in
"Unsupported device" and a -ENODEV - same as your check for
!cpcap->variant above.

-- 
Lee Jones