Subject: Re: [PATCH] media: i2c: imx274: implement enum_mbus_code
To:     Eugen.Hristev@microchip.com, sakari.ailus@linux.intel.com
Cc:     leonl@leopardimaging.com, linux-media@vger.kernel.org,
        skomatineni@nvidia.com, linux-kernel@vger.kernel.org
References: <20211118154009.307430-1-eugen.hristev@microchip.com>
 <fa26e991-9228-7ed7-833a-b296e6b32afc@lucaceresoli.net>
 <YZaMtGhqaXIOLhox@paasikivi.fi.intel.com>
 <aff9c675-cd7b-ef11-d337-c5c3de9b921a@microchip.com>
From:   Luca Ceresoli <luca@lucaceresoli.net>
Message-ID: <65d864d8-8c28-cf8e-43bf-a5a4c4dc11f7@lucaceresoli.net>
Date:   Thu, 18 Nov 2021 18:37:32 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <aff9c675-cd7b-ef11-d337-c5c3de9b921a@microchip.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 18/11/21 18:34, Eugen.Hristev@microchip.com 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@microchip.com>
>>>
>>> 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