linux-kernel - Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161218220105.GS16630@valkosipuli.retiisi.org.uk>
Date:   Mon, 19 Dec 2016 00:01:06 +0200
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Pavel Machek <pavel@....cz>
Cc:     Pali Rohár <pali.rohar@...il.com>,
        ivo.g.dimitrov.75@...il.com, sre@...nel.org,
        linux-media@...r.kernel.org, galak@...eaurora.org,
        mchehab@....samsung.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] media: Driver for Toshiba et8ek8 5MP sensor

Hi Pavel,

On Wed, Dec 14, 2016 at 09:12:02PM +0100, Pavel Machek wrote:
> Hi!
> 
> > On Wednesday 14 December 2016 13:24:51 Pavel Machek wrote:
> > >  
> > > Add driver for et8ek8 sensor, found in Nokia N900 main camera. Can be
> > > used for taking photos in 2.5MP resolution with fcam-dev.
> > > 
> > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
> > > Signed-off-by: Pavel Machek <pavel@....cz>
> > > 
> > > ---
> > > From v4 I did cleanups to coding style and removed various oddities.
> > > 
> > > Exposure value is now in native units, which simplifies the code.
> > > 
> > > The patch to add device tree bindings was already acked by device tree
> > > people.
> 
> > > +	default:
> > > +		WARN_ONCE(1, ET8EK8_NAME ": %s: invalid message length.\n",
> > > +			  __func__);
> > 
> > dev_warn_once()
> ...
> > > +	if (WARN_ONCE(cnt > ET8EK8_MAX_MSG,
> > > +		      ET8EK8_NAME ": %s: too many messages.\n", __func__)) {
> > 
> > Maybe replace it with dev_warn_once() too? That condition in WARN_ONCE
> > does not look nice...
> ...
> > > +			if (WARN(next->type != ET8EK8_REG_8BIT &&
> > > +				 next->type != ET8EK8_REG_16BIT,
> > > +				 "Invalid type = %d", next->type)) {
> > dev_warn()
> >
> > > +	WARN_ON(sensor->power_count < 0);
> > 
> > Rather some dev_warn()? Do we need stack trace here?
> 
> I don't see what is wrong with WARN(). These are not expected to
> trigger, if they do we'll fix it. If you feel strongly about this,
> feel free to suggest a patch.

I think WARN() is good. It's a driver bug and it deserves to be notified.

> 
> > > +static int et8ek8_i2c_reglist_find_write(struct i2c_client *client,
> > > +					 struct et8ek8_meta_reglist *meta,
> > > +					 u16 type)
> > > +{
> > > +	struct et8ek8_reglist *reglist;
> > > +
> > > +	reglist = et8ek8_reglist_find_type(meta, type);
> > > +	if (!reglist)
> > > +		return -EINVAL;
> > > +
> > > +	return et8ek8_i2c_write_regs(client, reglist->regs);
> > > +}
> > > +
> > > +static struct et8ek8_reglist **et8ek8_reglist_first(
> > > +		struct et8ek8_meta_reglist *meta)
> > > +{
> > > +	return &meta->reglist[0].ptr;
> > > +}
> > 
> > Above code looks like re-implementation of linked-list. Does not kernel
> > already provide some?
> 
> Its actually array of pointers as far as I can tell. I don't think any
> helpers would be useful here.
> 
> > > +	new = et8ek8_gain_table[gain];
> > > +
> > > +	/* FIXME: optimise I2C writes! */
> > 
> > Is this FIMXE still valid?
> 
> Probably. Lets optimize it after merge.

I guess it's been like this since 2008 or so. I guess the comment could be
simply removed, it's not a real problem.

> 
> > > +	if (sensor->power_count) {
> > > +		WARN_ON(1);
> > 
> > Such warning is probably not useful...
> 
> It should not happen, AFAICT. That's why I'd like to know if it does.
> 
> > > +#include "et8ek8_reg.h"
> > > +
> > > +/*
> > > + * Stingray sensor mode settings for Scooby
> > > + */
> > 
> > Are settings for this sensor Stingray enough?
> 
> Seems to work well enough for me. If more modes are needed, we can add
> them later.

AFAIR the module is called Stingray.

-- 
Kind regards,

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