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

[Bryan O'Donoghue <bod@kernel.org>]

On 18/03/2026 15:07, Neil Armstrong wrote:
> On 3/18/26 14:17, Bryan O'Donoghue wrote:
>> On 18/03/2026 10:15, Neil Armstrong wrote:
>>>> +    /*
>>>> +     * phy_configure_opts_mipi_dphy.lanes starts from zero to
>>>> +     * the maximum number of enabled lanes.
>>>> +     *
>>>> +     * TODO: add support for bitmask of enabled lanes and polarities
>>>> +     * of those lanes to the phy_configure_opts_mipi_dphy struct.
>>>> +     * For now take the polarities as zero and the position as fixed
>>>> +     * this is fine as no current upstream implementation maps otherwise.
>>>> +     */
>>>
>>> This is wrong since you loose the lanes mapping defined in DT, which is still in CAMSS
>>> but is a PHY property. The lanes layout is not a property of the CSI controller,
>>> CSI controller only need to know the lanes count, and not the layout.
>>
>> Lane layout is a PHY concern but, the PHY API gives us phy_configure_opts_mipi_dphy which should be extended to provide layout and polarity. This would then be of benefit to more than just qcom/camss.
> 
> Why ? the only concern between a controller and a PHY is the lane count to calculate the bandwidth, the actual pin layout is certainly not a controller concern.

Controllers already get the lane count by way of data-lanes = <x y z q> 
or <x y> or <x> if we didn't do that we would need to specify the 
data-lanes in the controller and again in the PHY.

>>
>> Right now none of the CAMSS users for this driver depend on any other mapping and I propose a separate series to fix phy_configure_opts_mipi_dphy rather than introduce data-lanes to DPHY.
> 
> None of the upstream users of camss.

No, we are establishing from x1 use of standard drivers/phy. New users 
will do it this way. The posted dtsi for the laptops can use the linear 
lane layout and default polarities.

In a follow on series we can extend phy_configure_opts_mipi_dphy to 
parse data-lanes = <> into count and mask, to the benefit of any user of 
phy_configure_opts_mipi_dphy.

Since that will touch more then qcom specific stuff and will touch at 
least two subsystems, that should be its own separate series.

> The problem is even larger, as you replied in [1], the csiphy is still exposed as a media element from the CAMSS driver, this means this driver is not complete,
> it should be a media driver entirely with eventually an internal PHY aux driver, but this would be entirely implementation specific.
> 
> Either the PHY is standalone and the PHY consumer only calls phy_open/init/configure/power_on/power_off/exit, otherwise it's not a fully standaline PHY but a composite device like here.

This is not a composite device any more than the existing upstream
implementations which follow the same model:

- Cadence CSI2RX + Cadence DPHY (TI J721E/AM62A)
- Rockchip rkisp1 + phy-rockchip-inno-csidphy

Both use phys = <&phandle>, the media driver manages V4L2 endpoints
and lane counts, the PHY driver handles the electrical layer via
phy_configure().

To this list we will add qcom camss, there's nothing exotic being proposed.

> I propose that you write a proper media driver for the qcom csiphy, which eventually spins a PHY driver as an aux device.

None of these SoC D-PHYs are written as V4L2 media drivers that spawn 
auxiliary devices. They all use the phys = <&phandle> model. The media 
driver manages the V4L2 endpoints and lane counts, passing the 
configuration down via phy_configure_opts_mipi_dphy.

I just don't see what is so special about CAMSS that it needs to have 
its own special PHY implementation. drivers/phy the standard API and 
specification of data-lanes etc in the controller seems pretty "bog 
standard".

---
bod