From patchwork Sun Nov 20 15:20:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pavel Machek X-Patchwork-Id: 38264 Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c8TvW-0007Rt-4P; Sun, 20 Nov 2016 15:21:06 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-5) with esmtp id 1c8TvT-0005HU-8S; Sun, 20 Nov 2016 16:21:05 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753080AbcKTPUn (ORCPT + 1 other); Sun, 20 Nov 2016 10:20:43 -0500 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:57707 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752778AbcKTPUn (ORCPT ); Sun, 20 Nov 2016 10:20:43 -0500 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id D24A8823C3; Sun, 20 Nov 2016 16:20:39 +0100 (CET) Date: Sun, 20 Nov 2016 16:20:36 +0100 From: Pavel Machek To: Sakari Ailus 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 Message-ID: <20161120152034.GA5189@amd> References: <20161023200355.GA5391@amd> <20161119232943.GF13965@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161119232943.GF13965@valkosipuli.retiisi.org.uk> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.11.20.151216 X-PMX-Spam: Gauge=IIIIIIIII, Probability=9%, Report=' BODY_PARA_IS_SENTENCE_URL 0.1, MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, KNOWN_FREEWEB_URI 0.05, MSGID_ADDED_BY_MTA 0.05, BODY_SIZE_5000_5999 0, BODY_SIZE_7000_LESS 0, INVALID_MSGID_NO_FQDN 0, IN_REP_TO 0, LEGITIMATE_NEGATE 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, REFERENCES 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __ATTACHMENT_SIZE_0_10K 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CD 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __FORWARDED_MSG 0, __FRAUD_BODY_WEBMAIL 0, __FRAUD_WEBMAIL 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_ATTACHMENT2 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __KNOWN_FREEWEB_URI2 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MIME_TEXT_P2 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __MULTIPLE_URI_TEXT 0, __NO_HTML_TAG_RAW 0, __REFERENCES 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NS , __URI_WITH_PATH 0, __USER_AGENT 0' Hi! > > + u32 min, max; > > + > > + v4l2_ctrl_handler_init(&sensor->ctrl_handler, 4); > > + > > + /* V4L2_CID_GAIN */ > > + v4l2_ctrl_new_std(&sensor->ctrl_handler, &et8ek8_ctrl_ops, > > + V4L2_CID_GAIN, 0, ARRAY_SIZE(et8ek8_gain_table) - 1, > > + 1, 0); > > + > > + /* V4L2_CID_EXPOSURE */ > > + min = et8ek8_exposure_rows_to_us(sensor, 1); > > + max = et8ek8_exposure_rows_to_us(sensor, > > + sensor->current_reglist->mode.max_exp); > > Haven't I suggested to use lines instead? I vaguely remember doing so... > this would remove quite some code from the driver. Do you have some more hints? I'll try to figure it out... > > +#ifdef CONFIG_PM > > + > > +static int et8ek8_suspend(struct device *dev) > > static int __maybe_unused ... > > Please check the smiapp patches I just sent to the list. The smiapp driver > had similar issues. Ok, I guess I figured it out from other code (no network at the moment). > > + if (sensor->power_count) { > > + gpiod_set_value(sensor->reset, 0); > > + clk_disable_unprepare(sensor->ext_clk); > > + sensor->power_count = 0; > > + } > > + > > You're missing v4l2_async_unregister_subdev() here. Added. > > + v4l2_device_unregister_subdev(&sensor->subdev); > > + device_remove_file(&client->dev, &dev_attr_priv_mem); > > + v4l2_ctrl_handler_free(&sensor->ctrl_handler); > > + media_entity_cleanup(&sensor->subdev.entity); > > + > > + return 0; > > +} > > +MODULE_DEVICE_TABLE(i2c, et8ek8_id_table); > > + > > +static const struct dev_pm_ops et8ek8_pm_ops = { > > + .suspend = et8ek8_suspend, > > + .resume = et8ek8_resume, > > How about using SET_SYSTEM_SLEEP_PM_OPS() here? Ok, I guess that saves few lines. > > +module_i2c_driver(et8ek8_i2c_driver); > > + > > +MODULE_AUTHOR("Sakari Ailus "); > > You should put your name here as well. :-) > > It's been a long time I even tried to use it. :-i Me? Ok, I can list myself there, but I don't really know much about that driver. > > + * Contact: Sakari Ailus > > + * Tuukka Toivonen > > Tuukka's e-mail is wrong here (the correct address is elsewhere in the > patch). Fixed. Ok, these cleanups are here. Pavel diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c index eb131b2..eb8c1b4 100644 --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c @@ -5,6 +5,7 @@ * * Contact: Sakari Ailus * Tuukka Toivonen + * Pavel Machek * * Based on code from Toni Leinonen . * @@ -1435,9 +1436,7 @@ static const struct v4l2_subdev_internal_ops et8ek8_internal_ops = { /* -------------------------------------------------------------------------- * I2C driver */ -#ifdef CONFIG_PM - -static int et8ek8_suspend(struct device *dev) +static int __maybe_unused et8ek8_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); @@ -1449,7 +1448,7 @@ static int et8ek8_suspend(struct device *dev) return __et8ek8_set_power(sensor, false); } -static int et8ek8_resume(struct device *dev) +static int __maybe_unused et8ek8_resume(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); struct v4l2_subdev *subdev = i2c_get_clientdata(client); @@ -1461,13 +1460,6 @@ static int et8ek8_resume(struct device *dev) return __et8ek8_set_power(sensor, true); } -#else - -#define et8ek8_suspend NULL -#define et8ek8_resume NULL - -#endif /* CONFIG_PM */ - static int et8ek8_probe(struct i2c_client *client, const struct i2c_device_id *devid) { @@ -1542,6 +1534,7 @@ static int __exit et8ek8_remove(struct i2c_client *client) v4l2_device_unregister_subdev(&sensor->subdev); device_remove_file(&client->dev, &dev_attr_priv_mem); v4l2_ctrl_handler_free(&sensor->ctrl_handler); + v4l2_async_unregister_subdev(&sensor->subdev); media_entity_cleanup(&sensor->subdev.entity); return 0; @@ -1559,8 +1552,7 @@ static const struct i2c_device_id et8ek8_id_table[] = { MODULE_DEVICE_TABLE(i2c, et8ek8_id_table); static const struct dev_pm_ops et8ek8_pm_ops = { - .suspend = et8ek8_suspend, - .resume = et8ek8_resume, + SET_SYSTEM_SLEEP_PM_OPS(et8ek8_suspend, et8ek8_resume) }; static struct i2c_driver et8ek8_i2c_driver = { @@ -1576,6 +1568,6 @@ static struct i2c_driver et8ek8_i2c_driver = { module_i2c_driver(et8ek8_i2c_driver); -MODULE_AUTHOR("Sakari Ailus "); +MODULE_AUTHOR("Sakari Ailus , Pavel Machek