Re: [PATCH v3 net-next 5/8] mfd: zl3073x: Add functions to work with register mailboxes

[Ivan Vecera]

On 17. 04. 25 3:22 odp., Andrew Lunn wrote:
> > > > +/*
> > > > + * Mailbox operations
> > > > + */
> > > > +int zl3073x_mb_dpll_read(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_dpll_write(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_output_read(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_output_write(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_ref_read(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_ref_write(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_synth_read(struct zl3073x_dev *zldev, u8 index);
> > > > +int zl3073x_mb_synth_write(struct zl3073x_dev *zldev, u8 index);
> > >
> > > I assume these are the only valid ways to access a mailbox?
> > >
> > > If so:
> > >
> > > > +static inline __maybe_unused int
> > > > +zl3073x_mb_read_ref_mb_mask(struct zl3073x_dev *zldev, u16 *value)
> > > > +{
> > > > +     __be16 temp;
> > > > +     int rc;
> > > > +
> > > > +     lockdep_assert_held(&zldev->mailbox_lock);
> > > > +     rc = regmap_bulk_read(zldev->regmap, ZL_REG_REF_MB_MASK, &temp,
> > > > +                           sizeof(temp));
> > > > +     if (rc)
> > > > +             return rc;
> > > > +
> > > > +     *value = be16_to_cpu(temp);
> > > > +     return rc;
> > > > +}
> > >
> > > These helpers can be made local to the core. You can then drop the
> > > lockdep_assert_held() from here, since the only way to access them is
> > > via the API you defined above, and add the checks in those API
> > > functions.
> >
> > This cannot be done this way... the above API just simplifies the
> > operation of read and write latch registers from/to mailbox.
> >
> > Whole operation is described in the commit description.
> >
> > E.g. read something about DPLL1
> > 1. Call zl3073x_mb_dpll_read(..., 1)
> >     This selects DPLL1 in the DPLL mailbox and performs read operation
> > and waits for finish
> > 2. Call zl3073x_mb_read_dpll_mode()
> >     This reads dpll_mode latch register
> >
> > write:
> > 1. Call zl3073x_mb_write_dpll_mode(...)
> >     This writes mode to dpll_mode latch register
> > 2. Call zl3073x_mb_dpll_read(..., 1)
> >     This writes all info from latch registers to DPLL1
> >
> > The point is that between step 1 and 2 nobody else cannot touch
> > latch_registers or mailbox select register and op semaphore.
>
> Again, think about your layering. What does your lower level mailbox
> API look like? What does the MFD need to export for a safe API?
>
> So maybe you need zl3073x_mb_read_u8(), zl3073x_mb_read_u16(),
> zl3073x_mb_read_u32(), as part of your mailbox API. These assert the
> lock is held.
>
> You could even make zl3073x_mb_read_u8() validate the register is in
> the upper range, and that zl3073x_read_u8() the register is in the
> lower range.

Yes, this LGTM... Anyway if the MB API would provide reading and writing 
MBs at once then zl3073x_mb_{read,write}_u* are not necessary as the 
only caller of these functions is MFD itself and they would be called 
under MB API that holds the lock.

Ivan