Moi,
On Mon, Feb 24, 2025 at 02:24:29PM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 24/02/2025 13:54, Mehdi Djait wrote:
> > Hi Sakari,
> >
> > On Mon, Feb 24, 2025 at 09:42:12AM +0000, Sakari Ailus wrote:
> > > Hi Mehdi,
> > >
> > > On Mon, Feb 24, 2025 at 08:59:34AM +0100, Mehdi Djait wrote:
> > > > Hi Laurent,
> > > >
> > > > On Mon, Feb 24, 2025 at 01:06:49AM +0200, Laurent Pinchart wrote:
> > > > > Hi Mehdi,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Thu, Feb 20, 2025 at 04:49:09PM +0100, Mehdi Djait wrote:
> > > > > > Make the clock producer reference lookup optional
> > > > > >
> > > > > > Add support for ACPI-based platforms by parsing the 'clock-frequency'
> > > > > > property when no clock producer is available
> > > > > >
> > > > > > Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/media/i2c/imx219.c | 14 ++++++++++++--
> > > > > > 1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > > > index 2d54cea113e1..a876a6d80a47 100644
> > > > > > --- a/drivers/media/i2c/imx219.c
> > > > > > +++ b/drivers/media/i2c/imx219.c
> > > > > > @@ -1103,12 +1103,22 @@ static int imx219_probe(struct i2c_client *client)
> > > > > > "failed to initialize CCI\n");
> > > > > > /* Get system clock (xclk) */
> > > > > > - imx219->xclk = devm_clk_get(dev, NULL);
> > > > > > + imx219->xclk = devm_clk_get_optional(dev, NULL);
> > > > > > if (IS_ERR(imx219->xclk))
> > > > > > return dev_err_probe(dev, PTR_ERR(imx219->xclk),
> > > > > > "failed to get xclk\n");
> > > > > > - imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > + if (imx219->xclk) {
> > > > > > + imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > > > > + } else {
> > > > > > + ret = fwnode_property_read_u32(dev_fwnode(dev),
> > > > > > + "clock-frequency",
> > > > > > + &imx219->xclk_freq);
> > > > > > + if (ret)
> > > > > > + return dev_err_probe(dev, ret,
> > > > > > + "failed to get clock frequency");
> > > > > > + }
> > > > > > +
> > > > >
> > > > > This doesn't seem specific to the imx219 driver. Could you turn this
> > > > > into a generic V4L2 sensor helper that would take a struct device and a
> > > > > clock name, and return the frequency, either retrieved from the clock,
> > > > > or from the clock-frequency property as a fallback ?
> > > > >
> > > > > Some drivers will also need to control the clock, so the clock should
> > > > > probably be returned too.
> > > > >
> > > >
> > > > Yes, I saw that many sensor drivers have the same issue.
> > > >
> > > > I will try to make it into a generic V4L2 helper and send the patches.
> > >
> > > There are other such functions in drivers/media/v4l2-core/v4l2-common.c.
> > > Perhaps this is where the new helper could be located as well?
> > >
> >
> > I was thinking about drivers/media/v4l2-core/v4l2-fwnode.c but if
> > v4l2-common.c is more appropriate we can go with that.
>
> I admit I have no clue about ACPI, but why is this v4l2 specific? Why
> doesn't clock framework do this for us?
The "clock-frequency" isn't really specific to ACPI but it's used on some
boards with DT, too, that precede the current clock bindings. Clocks aren't
generally available to OS in ACPI either but the sensor drivers still need
them. DisCo for Imaging uses "mipi-img-clock-frequency" which DisCo for
Imaging code deep down in the ACPI framework will offer to drivers as
"clock-frequency". A lot of this is actually specific to cameras. On top of
that, camera sensors tend to be devices that are used equally on both DT
and ACPI systems, it's quite uncommon elsewhere. Therefore I do think the
natural place for this code is actually the V4L2 framework.
--
Kind regards,
Sakari Ailus
