Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425749 > unrolled thread

Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-06-18 17:30 +0200
    Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor Pali Rohár <pali.rohar@gmail.com> - 2016-06-18 17:40 +0200
      Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-06-18 18:10 +0200
        Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor Pali Rohár <pali.rohar@gmail.com> - 2016-06-18 18:20 +0200
      Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus <sakari.ailus@iki.fi> - 2016-06-21 00:20 +0200

#1425749 — Re: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor

Hi!

> The sensor is found in Nokia N900 main camera
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

> +/*
> + *
> + * Register access helpers
> + *
> + */
> +
> +/*
> + * Read a 8/16/32-bit i2c register.  The value is returned in 'val'.
> + * Returns zero if successful, or non-zero otherwise.
> + */

Turn the first comment into "normal" comment style, and merge the two
comments?


> +static int et8ek8_i2c_read_reg(struct i2c_client *client, u16 data_length,
> +			       u16 reg, u32 *val)
> +{
> +	int r;
> +	struct i2c_msg msg[1];

Uff. That's a bit non-traditional. Use plain "struct i2c_msg msg;" if
you have just one?

> +		/* Now we start writing ... */
> +		r = et8ek8_i2c_buffered_write_regs(client, regs, cnt);
> +
> +		/* ... and then check that everything was OK */
> +		if (r < 0) {
> +			dev_err(&client->dev, "i2c transfer error !!!\n");
> +			return r;

I'd reduce number of "!"s in the message.

> +		/*
> +		 * If we ran into a sleep statement when going through
> +		 * the list, this is where we snooze for the required time
> +		 */
> +		if (next->type == ET8EK8_REG_DELAY) {
> +			set_current_state(TASK_UNINTERRUPTIBLE);
> +			schedule_timeout(msecs_to_jiffies(next->val));

Open-coded set_current_state(), and no restore, makes me
suspicious. Can msleep() be used here?

> +{
> +	int r;
> +	struct i2c_msg msg[1];

I'd avoid array for single entry. (You'll need to make it &msg below,
but...)

> +/*
> + * Return time of one row in microseconds, .8 fixed point format.
> + * If the sensor is not set to any mode, return zero.
> + */

typedef int fixedfp; then use it where .8 fixed point is used?

> +static int et8ek8_set_test_pattern(struct et8ek8_sensor *sensor, s32 mode)
> +{
...
> +	rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x111B,
> +				    tp_mode << 4);
> +	if (rval)
> +		goto out;
> +
> +	rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x1121,
> +				    cbh_mode << 7);
> +	if (rval)
> +		goto out;
> +
> +	rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x1124,
> +				    cbv_mode << 7);
> +	if (rval)
> +		goto out;
> +
> +	rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x112C, din_sw);
> +	if (rval)
> +		goto out;
> +
> +	rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x1420, r1420);
> +
> +out:
> +	return rval;
> +}

I'd avoid gotos when all it does is return.

> +/*
> + *
> + * Stingray sensor mode settings for Scooby
> + *
> + *
> + */

I'd fix it to normal comment style... and possibly remove it. Can you
understand what it says?

> +	},
> +	.regs = {
> +		{ ET8EK8_REG_8BIT, 0x1239, 0x4F },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x1238, 0x02 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x123B, 0x70 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x123A, 0x05 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x121B, 0x63 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x1220, 0x85 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x1221, 0x00 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x1222, 0x58 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x1223, 0x00 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x121D, 0x63 },	/*        */
> +		{ ET8EK8_REG_8BIT, 0x125D, 0x83 },	/*        */
> +		{ ET8EK8_REG_TERM, 0, 0}
> +	}

I'd remove the empty comments...

> +struct et8ek8_meta_reglist meta_reglist = {
> +	.version = "V14 03-June-2008",

Do we need the version?

> +	.reglist = {
> +		{ .ptr = &mode1_poweron_mode2_16vga_2592x1968_12_07fps },
> +		{ .ptr = &mode1_16vga_2592x1968_13_12fps_dpcm10_8 },
> +		{ .ptr = &mode3_4vga_1296x984_29_99fps_dpcm10_8 },
> +		{ .ptr = &mode4_svga_864x656_29_88fps },
> +		{ .ptr = &mode5_vga_648x492_29_93fps },
> +		{ .ptr = &mode2_16vga_2592x1968_3_99fps },
> +		{ .ptr = &mode_648x492_5fps },
> +		{ .ptr = &mode3_4vga_1296x984_5fps },
> +		{ .ptr = &mode_4vga_1296x984_25fps_dpcm10_8 },
> +		{ .ptr = 0 }
> +	}
> +};

I'd say .ptr = NULL.

