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

[Vignesh Raghavendra]

On 06/05/20 8:17 pm, Simon Glass wrote:
> 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?


Here is the cadence sierra PHY driver for which this series is a pre
requiste

https://elixir.bootlin.com/linux/latest/source/drivers/phy/cadence/phy-cadence-sierra.c

PHY Registers are 16 bit wide. Some implementations (SoCs) have arranged
these registers continuously (so they are aligned to 16 bit address
boundary) but some implementation place each register at 32 bit boundary
(wasting the upper 16 bits)

There are few other nits but above is the major one.

Custom regmap read()/write() hooks abstracts all these from actual
driver logic.

It is possible to re implement driver w/o regmap abstraction but this
would deviate the driver significantly from that of kernel. PHY drivers
often get updated with new register settings as and when HW
characterization progresses which need to be ported to kernel and
U-Boot. Having U-Boot driver same as kernel will ease porting effort and
also reduces the chances of errors.


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

This would mean U-Boot and Kernel DT files would be significantly
different and would defeat the whole purpose of maintaining common DT
bindings b/w kernel and U-Boot.

IMHO, regmap is pure SW abstraction that hides complexities of accessing
underlying register map therefore I don't think representing such
abstraction in DT will be accepted by upstream DT maintainers.

If there a way to use DM w/o DT changes, we can definitely explore that...

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