Hello Sakari,
Thanks for your feedback, no worries about the delay ...
On 9/9/2024 9:06 AM, Sakari Ailus wrote:
> Hi Sylwain,
>
> Apologies for the delay...
>
> On Mon, Jun 03, 2024 at 11:59:29AM +0200, Sylvain Petinot wrote:
>>>> +/*
>>>> + * The VD56G3 pixel array is organized as follows:
>>>> + *
>>>> + * +--------------------------------+
>>>> + * | | \
>>>> + * | +------------------------+ | |
>>>> + * | | | | |
>>>> + * | | | | |
>>>> + * | | | | |
>>>> + * | | | | |
>>>> + * | | | | |
>>>> + * | | Default resolution | | | Native height (1364)
>>>
>>> What's outside the default resolution? It doesn't appear the driver would
>>> allow capturing pixels out side this area.
>>
>> Well both native and default resolutions are supported in the
>> 'vd56g3_supported_modes' below.
>> However this quite exotic resolution (1364 x 1124) isn't well supported
>> by csi receivers, ISPs. That's why the default resolution of the driver
>> is 1120 x 1360 (multiple of 16).
>
> Ack.
>
> I'd still keep the native resolution as the default and allow configuring
> something else if the user space wants to.
>
> The desired resolution really depends on the use case (as well as the ISP).
Sure, I can change the default to the native... The choice I made was
more a way to simplify (my) life (several times we went into debugging
before realizing that it was the exotic resolution causing the unwanted
behavior).
But, that's probably better, I'll change it for V4.
>
>>>> + break;
>>>> + case V4L2_CID_EXPOSURE_AUTO:
>>>> + is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
>>>> + __v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
>>>> + __v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
>>>> + break;
>>>> + default:
>>>> + break;
>>>
>>> You could omit default here.
>>
>> I don't really like switch case without default. For sure I can omit,
>> but I prefer making it explicit.
>
> I'm ok with that.
>
> ...
>
>>>> +static int vd56g3_power_off(struct vd56g3 *sensor)
>>>> +{
>>>> + clk_disable_unprepare(sensor->xclk);
>>>> + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>>>> + regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
>>>> + return 0;
>>>
>>> You can make the return type void.
>>>
>>> Do you need two pairs of functions doing the same, or could you call
>>> vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
>>> remove functions, too?
>>
>> "Well, in fact, I tested both options before submitting V2 (I mean the
>> unification of vd56g3_runtime_resume/suspend functions with
>> vd56g3_power_on/off).
>>
>> The unification option has the advantage of simplifying the code and
>> removing two "useless" functions. The only drawback is that I had to
>> call v4l2_i2c_subdev_init() earlier in the probe() function, whereas
>> it's currently called in vd56g3_subdev_init() (currently at the end of
>> the probe()). OK, it's not a big deal, but I find that the resulting
>> code is not as well structured/divided (thus readable).
>>
>> I'm interested to get your feedback to decide wich option to push for V3.
>
> I'd prefer calling v4l2_i2c_subdev_init() earlier, in order to set the
> device context. These are usually among those things that should be done as
> early as possible, in order to avoid invalid pointers where much of the
> driver code expects something else.
Yes, the V3 I pushed include this modification.
>
> Btw. if you're not in too much hurry (I guess so?), we're just about to
> rework the sensor API, to include internal pads and embedded data, for
> better sensor configurability. It'll take a while before we're there
> though, but when this driver is merged, the existing API must continue to
> work.
>
I have a few branches with stream API (notably for the support of status
lines), but I need a to test a bit more before pushing.
--
Sylvain
