Hi,
On 18/11/21 18:34, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> On 11/18/21 7:26 PM, Sakari Ailus wrote:
>> Hi Luca,
>>
>> On Thu, Nov 18, 2021 at 06:11:35PM +0100, Luca Ceresoli wrote:
>>> Hi Eugen,
>>>
>>> On 18/11/21 16:40, Eugen Hristev wrote:
>>>> Current driver supports only SRGGB 10 bit RAW bayer format.
>>>> Add the enum_mbus_code implementation to report this format supported.
>>>>
>>>> # v4l2-ctl -d /dev/v4l-subdev3 --list-subdev-mbus-codes
>>>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>>> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>>> #
>>>>
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>>>
>>> Generally OK, but I have a few minor comments.
>>>
>>>> ---
>>>> drivers/media/i2c/imx274.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>>> index 2e804e3b70c4..25a4ef8f6187 100644
>>>> --- a/drivers/media/i2c/imx274.c
>>>> +++ b/drivers/media/i2c/imx274.c
>>>> @@ -1909,7 +1909,21 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>>>> return err;
>>>> }
>>>>
>>>> +static int imx274_enum_mbus_code(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_state *sd_state,
>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>> +{
>>>> + if (code->index > 0)
>>>> + return -EINVAL;
>>>
>>> Many driver do check code->pad too, so you might want to do
>>>
>>> if (code->pad > 0 || code->index > 0)
>>> return -EINVAL;
>>
>> The caller will have checked the pad exists, and there's a single one on
>> the subdev I suppose.
>>
>>>
>>> However I don't think it is strictly necessary, thus
>>>
>>>> +
>>>> + /* only supported format in the driver is Raw 10 bits SRGGB */
>>>> + code->code = MEDIA_BUS_FMT_SRGGB10_1X10;
>>>
>>> Maybe better:
>>>
>>> code->code = to_imx274(sd)->format.code
>>
>> Good idea.
>
> Hi,
>
> Initially I thought about this, but my idea was to keep it simple.
> If we return format.code, we are not enumerating anything, just
> returning the current format and that's it.
>
> If we want to be correct, I would rather add a struct with supported
> formats(currently just one ) and iterate through this structure.
>
> If in the future we want to support more formats (I see this sensor
> could support SRGGB 12 bits ), then it would support 2 formats, and
> returning priv->format.code would be incorrect here (it would be correct
> for a g_fmt only )
>
> So, how do you think I should proceed ?
> 1/ Create a struct with a single element and iterate through it
I dislike adding complexity (albeit small) that adds no features. Let's
leave this idea to the day somebody adds another format.
> 2/ Leave it like this and always return SRGGB10
> 3/ Do like Luca suggests and return format.code (which I am personally
> against )
No strong preference between 2 and 3.
--
Luca