> +struct v4l2_mbus_framefmt;
> +struct v4l2_subdev_pad_mbus_code_enum;
> +
> +struct et8ek8_mode {
> +	/* Physical sensor resolution and current image window */
> +	__u16 sensor_width;
> +	__u16 sensor_height;
> +	__u16 sensor_window_origin_x;
> +	__u16 sensor_window_origin_y;
> +	__u16 sensor_window_width;
> +	__u16 sensor_window_height;

If this can not be included from userland, convert __uX -> uX.

> +#define ET8EK8_MAX_LEN			32
> +struct et8ek8_meta_reglist {
> +	char version[ET8EK8_MAX_LEN];
> +	union {
> +		struct et8ek8_reglist *ptr;
> +	} reglist[];
> +};

What is going on here? union with single entry is strange...

(And thanks for doing all the work.)

Best regards,
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1425751

[Multipart message — attachments visible in raw view] — view raw

On Saturday 18 June 2016 17:22:59 Pavel Machek wrote:
> > +/*
> > + *
> > + * Stingray sensor mode settings for Scooby
> > + *
> > + *
> > + */
> 
> I'd fix it to normal comment style... and possibly remove it. Can you
> understand what it says?
> 
> > +	},
> > +	.regs = {
> > +		{ ET8EK8_REG_8BIT, 0x1239, 0x4F },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x1238, 0x02 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x123B, 0x70 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x123A, 0x05 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x121B, 0x63 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x1220, 0x85 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x1221, 0x00 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x1222, 0x58 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x1223, 0x00 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x121D, 0x63 },	/*        */
> > +		{ ET8EK8_REG_8BIT, 0x125D, 0x83 },	/*        */
> > +		{ ET8EK8_REG_TERM, 0, 0}
> > +	}
> 
> I'd remove the empty comments...
> 
> > +struct et8ek8_meta_reglist meta_reglist = {
> > +	.version = "V14 03-June-2008",
> 
> Do we need the version?
> 
> > +	.reglist = {
> > +		{ .ptr = &mode1_poweron_mode2_16vga_2592x1968_12_07fps },
> > +		{ .ptr = &mode1_16vga_2592x1968_13_12fps_dpcm10_8 },
> > +		{ .ptr = &mode3_4vga_1296x984_29_99fps_dpcm10_8 },
> > +		{ .ptr = &mode4_svga_864x656_29_88fps },
> > +		{ .ptr = &mode5_vga_648x492_29_93fps },
> > +		{ .ptr = &mode2_16vga_2592x1968_3_99fps },
> > +		{ .ptr = &mode_648x492_5fps },
> > +		{ .ptr = &mode3_4vga_1296x984_5fps },
> > +		{ .ptr = &mode_4vga_1296x984_25fps_dpcm10_8 },
> > +		{ .ptr = 0 }
> > +	}
> > +};
> 
> I'd say .ptr = NULL.
> 

Anyway, this code was generated from configuration ini files and perl 
script available from: https://gitorious.org/omap3camera/camera-firmware

Originally in Maemo above C structure is compiled into binary file and 
via request_firmware() loaded from userspace to kernel driver.

For me this sounds like a big overkill, so I included above reglist code 
direcly into et8ek8 kernel driver to avoid request_firmware() and 
separate userspace storage...

And for smia-sensor (front webcam) in that gitorious repository is also 
reglist structure. It is not needed? Can somebody investigate why it is 
not needed?

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1425761

Hi!

> > > +	.reglist = {
> > > +		{ .ptr = &mode1_poweron_mode2_16vga_2592x1968_12_07fps },
> > > +		{ .ptr = &mode1_16vga_2592x1968_13_12fps_dpcm10_8 },
> > > +		{ .ptr = &mode3_4vga_1296x984_29_99fps_dpcm10_8 },
> > > +		{ .ptr = &mode4_svga_864x656_29_88fps },
> > > +		{ .ptr = &mode5_vga_648x492_29_93fps },
> > > +		{ .ptr = &mode2_16vga_2592x1968_3_99fps },
> > > +		{ .ptr = &mode_648x492_5fps },
> > > +		{ .ptr = &mode3_4vga_1296x984_5fps },
> > > +		{ .ptr = &mode_4vga_1296x984_25fps_dpcm10_8 },
> > > +		{ .ptr = 0 }
> > > +	}
> > > +};
> > 
> > I'd say .ptr = NULL.
> > 
> 
> Anyway, this code was generated from configuration ini files and perl 
> script available from: https://gitorious.org/omap3camera/camera-firmware
> 
> Originally in Maemo above C structure is compiled into binary file and 
> via request_firmware() loaded from userspace to kernel driver.
> 
> For me this sounds like a big overkill, so I included above reglist code 
> direcly into et8ek8 kernel driver to avoid request_firmware() and 
> separate userspace storage...

Yep, that makes sense, thanks for explanation. I guess that means that
we should put a comment on top of the file explaining what is going
on.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1425762

[Multipart message — attachments visible in raw view] — view raw

