Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Wed, 25 Oct 2023 12:51:23 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Tomer Maimon <tmaimon77@gmail.com>
Cc:     jdelvare@suse.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, avifishman70@gmail.com,
        tali.perry1@gmail.com, joel@jms.id.au, andrew@codeconstruct.com.au,
        venture@google.com, yuenn@google.com, benjaminfair@google.com,
        j.neuschaefer@gmx.net, openbmc@lists.ozlabs.org,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2 2/2] hwmon: npcm750-pwm-fan: Add NPCM8xx support
Message-ID: <3c2a48d0-454a-4d40-ae8d-ff346d55d99d@roeck-us.net>
References: <20231018181925.1826042-1-tmaimon77@gmail.com>
 <20231018181925.1826042-3-tmaimon77@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231018181925.1826042-3-tmaimon77@gmail.com>
Precedence: bulk

On Wed, Oct 18, 2023 at 09:19:25PM +0300, Tomer Maimon wrote:
> Adding Pulse Width Modulation (PWM) and fan tacho NPCM8xx support to
> NPCM PWM and fan tacho driver.
> NPCM8xx uses a different number of PWM devices.
> 
> As part of adding NPCM8XX support:
> - Add NPCM8xx specific compatible string.
> - Add data to handle architecture-specific PWM and fan tacho parameters.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  drivers/hwmon/npcm750-pwm-fan.c | 161 +++++++++++++++++++++++++++-----
>  1 file changed, 136 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwmon/npcm750-pwm-fan.c b/drivers/hwmon/npcm750-pwm-fan.c
> index 10ed3f4335d4..324de4482e71 100644
> --- a/drivers/hwmon/npcm750-pwm-fan.c
> +++ b/drivers/hwmon/npcm750-pwm-fan.c
> @@ -45,11 +45,6 @@
>  #define NPCM7XX_PWM_CTRL_CH2_EN_BIT		BIT(12)
>  #define NPCM7XX_PWM_CTRL_CH3_EN_BIT		BIT(16)
>  
> -/* Define the maximum PWM channel number */
> -#define NPCM7XX_PWM_MAX_CHN_NUM			8
> -#define NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE	4
> -#define NPCM7XX_PWM_MAX_MODULES                 2
> -
>  /* Define the Counter Register, value = 100 for match 100% */
>  #define NPCM7XX_PWM_COUNTER_DEFAULT_NUM		255
>  #define NPCM7XX_PWM_CMR_DEFAULT_NUM		255
> @@ -138,11 +133,9 @@
>  #define NPCM7XX_FAN_TCPCFG_CPASEL	BIT(0)
>  
>  /* FAN General Definition */
> -/* Define the maximum FAN channel number */
> -#define NPCM7XX_FAN_MAX_MODULE			8
> +/* Define the PWM and FAN in a module */
> +#define NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE	4
>  #define NPCM7XX_FAN_MAX_CHN_NUM_IN_A_MODULE	2
> -#define NPCM7XX_FAN_MAX_CHN_NUM			16
> -
>  /*
>   * Get Fan Tach Timeout (base on clock 214843.75Hz, 1 cnt = 4.654us)
>   * Timeout 94ms ~= 0x5000
> @@ -171,6 +164,15 @@
>  #define FAN_PREPARE_TO_GET_FIRST_CAPTURE	0x01
>  #define FAN_ENOUGH_SAMPLE			0x02
>  
> +struct npcm_hwmon_info {
> +	u32 pwm_max_modules;
> +	u32 pwm_max_ch;
> +	u32 fan_max_modules;
> +	u32 fan_max_ch;
> +	const struct hwmon_chip_info *hinfo;
> +	const char *name;
> +};
> +
>  struct npcm7xx_fan_dev {
>  	u8 fan_st_flg;
>  	u8 fan_pls_per_rev;
> @@ -195,15 +197,16 @@ struct npcm7xx_pwm_fan_data {
>  	unsigned long fan_clk_freq;
>  	struct clk *pwm_clk;
>  	struct clk *fan_clk;
> -	struct mutex pwm_lock[NPCM7XX_PWM_MAX_MODULES];
> -	spinlock_t fan_lock[NPCM7XX_FAN_MAX_MODULE];
> -	int fan_irq[NPCM7XX_FAN_MAX_MODULE];
> -	bool pwm_present[NPCM7XX_PWM_MAX_CHN_NUM];
> -	bool fan_present[NPCM7XX_FAN_MAX_CHN_NUM];

I do not see the point of making this dynamic.
Sure, there is an additional pwm channel on NPCM8xx,
so just add another entry and handle (enable/disable)
the additional channel with the is_visible function.
As written there is a lot of churn, and I am sure the code is
much larger than it was just to save a single pwm entry.

> +	struct mutex *pwm_lock;
> +	spinlock_t *fan_lock;
> +	int *fan_irq;
> +	bool *pwm_present;
> +	bool *fan_present;
>  	u32 input_clk_freq;
>  	struct timer_list fan_timer;
> -	struct npcm7xx_fan_dev fan_dev[NPCM7XX_FAN_MAX_CHN_NUM];
> -	struct npcm7xx_cooling_device *cdev[NPCM7XX_PWM_MAX_CHN_NUM];
> +	struct npcm7xx_fan_dev *fan_dev;
> +	struct npcm7xx_cooling_device **cdev;
> +	const struct npcm_hwmon_info *info;
>  	u8 fan_select;
>  };
>  
> @@ -333,7 +336,7 @@ static void npcm7xx_fan_polling(struct timer_list *t)
>  	 * Polling two module per one round,
>  	 * FAN01 & FAN89 / FAN23 & FAN1011 / FAN45 & FAN1213 / FAN67 & FAN1415
>  	 */
> -	for (i = data->fan_select; i < NPCM7XX_FAN_MAX_MODULE;
> +	for (i = data->fan_select; i < data->info->fan_max_modules;
>  	      i = i + 4) {
>  		/* clear the flag and reset the counter (TCNT) */
>  		iowrite8(NPCM7XX_FAN_TICLR_CLEAR_ALL,
> @@ -659,6 +662,40 @@ static const struct hwmon_channel_info * const npcm7xx_info[] = {
>  	NULL
>  };
>  
> +static const struct hwmon_channel_info * const npcm8xx_info[] = {
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT),
> +	NULL
> +};

This should be handled with the is_visible() function, not with separate
data structures.

> +
>  static const struct hwmon_ops npcm7xx_hwmon_ops = {
>  	.is_visible = npcm7xx_is_visible,
>  	.read = npcm7xx_read,
> @@ -670,6 +707,29 @@ static const struct hwmon_chip_info npcm7xx_chip_info = {
>  	.info = npcm7xx_info,
>  };
>  
> +static const struct hwmon_chip_info npcm8xx_chip_info = {
> +	.ops = &npcm7xx_hwmon_ops,
> +	.info = npcm8xx_info,
> +};
> +
> +static const struct npcm_hwmon_info npxm7xx_hwmon_info = {
> +	.pwm_max_modules = 2,
> +	.pwm_max_ch = NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE * 2,
> +	.fan_max_modules = 8,
> +	.fan_max_ch = NPCM7XX_FAN_MAX_CHN_NUM_IN_A_MODULE * 8,
> +	.hinfo = &npcm7xx_chip_info,
> +	.name = "npcm7xx_pwm_fan",
> +};
> +
> +static const struct npcm_hwmon_info npxm8xx_hwmon_info = {
> +	.pwm_max_modules = 3,
> +	.pwm_max_ch = NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE * 3,
> +	.fan_max_modules = 8,
> +	.fan_max_ch = NPCM7XX_FAN_MAX_CHN_NUM_IN_A_MODULE * 8,

fan_max_modules and fan_max_ch do not change across chips, and
pwm_max_ch is always NPCM7XX_PWM_MAX_CHN_NUM_IN_A_MODULE times
the number of channels. This can all be dropped. Again, the only
real difference is the number of pwm channels, and that can be
handled in the _is_visible() function.

> +	.hinfo = &npcm8xx_chip_info,
> +	.name = "npcm8xx_pwm_fan",
> +};
> +
>  static u32 npcm7xx_pwm_init(struct npcm7xx_pwm_fan_data *data)
>  {
>  	int m, ch;
> @@ -693,7 +753,7 @@ static u32 npcm7xx_pwm_init(struct npcm7xx_pwm_fan_data *data)
>  	/* Setting PWM Prescale Register value register to both modules */
>  	prescale_val |= (prescale_val << NPCM7XX_PWM_PRESCALE_SHIFT_CH01);
>  
> -	for (m = 0; m < NPCM7XX_PWM_MAX_MODULES  ; m++) {
> +	for (m = 0; m < data->info->pwm_max_modules  ; m++) {
>  		iowrite32(prescale_val, NPCM7XX_PWM_REG_PR(data->pwm_base, m));
>  		iowrite32(NPCM7XX_PWM_PRESCALE2_DEFAULT,
>  			  NPCM7XX_PWM_REG_CSR(data->pwm_base, m));
> @@ -716,7 +776,7 @@ static void npcm7xx_fan_init(struct npcm7xx_pwm_fan_data *data)
>  	int i;
>  	u32 apb_clk_freq;
>  
> -	for (md = 0; md < NPCM7XX_FAN_MAX_MODULE; md++) {
> +	for (md = 0; md < data->info->fan_max_modules; md++) {
>  		/* stop FAN0~7 clock */
>  		iowrite8(NPCM7XX_FAN_TCKC_CLKX_NONE,
>  			 NPCM7XX_FAN_REG_TCKC(data->fan_base, md));
> @@ -905,6 +965,49 @@ static int npcm7xx_en_pwm_fan(struct device *dev,
>  	return 0;
>  }
>  
> +static int npcm_pwm_fan_alloc_data(struct device *dev,
> +				   struct npcm7xx_pwm_fan_data *data)
> +{
> +	data->pwm_lock = devm_kcalloc(dev, data->info->pwm_max_modules,
> +				      sizeof(*data->pwm_lock), GFP_KERNEL);
> +	if (!data->pwm_lock)
> +		return -ENOMEM;
> +
> +	data->fan_lock = devm_kcalloc(dev, data->info->fan_max_modules,
> +				      sizeof(*data->fan_lock), GFP_KERNEL);
> +	if (!data->fan_lock)
> +		return -ENOMEM;
> +
> +	data->fan_irq = devm_kcalloc(dev, data->info->fan_max_modules,
> +				     sizeof(*data->fan_irq), GFP_KERNEL);
> +	if (!data->fan_irq)
> +		return -ENOMEM;
> +
> +	data->pwm_present = devm_kcalloc(dev, data->info->pwm_max_ch,
> +					 sizeof(*data->pwm_present),
> +					 GFP_KERNEL);
> +	if (!data->pwm_present)
> +		return -ENOMEM;
> +
> +	data->fan_present = devm_kcalloc(dev, data->info->fan_max_ch,
> +					 sizeof(*data->fan_present),
> +					 GFP_KERNEL);
> +	if (!data->fan_present)
> +		return -ENOMEM;
> +
> +	data->fan_dev = devm_kcalloc(dev, data->info->fan_max_ch,
> +				     sizeof(*data->fan_dev), GFP_KERNEL);
> +	if (!data->fan_dev)
> +		return -ENOMEM;
> +
> +	data->cdev = devm_kcalloc(dev, data->info->pwm_max_ch,
> +				  sizeof(*data->cdev), GFP_KERNEL);
> +	if (!data->cdev)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -923,6 +1026,13 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	data->info = device_get_match_data(dev);
> +	if (!data->info)
> +		return -EINVAL;
> +
> +	if (npcm_pwm_fan_alloc_data(dev, data))
> +		return -ENOMEM;
> +
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm");
>  	if (!res) {
>  		dev_err(dev, "pwm resource not found\n");
> @@ -960,10 +1070,10 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  	output_freq = npcm7xx_pwm_init(data);
>  	npcm7xx_fan_init(data);
>  
> -	for (cnt = 0; cnt < NPCM7XX_PWM_MAX_MODULES  ; cnt++)
> +	for (cnt = 0; cnt < data->info->pwm_max_modules  ; cnt++)
>  		mutex_init(&data->pwm_lock[cnt]);
>  
> -	for (i = 0; i < NPCM7XX_FAN_MAX_MODULE; i++) {
> +	for (i = 0; i < data->info->fan_max_modules; i++) {
>  		spin_lock_init(&data->fan_lock[i]);
>  
>  		data->fan_irq[i] = platform_get_irq(pdev, i);
> @@ -988,15 +1098,15 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, "npcm7xx_pwm_fan",
> -						     data, &npcm7xx_chip_info,
> +	hwmon = devm_hwmon_device_register_with_info(dev, data->info->name,
> +						     data, data->info->hinfo,
>  						     NULL);
>  	if (IS_ERR(hwmon)) {
>  		dev_err(dev, "unable to register hwmon device\n");
>  		return PTR_ERR(hwmon);
>  	}
>  
> -	for (i = 0; i < NPCM7XX_FAN_MAX_CHN_NUM; i++) {
> +	for (i = 0; i < data->info->fan_max_ch; i++) {
>  		if (data->fan_present[i]) {
>  			/* fan timer initialization */
>  			data->fan_timer.expires = jiffies +
> @@ -1015,7 +1125,8 @@ static int npcm7xx_pwm_fan_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id of_pwm_fan_match_table[] = {
> -	{ .compatible = "nuvoton,npcm750-pwm-fan", },
> +	{ .compatible = "nuvoton,npcm750-pwm-fan", .data = &npxm7xx_hwmon_info},
> +	{ .compatible = "nuvoton,npcm845-pwm-fan", .data = &npxm8xx_hwmon_info},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, of_pwm_fan_match_table);