| Message ID | 20210216174146.106639-8-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: GMSL reliability improvements | expand |
On Tue, Feb 16, 2021 at 6:41 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > The MAX9271 chip manual prescribes a delay of 5 milliseconds > after the chip exists from low power state. exits Gr{oetje,eeting}s, Geert
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > The MAX9271 chip manual prescribes a delay of 5 milliseconds > after the chip exists from low power state. > > Adjust the required delay in the rdacm21 camera module and add it > to the rdacm20 that currently doesn't implement one. > This sounds to me like it should be a common function in the max9271 module: > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > usleep_range(5000, 8000); Especially as that MAX9271_DEFAULT_ADDR should probably be handled directly in the max9271.c file too, and the RDACM's shouldn't care about it. If we end up moving the max9271 'library' into more of a module/device then this would have to be done in it's 'probe' anyway, so it's likely better handled down there...? But ... it's not essential at this point in the series, so if you want to keep this patch as is, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/rdacm20.c | 1 + > drivers/media/i2c/rdacm21.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index ea30cc936531..39e4b4241870 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > + usleep_range(5000, 8000); > > /* Serial link disabled during config as it needs a valid pixel clock. */ > ret = max9271_set_serial_link(&dev->serializer, false); > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 179d107f494c..b22a2ca5340b 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > - usleep_range(3000, 5000); > + usleep_range(5000, 8000); > > /* Enable reverse channel and disable the serial link. */ > ret = max9271_set_serial_link(&dev->serializer, false); >
Hi Jacopo, Thank you for the patch. On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote: > On 16/02/2021 17:41, Jacopo Mondi wrote: > > The MAX9271 chip manual prescribes a delay of 5 milliseconds > > after the chip exists from low power state. > > > > Adjust the required delay in the rdacm21 camera module and add it > > to the rdacm20 that currently doesn't implement one. > > This sounds to me like it should be a common function in the max9271 module: > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > i2c_smbus_read_byte(dev->serializer.client); > > usleep_range(5000, 8000); > > Especially as that MAX9271_DEFAULT_ADDR should probably be handled > directly in the max9271.c file too, and the RDACM's shouldn't care about it. I think this is a good idea. With this addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > If we end up moving the max9271 'library' into more of a module/device > then this would have to be done in it's 'probe' anyway, so it's likely > better handled down there...? > > But ... it's not essential at this point in the series, so if you want > to keep this patch as is, > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/rdacm20.c | 1 + > > drivers/media/i2c/rdacm21.c | 2 +- > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > index ea30cc936531..39e4b4241870 100644 > > --- a/drivers/media/i2c/rdacm20.c > > +++ b/drivers/media/i2c/rdacm20.c > > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > /* Verify communication with the MAX9271: ping to wakeup. */ > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > i2c_smbus_read_byte(dev->serializer.client); > > + usleep_range(5000, 8000); > > > > /* Serial link disabled during config as it needs a valid pixel clock. */ > > ret = max9271_set_serial_link(&dev->serializer, false); > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > index 179d107f494c..b22a2ca5340b 100644 > > --- a/drivers/media/i2c/rdacm21.c > > +++ b/drivers/media/i2c/rdacm21.c > > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > /* Verify communication with the MAX9271: ping to wakeup. */ > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > i2c_smbus_read_byte(dev->serializer.client); > > - usleep_range(3000, 5000); > > + usleep_range(5000, 8000); > > > > /* Enable reverse channel and disable the serial link. */ > > ret = max9271_set_serial_link(&dev->serializer, false);
Hi On Mon, Feb 22, 2021 at 03:18:53AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote: > > On 16/02/2021 17:41, Jacopo Mondi wrote: > > > The MAX9271 chip manual prescribes a delay of 5 milliseconds > > > after the chip exists from low power state. > > > > > > Adjust the required delay in the rdacm21 camera module and add it > > > to the rdacm20 that currently doesn't implement one. > > > > This sounds to me like it should be a common function in the max9271 module: > > > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > i2c_smbus_read_byte(dev->serializer.client); > > > usleep_range(5000, 8000); > > > > Especially as that MAX9271_DEFAULT_ADDR should probably be handled > > directly in the max9271.c file too, and the RDACM's shouldn't care about it. > > I think this is a good idea. With this addressed, The address reprogramming was exactly why I refrained from adding a function to the max9271 library, as handling of the addresses there would introduce a precendece order in the function calls, ie the newly introduced function would require to be called first after a chip reset and that's something I considered better handled by the camera driver (even if it wouldn't be suprising a 'wake up' function to be called first). please also note that I will next try to make the max9271 a proper i2c driver so this might be even less relevant that what it is right now Thanks j > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > If we end up moving the max9271 'library' into more of a module/device > > then this would have to be done in it's 'probe' anyway, so it's likely > > better handled down there...? > > > > But ... it's not essential at this point in the series, so if you want > > to keep this patch as is, > > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > drivers/media/i2c/rdacm20.c | 1 + > > > drivers/media/i2c/rdacm21.c | 2 +- > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > > index ea30cc936531..39e4b4241870 100644 > > > --- a/drivers/media/i2c/rdacm20.c > > > +++ b/drivers/media/i2c/rdacm20.c > > > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > i2c_smbus_read_byte(dev->serializer.client); > > > + usleep_range(5000, 8000); > > > > > > /* Serial link disabled during config as it needs a valid pixel clock. */ > > > ret = max9271_set_serial_link(&dev->serializer, false); > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > index 179d107f494c..b22a2ca5340b 100644 > > > --- a/drivers/media/i2c/rdacm21.c > > > +++ b/drivers/media/i2c/rdacm21.c > > > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > i2c_smbus_read_byte(dev->serializer.client); > > > - usleep_range(3000, 5000); > > > + usleep_range(5000, 8000); > > > > > > /* Enable reverse channel and disable the serial link. */ > > > ret = max9271_set_serial_link(&dev->serializer, false); > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Feb 22, 2021 at 04:11:41PM +0100, Jacopo Mondi wrote: > On Mon, Feb 22, 2021 at 03:18:53AM +0200, Laurent Pinchart wrote: > > On Wed, Feb 17, 2021 at 01:33:01PM +0000, Kieran Bingham wrote: > > > On 16/02/2021 17:41, Jacopo Mondi wrote: > > > > The MAX9271 chip manual prescribes a delay of 5 milliseconds > > > > after the chip exists from low power state. > > > > > > > > Adjust the required delay in the rdacm21 camera module and add it > > > > to the rdacm20 that currently doesn't implement one. > > > > > > This sounds to me like it should be a common function in the max9271 module: > > > > > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > > i2c_smbus_read_byte(dev->serializer.client); > > > > usleep_range(5000, 8000); > > > > > > Especially as that MAX9271_DEFAULT_ADDR should probably be handled > > > directly in the max9271.c file too, and the RDACM's shouldn't care about it. > > > > I think this is a good idea. With this addressed, > > The address reprogramming was exactly why I refrained from adding a > function to the max9271 library, as handling of the addresses there > would introduce a precendece order in the function calls, ie the newly > introduced function would require to be called first after a chip > reset and that's something I considered better handled by the camera > driver (even if it wouldn't be suprising a 'wake up' function to be > called first). I'm not sure this would be an issue, it's fine for library code to set requirements on how APIs have to be used, including which order functions have to be called. > please also note that I will next try to make the max9271 a proper i2c > driver so this might be even less relevant that what it is right now Let's see how that works out. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > If we end up moving the max9271 'library' into more of a module/device > > > then this would have to be done in it's 'probe' anyway, so it's likely > > > better handled down there...? > > > > > > But ... it's not essential at this point in the series, so if you want > > > to keep this patch as is, > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > > --- > > > > drivers/media/i2c/rdacm20.c | 1 + > > > > drivers/media/i2c/rdacm21.c | 2 +- > > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > > > > index ea30cc936531..39e4b4241870 100644 > > > > --- a/drivers/media/i2c/rdacm20.c > > > > +++ b/drivers/media/i2c/rdacm20.c > > > > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > > i2c_smbus_read_byte(dev->serializer.client); > > > > + usleep_range(5000, 8000); > > > > > > > > /* Serial link disabled during config as it needs a valid pixel clock. */ > > > > ret = max9271_set_serial_link(&dev->serializer, false); > > > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > > > > index 179d107f494c..b22a2ca5340b 100644 > > > > --- a/drivers/media/i2c/rdacm21.c > > > > +++ b/drivers/media/i2c/rdacm21.c > > > > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > > > > /* Verify communication with the MAX9271: ping to wakeup. */ > > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > > > > i2c_smbus_read_byte(dev->serializer.client); > > > > - usleep_range(3000, 5000); > > > > + usleep_range(5000, 8000); > > > > > > > > /* Enable reverse channel and disable the serial link. */ > > > > ret = max9271_set_serial_link(&dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c index ea30cc936531..39e4b4241870 100644 --- a/drivers/media/i2c/rdacm20.c +++ b/drivers/media/i2c/rdacm20.c @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) /* Verify communication with the MAX9271: ping to wakeup. */ dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; i2c_smbus_read_byte(dev->serializer.client); + usleep_range(5000, 8000); /* Serial link disabled during config as it needs a valid pixel clock. */ ret = max9271_set_serial_link(&dev->serializer, false); diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c index 179d107f494c..b22a2ca5340b 100644 --- a/drivers/media/i2c/rdacm21.c +++ b/drivers/media/i2c/rdacm21.c @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) /* Verify communication with the MAX9271: ping to wakeup. */ dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; i2c_smbus_read_byte(dev->serializer.client); - usleep_range(3000, 5000); + usleep_range(5000, 8000); /* Enable reverse channel and disable the serial link. */ ret = max9271_set_serial_link(&dev->serializer, false);
The MAX9271 chip manual prescribes a delay of 5 milliseconds after the chip exists from low power state. Adjust the required delay in the rdacm21 camera module and add it to the rdacm20 that currently doesn't implement one. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/rdacm20.c | 1 + drivers/media/i2c/rdacm21.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)