Hi Hans,
On Mon, Jul 31, 2023 at 02:54:13PM +0200, Hans de Goede wrote:
> Hi,
>
> On 7/31/23 14:44, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Tue, Jun 27, 2023 at 03:18:15PM +0200, Hans de Goede wrote:
> >> On ACPI systems the following 2 scenarios are possible:
> >>
> >> 1. The xvclk is fully controlled by ACPI powermanagement, so there
> >> is no "xvclk" for the driver to get (since it is abstracted away).
> >> In this case there will be a "clock-frequency" device property
> >> to tell the driver the xvclk rate.
> >>
> >> 2. There is a xvclk modelled in the clk framework for the driver,
> >> but the clk-generator may not be set to the right frequency
> >> yet. In this case there will also be a "clock-frequency" device
> >> property and the driver is expected to set the rate of the xvclk
> >> through this frequency through the clk framework.
> >>
> >> Handle both these scenarios by switching to devm_clk_get_optional()
> >> and checking for a "clock-frequency" device property.
> >>
> >> This is modelled after how the same issue was fixed for the ov8865 in
> >> commit 73dcffeb2ff9 ("media: i2c: Support 19.2MHz input clock in ov8865").
> >>
> >> Acked-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> drivers/media/i2c/ov2680.c | 26 ++++++++++++++++++++++++--
> >> 1 file changed, 24 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> >> index b7c23286700e..a6a83f0e53f3 100644
> >> --- a/drivers/media/i2c/ov2680.c
> >> +++ b/drivers/media/i2c/ov2680.c
> >> @@ -698,6 +698,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> >> {
> >> struct device *dev = sensor->dev;
> >> struct gpio_desc *gpio;
> >> + unsigned int rate = 0;
> >> int ret;
> >>
> >> /*
> >> @@ -718,13 +719,34 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
> >>
> >> sensor->pwdn_gpio = gpio;
> >>
> >> - sensor->xvclk = devm_clk_get(dev, "xvclk");
> >> + sensor->xvclk = devm_clk_get_optional(dev, "xvclk");
> >> if (IS_ERR(sensor->xvclk)) {
> >> dev_err(dev, "xvclk clock missing or invalid\n");
> >> return PTR_ERR(sensor->xvclk);
> >> }
> >>
> >> - sensor->xvclk_freq = clk_get_rate(sensor->xvclk);
> >> + /*
> >> + * We could have either a 24MHz or 19.2MHz clock rate from either DT or
> >> + * ACPI... but we also need to support the weird IPU3 case which will
> >> + * have an external clock AND a clock-frequency property. Check for the
> >
> > Where does this happen? This puts the driver in an awful situation. :-(
>
> This happens on IPU3 setups where the INT3472 device represents an actual
> i2c attached sensor PMIC (rather then just some GPIOs) in this case
> there is a clk generator inside the PMIC which is used and that is programmable,
> so the driver needs to set the clk-rate.
>
> Note this patch is pretty much a 1:1 copy of the same patch for the ov8865
> and ov7251 drivers.
>
> I guess it might be good to start a discussion about doing this more
> elegantly but that seems out of scope for this series.
Works for me.
Do you happen to know which systems use the clock generator feature of the
PMIC?
I guess it could be as simple as putting this to tps68470 platform data for
the clock. And then hope no other PMICs will be used with this format.
--
Kind regards,
Sakari Ailus
