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

From: Pavel Machek <pavel@ucw.cz>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: ivo.g.dimitrov.75@gmail.com, sre@kernel.org,
	pali.rohar@gmail.com, linux-media@vger.kernel.org,
	galak@codeaurora.org, mchehab@osg.samsung.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] media: Driver for Toshiba et8ek8 5MP sensor
Date: Tue, 13 Dec 2016 22:05:06 +0100	[thread overview]
Message-ID: <20161213210506.GA11569@amd> (raw)
In-Reply-To: <20161023201954.GI9460@valkosipuli.retiisi.org.uk>

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]