Re: [PATCH v5 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[Konrad Dybcio]

On 3/27/26 3:23 AM, Hangxiang Ma wrote:
> On 3/26/2026 9:04 AM, Bryan O'Donoghue wrote:
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/time64.h>
>> +
>> +#include "phy-qcom-mipi-csi2.h"
>> +
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(offset, n)    ((offset) + 0x4 * (n))
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL0_PHY_SW_RESET    BIT(0)
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE    BIT(7)
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B    BIT(0)
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID    BIT(1)
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL10_IRQ_CLEAR_CMD    BIT(0)
>> +#define CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(offset, n)    ((offset) + 0xb0 + 0x4 * (n))
>>
> Hi Bryan, one minor observation on the following macro:
> 
>     CSIPHY_3PH_CMN_CSI_COMMON_STATUSn
> 
> The 0xb0 offset implicitly assumes a fixed distance between the
> common_ctrl and common_status register blocks. This holds for the PHYs
> covered by this series, but on some other platforms (e.g. Kaanapali,
> Pakala) the offset differs.
> 
> That said, I think keeping this fixed value is reasonable for the scope
> of the current PHY series, and it does help keep the macro set simple.
> It might just be worth documenting this assumption (e.g. via a comment
> or in the commit message).
> 
> Alternatively, if future PHY variants need to support different layouts,
> this could be made more extensible by moving the status base offset into
> the per-PHY data (similar to other register layout parameters). But I
> don’t think that needs to block the current series.

If the register contents are generally similar but the bit positions
and/or reg offsets differ, regmap_fields may be useful

Konrad