Re: [PATCH] [media] staging: atomisp: use clock framework for camera clocks

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 09/20/2017 04:12 AM, Andy Shevchenko wrote:
> On Tue, 2017-09-19 at 15:45 -0500, Pierre-Louis Bossart wrote:
>> The Atom ISP driver initializes and configures PMC clocks which are
>> already handled by the clock framework.
>>
>> Remove all legacy vlv2_platform_clock stuff and move to the clk API to
>> avoid conflicts, e.g. with audio machine drivers enabling the MCLK for
>> external codecs
>>
> I think it might have a Fixes: tag as well (though I dunno which commit
> could be considered as anchor).
The initial integration of the atomisp driver already had this problem, 
i'll add a reference to
'a49d25364dfb9 ("staging/atomisp: Add support for the Intel IPU v2")'
>
> (I doubt Git is so clever to remove files based on information out of
> the diff, can you check it and if needed to resend without -D implied?)
Gee, I thought -C -M -D were the standard options to checkpatch, never 
realized it'd prevent patches from applying. Thanks for the tip.

>
> Other than that - nice clean up!
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
I'll add your Reviewed-by in the v2. Thanks for the review.
>
>
>> Tested-by: Carlo Caione <carlo@endlessm.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.
>> com>
>> ---
>>   drivers/staging/media/atomisp/Kconfig              |   1 +
>>   drivers/staging/media/atomisp/platform/Makefile    |   1 -
>>   .../staging/media/atomisp/platform/clock/Makefile  |   6 -
>>   .../platform/clock/platform_vlv2_plat_clk.c        |  40 ----
>>   .../platform/clock/platform_vlv2_plat_clk.h        |  27 ---
>>   .../media/atomisp/platform/clock/vlv2_plat_clock.c | 247 ------------
>> ---------
>>   .../platform/intel-mid/atomisp_gmin_platform.c     |  63 +++++-
>>   7 files changed, 52 insertions(+), 333 deletions(-)
>>   delete mode 100644
>> drivers/staging/media/atomisp/platform/clock/Makefile
>>   delete mode 100644
>> drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.c
>>   delete mode 100644
>> drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.h
>>   delete mode 100644
>> drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
>>
>> diff --git a/drivers/staging/media/atomisp/Kconfig
>> b/drivers/staging/media/atomisp/Kconfig
>> index 8eb13c3ba29c..7cdebea35ccf 100644
>> --- a/drivers/staging/media/atomisp/Kconfig
>> +++ b/drivers/staging/media/atomisp/Kconfig
>> @@ -1,6 +1,7 @@
>>   menuconfig INTEL_ATOMISP
>>           bool "Enable support to Intel MIPI camera drivers"
>>           depends on X86 && EFI && MEDIA_CONTROLLER && PCI && ACPI
>> +	select COMMON_CLK
>>           help
>>             Enable support for the Intel ISP2 camera interfaces and
>> MIPI
>>             sensor drivers.
>> diff --git a/drivers/staging/media/atomisp/platform/Makefile
>> b/drivers/staging/media/atomisp/platform/Makefile
>> index df157630bda9..0e3b7e1c81c6 100644
>> --- a/drivers/staging/media/atomisp/platform/Makefile
>> +++ b/drivers/staging/media/atomisp/platform/Makefile
>> @@ -2,5 +2,4 @@
>>   # Makefile for camera drivers.
>>   #
>>   
>> -obj-$(CONFIG_INTEL_ATOMISP) += clock/
>>   obj-$(CONFIG_INTEL_ATOMISP) += intel-mid/
>> diff --git a/drivers/staging/media/atomisp/platform/clock/Makefile
>> b/drivers/staging/media/atomisp/platform/clock/Makefile
>> deleted file mode 100644
>> index 82fbe8b6968a..000000000000
>> diff --git
>> a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
>> c
>> b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
>> c
>> deleted file mode 100644
>> index 0aae9b0283bb..000000000000
>> diff --git
>> a/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
>> h
>> b/drivers/staging/media/atomisp/platform/clock/platform_vlv2_plat_clk.
>> h
>> deleted file mode 100644
>> index b730ab0e8223..000000000000
>> diff --git
>> a/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
>> b/drivers/staging/media/atomisp/platform/clock/vlv2_plat_clock.c
>> deleted file mode 100644
>> index f96789a31819..000000000000
>> diff --git a/drivers/staging/media/atomisp/platform/intel-
>> mid/atomisp_gmin_platform.c
>> b/drivers/staging/media/atomisp/platform/intel-
>> mid/atomisp_gmin_platform.c
>> index edaae93af8f9..17b4cfae5abf 100644
>> --- a/drivers/staging/media/atomisp/platform/intel-
>> mid/atomisp_gmin_platform.c
>> +++ b/drivers/staging/media/atomisp/platform/intel-
>> mid/atomisp_gmin_platform.c
>> @@ -4,10 +4,10 @@
>>   #include <linux/efi.h>
>>   #include <linux/pci.h>
>>   #include <linux/acpi.h>
>> +#include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <media/v4l2-subdev.h>
>>   #include <linux/mfd/intel_soc_pmic.h>
>> -#include "../../include/linux/vlv2_plat_clock.h"
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/gpio.h>
>> @@ -17,11 +17,7 @@
>>   
>>   #define MAX_SUBDEVS 8
>>   
>> -/* Should be defined in vlv2_plat_clock API, isn't: */
>> -#define VLV2_CLK_PLL_19P2MHZ 1
>> -#define VLV2_CLK_XTAL_19P2MHZ 0
>> -#define VLV2_CLK_ON      1
>> -#define VLV2_CLK_OFF     2
>> +#define VLV2_CLK_PLL_19P2MHZ 1 /* XTAL on CHT */
>>   #define ELDO1_SEL_REG	0x19
>>   #define ELDO1_1P8V	0x16
>>   #define ELDO1_CTRL_SHIFT 0x00
>> @@ -33,6 +29,7 @@ struct gmin_subdev {
>>   	struct v4l2_subdev *subdev;
>>   	int clock_num;
>>   	int clock_src;
>> +	struct clk *pmc_clk;
>>   	struct gpio_desc *gpio0;
>>   	struct gpio_desc *gpio1;
>>   	struct regulator *v1p8_reg;
>> @@ -344,6 +341,9 @@ static int gmin_platform_deinit(void)
>>   	return 0;
>>   }
>>   
>> +#define GMIN_PMC_CLK_NAME 14 /* "pmc_plt_clk_[0..5]" */
>> +static char gmin_pmc_clk_name[GMIN_PMC_CLK_NAME];
>> +
>>   static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev
>> *subdev)
>>   {
>>   	int i, ret;
>> @@ -377,6 +377,37 @@ static struct gmin_subdev *gmin_subdev_add(struct
>> v4l2_subdev *subdev)
>>   	gmin_subdevs[i].gpio0 = gpiod_get_index(dev, NULL, 0,
>> GPIOD_OUT_LOW);
>>   	gmin_subdevs[i].gpio1 = gpiod_get_index(dev, NULL, 1,
>> GPIOD_OUT_LOW);
>>   
>> +	/* get PMC clock with clock framework */
>> +	snprintf(gmin_pmc_clk_name,
>> +		 sizeof(gmin_pmc_clk_name),
>> +		 "%s_%d", "pmc_plt_clk", gmin_subdevs[i].clock_num);
>> +
>> +	gmin_subdevs[i].pmc_clk = devm_clk_get(dev,
>> gmin_pmc_clk_name);
>> +	if (IS_ERR(gmin_subdevs[i].pmc_clk)) {
>> +		ret = PTR_ERR(gmin_subdevs[i].pmc_clk);
>> +
>> +		dev_err(dev,
>> +			"Failed to get clk from %s : %d\n",
>> +			gmin_pmc_clk_name,
>> +			ret);
>> +
>> +		return NULL;
>> +	}
>> +
>> +	/*
>> +	 * The firmware might enable the clock at
>> +	 * boot (this information may or may not
>> +	 * be reflected in the enable clock register).
>> +	 * To change the rate we must disable the clock
>> +	 * first to cover these cases. Due to common
>> +	 * clock framework restrictions that do not allow
>> +	 * to disable a clock that has not been enabled,
>> +	 * we need to enable the clock first.
>> +	 */
>> +	ret = clk_prepare_enable(gmin_subdevs[i].pmc_clk);
>> +	if (!ret)
>> +		clk_disable_unprepare(gmin_subdevs[i].pmc_clk);
>> +
>>   	if (!IS_ERR(gmin_subdevs[i].gpio0)) {
>>   		ret = gpiod_direction_output(gmin_subdevs[i].gpio0,
>> 0);
>>   		if (ret)
>> @@ -539,13 +570,21 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev
>> *subdev, int on)
>>   {
>>   	int ret = 0;
>>   	struct gmin_subdev *gs = find_gmin_subdev(subdev);
>> +	struct i2c_client *client = v4l2_get_subdevdata(subdev);
>> +
>> +	if (on) {
>> +		ret = clk_set_rate(gs->pmc_clk, gs->clock_src);
>> +
>> +		if (ret)
>> +			dev_err(&client->dev, "unable to set PMC rate
>> %d\n",
>> +				gs->clock_src);
>>   
>> -	if (on)
>> -		ret = vlv2_plat_set_clock_freq(gs->clock_num, gs-
>>> clock_src);
>> -	if (ret)
>> -		return ret;
>> -	return vlv2_plat_configure_clock(gs->clock_num,
>> -					 on ? VLV2_CLK_ON :
>> VLV2_CLK_OFF);
>> +		ret = clk_prepare_enable(gs->pmc_clk);
>> +	} else {
>> +		clk_disable_unprepare(gs->pmc_clk);
>> +	}
>> +
>> +	return ret;
>>   }
>>   
>>   static int gmin_csi_cfg(struct v4l2_subdev *sd, int flag)