Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

[Bjorn Andersson <andersson@kernel.org>]

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