Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a9199018-cbc4-e545-5321-ebc2d3096ff3@quicinc.com>
Date:   Thu, 27 Jul 2023 17:40:36 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v3 1/3] input: pm8xxx-vib: refactor to easily support new
 SPMI vibrator
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <robh+dt@kernel.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <dmitry.baryshkov@linaro.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        <linux-input@vger.kernel.org>
CC:     <quic_collinsd@quicinc.com>, <quic_subbaram@quicinc.com>,
        <quic_kamalw@quicinc.com>, <jestar@qti.qualcomm.com>
References: <20230725054138.129497-1-quic_fenglinw@quicinc.com>
 <20230725054138.129497-2-quic_fenglinw@quicinc.com>
 <5dd56c31-7ca3-dd39-0623-e4fd18ac6f68@linaro.org>
 <053c9571-d709-2826-fced-a00dd7255b8b@quicinc.com>
 <2a09e743-7423-65b0-c70d-87ae8105182a@linaro.org>
 <4e416602-8dea-fa6d-d083-f93b730552c3@quicinc.com>
 <a27ad44c-bbc9-0a2e-44fe-ee9b787d0cd4@linaro.org>
From:   Fenglin Wu <quic_fenglinw@quicinc.com>
In-Reply-To: <a27ad44c-bbc9-0a2e-44fe-ee9b787d0cd4@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/27/2023 5:22 PM, Krzysztof Kozlowski wrote:
> On 27/07/2023 09:43, Fenglin Wu wrote:
>>
>>
>> On 7/27/2023 3:07 PM, Krzysztof Kozlowski wrote:
>>> On 25/07/2023 08:16, Fenglin Wu wrote:
>>>>>>     
>>>>>> -static const struct pm8xxx_regs pm8058_regs = {
>>>>>> -	.drv_addr = 0x4A,
>>>>>> -	.drv_mask = 0xf8,
>>>>>> -	.drv_shift = 3,
>>>>>> -	.drv_en_manual_mask = 0xfc,
>>>>>> +static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
>>>>>
>>>>> Change from const to non-const is wrong. How do you support multiple
>>>>> devices? No, this is way too fragile now.
>>>>>
>>>>
>>>> The register definition is no longer used as the match data, hw_type is
>>>> used.
>>>>
>>>> The last suggestion was getting the register base address from the DT
>>>> and it has to be added into the offset of SPMI vibrator registers
>>>> (either in the previous hard-coded format or the later the reg_filed
>>>> data structure), so it's not appropriated to make it constant.
>>>>
>>>> I don't understand this question: "How do you support multiple devices?"
>>>> For SSBI vibrator, since all the registers are fixed, and I would assume
>>>> that there is no chance to support multiple vibrator devices on the same
>>>> SSBI bus. If they are not on the same bus, the regmap device will be
>>>> different while the registers definition is the same, and we are still
>>>> able to support multiple devices, right?
>>>
>>> No, you have static memory. One device probes and changes static memory
>>> to reg+=base1. Second device probes and changes the same to reg+=base2.
>>
>> Thanks, got it.  I can update it with following 2 options:
>>
>> 1) keep the register definition in 'reg_filed' data structure and make
>> it constant, copy it to a dynamically allocated memory before adding the
>> 'reg_base' to the '.reg' variable.
>>
>> 2) Define the register offsets as constant data and add the 'reg_base'
>> to the 'reg' while using 'regmap_read()'/'regmap_write()' functions.
>>
>> which one is the preferred way?
> 
> Depends on the code. I am not sure if 2 would work with regmap_fields.
> OTOH, I wonder if the device could just create its own regmap instead of
> using parents? Then there would be no need of this offset dance.
> 
> Anyway, adding offset only for some variants seems also not needed. You
> should add offset to each variant, because each device has this offset.
> 
> Best regards,
> Krzysztof
> Thanks for the suggestion.

The Qualcomm SPMI device has to use the 'regmap' from its parent with 16 
'reg_bits' and 8 'val_bits' config, the higher 8-bit 'reg_bits' is the 
peripheral ID (PID) and it could be different in different PMICs even 
for the same type of HW module, and (PID << 8) is the 'reg_base' here.

I assume that you are not in favor of copying the constant data into a 
dynamic allocated memory, so I will go with option 2.
Thanks