Messages in this thread | | | Date | Sun, 19 Mar 2023 21:10:19 -0700 | From | Bjorn Andersson <> | Subject | Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field() |
| |
On Fri, Mar 17, 2023 at 09:58:04PM +0100, Linus Walleij wrote: > On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote: > > > Use qcom_scm_io_update_field() exported function introduced > > in last commit. > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > Fine by me, but I want you to first consider switching the > custom register accessors to regmap. >
I took a quick look at it and there seem to be two ways that it can be done.
We can retain the MSM_ACCESSOR() macros that generates the custom register accessors, but plug in a regmap between these accessors and the mmio operations. But this just adds a few extra hops inbetween the driver and the volatile read/write, with a slight increase of memory, without any obvious benefits.
The more alluring alternative is to replace the custom accessors with reg_fields. This would allow us to replace some (perhaps many) of the bit-manipulation with regmap_update_bits().
But at minimum we'd need one reg_field per register, per pin, so that's 5 reg_fields per pin which adds up to ~10-24kb extra space, depending on platform.
Even more alluring would be to have reg_fields describing the actual fields in the registers, which would allow us to better utilize the regmap API directly. This would cost us 35-75kb of heap.
IMHO this is quite a significant effort, and given that the driver seems to be doing its job I'd rather see such efforts being focused elsewhere.
Regards, Bjorn
| |