Hi Laurent,
On 09/27/2012 12:29 PM, Laurent Pinchart wrote:
> On Thursday 27 September 2012 00:00:20 Sylwester Nawrocki wrote:
>> On 09/26/2012 10:32 PM, Mauro Carvalho Chehab wrote:
>>> Em Thu, 13 Sep 2012 12:02:14 +0100 Sangwook Lee escreveu:
>>>> This patch adds driver for S5K4ECGX sensor with embedded ISP SoC,
>>>> S5K4ECGX, which is a 5M CMOS Image sensor from Samsung
>>>> The driver implements preview mode of the S5K4ECGX sensor.
>>>> capture (snapshot) operation, face detection are missing now.
>>>> Following controls are supported:
>>>> contrast/saturation/brightness/sharpness
>>>>
>>>> Signed-off-by: Sangwook Lee<sangwook.lee@xxxxxxxxxx>
>>>> Reviewed-by: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx>
>>>> Cc: Francesco Lavra<francescolavra.fl@xxxxxxxxx>
>>>> Cc: Scott Bambrough<scott.bambrough@xxxxxxxxxx>
>>>> Cc: Homin Lee<suapapa@xxxxxxxxxxxxxx>
>>>
>>> ...
>>>
>>>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd)
>>>> +{
>>>> + const struct firmware *fw;
>>>> + const u8 *ptr;
>>>> + int err, i, regs_num;
>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> + u16 val;
>>>> + u32 addr, crc, crc_file, addr_inc = 0;
>>>> +
>>>> + err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
>>>
>>> The patch looks correct on my eyes... Yet, calling request_firmware()
>>> might not be the right thing to do. The thing is that newer versions of
>>> udev refuse to load firmware synchronously during probe/init time.
>>>
>>> As this function is actually called by s_power, maybe this driver doesn't
>>> suffer from that new udev behavior, so, I'll be merging it as-is. However,
>>> I suggest you to take a deeper review on that and, if possible, test it
>>> with the latest udev.
>>
>> True, it's indeed a bit tricky. The host interface driver this sensor driver
>> has been tested with calls s_power on a subdev only in response to a video
>> device open(). During probe only subdev's .registered() callback is called.
>> And there is no request_firmware() there, as it is not needed to boot the
>> sensor's MCU. The "firmware" is really just a set of settings that are
>> normally needed only before streaming needs to be started.
>
> That triggers a red flag warning :-) What kind of settings do you have in the
> "firmware" ?
What's in this binary file is about 3 000 register values that are needed
to be written to a sensor after it is powered on and before it can be used.
I think this is some sort of a "patch" file that redefines initial state
of the sensor, default register values or something. Perhaps some tuning
values prepared after the sensor was manufactured. Since it is undocumented
I could only guess.
I don't think it is really unusual to load such a patch file by means
of the firmware API. Some ISP drivers do this as well, to customize
to a particular image sensor connected to them. Besides of requiring
a proper firmware file.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