On Saturday 18 June 2016 18:04:23 Pavel Machek wrote:
> Hi!
> 
> > > > +	.reglist = {
> > > > +		{ .ptr = &mode1_poweron_mode2_16vga_2592x1968_12_07fps },
> > > > +		{ .ptr = &mode1_16vga_2592x1968_13_12fps_dpcm10_8 },
> > > > +		{ .ptr = &mode3_4vga_1296x984_29_99fps_dpcm10_8 },
> > > > +		{ .ptr = &mode4_svga_864x656_29_88fps },
> > > > +		{ .ptr = &mode5_vga_648x492_29_93fps },
> > > > +		{ .ptr = &mode2_16vga_2592x1968_3_99fps },
> > > > +		{ .ptr = &mode_648x492_5fps },
> > > > +		{ .ptr = &mode3_4vga_1296x984_5fps },
> > > > +		{ .ptr = &mode_4vga_1296x984_25fps_dpcm10_8 },
> > > > +		{ .ptr = 0 }
> > > > +	}
> > > > +};
> > > 
> > > I'd say .ptr = NULL.
> > 
> > Anyway, this code was generated from configuration ini files and
> > perl script available from:
> > https://gitorious.org/omap3camera/camera-firmware
> > 
> > Originally in Maemo above C structure is compiled into binary file
> > and via request_firmware() loaded from userspace to kernel driver.
> > 
> > For me this sounds like a big overkill, so I included above reglist
> > code direcly into et8ek8 kernel driver to avoid request_firmware()
> > and separate userspace storage...
> 
> Yep, that makes sense, thanks for explanation. I guess that means
> that we should put a comment on top of the file explaining what is
> going on.
> 
> Best regards,
> 									Pavel

Here is that original stingraytsb_v14_simple.ini file:

https://gitorious.org/omap3camera/camera-firmware?p=omap3camera:camera-firmware.git;a=tree;f=ini

Looks like are are some "non simple" stingraytsb_v14 files too, but I
have no idea which modes defines...

Sakari, any idea?
Which we should include into kernel et8ek8 kernel driver?

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1427096

Hi Pali,

On Sat, Jun 18, 2016 at 05:37:33PM +0200, Pali Rohár wrote:
> On Saturday 18 June 2016 17:22:59 Pavel Machek wrote:
> > > +/*
> > > + *
> > > + * Stingray sensor mode settings for Scooby
> > > + *
> > > + *
> > > + */
> > 
> > I'd fix it to normal comment style... and possibly remove it. Can you
> > understand what it says?
> > 
> > > +	},
> > > +	.regs = {
> > > +		{ ET8EK8_REG_8BIT, 0x1239, 0x4F },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x1238, 0x02 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x123B, 0x70 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x123A, 0x05 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x121B, 0x63 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x1220, 0x85 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x1221, 0x00 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x1222, 0x58 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x1223, 0x00 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x121D, 0x63 },	/*        */
> > > +		{ ET8EK8_REG_8BIT, 0x125D, 0x83 },	/*        */
> > > +		{ ET8EK8_REG_TERM, 0, 0}
> > > +	}
> > 
> > I'd remove the empty comments...
> > 
> > > +struct et8ek8_meta_reglist meta_reglist = {
> > > +	.version = "V14 03-June-2008",
> > 
> > Do we need the version?
> > 
> > > +	.reglist = {
> > > +		{ .ptr = &mode1_poweron_mode2_16vga_2592x1968_12_07fps },
> > > +		{ .ptr = &mode1_16vga_2592x1968_13_12fps_dpcm10_8 },
> > > +		{ .ptr = &mode3_4vga_1296x984_29_99fps_dpcm10_8 },
> > > +		{ .ptr = &mode4_svga_864x656_29_88fps },
> > > +		{ .ptr = &mode5_vga_648x492_29_93fps },
> > > +		{ .ptr = &mode2_16vga_2592x1968_3_99fps },
> > > +		{ .ptr = &mode_648x492_5fps },
> > > +		{ .ptr = &mode3_4vga_1296x984_5fps },
> > > +		{ .ptr = &mode_4vga_1296x984_25fps_dpcm10_8 },
> > > +		{ .ptr = 0 }
> > > +	}
> > > +};
> > 
> > I'd say .ptr = NULL.
> > 
> 
> Anyway, this code was generated from configuration ini files and perl 
> script available from: https://gitorious.org/omap3camera/camera-firmware
> 
> Originally in Maemo above C structure is compiled into binary file and 
> via request_firmware() loaded from userspace to kernel driver.
> 
> For me this sounds like a big overkill, so I included above reglist code 
> direcly into et8ek8 kernel driver to avoid request_firmware() and 
> separate userspace storage...

The idea at the time was that it'd be possible to support changes in the
register list without software changes as well as using different register
lists on a different device.

That's not really how device drivers are written. Considering that the use
cases for this very sensor driver seem rather different now than they did
then, I think it's safe to assume there won't be significant (if any at all)
changes to these configurations going forward.

I don't think there's really any value as such in maintaining the
compatibility with the data structures used in the camera-firmware
repository above.

> 
> And for smia-sensor (front webcam) in that gitorious repository is also 
> reglist structure. It is not needed? Can somebody investigate why it is 
> not needed?

The front camera is SMIA compliant and works (at least should!) with the
smiapp driver. I vaguely remember using it with platform data long, long
time ago. And I'm sure someone was working on it, I just don't remember now
who. :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web