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

[Pratyush Yadav]

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?

> 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.

> - 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?

> - 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.

-- 
Regards,
Pratyush Yadav
Texas Instruments India