Message-ID: <15d270ca-1068-b926-efc9-a14ddfc90a54@quicinc.com>
Date:   Wed, 8 Mar 2023 15:22:30 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH V8 4/7] pinctrl: qcom: Add IPQ9574 pinctrl driver
Content-Language: en-US
To:     <andy.shevchenko@gmail.com>
CC:     <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <mturquette@baylibre.com>,
        <sboyd@kernel.org>, <linus.walleij@linaro.org>,
        <catalin.marinas@arm.com>, <will@kernel.org>,
        <p.zabel@pengutronix.de>, <shawnguo@kernel.org>, <arnd@arndb.de>,
        <marcel.ziswiler@toradex.com>, <dmitry.baryshkov@linaro.org>,
        <nfraprado@collabora.com>, <broonie@kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-gpio@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <quic_srichara@quicinc.com>, <quic_gokulsri@quicinc.com>,
        <quic_sjaganat@quicinc.com>, <quic_kathirav@quicinc.com>,
        <quic_arajkuma@quicinc.com>, <quic_anusha@quicinc.com>,
        <quic_poovendh@quicinc.com>
References: <20230214163116.9924-1-quic_devipriy@quicinc.com>
 <20230214163116.9924-5-quic_devipriy@quicinc.com>
 <ZAZ+GeGu8mW1XqpG@surfacebook>
From:   Devi Priya <quic_devipriy@quicinc.com>
In-Reply-To: <ZAZ+GeGu8mW1XqpG@surfacebook>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/7/2023 5:28 AM, andy.shevchenko@gmail.com wrote:
> Tue, Feb 14, 2023 at 10:01:13PM +0530, Devi Priya kirjoitti:
>> Add pinctrl definitions for the TLMM of IPQ9574
> 
> ...
> 
>> +	depends on OF
> 
> No compile test on non-OF configurations?
Hi Andy,
As per the generic convention followed in other
SoCs, we do not have compile test on non-OF configurations
> 
>> +	depends on ARM64 || COMPILE_TEST
> 
> ...
> 
>> +#define FUNCTION(fname)			                \
> 
> PINCTRL_PINFUNCTION() ?
I see that there are quite a bunch of files that has to
be modified for using the generic data type and
macro for the pin function definition
We shall post a separate series to accommodate the changes
> 
>> +	[msm_mux_##fname] = {		                \
>> +		.name = #fname,				\
>> +		.groups = fname##_groups,               \
>> +		.ngroups = ARRAY_SIZE(fname##_groups),	\
>> +	}
> 
> ...
> 
>> +#define REG_SIZE 0x1000
>> +#define PINGROUP(id, f1, f2, f3, f4, f5, f6, f7, f8, f9)	\
>> +	{					        \
>> +		.name = "gpio" #id,			\
>> +		.pins = gpio##id##_pins,		\
>> +		.npins = (unsigned int)ARRAY_SIZE(gpio##id##_pins),	\
> 
> Can you embed struct pingroup?
Will take care of this in a separate series
> 
>> +		.funcs = (int[]){			\
>> +			msm_mux_gpio, /* gpio mode */	\
>> +			msm_mux_##f1,			\
>> +			msm_mux_##f2,			\
>> +			msm_mux_##f3,			\
>> +			msm_mux_##f4,			\
>> +			msm_mux_##f5,			\
>> +			msm_mux_##f6,			\
>> +			msm_mux_##f7,			\
>> +			msm_mux_##f8,			\
>> +			msm_mux_##f9			\
>> +		},				        \
>> +		.nfuncs = 10,				\
>> +		.ctl_reg = REG_SIZE * id,			\
>> +		.io_reg = 0x4 + REG_SIZE * id,		\
>> +		.intr_cfg_reg = 0x8 + REG_SIZE * id,		\
>> +		.intr_status_reg = 0xc + REG_SIZE * id,	\
>> +		.intr_target_reg = 0x8 + REG_SIZE * id,	\
>> +		.mux_bit = 2,			\
>> +		.pull_bit = 0,			\
>> +		.drv_bit = 6,			\
>> +		.oe_bit = 9,			\
>> +		.in_bit = 0,			\
>> +		.out_bit = 1,			\
>> +		.intr_enable_bit = 0,		\
>> +		.intr_status_bit = 0,		\
>> +		.intr_target_bit = 5,		\
>> +		.intr_target_kpss_val = 3,	\
>> +		.intr_raw_status_bit = 4,	\
>> +		.intr_polarity_bit = 1,		\
>> +		.intr_detection_bit = 2,	\
>> +		.intr_detection_width = 2,	\
>> +	}
> 
> ...
> 
>> +	PINGROUP(62, blsp1_spi, audio_sec, audio_pdm1, audio_sec, pta, prng_rosc2, gcc_plltest,
>> +		 _, _),
> 
> Can be one line.
Okay
> 
> ...
> 
>> +static const struct of_device_id ipq9574_pinctrl_of_match[] = {
>> +	{ .compatible = "qcom,ipq9574-tlmm", },
> 
>> +	{ },
> 
> No comma for terminator line.
Okay
> 
>> +};
> 
> No MODULE_DEVICE_TABLE()?
The MODULE_DEVICE_TABLE(of, ipq9574_pinctrl_of_match) entry has
been added at the end of the file
> 
Best Regards,
Devi Priya