From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor Date: Tue, 10 Sep 2019 22:23:39 +0900 Message-ID: References: <20190808092215.5608-1-dongchun.zhu@mediatek.com> <20190808092215.5608-3-dongchun.zhu@mediatek.com> <20190823100126.GB33937@chromium.org> <1568021243.21623.250.camel@mhfsdcap03> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1568021243.21623.250.camel@mhfsdcap03> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Dongchun Zhu Cc: Mark Rutland , devicetree@vger.kernel.org, Nicolas Boichat , srv_heupstream , shengnan.wang@mediatek.com, Louis Kuo , Sj Huang , Rob Herring , "moderated list:ARM/Mediatek SoC support" , Sakari Ailus , Matthias Brugger , Cao Bing Bu , Mauro Carvalho Chehab , "list@263.net:IOMMU DRIVERS , Joerg Roedel , " , Linux Media Mailing List List-Id: devicetree@vger.kernel.org Hi Dongchun, On Mon, Sep 9, 2019 at 6:27 PM Dongchun Zhu wrote: > > Hi Tomasz, > > On Fri, 2019-08-23 at 19:01 +0900, Tomasz Figa wrote: > > Hi Dongchun, > > > > On Thu, Aug 08, 2019 at 05:22:15PM +0800, dongchun.zhu@mediatek.com wrote: [snip] > > > + > > > /* vertical-timings from sensor */ > > > #define OV8856_REG_VTS 0x380e > > > #define OV8856_VTS_MAX 0x7fff > > > @@ -64,6 +80,14 @@ > > > > > > #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd) > > > > > > +static const char * const ov8856_supply_names[] = { > > > + "dovdd", /* Digital I/O power */ > > > + "avdd", /* Analog power */ > > > + "dvdd", /* Digital core power */ > > > +}; > > > + > > > +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names) > > > + > > > enum { > > > OV8856_LINK_FREQ_720MBPS, > > > OV8856_LINK_FREQ_360MBPS, > > > @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] = { > > > {0x5e00, 0x00} > > > }; > > > > > > +static const struct ov8856_reg mode_3264x2448_regs[] = { [snip] > > > +}; > > > + > > > > It would be better if we could find the differences between the two arrays > > and handle them incrementally. > > > > This approach is not recommended. > Not recommended by whom? :) I myself recommend that approach. I'm sorry, but I'm going to NACK this patch (including the chromeos-4.19 tree), unless there is a very good technical reason not to do it the way I'm suggesting. The other drivers do it that way and I see no reason why this one should be an exception. > For these two arrays, sensor input clock frequencies (19.2MHz, 24MHz) > are different, corresponding to different PLL register setting. > > Besides, there are also some differences in image resolution and > hts/vts, including 0x3614 register that reflecting sensor revision. > What would be the reason preventing us from handling that in driver code? Note that I do _not_ mean just taking addresses and values that are different and putting them to a separate array. What I'm asking for is to handle the differences in a programmatic way - with dedicated code in the driver setting appropriate registers. [snip] > > > + fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10; > > > + else > > > + fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10; > > > + > > > fmt->field = V4L2_FIELD_NONE; > > > } > > > > > > @@ -850,6 +1333,17 @@ static int ov8856_start_streaming(struct ov8856 *ov8856) > > > return ret; > > > } > > > > > > + /* update R3614 for 1B module */ > > > > What's R3614? > > > > R3614 is the register 0x3614, which reflects the sensor revision. > For instance, it would be 0x20 for 1B module, while 0x60 for 2A module. > My point is - this comment doesn't mean anything for a person reading it. The code below is actually more meaningful - you can see that the clock settings register is written with a value for 1B. > > > + if (ov8856->is_1B_module) { > > > + ret = ov8856_write_reg(ov8856, OV8856_CLK_REG, > > > + OV8856_REG_VALUE_08BIT, > > > + OV8856_CLK_REG_1B_VAL); Please define this value according to what it means, not a fixed constant for 1B sensor revision. > > > + if (ret) { > > > + dev_err(&client->dev, "failed to set R3614"); > > > + return ret; > > > + } > > > + } > > > + > > > ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler); > > > if (ret) > > > return ret; > > > @@ -882,6 +1376,8 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) > > > if (ov8856->streaming == enable) > > > return 0; > > > > > > + dev_dbg(&client->dev, "hardware version: (%d)\n", ov8856->is_1B_module); > > > + > > > mutex_lock(&ov8856->mutex); > > > if (enable) { > > > ret = pm_runtime_get_sync(&client->dev); > > > @@ -908,6 +1404,54 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable) > > > return ret; > > > } > > > > > > +/* Calculate the delay in us by clock rate and clock cycles */ > > > +static inline u32 ov8856_cal_delay(u32 cycles) > > > +{ > > > + return DIV_ROUND_UP(cycles, OV8856_XVCLK_FREQ / 1000 / 1000); > > > +} > > > + > > > +static int __ov8856_power_on(struct ov8856 *ov8856) > > > +{ > > > + int ret; > > > + u32 delay_us; > > > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > > > + > > > + ret = clk_prepare_enable(ov8856->xvclk); > > > + if (ret < 0) { > > > + dev_err(&client->dev, "Failed to enable xvclk\n"); > > > + return ret; > > > + } > > > + > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > > > > According to my datasheet, this sensor doesn't have a reset pin. The one I > > can see there is XSHUTDN, which I would call "n_shutdn" here. > > > > I would rename this pin in next release. > BTW, how do you define "n_shutdn" or "shuutdn"? > If GPIO is actively high, then "n_shutdn"? > If the GPIO is active-high (means shutdown on high) then it's just "shutdn_gpio". However, the datasheet says it's active-low (means shutdown on low), so that should be "n_shutdn_gpio". > > > + > > > + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > > + if (ret < 0) { > > > + dev_err(&client->dev, "Failed to enable regulators\n"); > > > + goto disable_clk; > > > + } > > > + > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, 0); > > > > According to the datasheet, XSHUTDN should be 0 for shutdown and 1 for > > running. Why is it the other way around here? > > > > For GPIO, the definition of bit field of flags defined in DT seems > reversed. > This would be fixed in next release. > > > > + > > > + /* 8192 cycles prior to first SCCB transaction */ > > > + delay_us = ov8856_cal_delay(8192); > > > > If we pass a constant to the function and the function itself only uses > > constants inside, could we just define a constant delay instead? > > > > This calculation refers to powering up sequence in datasheet. > Did you mean using usleep_range() directly? My point is, we can just #define OV8856_SCCB_INIT_DELAY_US (8192 * [...]) [...] usleep_range(OV8856_SCCB_INIT_DELAY_US, OV8856_SCCB_INIT_DELAY_US + 200); > > > > + usleep_range(delay_us * 2, delay_us * 4); > > > > Normally one one just give some small delta here, like +/- 100 us. > > > > Fixed in next release. > > > > + > > > + return 0; > > > + > > > +disable_clk: > > > + clk_disable_unprepare(ov8856->xvclk); > > > + > > > + return ret; > > > +} > > > + > > > +static void __ov8856_power_off(struct ov8856 *ov8856) > > > +{ > > > + clk_disable_unprepare(ov8856->xvclk); > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, 1); > > > + > > > + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies); > > > +} > > > + > > > static int __maybe_unused ov8856_suspend(struct device *dev) > > > { > > > struct i2c_client *client = to_i2c_client(dev); > > > @@ -915,8 +1459,8 @@ static int __maybe_unused ov8856_suspend(struct device *dev) > > > struct ov8856 *ov8856 = to_ov8856(sd); > > > > > > mutex_lock(&ov8856->mutex); > > > - if (ov8856->streaming) > > > - ov8856_stop_streaming(ov8856); > > > + > > > + __ov8856_power_off(ov8856); > > > > This change is incorrect because it will power off even if the device was > > already powered off, causing reference count mismatch. The original code > > was okay. > > > > Then do we need to power off sensor per power off sequence? > I thought this function would be called by pm_runtime_put when power > count is 0. > This is the system suspend callback, not runtime suspend callback. It's only called when the system goes to sleep. > > > > > > mutex_unlock(&ov8856->mutex); > > > > > > @@ -1089,6 +1633,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856) > > > return -ENXIO; > > > } > > > > > > + /* set R3614 to distinguish harward versions */ > > > > hardware > > > > Sorry for the typo. > Fixed in next release. > Also a similar comment for R3614, as above. It doesn't have any meaning to someone without the datasheet in front of them. Please just remove it. Best regards, Tomasz