Received-SPF: pass (google.com: domain of linux-kernel+bounces-109182-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <2f2a8047-d885-4c65-9b07-23b79f1ca2a8@quicinc.com>
Date: Wed, 20 Mar 2024 18:41:17 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/8] media: qcom: camss: Add subdev notify support
Content-Language: en-US
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        Depeng Shao
	<quic_depengs@quicinc.com>, <rfoss@kernel.org>,
        <todor.too@gmail.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <mchehab@kernel.org>,
        <quic_yon@quicinc.com>
CC: <linux-kernel@vger.kernel.org>, <linux-media@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>
References: <20240320141136.26827-1-quic_depengs@quicinc.com>
 <20240320141136.26827-3-quic_depengs@quicinc.com>
 <21d2139f-8547-466a-9be1-fbeae1194869@linaro.org>
From: "Gjorgji Rosikopulos (Consultant)" <quic_grosikop@quicinc.com>
In-Reply-To: <21d2139f-8547-466a-9be1-fbeae1194869@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

Hi Bryan,

On 3/20/2024 6:08 PM, Bryan O'Donoghue wrote:
> On 20/03/2024 14:11, Depeng Shao wrote:
>> From: Yongsheng Li <quic_yon@quicinc.com>
>>
>> The buf done irq and register update register are moved
>> to CSID in SM8550, so but the write master configuration
>> in VFE, in case adapt existing code logic. So add buf
>> done and register related subdev event, and use the notify
>> interface in the v4l2_device structure to communicate
>> between CSID and VFE driver.
> 
> 
> Shouldn't it be possible to just have a function to write internally ?
> 
> You know the indexes of the CSID -> VFE connection.
> 
> The subdev notify is I think not the right fit for this purpose within
> our driver.
> 

<snip>

> 
> I'm really not sure I see a good reason for this.
> 
> Why can't we just define calls between vfe and csid similar to
> 
> drivers/media/platform/qcom/camss/camss-csid.c:        ret = vfe_get(vfe);


Maybe we need to rethink and redesign this part of the driver.

In the initial version when this driver was introduced the CSID was
independent device from hw perspective,
and represented as separate sub-device.

With the newer architectures CSID was part of IFE hw (handled by the VFE
sub-device in this driver)
and vfe_get was introduced, but i believe it was not an issue because
the CISD still was kind of independent.

In the patch series:
"https://lore.kernel.org/lkml/20240319173935.481-4-quic_grosikop@quicinc.com/T/"

We try to decouple CSID from VFE and remove direct dependency
introducing parent_dev_ops,
where depending of the topology and parent device (VFE in this case or
other parent in future chipsets which contain CSID)
can reuse the code.

I am not sure if introducing parent_dev_ops is right way to go but we
can discuss further and see how to extend the driver
in proper way.

Just to add i am not saying that adding direct calls to VFE is not
proper but just close the door for other sub-device contain CSID to use
the code. :-)


Regards,
~Gjorgji

> 
> ---
> bod
>