| Message ID | 20191029124211.15052-1-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Fix check for PLL1 exceeding max allowed rate | expand |
On Tue, Oct 29, 2019 at 7:42 AM Adam Ford <aford173@gmail.com> wrote: > > The variable _rate is by ov5640_compute_sys_clk() which returns > zero if the PLL exceeds 1GHz. Unfortunately, the check to see > if the max PLL1 output is checking 'rate' and not '_rate' and > 'rate' does not ever appear to be 0. > > This patch changes the check against the returned value of > '_rate' to determine if the PLL1 output exceeds 1GHz. > > Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate") > I haven't seen any responses to this patch. Has anyone had a chance to review this? It's been nearly a month. I think it would be appropriate to backport to stable if deemed acceptable. adam > Signed-off-by: Adam Ford <aford173@gmail.com> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 5e495c833d32..bb968e764f31 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -874,7 +874,7 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor, > * We have reached the maximum allowed PLL1 output, > * increase sysdiv. > */ > - if (!rate) > + if (!_rate) > break; > > /* > -- > 2.17.1 >
Hi Adam, On Tue, Nov 26, 2019 at 08:08:05PM -0600, Adam Ford wrote: > On Tue, Oct 29, 2019 at 7:42 AM Adam Ford <aford173@gmail.com> wrote: > > > > The variable _rate is by ov5640_compute_sys_clk() which returns > > zero if the PLL exceeds 1GHz. Unfortunately, the check to see > > if the max PLL1 output is checking 'rate' and not '_rate' and > > 'rate' does not ever appear to be 0. This seems a bit convoluted. What about: "The PLL calculation routine checks the wrong variable 'rate' to verify that the calculated PLL1 output frequency does not exceed 1GHz. Fix this by using the correct '_rate' one." Or something against these lines > > > > This patch changes the check against the returned value of > > '_rate' to determine if the PLL1 output exceeds 1GHz. > > > > Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate") > > > > I haven't seen any responses to this patch. Has anyone had a chance > to review this? It's been nearly a month. You're totally right! Sorry about this > I think it would be appropriate to backport to stable if deemed acceptable. > Indeed! This fixes a real issue Acked-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > adam > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 5e495c833d32..bb968e764f31 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -874,7 +874,7 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor, > > * We have reached the maximum allowed PLL1 output, > > * increase sysdiv. > > */ > > - if (!rate) > > + if (!_rate) > > break; > > > > /* > > -- > > 2.17.1 > >
On Thu, Nov 28, 2019 at 4:19 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Adam, > > On Tue, Nov 26, 2019 at 08:08:05PM -0600, Adam Ford wrote: > > On Tue, Oct 29, 2019 at 7:42 AM Adam Ford <aford173@gmail.com> wrote: > > > > > > The variable _rate is by ov5640_compute_sys_clk() which returns > > > zero if the PLL exceeds 1GHz. Unfortunately, the check to see > > > if the max PLL1 output is checking 'rate' and not '_rate' and > > > 'rate' does not ever appear to be 0. > > This seems a bit convoluted. What about: > > "The PLL calculation routine checks the wrong variable 'rate' to > verify that the calculated PLL1 output frequency does not exceed > 1GHz. Fix this by using the correct '_rate' one." > > Or something against these lines That makes sense. I'll do a V2 for this patch and change the description and add your 'acked-by' note to it. I probably won't get to it until this weekend, since today is a holiday in the US, and I am traveling. adam > > > > > > > This patch changes the check against the returned value of > > > '_rate' to determine if the PLL1 output exceeds 1GHz. > > > > > > Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate") > > > > > > > I haven't seen any responses to this patch. Has anyone had a chance > > to review this? It's been nearly a month. > > You're totally right! Sorry about this > > > I think it would be appropriate to backport to stable if deemed acceptable. > > > > Indeed! This fixes a real issue > > Acked-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > adam > > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > > index 5e495c833d32..bb968e764f31 100644 > > > --- a/drivers/media/i2c/ov5640.c > > > +++ b/drivers/media/i2c/ov5640.c > > > @@ -874,7 +874,7 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor, > > > * We have reached the maximum allowed PLL1 output, > > > * increase sysdiv. > > > */ > > > - if (!rate) > > > + if (!_rate) > > > break; > > > > > > /* > > > -- > > > 2.17.1 > > >
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 5e495c833d32..bb968e764f31 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -874,7 +874,7 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor, * We have reached the maximum allowed PLL1 output, * increase sysdiv. */ - if (!rate) + if (!_rate) break; /*
The variable _rate is by ov5640_compute_sys_clk() which returns zero if the PLL exceeds 1GHz. Unfortunately, the check to see if the max PLL1 output is checking 'rate' and not '_rate' and 'rate' does not ever appear to be 0. This patch changes the check against the returned value of '_rate' to determine if the PLL1 output exceeds 1GHz. Fixes: aa2882481cad ("media: ov5640: Adjust the clock based on the expected rate") Signed-off-by: Adam Ford <aford173@gmail.com>