| Message ID | 20181004212903.364064-1-lkundrak@v3.sk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [media] ov7670: make "xclk" clock optional | expand |
On Thu 2018-10-04 23:29:03, Lubomir Rintel wrote: > When the "xclk" clock was added, it was made mandatory. This broke the > driver on an OLPC plaform which doesn't know such clock. Make it > optional. > > Tested on a OLPC XO-1 laptop. > > Cc: stable@vger.kernel.org # 4.11+ > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> Acked-by: Pavel Machek <pavel@ucw.cz>
Hi Lubomir, +Sakari in Cc I just noticed this, and the patch is now in v4.20, but let me comment anyway on this. On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > When the "xclk" clock was added, it was made mandatory. This broke the > driver on an OLPC plaform which doesn't know such clock. Make it > optional. > I don't think this is correct. The sensor needs a clock to work. With this patch clock_speed which is used to calculate the framerate is defaulted to 30MHz, crippling all the calculations if that default value doesn't match what is actually installed on the board. If this patch breaks the OLPC, then might it be the DTS for said device needs to be fixed instead of working around the issue here? Also, the DT bindings should be updated too if we decide this property can be omitted. At this point, with a follow-up patch. Thanks j > Tested on a OLPC XO-1 laptop. > > Cc: stable@vger.kernel.org # 4.11+ > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > --- > drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > index 31bf577b0bd3..64d1402882c8 100644 > --- a/drivers/media/i2c/ov7670.c > +++ b/drivers/media/i2c/ov7670.c > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, > info->pclk_hb_disable = true; > } > > - info->clk = devm_clk_get(&client->dev, "xclk"); > - if (IS_ERR(info->clk)) > - return PTR_ERR(info->clk); > - ret = clk_prepare_enable(info->clk); > - if (ret) > - return ret; > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ > + if (IS_ERR(info->clk)) { > + ret = PTR_ERR(info->clk); > + if (ret == -ENOENT) > + info->clk = NULL; > + else > + return ret; > + } > + if (info->clk) { > + ret = clk_prepare_enable(info->clk); > + if (ret) > + return ret; > > - info->clock_speed = clk_get_rate(info->clk) / 1000000; > - if (info->clock_speed < 10 || info->clock_speed > 48) { > - ret = -EINVAL; > - goto clk_disable; > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > + if (info->clock_speed < 10 || info->clock_speed > 48) { > + ret = -EINVAL; > + goto clk_disable; > + } > } > > ret = ov7670_init_gpio(client, info); > -- > 2.19.0 >
Hello, On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote: > Hi Lubomir, > +Sakari in Cc > > I just noticed this, and the patch is now in v4.20, but let me comment > anyway on this. > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > > When the "xclk" clock was added, it was made mandatory. This broke the > > driver on an OLPC plaform which doesn't know such clock. Make it > > optional. > > > > I don't think this is correct. The sensor needs a clock to work. > > With this patch clock_speed which is used to calculate > the framerate is defaulted to 30MHz, crippling all the calculations if > that default value doesn't match what is actually installed on the > board. How come? I kept this: + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > If this patch breaks the OLPC, then might it be the DTS for said > device needs to be fixed instead of working around the issue here? No. Device tree is an ABI, and you can't just add mandatory properties. There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine. You'd need to update all machines in the wild which is not realistic. Alternatively, something else than DT could provide the clock. If this gets in, then the OLPC would work even without the xclk patch: https://lore.kernel.org/lkml/20181105073054.24407-12-lkundrak@v3.sk/ (I just got a kbuild failure message, so I'll surely be following up with a v2.) > Also, the DT bindings should be updated too if we decide this property > can be omitted. At this point, with a follow-up patch. Yes. > > Thanks Cheers Lubo > j > > > Tested on a OLPC XO-1 laptop. > > > > Cc: stable@vger.kernel.org # 4.11+ > > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > --- > > drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > index 31bf577b0bd3..64d1402882c8 100644 > > --- a/drivers/media/i2c/ov7670.c > > +++ b/drivers/media/i2c/ov7670.c > > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, > > info->pclk_hb_disable = true; > > } > > > > - info->clk = devm_clk_get(&client->dev, "xclk"); > > - if (IS_ERR(info->clk)) > > - return PTR_ERR(info->clk); > > - ret = clk_prepare_enable(info->clk); > > - if (ret) > > - return ret; > > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ > > + if (IS_ERR(info->clk)) { > > + ret = PTR_ERR(info->clk); > > + if (ret == -ENOENT) > > + info->clk = NULL; > > + else > > + return ret; > > + } > > + if (info->clk) { > > + ret = clk_prepare_enable(info->clk); > > + if (ret) > > + return ret; > > > > - info->clock_speed = clk_get_rate(info->clk) / 1000000; > > - if (info->clock_speed < 10 || info->clock_speed > 48) { > > - ret = -EINVAL; > > - goto clk_disable; > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > + if (info->clock_speed < 10 || info->clock_speed > 48) { > > + ret = -EINVAL; > > + goto clk_disable; > > + } > > } > > > > ret = ov7670_init_gpio(client, info); > > -- > > 2.19.0 > >
Hi Lubo, On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote: > Hello, > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote: > > Hi Lubomir, > > +Sakari in Cc > > > > I just noticed this, and the patch is now in v4.20, but let me comment > > anyway on this. > > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > > > When the "xclk" clock was added, it was made mandatory. This broke the > > > driver on an OLPC plaform which doesn't know such clock. Make it > > > optional. > > > > > > > I don't think this is correct. The sensor needs a clock to work. > > > > With this patch clock_speed which is used to calculate > > the framerate is defaulted to 30MHz, crippling all the calculations if > > that default value doesn't match what is actually installed on the > > board. > > How come? I kept this: > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; Yes, but only if if (info->clk) { } if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the beginning of the probe routine. Am I missing something obvious here? > > > > > If this patch breaks the OLPC, then might it be the DTS for said > > device needs to be fixed instead of working around the issue here? > > No. Device tree is an ABI, and you can't just add mandatory properties. > Well, as I read the ov7670 bindings documentation: Required Properties: - compatible: should be "ovti,ov7670" - clocks: reference to the xclk input clock. - clock-names: should be "xclk". It was mandatory already since the bindings have been first created: bba582894a ("[media] ov7670: document device tree bindings") And yes, bindings establishes an ABI we have not to break or make incompatible with DTs created for an older version of the same binding, but the DTs itself is free to change and we need to do so to update it when required (to fix bugs, add new components, enable/disable them etc). > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine. > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry about this. A quick read of the wikipedia page for "OpenFirmware" gives me back that it a standardized firmware interface: "Open Firmware allows the system to load platform-independent drivers directly from the PCI card, improving compatibility". I know nothing on this, and that's not the point, so I'll better stop here and refrain to express how much the "loading platform-independent (BINARY) drivers from the PCI card" scares me :p > You'd need to update all machines in the wild which is not realistic. Machines which have received a kernel update which includes the patch that makes the clock for the sensor driver mandatory [1], will have their board files updated by the same kernel update, with the proper clock provider instantiated for that sensor. That's what I would expect from a kernel update for those devices (or any device in general..) If this didn't happen, blame OLPC kernel maintainers :p [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12 > Alternatively, something else than DT could provide the clock. If this > gets in, then the OLPC would work even without the xclk patch: > https://lore.kernel.org/lkml/20181105073054.24407-12-lkundrak@v3.sk/ That's what I meant, more or less. If you don't have a DTS you have a board file, isn't it? ( arch/x86/platform/olpc/ maybe? ) The patch you linked here makes the video interface (the marvel-ccic one) provide the clock source for the sensor: + clkdev_create(mcam->mclk, "xclk", "%d-%04x", + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); + While I would expect the board file to do that, as that's where all pieces gets put together, and it knows which clock source has to be fed to the sensor depending on your hardware design. As I don't know much of x86 or openfirmare, feel free to explain me why it is not possible ;) Anyway, my whole point is that the sensor needs a clock to work. With your patch if it is not provided it gets defaulted (if I'm not mis-reading the code) to a value that would break frame interval calculations. This is what concerns me and I would prefer the driver to fail probing quite nosily to make sure all its users (dts, board files etc) gets updated. > > (I just got a kbuild failure message, so I'll surely be following up > with a v2.) > > > Also, the DT bindings should be updated too if we decide this property > > can be omitted. At this point, with a follow-up patch. > > Yes. > This would actually be an ABI change (one that would not break retro-compatibility probably, but still...) Thanks j > > > > Thanks > > Cheers > Lubo > > > j > > > > > Tested on a OLPC XO-1 laptop. > > > > > > Cc: stable@vger.kernel.org # 4.11+ > > > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > > --- > > > drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- > > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > index 31bf577b0bd3..64d1402882c8 100644 > > > --- a/drivers/media/i2c/ov7670.c > > > +++ b/drivers/media/i2c/ov7670.c > > > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, > > > info->pclk_hb_disable = true; > > > } > > > > > > - info->clk = devm_clk_get(&client->dev, "xclk"); > > > - if (IS_ERR(info->clk)) > > > - return PTR_ERR(info->clk); > > > - ret = clk_prepare_enable(info->clk); > > > - if (ret) > > > - return ret; > > > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ > > > + if (IS_ERR(info->clk)) { > > > + ret = PTR_ERR(info->clk); > > > + if (ret == -ENOENT) > > > + info->clk = NULL; > > > + else > > > + return ret; > > > + } > > > + if (info->clk) { > > > + ret = clk_prepare_enable(info->clk); > > > + if (ret) > > > + return ret; > > > > > > - info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > - if (info->clock_speed < 10 || info->clock_speed > 48) { > > > - ret = -EINVAL; > > > - goto clk_disable; > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > + if (info->clock_speed < 10 || info->clock_speed > 48) { > > > + ret = -EINVAL; > > > + goto clk_disable; > > > + } > > > } > > > > > > ret = ov7670_init_gpio(client, info); > > > -- > > > 2.19.0 > > > >
On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote: > Hi Lubo, > > On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote: > > Hello, > > > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote: > > > Hi Lubomir, > > > +Sakari in Cc > > > > > > I just noticed this, and the patch is now in v4.20, but let me comment > > > anyway on this. > > > > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > > > > When the "xclk" clock was added, it was made mandatory. This broke the > > > > driver on an OLPC plaform which doesn't know such clock. Make it > > > > optional. > > > > > > > > > > I don't think this is correct. The sensor needs a clock to work. > > > > > > With this patch clock_speed which is used to calculate > > > the framerate is defaulted to 30MHz, crippling all the calculations if > > > that default value doesn't match what is actually installed on the > > > board. > > > > How come? I kept this: > > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > Yes, but only if > if (info->clk) { } > > if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the > beginning of the probe routine. Am I missing something obvious here? Maybe. Or I am. I thought you care about the situation where you *have* the clk, and thus you shouldn't be caring about the defaults? > > > If this patch breaks the OLPC, then might it be the DTS for said > > > device needs to be fixed instead of working around the issue here? > > > > No. Device tree is an ABI, and you can't just add mandatory properties. > > > > Well, as I read the ov7670 bindings documentation: > > Required Properties: > - compatible: should be "ovti,ov7670" > - clocks: reference to the xclk input clock. > - clock-names: should be "xclk". > > It was mandatory already since the bindings have been first created: > bba582894a ("[media] ov7670: document device tree bindings") > > And yes, bindings establishes an ABI we have not to break or make > incompatible with DTs created for an older version of the same binding, > but the DTs itself is free to change and we need to do so to update > it when required (to fix bugs, add new components, enable/disable them > etc). Ah, right, you're correct. No DTS ABI breakage there. I guess it would be fine to revert my patch if we provide the xclk on the OLPC instead. > > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine. > > > > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry > about this. Well, you're sort of right here. The XO-1.75 generation is ARM, the XO- 1 is x86. They both use devicetree provided by the firmware. However, they predate FDT (or the definitions of bindings that are used here for tht matter) and the trees are unfortunately quite incomplete. The sensor is not there. > A quick read of the wikipedia page for "OpenFirmware" gives me back > that it a standardized firmware interface: > "Open Firmware allows the system to load platform-independent drivers > directly from the PCI card, improving compatibility". > > I know nothing on this, and that's not the point, so I'll better stop > here and refrain to express how much the "loading platform-independent > (BINARY) drivers from the PCI card" scares me :p > > > You'd need to update all machines in the wild which is not realistic. > > Machines which have received a kernel update which includes the patch > that makes the clock for the sensor driver mandatory [1], will have their > board files updated by the same kernel update, with the proper clock > provider instantiated for that sensor. > > That's what I would expect from a kernel update for those devices (or > any device in general..) > > If this didn't happen, blame OLPC kernel maintainers :p > > [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12 > > > Alternatively, something else than DT could provide the clock. If this > > gets in, then the OLPC would work even without the xclk patch: > > https://lore.kernel.org/lkml/20181105073054.24407-12-lkundrak@v3.sk/ > > That's what I meant, more or less. > > If you don't have a DTS you have a board file, isn't it? > ( arch/x86/platform/olpc/ maybe? ) The device tree on XO-1 is not constructed from a FDT, it's gotten from the OpenFirmware. See arch/x86/platform/olpc/olpc_dt.c But as I said -- it's hopelessly incomplete and is not going to be of much help here. If you're curious, I've recently uploaded /proc/device- tree dumps here, since someone else asked: https://people.freedesktop.org/~lkundrak/olpc/ > The patch you linked here makes the video interface (the marvel-ccic > one) provide the clock source for the sensor: > > + clkdev_create(mcam->mclk, "xclk", "%d-%04x", > + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > + > > While I would expect the board file to do that, as that's where all > pieces gets put together, and it knows which clock source has to be > fed to the sensor depending on your hardware design. As I don't know > much of x86 or openfirmare, feel free to explain me why it is not > possible ;) Well, maybe. I don't think so. The clock is provided by the Cafe chip (the bridge chip, also has i2c controller for the sensor control) that sits on a PCI, which is discoverable. In theory there could be more of them. The driver is oddly structured for historic reasons. I'm mainly interested in not breaking it more than it is. A good devicetree would help, but we don't have the luxury. > Anyway, my whole point is that the sensor needs a clock to work. With > your patch if it is not provided it gets defaulted (if I'm not > mis-reading the code) to a value that would break frame interval > calculations. This is what concerns me and I would prefer the driver > to fail probing quite nosily to make sure all its users (dts, board > files etc) gets updated. I still don't get this. It defaults to 30 Hz* as it used to before the patch that introduced mandatory xclk, which seems perfectly reasonable. Which configurations break? * you said MHz before, I suppose that was a mistake? > > (I just got a kbuild failure message, so I'll surely be following up > > with a v2.) > > > > > Also, the DT bindings should be updated too if we decide this property > > > can be omitted. At this point, with a follow-up patch. > > > > Yes. > > > This would actually be an ABI change (one that would not break > retro-compatibility probably, but still...) That, I think, is an okay thing to do. Thanks Lubo > > Thanks > j > > > > Thanks > > > > Cheers > > Lubo > > > > > j > > > > > > > Tested on a OLPC XO-1 laptop. > > > > > > > > Cc: stable@vger.kernel.org # 4.11+ > > > > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > > > --- > > > > drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- > > > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > > index 31bf577b0bd3..64d1402882c8 100644 > > > > --- a/drivers/media/i2c/ov7670.c > > > > +++ b/drivers/media/i2c/ov7670.c > > > > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, > > > > info->pclk_hb_disable = true; > > > > } > > > > > > > > - info->clk = devm_clk_get(&client->dev, "xclk"); > > > > - if (IS_ERR(info->clk)) > > > > - return PTR_ERR(info->clk); > > > > - ret = clk_prepare_enable(info->clk); > > > > - if (ret) > > > > - return ret; > > > > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ > > > > + if (IS_ERR(info->clk)) { > > > > + ret = PTR_ERR(info->clk); > > > > + if (ret == -ENOENT) > > > > + info->clk = NULL; > > > > + else > > > > + return ret; > > > > + } > > > > + if (info->clk) { > > > > + ret = clk_prepare_enable(info->clk); > > > > + if (ret) > > > > + return ret; > > > > > > > > - info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > - if (info->clock_speed < 10 || info->clock_speed > 48) { > > > > - ret = -EINVAL; > > > > - goto clk_disable; > > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > + if (info->clock_speed < 10 || info->clock_speed > 48) { > > > > + ret = -EINVAL; > > > > + goto clk_disable; > > > > + } > > > > } > > > > > > > > ret = ov7670_init_gpio(client, info); > > > > -- > > > > 2.19.0 > > > >
Hi Lubo, On Mon, Nov 05, 2018 at 04:22:15PM +0100, Lubomir Rintel wrote: > On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote: > > Hi Lubo, > > > > On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote: > > > Hello, > > > > > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote: > > > > Hi Lubomir, > > > > +Sakari in Cc > > > > > > > > I just noticed this, and the patch is now in v4.20, but let me comment > > > > anyway on this. > > > > > > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > > > > > When the "xclk" clock was added, it was made mandatory. This broke the > > > > > driver on an OLPC plaform which doesn't know such clock. Make it > > > > > optional. > > > > > > > > > > > > > I don't think this is correct. The sensor needs a clock to work. > > > > > > > > With this patch clock_speed which is used to calculate > > > > the framerate is defaulted to 30MHz, crippling all the calculations if > > > > that default value doesn't match what is actually installed on the > > > > board. > > > > > > How come? I kept this: > > > > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > Yes, but only if > > if (info->clk) { } > > > > if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the > > beginning of the probe routine. Am I missing something obvious here? > > Maybe. Or I am. > > I thought you care about the situation where you *have* the clk, and > thus you shouldn't be caring about the defaults? > I care about the fact that with this version, the clock speed might be default to a totally random value making the driver malfunctioning. My specific use case doesn't matter. > > > > If this patch breaks the OLPC, then might it be the DTS for said > > > > device needs to be fixed instead of working around the issue here? > > > > > > No. Device tree is an ABI, and you can't just add mandatory properties. > > > > > > > Well, as I read the ov7670 bindings documentation: > > > > Required Properties: > > - compatible: should be "ovti,ov7670" > > - clocks: reference to the xclk input clock. > > - clock-names: should be "xclk". > > > > It was mandatory already since the bindings have been first created: > > bba582894a ("[media] ov7670: document device tree bindings") > > > > And yes, bindings establishes an ABI we have not to break or make > > incompatible with DTs created for an older version of the same binding, > > but the DTs itself is free to change and we need to do so to update > > it when required (to fix bugs, add new components, enable/disable them > > etc). > > Ah, right, you're correct. No DTS ABI breakage there. I guess it would > be fine to revert my patch if we provide the xclk on the OLPC instead. > Thanks I feel that would be the right thing to do. > > > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine. > > > > > > > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry > > about this. > > Well, you're sort of right here. The XO-1.75 generation is ARM, the XO- > 1 is x86. They both use devicetree provided by the firmware. However, > they predate FDT (or the definitions of bindings that are used here for > tht matter) and the trees are unfortunately quite incomplete. The > sensor is not there. Ouch. I see... > > > A quick read of the wikipedia page for "OpenFirmware" gives me back > > that it a standardized firmware interface: > > "Open Firmware allows the system to load platform-independent drivers > > directly from the PCI card, improving compatibility". > > > > I know nothing on this, and that's not the point, so I'll better stop > > here and refrain to express how much the "loading platform-independent > > (BINARY) drivers from the PCI card" scares me :p > > > > > You'd need to update all machines in the wild which is not realistic. > > > > Machines which have received a kernel update which includes the patch > > that makes the clock for the sensor driver mandatory [1], will have their > > board files updated by the same kernel update, with the proper clock > > provider instantiated for that sensor. > > > > That's what I would expect from a kernel update for those devices (or > > any device in general..) > > > > If this didn't happen, blame OLPC kernel maintainers :p > > > > [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12 > > > > > Alternatively, something else than DT could provide the clock. If this > > > gets in, then the OLPC would work even without the xclk patch: > > > https://lore.kernel.org/lkml/20181105073054.24407-12-lkundrak@v3.sk/ > > > > That's what I meant, more or less. > > > > If you don't have a DTS you have a board file, isn't it? > > ( arch/x86/platform/olpc/ maybe? ) > > The device tree on XO-1 is not constructed from a FDT, it's gotten from > the OpenFirmware. See arch/x86/platform/olpc/olpc_dt.c > Ouch^2 > But as I said -- it's hopelessly incomplete and is not going to be of > much help here. If you're curious, I've recently uploaded /proc/device- > tree dumps here, since someone else asked: > > https://people.freedesktop.org/~lkundrak/olpc/ > > > The patch you linked here makes the video interface (the marvel-ccic > > one) provide the clock source for the sensor: > > > > + clkdev_create(mcam->mclk, "xclk", "%d-%04x", > > + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > > + > > > > While I would expect the board file to do that, as that's where all > > pieces gets put together, and it knows which clock source has to be > > fed to the sensor depending on your hardware design. As I don't know > > much of x86 or openfirmare, feel free to explain me why it is not > > possible ;) > > Well, maybe. I don't think so. The clock is provided by the Cafe chip > (the bridge chip, also has i2c controller for the sensor control) that > sits on a PCI, which is discoverable. In theory there could be more of > them. > > The driver is oddly structured for historic reasons. I'm mainly > interested in not breaking it more than it is. A good devicetree would > help, but we don't have the luxury. > I see. If the cci camera interface should be providing the clock in this setup, and it is not possible to get it from anywhere else, I would say let's go for that, even if it's a bit of a shortcut. > > Anyway, my whole point is that the sensor needs a clock to work. With > > your patch if it is not provided it gets defaulted (if I'm not > > mis-reading the code) to a value that would break frame interval > > calculations. This is what concerns me and I would prefer the driver > > to fail probing quite nosily to make sure all its users (dts, board > > files etc) gets updated. > > I still don't get this. It defaults to 30 Hz* as it used to before the > patch that introduced mandatory xclk, which seems perfectly reasonable. > Which configurations break? I don't have anything that breaks, as my platforms provides a clock source to the driver. I'm just against changing the driver to workaround a platform issue, opening possibilities for future breakages and going against what the DT bindings describes as mandatory. > > * you said MHz before, I suppose that was a mistake? I see a "clk_get_rate() / 10^6", so it's defintely 30MHz (the sensor supports input clock rates in the [10-48]MHz range. All image sensor I know of work with frequencies in the 10^6Hz order of magnitude. > > > > (I just got a kbuild failure message, so I'll surely be following up > > > with a v2.) > > > > > > > Also, the DT bindings should be updated too if we decide this property > > > > can be omitted. At this point, with a follow-up patch. > > > > > > Yes. > > > > > This would actually be an ABI change (one that would not break > > retro-compatibility probably, but still...) > > That, I think, is an okay thing to do. > It would be ok, but I think dropping a mandatory property to allow broken platform to continue stay broken is a bad idea. I understand it might be a pain to fix for you, given the DT-loaded-from-PCI non-sense, but looking at your mentioned olpc_dt.c file, that DTS could be patched at run-time; but I understand having the cci providing the clock might be for sure easier. What's your idea, do you still feel like this should be worked around in the driver? Thanks j > > Thanks > Lubo > > > > > Thanks > > j > > > > > > Thanks > > > > > > Cheers > > > Lubo > > > > > > > j > > > > > > > > > Tested on a OLPC XO-1 laptop. > > > > > > > > > > Cc: stable@vger.kernel.org # 4.11+ > > > > > Fixes: 0a024d634cee ("[media] ov7670: get xclk") > > > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> > > > > > --- > > > > > drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- > > > > > 1 file changed, 17 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c > > > > > index 31bf577b0bd3..64d1402882c8 100644 > > > > > --- a/drivers/media/i2c/ov7670.c > > > > > +++ b/drivers/media/i2c/ov7670.c > > > > > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, > > > > > info->pclk_hb_disable = true; > > > > > } > > > > > > > > > > - info->clk = devm_clk_get(&client->dev, "xclk"); > > > > > - if (IS_ERR(info->clk)) > > > > > - return PTR_ERR(info->clk); > > > > > - ret = clk_prepare_enable(info->clk); > > > > > - if (ret) > > > > > - return ret; > > > > > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ > > > > > + if (IS_ERR(info->clk)) { > > > > > + ret = PTR_ERR(info->clk); > > > > > + if (ret == -ENOENT) > > > > > + info->clk = NULL; > > > > > + else > > > > > + return ret; > > > > > + } > > > > > + if (info->clk) { > > > > > + ret = clk_prepare_enable(info->clk); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > > > - info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > > - if (info->clock_speed < 10 || info->clock_speed > 48) { > > > > > - ret = -EINVAL; > > > > > - goto clk_disable; > > > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > > + if (info->clock_speed < 10 || info->clock_speed > 48) { > > > > > + ret = -EINVAL; > > > > > + goto clk_disable; > > > > > + } > > > > > } > > > > > > > > > > ret = ov7670_init_gpio(client, info); > > > > > -- > > > > > 2.19.0 > > > > > >
On Mon, 2018-11-05 at 18:15 +0100, jacopo mondi wrote: > Hi Lubo, > > On Mon, Nov 05, 2018 at 04:22:15PM +0100, Lubomir Rintel wrote: > > On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote: > > > Hi Lubo, > > > > > > On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote: > > > > Hello, > > > > > > > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote: > > > > > Hi Lubomir, > > > > > +Sakari in Cc > > > > > > > > > > I just noticed this, and the patch is now in v4.20, but let me comment > > > > > anyway on this. > > > > > > > > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote: > > > > > > When the "xclk" clock was added, it was made mandatory. This broke the > > > > > > driver on an OLPC plaform which doesn't know such clock. Make it > > > > > > optional. > > > > > > > > > > > > > > > > I don't think this is correct. The sensor needs a clock to work. > > > > > > > > > > With this patch clock_speed which is used to calculate > > > > > the framerate is defaulted to 30MHz, crippling all the calculations if > > > > > that default value doesn't match what is actually installed on the > > > > > board. > > > > > > > > How come? I kept this: > > > > > > > > + info->clock_speed = clk_get_rate(info->clk) / 1000000; > > > > > > Yes, but only if > > > if (info->clk) { } > > > > > > if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the > > > beginning of the probe routine. Am I missing something obvious here? > > > > Maybe. Or I am. > > > > I thought you care about the situation where you *have* the clk, and > > thus you shouldn't be caring about the defaults? > > > > I care about the fact that with this version, the clock speed might be > default to a totally random value making the driver malfunctioning. My > specific use case doesn't matter. > > > > > > If this patch breaks the OLPC, then might it be the DTS for said > > > > > device needs to be fixed instead of working around the issue here? > > > > > > > > No. Device tree is an ABI, and you can't just add mandatory properties. > > > > > > > > > > Well, as I read the ov7670 bindings documentation: > > > > > > Required Properties: > > > - compatible: should be "ovti,ov7670" > > > - clocks: reference to the xclk input clock. > > > - clock-names: should be "xclk". > > > > > > It was mandatory already since the bindings have been first created: > > > bba582894a ("[media] ov7670: document device tree bindings") > > > > > > And yes, bindings establishes an ABI we have not to break or make > > > incompatible with DTs created for an older version of the same binding, > > > but the DTs itself is free to change and we need to do so to update > > > it when required (to fix bugs, add new components, enable/disable them > > > etc). > > > > Ah, right, you're correct. No DTS ABI breakage there. I guess it would > > be fine to revert my patch if we provide the xclk on the OLPC instead. > > > > Thanks I feel that would be the right thing to do. > > > > > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine. > > > > > > > > > > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry > > > about this. > > > > Well, you're sort of right here. The XO-1.75 generation is ARM, the XO- > > 1 is x86. They both use devicetree provided by the firmware. However, > > they predate FDT (or the definitions of bindings that are used here for > > tht matter) and the trees are unfortunately quite incomplete. The > > sensor is not there. > > Ouch. I see... > > > > A quick read of the wikipedia page for "OpenFirmware" gives me back > > > that it a standardized firmware interface: > > > "Open Firmware allows the system to load platform-independent drivers > > > directly from the PCI card, improving compatibility". > > > > > > I know nothing on this, and that's not the point, so I'll better stop > > > here and refrain to express how much the "loading platform-independent > > > (BINARY) drivers from the PCI card" scares me :p > > > > > > > You'd need to update all machines in the wild which is not realistic. > > > > > > Machines which have received a kernel update which includes the patch > > > that makes the clock for the sensor driver mandatory [1], will have their > > > board files updated by the same kernel update, with the proper clock > > > provider instantiated for that sensor. > > > > > > That's what I would expect from a kernel update for those devices (or > > > any device in general..) > > > > > > If this didn't happen, blame OLPC kernel maintainers :p > > > > > > [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12 > > > > > > > Alternatively, something else than DT could provide the clock. If this > > > > gets in, then the OLPC would work even without the xclk patch: > > > > https://lore.kernel.org/lkml/20181105073054.24407-12-lkundrak@v3.sk/ > > > > > > That's what I meant, more or less. > > > > > > If you don't have a DTS you have a board file, isn't it? > > > ( arch/x86/platform/olpc/ maybe? ) > > > > The device tree on XO-1 is not constructed from a FDT, it's gotten from > > the OpenFirmware. See arch/x86/platform/olpc/olpc_dt.c > > > > Ouch^2 > > > But as I said -- it's hopelessly incomplete and is not going to be of > > much help here. If you're curious, I've recently uploaded /proc/device- > > tree dumps here, since someone else asked: > > > > https://people.freedesktop.org/~lkundrak/olpc/ > > > > > The patch you linked here makes the video interface (the marvel-ccic > > > one) provide the clock source for the sensor: > > > > > > + clkdev_create(mcam->mclk, "xclk", "%d-%04x", > > > + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > > > + > > > > > > While I would expect the board file to do that, as that's where all > > > pieces gets put together, and it knows which clock source has to be > > > fed to the sensor depending on your hardware design. As I don't know > > > much of x86 or openfirmare, feel free to explain me why it is not > > > possible ;) > > > > Well, maybe. I don't think so. The clock is provided by the Cafe chip > > (the bridge chip, also has i2c controller for the sensor control) that > > sits on a PCI, which is discoverable. In theory there could be more of > > them. > > > > The driver is oddly structured for historic reasons. I'm mainly > > interested in not breaking it more than it is. A good devicetree would > > help, but we don't have the luxury. > > > > I see. If the cci camera interface should be providing the clock in > this setup, and it is not possible to get it from anywhere else, I > would say let's go for that, even if it's a bit of a shortcut. > > > > Anyway, my whole point is that the sensor needs a clock to work. With > > > your patch if it is not provided it gets defaulted (if I'm not > > > mis-reading the code) to a value that would break frame interval > > > calculations. This is what concerns me and I would prefer the driver > > > to fail probing quite nosily to make sure all its users (dts, board > > > files etc) gets updated. > > > > I still don't get this. It defaults to 30 Hz* as it used to before the > > patch that introduced mandatory xclk, which seems perfectly reasonable. > > Which configurations break? > > I don't have anything that breaks, as my platforms provides a clock > source to the driver. I'm just against changing the driver to > workaround a platform issue, opening possibilities for future > breakages and going against what the DT bindings describes as > mandatory. > > > * you said MHz before, I suppose that was a mistake? > > I see a "clk_get_rate() / 10^6", so it's defintely 30MHz (the sensor > supports input clock rates in the [10-48]MHz range. All image sensor > I know of work with frequencies in the 10^6Hz order of magnitude. > > > > > (I just got a kbuild failure message, so I'll surely be following up > > > > with a v2.) > > > > > > > > > Also, the DT bindings should be updated too if we decide this property > > > > > can be omitted. At this point, with a follow-up patch. > > > > > > > > Yes. > > > > > > > This would actually be an ABI change (one that would not break > > > retro-compatibility probably, but still...) > > > > That, I think, is an okay thing to do. > > > > It would be ok, but I think dropping a mandatory property to allow > broken platform to continue stay broken is a bad idea. > > I understand it might be a pain to fix for you, given the DT-loaded-from-PCI > non-sense, but looking at your mentioned olpc_dt.c file, that DTS > could be patched at run-time; but I understand having the cci > providing the clock might be for sure easier. > > What's your idea, do you still feel like this should be worked around > in the driver? Well, sort of. The issue I was initially solving was that the original xclk patch broke a platform that used to work. That is a regression and thus a complete no-go regardless of how optimal the original solution was. In such case reverting to the previous behavior is certainly an option. Once there's a better solution (and that might be my marvel-ccic patch) I, of course, have no objection to reverting the patch you find objectionable and would be very happy to Ack it if needed. Thank you, Lubo
diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c index 31bf577b0bd3..64d1402882c8 100644 --- a/drivers/media/i2c/ov7670.c +++ b/drivers/media/i2c/ov7670.c @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client, info->pclk_hb_disable = true; } - info->clk = devm_clk_get(&client->dev, "xclk"); - if (IS_ERR(info->clk)) - return PTR_ERR(info->clk); - ret = clk_prepare_enable(info->clk); - if (ret) - return ret; + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */ + if (IS_ERR(info->clk)) { + ret = PTR_ERR(info->clk); + if (ret == -ENOENT) + info->clk = NULL; + else + return ret; + } + if (info->clk) { + ret = clk_prepare_enable(info->clk); + if (ret) + return ret; - info->clock_speed = clk_get_rate(info->clk) / 1000000; - if (info->clock_speed < 10 || info->clock_speed > 48) { - ret = -EINVAL; - goto clk_disable; + info->clock_speed = clk_get_rate(info->clk) / 1000000; + if (info->clock_speed < 10 || info->clock_speed > 48) { + ret = -EINVAL; + goto clk_disable; + } } ret = ov7670_init_gpio(client, info);
When the "xclk" clock was added, it was made mandatory. This broke the driver on an OLPC plaform which doesn't know such clock. Make it optional. Tested on a OLPC XO-1 laptop. Cc: stable@vger.kernel.org # 4.11+ Fixes: 0a024d634cee ("[media] ov7670: get xclk") Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> --- drivers/media/i2c/ov7670.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)