Re: [PATCH 2/2] spmi: spmi-pmic-arb: add support for PMIC arbiter v8.5

[Dmitry Baryshkov]

On Fri, Apr 17, 2026 at 01:24:07PM +0800, Fenglin Wu wrote:
> 
> On 4/2/2026 12:18 PM, Fenglin Wu wrote:
> > 
> > On 4/1/2026 7:22 PM, Dmitry Baryshkov wrote:
> > > On Wed, Apr 01, 2026 at 02:41:24AM -0700, Fenglin Wu wrote:
> > > > PMIC arbiter v8.5 is an extension of PMIC arbiter v8 that updated
> > > > the definition of the channel status register bit fields. Add support
> > > > to handle this difference.
> > > > 
> > > > Signed-off-by: Fenglin Wu <fenglin.wu@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >   drivers/spmi/spmi-pmic-arb.c | 69
> > > > ++++++++++++++++++++++++++++++++++++++------
> > > >   1 file changed, 60 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/spmi/spmi-pmic-arb.c
> > > > b/drivers/spmi/spmi-pmic-arb.c
> > > > index 69f8d456324a..deeaa39bb647 100644
> > > > --- a/drivers/spmi/spmi-pmic-arb.c
> > > > +++ b/drivers/spmi/spmi-pmic-arb.c
> > > > @@ -28,6 +28,7 @@
> > > >   #define PMIC_ARB_VERSION_V5_MIN        0x50000000
> > > >   #define PMIC_ARB_VERSION_V7_MIN        0x70000000
> > > >   #define PMIC_ARB_VERSION_V8_MIN        0x80000000
> > > > +#define PMIC_ARB_VERSION_V8P5_MIN    0x80050000
> > > >   #define PMIC_ARB_INT_EN            0x0004
> > > >     #define PMIC_ARB_FEATURES        0x0004
> > > > @@ -63,11 +64,34 @@
> > > >   #define SPMI_OWNERSHIP_PERIPH2OWNER(X)    ((X) & 0x7)
> > > >     /* Channel Status fields */
> > > > -enum pmic_arb_chnl_status {
> > > > -    PMIC_ARB_STATUS_DONE    = BIT(0),
> > > > -    PMIC_ARB_STATUS_FAILURE    = BIT(1),
> > > > -    PMIC_ARB_STATUS_DENIED    = BIT(2),
> > > > -    PMIC_ARB_STATUS_DROPPED    = BIT(3),
> > > > +struct pmic_arb_chnl_status_mask {
> > > > +    u8    done;
> > > > +    u8    failure;
> > > > +    u8    crc;
> > > > +    u8    parity;
> > > > +    u8    nack;
> > > > +    u8    denied;
> > > > +    u8    dropped;
> > > > +};
> > > > +
> > > > +static const struct pmic_arb_chnl_status_mask chnl_status_mask = {
> > > > +    .done        = BIT(0),
> > > > +    .failure    = BIT(1),
> > > > +    .crc        = 0,
> > > > +    .parity        = 0,
> > > > +    .nack        = 0,
> > > > +    .denied        = BIT(2),
> > > > +    .dropped    = BIT(3),
> > > > +};
> > > > +
> > > > +static const struct pmic_arb_chnl_status_mask
> > > > chnl_status_mask_v8p5 = {
> > > > +    .done        = BIT(0),
> > > > +    .failure    = BIT(1),
> > > > +    .crc        = BIT(2),
> > > > +    .parity        = BIT(3),
> > > > +    .nack        = BIT(4),
> > > > +    .denied        = BIT(5),
> > > > +    .dropped    = BIT(6),
> > > Would it be better to extract generation-specific callback to decode the
> > > error rather than defining the list of masks?
> > 
> > Are you proposing to add a callback in pmic_arb_ver_ops, like
> > '*check_chnl_status', and create separate implementations for PMIC
> > arbiter versions before and after v8.5?
> > 
> > This approach would add more extensive code changes with some code
> > duplication, especially for handling common error bits shared across all
> > versions—even if they only print error messages and return an error
> > code. Is that a concern?
> > 
> > Fenglin
> 
> Hi Dmitry,
> 
> Please let me know if this your preferred way and if you are fine with the
> concern that I mentioned.
> 
> I can come up with this approach and post a new patch.

Sorry.

Yes, a somewhat duplicate code would be better than having a bitfields
where the individual fields will differ from platform to platform.

-- 
With best wishes
Dmitry