Re: [PATCH v5 5/6] mfd: motorola-cpcap: diverge configuration per-board
From: Svyatoslav Ryhel
Date: Wed May 20 2026 - 15:36:07 EST
ср, 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.
> > > > 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.
>
Noted
> > > > + 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.
>
Noted
> --
> Lee Jones