Re: [PATCH v12 02/38] ARM: ep93xx: add regmap aux_dev

[Nikita Shubin]

Hello Stephen !

On Wed, 2024-09-11 at 15:46 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2024-09-09 01:10:27)
> > diff --git a/include/linux/soc/cirrus/ep93xx.h
> > b/include/linux/soc/cirrus/ep93xx.h
> > index 56fbe2dc59b1..a27447971302 100644
> > --- a/include/linux/soc/cirrus/ep93xx.h
> > +++ b/include/linux/soc/cirrus/ep93xx.h
> > @@ -3,6 +3,18 @@
> >  #define _SOC_EP93XX_H
> >  
> >  struct platform_device;
> > +struct regmap;
> > +struct spinlock_t;
> > +
> > +enum ep93xx_soc_model {
> > +       EP93XX_9301_SOC,
> > +       EP93XX_9307_SOC,
> > +       EP93XX_9312_SOC,
> > +};
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/container_of.h>
> >  
> >  #define EP93XX_CHIP_REV_D0     3
> >  #define EP93XX_CHIP_REV_D1     4
> > @@ -10,6 +22,20 @@ struct platform_device;
> >  #define EP93XX_CHIP_REV_E1     6
> >  #define EP93XX_CHIP_REV_E2     7
> >  
> > +struct ep93xx_regmap_adev {
> > +       struct auxiliary_device adev;
> > +       struct regmap *map;
> > +       void __iomem *base;
> > +       spinlock_t *lock;
> > +       void (*write)(struct regmap *map, spinlock_t *lock,
> > unsigned int reg,
> > +                     unsigned int val);
> > +       void (*update_bits)(struct regmap *map, spinlock_t *lock,
> > +                           unsigned int reg, unsigned int mask,
> > unsigned int val);
> > +};
> 
> Why can't the single 'struct regmap' be passed as the auxiliary
> device's
> platform_data? The lock could be put into the regmap locking routines
> and the write/update_bits would be standard regmap API calls. Doing
> that
> would make the auxiliary device driver simpler because it wouldn't
> depend on this struct.
> 
> The device name could encode the SoC number as well so the auxiliary
> device driver could match different names and do different things.

Unfortunately ep93xx sw locked register are trickier than they appear
at first glance:

```
+ * Logic safeguards are included to condition the control signals for
+ * power connection to the matrix to prevent part damage. In addition,
a
+ * software lock register is included that must be written with 0xAA
+ * before each register write to change the values of the four switch
+ * matrix control registers.
```

- only a few registers are sw locked, so we are not using lock's
everywhere only when it is really needed
- lock is cleared on any write to this register, so we must ensure we
are performing 2 write operations, first writing 0xaa, second the
protected register itself under a single lock
- luckily we can read without clearing the lock

That's why we have such a funny ep93xx_regmap_update_bits() with forced
update_bits.

It was discussed in detail a five iteration ago or so with Andy
Shevchenko, when he was reviewing clk and pinctrl (and thank you once
again Andy).