Hi,
On 7/31/23 15:35, Sakari Ailus wrote:
> 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?
This is used at least on the Microsoft Surface Go devices most folks use for IPU3 development.
and have the tps68470
>
> 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.
Right, after your email from earlier today I was actually thinking along the following lines to fix this:
1. There already is a struct tps68470_clk_platform_data which currently just contains the consumer dev_name() + con-id, we could extend this with an init_clk_rate member
2. Have the int3472 glue code fill init_clk_rate with info from the sensor's SSDB. This does require the int3472 code to make an extra SSDB() ACPI call. The ssdb struct definition has moved to include/media/ipu-bridge.h now, so that is already shared.
3. Make the tps68470 driver set the clk-rate to init_clk_rate if that is set to non 0
Then the clk_set_rate() call can be dropped from the drivers, note we do still need the other complexity with getting the clk + then getting the rate from the clk with a fallback to the property.
AFAICT doing this in a follow up series seems quite doable.
Regards,
Hans
