| Message ID | 1528709357-7251-3-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: > Add support of module being physically mounted upside down. > In this case, mirror and flip are enabled to fix captured images > orientation. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ > drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > index 8e36da0..f76eb7e 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > @@ -13,6 +13,8 @@ Optional Properties: > This is an active low signal to the OV5640. > - powerdown-gpios: reference to the GPIO connected to the powerdown pin, > if any. This is an active high signal to the OV5640. > +- rotation: integer property; valid values are 0 (sensor mounted upright) > + and 180 (sensor mounted upside down). > > The device node must contain one 'port' child node for its digital output > video port, in accordance with the video interface bindings defined in > @@ -51,6 +53,7 @@ Examples: > DVDD-supply = <&vgen2_reg>; /* 1.5v */ > powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; > reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; > + rotation = <180>; > > port { > /* MIPI CSI-2 bus endpoint */ > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 41039e5..5529b14 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -215,6 +215,7 @@ struct ov5640_dev { > struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES]; > struct gpio_desc *reset_gpio; > struct gpio_desc *pwdn_gpio; > + bool upside_down; > > /* lock to protect all members below */ > struct mutex lock; > @@ -2222,6 +2223,8 @@ static int ov5640_set_ctrl_light_freq(struct ov5640_dev *sensor, int value) > static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) > { > /* > + * If sensor is mounted upside down, mirror logic is inversed. > + * > * Sensor is a BSI (Back Side Illuminated) one, > * so image captured is physically mirrored. > * This is why mirror logic is inversed in > @@ -2235,11 +2238,14 @@ static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) > */ > return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG21, > BIT(2) | BIT(1), > - (!value) ? (BIT(2) | BIT(1)) : 0); > + (!(value ^ sensor->upside_down)) ? > + (BIT(2) | BIT(1)) : 0); > } > > static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > { > + /* If sensor is mounted upside down, flip logic is inversed */ > + > /* > * TIMING TC REG20: > * - [2]: ISP vflip > @@ -2247,7 +2253,8 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) > */ > return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG20, > BIT(2) | BIT(1), > - value ? (BIT(2) | BIT(1)) : 0); > + (value ^ sensor->upside_down) ? > + (BIT(2) | BIT(1)) : 0); > } > > static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > @@ -2625,6 +2632,7 @@ static int ov5640_probe(struct i2c_client *client, > struct fwnode_handle *endpoint; > struct ov5640_dev *sensor; > struct v4l2_mbus_framefmt *fmt; > + u32 rotation; > int ret; > > sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > @@ -2650,6 +2658,22 @@ static int ov5640_probe(struct i2c_client *client, > > sensor->ae_target = 52; > > + /* optional indication of physical rotation of sensor */ > + ret = fwnode_property_read_u32(of_fwnode_handle(client->dev.of_node), Instead of of_fwnode_handle(), please use dev_fwnode(&client->dev) --- as the driver already does elsewhere. I can make the change if you're happy with that; the patches seem fine otherwise. > + "rotation", &rotation); > + if (!ret) { > + switch (rotation) { > + case 180: > + sensor->upside_down = true; > + /* fall through */ > + case 0: > + break; > + default: > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", > + rotation); > + } > + } > + > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), > NULL); > if (!endpoint) {
Hi Sakari, I'm fine with the change to dev_fwnode(&client->dev). Many thanks Sakari, Hugues. On 06/11/2018 12:10 PM, Sakari Ailus wrote: > On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: >> Add support of module being physically mounted upside down. >> In this case, mirror and flip are enabled to fix captured images >> orientation. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ >> drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- >> 2 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> index 8e36da0..f76eb7e 100644 >> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> @@ -13,6 +13,8 @@ Optional Properties: >> This is an active low signal to the OV5640. >> - powerdown-gpios: reference to the GPIO connected to the powerdown pin, >> if any. This is an active high signal to the OV5640. >> +- rotation: integer property; valid values are 0 (sensor mounted upright) >> + and 180 (sensor mounted upside down). >> >> The device node must contain one 'port' child node for its digital output >> video port, in accordance with the video interface bindings defined in >> @@ -51,6 +53,7 @@ Examples: >> DVDD-supply = <&vgen2_reg>; /* 1.5v */ >> powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; >> reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; >> + rotation = <180>; >> >> port { >> /* MIPI CSI-2 bus endpoint */ >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 41039e5..5529b14 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -215,6 +215,7 @@ struct ov5640_dev { >> struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES]; >> struct gpio_desc *reset_gpio; >> struct gpio_desc *pwdn_gpio; >> + bool upside_down; >> >> /* lock to protect all members below */ >> struct mutex lock; >> @@ -2222,6 +2223,8 @@ static int ov5640_set_ctrl_light_freq(struct ov5640_dev *sensor, int value) >> static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) >> { >> /* >> + * If sensor is mounted upside down, mirror logic is inversed. >> + * >> * Sensor is a BSI (Back Side Illuminated) one, >> * so image captured is physically mirrored. >> * This is why mirror logic is inversed in >> @@ -2235,11 +2238,14 @@ static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) >> */ >> return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG21, >> BIT(2) | BIT(1), >> - (!value) ? (BIT(2) | BIT(1)) : 0); >> + (!(value ^ sensor->upside_down)) ? >> + (BIT(2) | BIT(1)) : 0); >> } >> >> static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) >> { >> + /* If sensor is mounted upside down, flip logic is inversed */ >> + >> /* >> * TIMING TC REG20: >> * - [2]: ISP vflip >> @@ -2247,7 +2253,8 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) >> */ >> return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG20, >> BIT(2) | BIT(1), >> - value ? (BIT(2) | BIT(1)) : 0); >> + (value ^ sensor->upside_down) ? >> + (BIT(2) | BIT(1)) : 0); >> } >> >> static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) >> @@ -2625,6 +2632,7 @@ static int ov5640_probe(struct i2c_client *client, >> struct fwnode_handle *endpoint; >> struct ov5640_dev *sensor; >> struct v4l2_mbus_framefmt *fmt; >> + u32 rotation; >> int ret; >> >> sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> @@ -2650,6 +2658,22 @@ static int ov5640_probe(struct i2c_client *client, >> >> sensor->ae_target = 52; >> >> + /* optional indication of physical rotation of sensor */ >> + ret = fwnode_property_read_u32(of_fwnode_handle(client->dev.of_node), > > Instead of of_fwnode_handle(), please use dev_fwnode(&client->dev) --- as the > driver already does elsewhere. > > I can make the change if you're happy with that; the patches seem fine > otherwise. > >> + "rotation", &rotation); >> + if (!ret) { >> + switch (rotation) { >> + case 180: >> + sensor->upside_down = true; >> + /* fall through */ >> + case 0: >> + break; >> + default: >> + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", >> + rotation); >> + } >> + } >> + >> endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), >> NULL); >> if (!endpoint) { >
On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: > Add support of module being physically mounted upside down. > In this case, mirror and flip are enabled to fix captured images > orientation. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ Please split bindings to separate patches. > drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > index 8e36da0..f76eb7e 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > @@ -13,6 +13,8 @@ Optional Properties: > This is an active low signal to the OV5640. > - powerdown-gpios: reference to the GPIO connected to the powerdown pin, > if any. This is an active high signal to the OV5640. > +- rotation: integer property; valid values are 0 (sensor mounted upright) > + and 180 (sensor mounted upside down). Didn't we just add this as a common property? If so, just reference the common definition. If not, it needs a common definition.
Hi Rob, Hugues, On Tue, Jun 12, 2018 at 04:06:28PM -0600, Rob Herring wrote: > On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: > > Add support of module being physically mounted upside down. > > In this case, mirror and flip are enabled to fix captured images > > orientation. > > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > > --- > > .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ > > Please split bindings to separate patches. > > > drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- > > 2 files changed, 29 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > > index 8e36da0..f76eb7e 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > > @@ -13,6 +13,8 @@ Optional Properties: > > This is an active low signal to the OV5640. > > - powerdown-gpios: reference to the GPIO connected to the powerdown pin, > > if any. This is an active high signal to the OV5640. > > +- rotation: integer property; valid values are 0 (sensor mounted upright) > > + and 180 (sensor mounted upside down). > > Didn't we just add this as a common property? If so, just reference the > common definition. If not, it needs a common definition. The common definition is there --- and this text is actually the same as for the smiapp DT bindings --- which you acked. :-) I thought this would be fine as well, and this patch is actually already in a pull request to Mauro. I put the smiapp bindings to the same patch with the driver change as they were pretty small both but we'll split these in the future. I've marked the pull request as deferred for now; let me know whether you're still ok with this going in as such. Thanks.
Hi Rob, thanks for review, On 06/13/2018 12:06 AM, Rob Herring wrote: > On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: >> Add support of module being physically mounted upside down. >> In this case, mirror and flip are enabled to fix captured images >> orientation. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ > > Please split bindings to separate patches. OK, will do in next patchset. > >> drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- >> 2 files changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> index 8e36da0..f76eb7e 100644 >> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >> @@ -13,6 +13,8 @@ Optional Properties: >> This is an active low signal to the OV5640. >> - powerdown-gpios: reference to the GPIO connected to the powerdown pin, >> if any. This is an active high signal to the OV5640. >> +- rotation: integer property; valid values are 0 (sensor mounted upright) >> + and 180 (sensor mounted upside down). > > Didn't we just add this as a common property? If so, just reference the > common definition. If not, it needs a common definition. > A common definition has been introduced by Sakari, I'm reusing it, see: https://www.mail-archive.com/linux-media@vger.kernel.org/msg132517.html I would so propose: >> +- rotation: as defined in >> + Documentation/devicetree/bindings/media/video-interfaces.txt. Best regards, Hugues.
On Wed, Jun 13, 2018 at 08:10:02AM +0000, Hugues FRUCHET wrote: > Hi Rob, thanks for review, > > On 06/13/2018 12:06 AM, Rob Herring wrote: > > On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: > >> Add support of module being physically mounted upside down. > >> In this case, mirror and flip are enabled to fix captured images > >> orientation. > >> > >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >> --- > >> .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ > > > > Please split bindings to separate patches. > > OK, will do in next patchset. > > > > >> drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- > >> 2 files changed, 29 insertions(+), 2 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >> index 8e36da0..f76eb7e 100644 > >> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >> @@ -13,6 +13,8 @@ Optional Properties: > >> This is an active low signal to the OV5640. > >> - powerdown-gpios: reference to the GPIO connected to the powerdown pin, > >> if any. This is an active high signal to the OV5640. > >> +- rotation: integer property; valid values are 0 (sensor mounted upright) > >> + and 180 (sensor mounted upside down). > > > > Didn't we just add this as a common property? If so, just reference the > > common definition. If not, it needs a common definition. > > > > A common definition has been introduced by Sakari, I'm reusing it, see: > https://www.mail-archive.com/linux-media@vger.kernel.org/msg132517.html > > I would so propose: > >> +- rotation: as defined in > >> + Documentation/devicetree/bindings/media/video-interfaces.txt. Shouldn't the description still include the valid values? As far as I can tell, these are ultimately device specific albeit more or less the same for *this kind* of sensors. The file already contains a reference to video-interfaces.txt.
Hi Sakari, Rob, Find a new proposal below: On 06/13/2018 10:24 AM, Sakari Ailus wrote: > On Wed, Jun 13, 2018 at 08:10:02AM +0000, Hugues FRUCHET wrote: >> Hi Rob, thanks for review, >> >> On 06/13/2018 12:06 AM, Rob Herring wrote: >>> On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: >>>> Add support of module being physically mounted upside down. >>>> In this case, mirror and flip are enabled to fix captured images >>>> orientation. >>>> >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >>>> --- >>>> .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ >>> >>> Please split bindings to separate patches. >> >> OK, will do in next patchset. >> >>> >>>> drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- >>>> 2 files changed, 29 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >>>> index 8e36da0..f76eb7e 100644 >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt >>>> @@ -13,6 +13,8 @@ Optional Properties: >>>> This is an active low signal to the OV5640. >>>> - powerdown-gpios: reference to the GPIO connected to the powerdown pin, >>>> if any. This is an active high signal to the OV5640. >>>> +- rotation: integer property; valid values are 0 (sensor mounted upright) >>>> + and 180 (sensor mounted upside down). >>> >>> Didn't we just add this as a common property? If so, just reference the >>> common definition. If not, it needs a common definition. >>> >> >> A common definition has been introduced by Sakari, I'm reusing it, see: >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg132517.html >> >> I would so propose: >> >> +- rotation: as defined in >> >> + Documentation/devicetree/bindings/media/video-interfaces.txt. > > Shouldn't the description still include the valid values? As far as I can > tell, these are ultimately device specific albeit more or less the same for > *this kind* of sensors. Yes you're right, let's put both together: +- rotation: as defined in + Documentation/devicetree/bindings/media/video-interfaces.txt, + valid values are 0 (sensor mounted upright) and 180 (sensor + mounted upside down). > > The file already contains a reference to video-interfaces.txt. > Yes but it was related to 'port' child node. Best regards, Hugues.
On Wed, Jun 13, 2018 at 10:09:58AM +0000, Hugues FRUCHET wrote: > Hi Sakari, Rob, > > Find a new proposal below: > > On 06/13/2018 10:24 AM, Sakari Ailus wrote: > > On Wed, Jun 13, 2018 at 08:10:02AM +0000, Hugues FRUCHET wrote: > >> Hi Rob, thanks for review, > >> > >> On 06/13/2018 12:06 AM, Rob Herring wrote: > >>> On Mon, Jun 11, 2018 at 11:29:17AM +0200, Hugues Fruchet wrote: > >>>> Add support of module being physically mounted upside down. > >>>> In this case, mirror and flip are enabled to fix captured images > >>>> orientation. > >>>> > >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >>>> --- > >>>> .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ > >>> > >>> Please split bindings to separate patches. > >> > >> OK, will do in next patchset. > >> > >>> > >>>> drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- > >>>> 2 files changed, 29 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >>>> index 8e36da0..f76eb7e 100644 > >>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt > >>>> @@ -13,6 +13,8 @@ Optional Properties: > >>>> This is an active low signal to the OV5640. > >>>> - powerdown-gpios: reference to the GPIO connected to the powerdown pin, > >>>> if any. This is an active high signal to the OV5640. > >>>> +- rotation: integer property; valid values are 0 (sensor mounted upright) > >>>> + and 180 (sensor mounted upside down). > >>> > >>> Didn't we just add this as a common property? If so, just reference the > >>> common definition. If not, it needs a common definition. > >>> > >> > >> A common definition has been introduced by Sakari, I'm reusing it, see: > >> https://www.mail-archive.com/linux-media@vger.kernel.org/msg132517.html > >> > >> I would so propose: > >> >> +- rotation: as defined in > >> >> + Documentation/devicetree/bindings/media/video-interfaces.txt. > > > > Shouldn't the description still include the valid values? As far as I can > > tell, these are ultimately device specific albeit more or less the same for > > *this kind* of sensors. > > Yes you're right, let's put both together: > +- rotation: as defined in > + Documentation/devicetree/bindings/media/video-interfaces.txt, > + valid values are 0 (sensor mounted upright) and 180 (sensor > + mounted upside down). I'll improve the description for smiapp as well.
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt index 8e36da0..f76eb7e 100644 --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt @@ -13,6 +13,8 @@ Optional Properties: This is an active low signal to the OV5640. - powerdown-gpios: reference to the GPIO connected to the powerdown pin, if any. This is an active high signal to the OV5640. +- rotation: integer property; valid values are 0 (sensor mounted upright) + and 180 (sensor mounted upside down). The device node must contain one 'port' child node for its digital output video port, in accordance with the video interface bindings defined in @@ -51,6 +53,7 @@ Examples: DVDD-supply = <&vgen2_reg>; /* 1.5v */ powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>; reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>; + rotation = <180>; port { /* MIPI CSI-2 bus endpoint */ diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 41039e5..5529b14 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -215,6 +215,7 @@ struct ov5640_dev { struct regulator_bulk_data supplies[OV5640_NUM_SUPPLIES]; struct gpio_desc *reset_gpio; struct gpio_desc *pwdn_gpio; + bool upside_down; /* lock to protect all members below */ struct mutex lock; @@ -2222,6 +2223,8 @@ static int ov5640_set_ctrl_light_freq(struct ov5640_dev *sensor, int value) static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) { /* + * If sensor is mounted upside down, mirror logic is inversed. + * * Sensor is a BSI (Back Side Illuminated) one, * so image captured is physically mirrored. * This is why mirror logic is inversed in @@ -2235,11 +2238,14 @@ static int ov5640_set_ctrl_hflip(struct ov5640_dev *sensor, int value) */ return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG21, BIT(2) | BIT(1), - (!value) ? (BIT(2) | BIT(1)) : 0); + (!(value ^ sensor->upside_down)) ? + (BIT(2) | BIT(1)) : 0); } static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) { + /* If sensor is mounted upside down, flip logic is inversed */ + /* * TIMING TC REG20: * - [2]: ISP vflip @@ -2247,7 +2253,8 @@ static int ov5640_set_ctrl_vflip(struct ov5640_dev *sensor, int value) */ return ov5640_mod_reg(sensor, OV5640_REG_TIMING_TC_REG20, BIT(2) | BIT(1), - value ? (BIT(2) | BIT(1)) : 0); + (value ^ sensor->upside_down) ? + (BIT(2) | BIT(1)) : 0); } static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl) @@ -2625,6 +2632,7 @@ static int ov5640_probe(struct i2c_client *client, struct fwnode_handle *endpoint; struct ov5640_dev *sensor; struct v4l2_mbus_framefmt *fmt; + u32 rotation; int ret; sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); @@ -2650,6 +2658,22 @@ static int ov5640_probe(struct i2c_client *client, sensor->ae_target = 52; + /* optional indication of physical rotation of sensor */ + ret = fwnode_property_read_u32(of_fwnode_handle(client->dev.of_node), + "rotation", &rotation); + if (!ret) { + switch (rotation) { + case 180: + sensor->upside_down = true; + /* fall through */ + case 0: + break; + default: + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n", + rotation); + } + } + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL); if (!endpoint) {
Add support of module being physically mounted upside down. In this case, mirror and flip are enabled to fix captured images orientation. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- .../devicetree/bindings/media/i2c/ov5640.txt | 3 +++ drivers/media/i2c/ov5640.c | 28 ++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-)