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


Groups > linux.kernel > #1541533

Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor

From Pavel Machek <pavel@ucw.cz>
Newsgroups linux.kernel
Subject Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor
Date 2016-12-13 22:20 +0100
Message-ID <sO2Eq-6bg-27@gated-at.bofh.it> (permalink)
References <svxfH-5uo-3@gated-at.bofh.it> <svxz4-5CF-7@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw


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

Hi!

I have finally found the old mail you were refering to. Let me go
through it.

> > +/*
> > + * Convert exposure time `us' to rows. Modify `us' to make it to
> > + * correspond to the actual exposure time.
> > + */
> > +static int et8ek8_exposure_us_to_rows(struct et8ek8_sensor *sensor, u32 *us)
> 
> Should a driver do something like this to begin with?
> 
> The smiapp driver does use the native unit of exposure (lines) for the
> control and I think the et8ek8 driver should do so as well.
> 
> The HBLANK, VBLANK and PIXEL_RATE controls are used to provide the user with
> enough information to perform the conversion (if necessary).

Well... I believe exposure in usec is preffered format for userspace
to work with (because then it can change resolution and keep camera
settings) but I see kernel code is quite ugly. Let me see what I can do...

> > +	if (ret) {
> > +		dev_warn(dev, "can't get clock-frequency\n");
> > +		return ret;
> > +	}
> > +
> > +	mutex_init(&sensor->power_lock);
> 
> mutex_destroy() should be called on the mutex if probe fails after this and
> in remove().

Ok.

> > +static int __exit et8ek8_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct et8ek8_sensor *sensor = to_et8ek8_sensor(subdev);
> > +
> > +	if (sensor->power_count) {
> > +		gpiod_set_value(sensor->reset, 0);
> > +		clk_disable_unprepare(sensor->ext_clk);
> 
> How about the regulator? Could you call et8ek8_power_off() instead?

Hmm. Actually if we hit this, it indicates something funny is going
on, no? I guess I'll add WARN_ON there...

> > +++ b/drivers/media/i2c/et8ek8/et8ek8_reg.h
> > @@ -0,0 +1,96 @@
> > +/*
> > + * et8ek8.h
> 
> et8ek8_reg.h

Ok.

Thanks for patience,
								Pavel

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

Back to linux.kernel | Previous | NextNext in thread | Find similar | Unroll thread


Thread

Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor Pavel Machek <pavel@ucw.cz> - 2016-12-13 22:20 +0100
  Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor Sakari Ailus <sakari.ailus@iki.fi> - 2016-12-18 23:00 +0100

csiph-web