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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Pavel,

On Tue, Dec 13, 2016 at 10:05:06PM +0100, Pavel Machek wrote:
> 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...

That's not so important IMO --- the granularity may matter and there's no
way you can properly communicate that to the user if you use a non-native
unit.

My preference is the native unit, but considering that you've got an
existing user space application and perhaps even more importantly, it's very
unlikely the device would be used elsewhere.

The smiapp driver uses lines. Up to you.

> 
> > > +	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...

Yes. A WARN_ON() would be good.

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

Same to you! :-)

-- 
Kind regards,

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