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.
Regards,
Hans
>
>> + * clock-frequency property and if found, set that rate if we managed
>> + * to acquire a clock. This should cover the ACPI case. If the system
>> + * uses devicetree then the configured rate should already be set, so
>> + * we can just read it.
>> + */
>> + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>> + &rate);
>> + if (ret && !sensor->xvclk)
>> + return dev_err_probe(dev, ret, "invalid clock config\n");
>> +
>> + if (!ret && sensor->xvclk) {
>> + ret = clk_set_rate(sensor->xvclk, rate);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "failed to set clock rate\n");
>> + }
>> +
>> + sensor->xvclk_freq = rate ?: clk_get_rate(sensor->xvclk);
>> if (sensor->xvclk_freq != OV2680_XVCLK_VALUE) {
>> dev_err(dev, "wrong xvclk frequency %d HZ, expected: %d Hz\n",
>> sensor->xvclk_freq, OV2680_XVCLK_VALUE);
>
