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

[Lee Jones]

On Wed, 20 May 2026, Svyatoslav Ryhel wrote:

> ср, 20 трав. 2026 р. о 19:05 Lee Jones <lee@xxxxxxxxxx> пише:
> >
> > On Wed, 20 May 2026, Svyatoslav Ryhel wrote:
> >
> > > ср, 20 трав. 2026 р. о 18:08 Lee Jones <lee@xxxxxxxxxx> пише:
> > > >
> > > > 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@xxxxxxxxx>
> > > > > ---
> > > > >  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.
> >
> 
> What if next added device does not have cpcap-battery, cpcap-led-adl,
> cpcap-pwrbutton, uses external codec and would require cpcap-watchdog
> which is not yet present. This will cause issues. Even Mot introduces
> in the last patch of this patchset has slightly different composition.

We usually just code-up what we have _today_.

If new devices come in with different requirements, it takes 5 mins to rejig.

-- 
Lee Jones