Re: [PATCH] media: qcom: camss: configure C-PHY in the new csiphy_stream_on()

[Anusha Arun Nandi]

Hi Bryan,

Thank you for the feedback!

Will rename cphy to is_cphy as suggested.

Regarding the lane positions and polarities, you are right, the PHY
driver already handles this in phy_qcom_mipi_csi2_parse_routing(),
parsing both data-lanes and lane-polarities from the firmware node at
bind time. There is no reason to pass them again through the configure
opts at stream-on. I will drop that loop from csiphy_stream_on() in the
next revision and keep the phy_configure_opts_mipi_cphy usage limited to
the dynamic per-stream timing parameters only.

Thank you,
Anusha

On 10/17/2026 6:44 AM, Bryan O'Donoghue wrote:
> On 17/10/2026 05:05, Anusha Arun Nandi wrote:
>> +    const bool cphy = (lncfg->phy_cfg == V4L2_MBUS_CSI2_CPHY);
> 
> suggest is_cphy
> 
>>       s64 link_freq;
>> +    int i;
>>       int ret;
>>   -    dphy_cfg = &dphy_opts.mipi_dphy;
>> -
>> -    link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>> num_lanes, false);
>> +    link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp,
>> num_lanes, cphy);
>>         if (link_freq <= 0) {
>>           dev_err(dev,
>> @@ -346,23 +345,49 @@ static int csiphy_stream_on(struct csiphy_device
>> *csiphy)
>>           return -EINVAL;
>>       }
>>   -    ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq,
>> num_lanes,
>> -                             dphy_cfg);
>> -    if (ret < 0) {
>> -        dev_err(dev, "Invalid config for link frequency %lld\n",
>> -            link_freq);
>> -        return ret;
>> -    }
>> +    if (cphy) {
>> +        struct phy_configure_opts_mipi_cphy *cphy_cfg =
>> &phy_opts.mipi_cphy;
>>   -    ret = phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
>> -    if (ret) {
>> -        dev_err(dev, "Failed to set phy mode\n");
>> -        return ret;
>> +        ret = phy_mipi_cphy_get_default_config_for_hsclk(link_freq,
>> num_lanes,
>> +                                 cphy_cfg);
>> +        if (ret < 0) {
>> +            dev_err(dev, "Invalid config for link frequency %lld\n",
>> +                link_freq);
>> +            return ret;
>> +        }
>> +
>> +        /* Set data trio lane positions and polarities (no clock
>> lane) */
>> +        for (i = 0; i < num_lanes; i++) {
>> +            cphy_cfg->lane_positions[i] = lncfg->data[i].pos;
>> +            cphy_cfg->lane_polarities[i] = lncfg->data[i].pol;
>> +        }
>> +
>> +        ret = phy_set_mode(csiphy->phy, PHY_MODE_MIPI_CPHY);
>> +        if (ret) {
>> +            dev_err(dev, "Failed to set phy mode\n");
>> +            return ret;
> 
> We already derive the lane positions and polarities in the PHY driver.
> There's no reason for the CSID block to know or care what the polarities
> or positions are.
> 
> drivers/phy/qualcomm/phy-qcom-mipi-csi2-
> core.c::phy_qcom_mipi_csi2_parse_routing()
> 
> I guess the PHY already knows the polarities and positions - or at least
> knows them for the DPHY case.
> 
>         /* lane-polarities: optional, up to num_data_lanes + 1 entries */
>         memset(lane_polarities, 0x00, sizeof(lane_polarities));
>         num_polarities = fwnode_property_count_u32(ep, "lane-polarities");
>         if (num_polarities > 0) {
>                 if (num_polarities != stream_cfg->num_data_lanes + 1) {
>                         ret = -EINVAL;
>                         dev_err(dev, "clock+data-lane %d/polarities %d
> mismatch\n",
>                                 stream_cfg->num_data_lanes + 1,
> num_polarities);
>                         goto out_put;
>                 }
> 
>                 ret = fwnode_property_read_u32_array(ep, "lane-
> polarities", lane_polarities,
>                                                      num_polarities);
>                 if (ret) {
>                         dev_err(dev, "Failed to read lane-polarities:
> %d\n", ret);
>                         goto out_put;
>                 }
>         }
> 
>         csi2phy->stream_cfg.lane_cfg.clk.pos = CSI2_DEFAULT_CLK_LANE;
>         csi2phy->stream_cfg.lane_cfg.clk.pol = lane_polarities[0];
> 
>         for (i = 0; i < csi2phy->stream_cfg.num_data_lanes; i++) {
>                 if (data_lanes[i] < 1 || data_lanes[i] >
> CSI2_MAX_DATA_LANES) {
>                         dev_err(dev, "Invalid lane %d\n", data_lanes[i]);
>                         ret = -EINVAL;
>                         goto out_put;
>                 }
> 
>                 /* Convert data-lanes = <1 2 3 4> to bit positions */
>                 csi2phy->stream_cfg.lane_cfg.data[i].pos = data_lanes[i]
> - 1;
>                 csi2phy->stream_cfg.lane_cfg.data[i].pol =
> lane_polarities[i + 1];
>         }
> 
> ---
> bod