Re: [PATCH v2 3/8] media: platform: amd: Add helpers to configure isp4 mipi phy

["Du, Bin" <bin.du@amd.com>]

Thanks Laurent Pinchart for the review.

On 8/5/2025 6:53 PM, Laurent Pinchart wrote:
> On Tue, Aug 05, 2025 at 05:53:30PM +0800, Du, Bin wrote:
>> On 7/28/2025 2:33 PM, Sakari Ailus wrote:
>>> On Wed, Jun 18, 2025 at 05:19:54PM +0800, Bin Du wrote:
>>>> The helper functions is for configuring, starting and stop the MIPI PHY.
>>>> All configurations related to MIPI PHY configuration and calibration
>>>> parameters are encapsulated in two helper functions: start and stop
>>>> mipi phy.
>>>>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> ---
>>>>    drivers/media/platform/amd/isp4/Makefile   |    1 +
>>>>    drivers/media/platform/amd/isp4/isp4_phy.c | 1547 ++++++++++++++++++++
>>>>    drivers/media/platform/amd/isp4/isp4_phy.h |   14 +
>>>>    3 files changed, 1562 insertions(+)
>>>>    create mode 100644 drivers/media/platform/amd/isp4/isp4_phy.c
>>>>    create mode 100644 drivers/media/platform/amd/isp4/isp4_phy.h
>>>>
>>>> diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile
>>>> index 8ca1c4dfe246..0e36201fbb30 100644
>>>> --- a/drivers/media/platform/amd/isp4/Makefile
>>>> +++ b/drivers/media/platform/amd/isp4/Makefile
>>>> @@ -4,6 +4,7 @@
>>>>    
>>>>    obj-$(CONFIG_AMD_ISP4) += amd_capture.o
>>>>    amd_capture-objs := isp4.o	\
>>>> +			isp4_phy.o \
>>>>    			isp4_hw.o	\
>>>>    
>>>>    ccflags-y += -I$(srctree)/drivers/media/platform/amd/isp4
>>>> diff --git a/drivers/media/platform/amd/isp4/isp4_phy.c b/drivers/media/platform/amd/isp4/isp4_phy.c
>>>> new file mode 100644
>>>> index 000000000000..8d31a21074bb
>>>> --- /dev/null

[snip]

>>>> +
>>>> +static const struct isp4phy_mipi_reg *isp_mipi_phy_reg[ISP_MIPI_PHY_ID_MAX] = {
>>>> +	(struct isp4phy_mipi_reg *)ISP_MIPI_PHY0_REG0,
>>>> +	(struct isp4phy_mipi_reg *)ISP_MIPI_PHY1_REG0,
>>>> +	(struct isp4phy_mipi_reg *)ISP_MIPI_PHY2_REG0,
>>>
>>> That's an interesting way to prefill structs. I don't think these macros
>>> expand to valid pointers.
>>
>> Yes, these are valid pointers, but they are not pointing to system
>> memory, they are phy registers address, can be accessed by adding the
>> mmio base.
> 
> Please don't do that. Store register addresses in macros, and use them
> through the code as appropriate. Stop passing the base mmio address
> around through functions, store it in a PHY object, and pass that object
> to the isp4phy_start() instead of a dev pointer, base pointer and phy
> id.
> 

Sure, will do that and update in the next patch

>>>> +};
>>>> +
>>>> +static const struct isp4phy_mipi_reg_seq startup_seq_general_common_config[] = {
>>>> +	{ PPI_STARTUP_RW_COMMON_DPHY_10, PPI_STARTUP_RW_COMMON_DPHY_10_PHY_READY_ADDR_MASK, 0x30 },
>>>> +	{
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2,
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2_GLOBAL_ULPS_OVR_VAL_MASK, 0x0
>>>> +	},
>>>> +	{
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2,
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_2_GLOBAL_ULPS_OVR_EN_MASK, 0x1
>>>> +	},
>>>> +	{
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_0,
>>>> +		CORE_DIG_ANACTRL_RW_COMMON_ANACTRL_0_CB_LP_DCO_EN_DLY_MASK, 0x3F
>>>> +	},
>>>> +	{
>>>> +		PPI_STARTUP_RW_COMMON_STARTUP_1_1,
>>>> +		PPI_STARTUP_RW_COMMON_STARTUP_1_1_PHY_READY_DLY_MASK, 0x233
>>>> +	},
>>>> +	{ PPI_STARTUP_RW_COMMON_DPHY_6, PPI_STARTUP_RW_COMMON_DPHY_6_LP_DCO_CAL_ADDR_MASK, 0x27 },
>>>> +	{ PPI_CALIBCTRL_RW_COMMON_BG_0, PPI_CALIBCTRL_RW_COMMON_BG_0_BG_MAX_COUNTER_MASK, 0x1F4 },
>>>> +	{ PPI_RW_LPDCOCAL_NREF, PPI_RW_LPDCOCAL_NREF_LPDCOCAL_NREF_MASK, 0x320 },
>>>> +	{ PPI_RW_LPDCOCAL_NREF_RANGE, PPI_RW_LPDCOCAL_NREF_RANGE_LPDCOCAL_NREF_RANGE_MASK, 0x1B },
>>>> +	{ PPI_RW_LPDCOCAL_TWAIT_CONFIG, PPI_RW_LPDCOCAL_TWAIT_CONFIG_LPDCOCAL_TWAIT_PON_MASK, 0x7F},
>>>> +	{ PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_VT_NREF_RANGE_MASK, 0x1B },
>>>> +	{ PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_USE_IDEAL_NREF_MASK, 0x1 },
>>>> +	{ PPI_RW_LPDCOCAL_VT_CONFIG, PPI_RW_LPDCOCAL_VT_CONFIG_LPDCOCAL_VT_TRACKING_EN_MASK, 0x0 },
>>>> +	{ PPI_RW_LPDCOCAL_COARSE_CFG, PPI_RW_LPDCOCAL_COARSE_CFG_NCOARSE_START_MASK, 0x1 },
>>>> +	{ PPI_RW_COMMON_CFG, PPI_RW_COMMON_CFG_CFG_CLK_DIV_FACTOR_MASK, 0x3 },
>>>> +};
>>>> +
>>>> +static const struct isp4phy_mipi_reg_seq startup_seq_common[] = {
>>>> +	{ PPI_STARTUP_RW_COMMON_DPHY_2, PPI_STARTUP_RW_COMMON_DPHY_2_RCAL_ADDR_MASK, 0x5 },
>>>> +	{ PPI_RW_TERMCAL_CFG_0, PPI_RW_TERMCAL_CFG_0_TERMCAL_TIMER_MASK, 0x17 },
>>>> +	{ PPI_RW_OFFSETCAL_CFG_0, PPI_RW_OFFSETCAL_CFG_0_OFFSETCAL_WAIT_THRESH_MASK, 0x4 },
>>>> +	{ PPI_RW_LPDCOCAL_TIMEBASE, PPI_RW_LPDCOCAL_TIMEBASE_LPDCOCAL_TIMEBASE_MASK, 0x5F },
>>>> +	{
>>>> +		PPI_RW_LPDCOCAL_TWAIT_CONFIG,
>>>> +		PPI_RW_LPDCOCAL_TWAIT_CONFIG_LPDCOCAL_TWAIT_COARSE_MASK, 0x1D
>>>> +	},

[snip]

--
Regards,
Bin