| Message ID | 1511975472-26659-3-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hugues, On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote: > /* read exposure, in number of line periods */ > static int ov5640_get_exposure(struct ov5640_dev *sensor) > { > @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > ov5640_reset(sensor); > ov5640_power(sensor, true); > > + ret = ov5640_check_chip_id(sensor); > + if (ret) > + goto power_off; Wouldn't it make more sense to add this check in ov5640_probe() function instead?
On 11/29/2017 09:11 AM, Hugues Fruchet wrote: > Verify that chip identifier is correct before starting streaming > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/i2c/ov5640.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 61071f5..a576d11 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -34,7 +34,8 @@ > > #define OV5640_DEFAULT_SLAVE_ID 0x3c > > -#define OV5640_REG_CHIP_ID 0x300a > +#define OV5640_REG_CHIP_ID_HIGH 0x300a > +#define OV5640_REG_CHIP_ID_LOW 0x300b There is no need to separate low and high byte addresses. See below. > #define OV5640_REG_PAD_OUTPUT00 0x3019 > #define OV5640_REG_SC_PLL_CTRL0 0x3034 > #define OV5640_REG_SC_PLL_CTRL1 0x3035 > @@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, > return ret; > } > > +static int ov5640_check_chip_id(struct ov5640_dev *sensor) > +{ > + struct i2c_client *client = sensor->i2c_client; > + int ret; > + u8 chip_id_h, chip_id_l; > + > + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h); > + if (ret) > + return ret; > + > + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l); > + if (ret) > + return ret; > + > + if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) { > + dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x%x\n", > + __func__, chip_id_h, chip_id_l); > + return -EINVAL; > + } > + This should all be be replaced by: u16 chip_id; ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id); etc. > + return 0; > +} > + > /* read exposure, in number of line periods */ > static int ov5640_get_exposure(struct ov5640_dev *sensor) > { > @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > ov5640_reset(sensor); > ov5640_power(sensor, true); > > + ret = ov5640_check_chip_id(sensor); > + if (ret) > + goto power_off; > + > ret = ov5640_init_slave_id(sensor); > if (ret) > goto power_off;
Hi Steve, thanks for review, comments below. On 12/03/2017 10:34 PM, Steve Longerbeam wrote: > > > On 11/29/2017 09:11 AM, Hugues Fruchet wrote: >> Verify that chip identifier is correct before starting streaming >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/i2c/ov5640.c | 30 +++++++++++++++++++++++++++++- >> 1 file changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 61071f5..a576d11 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -34,7 +34,8 @@ >> #define OV5640_DEFAULT_SLAVE_ID 0x3c >> -#define OV5640_REG_CHIP_ID 0x300a >> +#define OV5640_REG_CHIP_ID_HIGH 0x300a >> +#define OV5640_REG_CHIP_ID_LOW 0x300b > > There is no need to separate low and high byte addresses. > See below. > >> #define OV5640_REG_PAD_OUTPUT00 0x3019 >> #define OV5640_REG_SC_PLL_CTRL0 0x3034 >> #define OV5640_REG_SC_PLL_CTRL1 0x3035 >> @@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev >> *sensor, >> return ret; >> } >> +static int ov5640_check_chip_id(struct ov5640_dev *sensor) >> +{ >> + struct i2c_client *client = sensor->i2c_client; >> + int ret; >> + u8 chip_id_h, chip_id_l; >> + >> + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h); >> + if (ret) >> + return ret; >> + >> + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l); >> + if (ret) >> + return ret; >> + >> + if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) { >> + dev_err(&client->dev, "%s: wrong chip identifier, expected >> 0x5640, got 0x%x%x\n", >> + __func__, chip_id_h, chip_id_l); >> + return -EINVAL; >> + } >> + > > This should all be be replaced by: > > u16 chip_id; > > ret = ov5640_read_reg16(sensor, OV5640_REG_CHIP_ID, &chip_id); > > etc. > Done, thanks. >> + return 0; >> +} >> + >> /* read exposure, in number of line periods */ >> static int ov5640_get_exposure(struct ov5640_dev *sensor) >> { >> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev >> *sensor, bool on) >> ov5640_reset(sensor); >> ov5640_power(sensor, true); >> + ret = ov5640_check_chip_id(sensor); >> + if (ret) >> + goto power_off; >> + >> ret = ov5640_init_slave_id(sensor); >> if (ret) >> goto power_off; > Best regards, Hugues.
Thanks Fabio for review, This make sense, I'll try to change my code that way. On 11/30/2017 08:07 PM, Fabio Estevam wrote: > Hi Hugues, > > On Wed, Nov 29, 2017 at 3:11 PM, Hugues Fruchet <hugues.fruchet@st.com> wrote: > >> /* read exposure, in number of line periods */ >> static int ov5640_get_exposure(struct ov5640_dev *sensor) >> { >> @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) >> ov5640_reset(sensor); >> ov5640_power(sensor, true); >> >> + ret = ov5640_check_chip_id(sensor); >> + if (ret) >> + goto power_off; > > Wouldn't it make more sense to add this check in ov5640_probe() > function instead? > Best regards, Hugues.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 61071f5..a576d11 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -34,7 +34,8 @@ #define OV5640_DEFAULT_SLAVE_ID 0x3c -#define OV5640_REG_CHIP_ID 0x300a +#define OV5640_REG_CHIP_ID_HIGH 0x300a +#define OV5640_REG_CHIP_ID_LOW 0x300b #define OV5640_REG_PAD_OUTPUT00 0x3019 #define OV5640_REG_SC_PLL_CTRL0 0x3034 #define OV5640_REG_SC_PLL_CTRL1 0x3035 @@ -926,6 +927,29 @@ static int ov5640_load_regs(struct ov5640_dev *sensor, return ret; } +static int ov5640_check_chip_id(struct ov5640_dev *sensor) +{ + struct i2c_client *client = sensor->i2c_client; + int ret; + u8 chip_id_h, chip_id_l; + + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_HIGH, &chip_id_h); + if (ret) + return ret; + + ret = ov5640_read_reg(sensor, OV5640_REG_CHIP_ID_LOW, &chip_id_l); + if (ret) + return ret; + + if (!(chip_id_h == 0x56 && chip_id_l == 0x40)) { + dev_err(&client->dev, "%s: wrong chip identifier, expected 0x5640, got 0x%x%x\n", + __func__, chip_id_h, chip_id_l); + return -EINVAL; + } + + return 0; +} + /* read exposure, in number of line periods */ static int ov5640_get_exposure(struct ov5640_dev *sensor) { @@ -1562,6 +1586,10 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on) ov5640_reset(sensor); ov5640_power(sensor, true); + ret = ov5640_check_chip_id(sensor); + if (ret) + goto power_off; + ret = ov5640_init_slave_id(sensor); if (ret) goto power_off;
Verify that chip identifier is correct before starting streaming Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/i2c/ov5640.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)