[PATCH v2 2/4] regmap: Allow providing read/write callbacks through struct regmap_config

[Simon Glass]

Hi Pratyush,

On Tue, 5 May 2020 at 13:47, Pratyush Yadav <p.yadav at ti.com> wrote:
>
> Hi Simon,
>
> I would be taking up this series and address the comments.
>
> On 10/12/19 03:18PM, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Tue, 5 Nov 2019 at 04:47, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
> > >
> > > Some linux drivers provide their own read/write functions to access data
> > > from/of the regmap. Adding support for it.
> > >
> > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Only use custom accessors if {,SPL,TPL}_REGMAP_ACCESSORS is enabled
> > >
> > >  drivers/core/Kconfig  | 25 +++++++++++++++++++++++++
> > >  drivers/core/regmap.c | 22 ++++++++++++++++++++--
> > >  include/regmap.h      | 28 +++++++++++++++++++++++++---
> > >  3 files changed, 70 insertions(+), 5 deletions(-)
> >
> > Coming back to the discussion on driver model....
>
> IIUC, you are suggesting that we create a uclass for regmap, and fill in
> the ops with the driver's custom read/write functions, correct? This
> would mean we will have to create udevice for each regmap. A driver can
> have multiple regmaps (up to 18 for example in the Linux Cadence Sierra
> PHY driver in kernel). Creating a device for each regmap is very
> inefficient. Is the overhead really worth it? Does it solve any
> significant problem? Also, a regmap is not a device, it is an
> abstraction layer to simplify register access. Does it then make sense
> to model it as a device?

I was actually referring to the access method of the regmap but I
suppose the effect is the same. This question has been running for a
while.

The issue is that this is getting more and more complicated. We use
devices to model complicated things. It is an abstraction. It doesn't
have to be a physical device.

With regmap we are creating an ad-hoc structure and associated
infrastructure to deal with this one case. It is not possible to see
the regmaps with 'dm dump', for example.

>
> > How do you specify the fields? I would expect that this would be done
> > in the driver tree? Perhaps in a subnode of the device?
> >
> > Just to state what I see as the advantages of using a separate device
> > for access:
> >
> > - Remove the #ifdef in the regmap struct
>
> If you really want to remove the #ifdefs, we can set reg_read to a
> default, and let drivers override if when creating a regmap. Then in
> regmap_read, just call reg_read. This gets rid of the #ifdefs with a
> small change. I suspect this will have a much smaller impact on memory
> usage than using a udevice.

Yes that sounds good, but you are getting closer and closer to it
being a device.

>
> > - Easy to specify the behaviour in a device-tree node
>
> Quoting from the kernel's documentation of regmap_config:
>
>   reg_read: Optional callback that if filled will be used to perform all
>   the reads from the registers. Should only be provided for devices
>   whose read operation cannot be represented as a simple read operation
>   on a bus such as SPI, I2C, etc. Most of the devices do not need this.
>   reg_write: Same as above for writing.
>
> These custom accessors are meant to be used when the read or write needs
> more work to be done than the standard regmap reads/writes. And so they
> are supposed to be driver-specific functions. I don't think it is at all
> possible to specify something like this in devicetree. Am I missing
> something?

Can you point to an example of this extra read/write code?

The kernel is a rabbit-warren of custom interfaces. I am trying to
keep driver model uniform, so long as the cost is reasonable. I'm not
sure that it is worth having a regmap device for simple things, but as
things get more complex, I think we should look at it.

Of course you can specify this in the device tree: just use a
compatible string to select the appropriate regmap driver.

spi {
    compatible = "vendor,myspi";
    regmaps = <&regmap_simple 4
           &regmap_mailbox 0 FLAGS>;
}

regmap_mailbox: regmap-mailbox {
    compatible = "regmap,mailbox";
    other props
}

struct regmap *regmap = regmap_get_by_index(dev, 1)

regmap_read(regmap, ...)

>
> > - Easy to extend as the child device can do what it likes with respect to access
> >
> > Disadvantage is that it takes a bit more space.

Yes space is a concern. A device takes about 84 bytes I think. But as
we add more and more complexity we might as well take the hit.

Regards,
Simon