Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Sat, 25 Feb 2023 07:40:31 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Naresh Solanki <naresh.solanki@9elements.com>
Cc:     Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/4] hwmon: (pmbus/core): Add interrupt support
Message-ID: <20230225154031.GA448701@roeck-us.net>
References: <20230217083631.657430-1-Naresh.Solanki@9elements.com>
 <20230217083631.657430-3-Naresh.Solanki@9elements.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230217083631.657430-3-Naresh.Solanki@9elements.com>
Precedence: bulk

On Fri, Feb 17, 2023 at 09:36:30AM +0100, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Implement PMBUS irq handler.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ...
> Changes in V3:
> - Remove pmbus word check for SMBALERT writes
> - Remove variable ret & use err instead.
> - Use dev_dbg_once instead of error.
> - Remove error print when writing to misc_status register.
> - Move client irq check to pmbus_irq_setup.
> ---
>  drivers/hwmon/pmbus/pmbus.h      |  2 +-
>  drivers/hwmon/pmbus/pmbus_core.c | 78 ++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 713ea7915425..11e84e141126 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -26,7 +26,7 @@ enum pmbus_regs {
>  
>  	PMBUS_CAPABILITY		= 0x19,
>  	PMBUS_QUERY			= 0x1A,
> -
> +	PMBUS_SMBALERT_MASK		= 0x1B,
>  	PMBUS_VOUT_MODE			= 0x20,
>  	PMBUS_VOUT_COMMAND		= 0x21,
>  	PMBUS_VOUT_TRIM			= 0x22,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index f8ac9016ea0e..d0415d5ac7d9 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -3105,6 +3105,80 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>  }
>  #endif
>  
> +static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
> +{
> +	return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
> +}
> +
> +static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
> +{
> +	struct pmbus_data *data = pdata;
> +	struct i2c_client *client = to_i2c_client(data->dev);
> +	int i, status;
> +
> +	mutex_lock(&data->update_lock);
> +	for (i = 0; i < data->info->pages; i++) {
> +		status = pmbus_read_status_word(client, i);
> +		if (status < 0) {
> +			mutex_unlock(&data->update_lock);
> +			return status;

Unfortunately this is not a valid return value for interrupt handlers.
I think the best approach here would be to just continue with the loop.

> +		}
> +
> +		if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N))
> +			pmbus_clear_fault_page(client, i);

Should those fault flags even be checked ? If another bit is set, they will
be cleared anyway, and "clear flags only if some other flag is set" seems
a bit inconsistent.

Overall it seems to me that we should maybe just call pmbus_clear_faults()
at the end of the function. Sure, that clears faults on pages where there
is no fault, but the above code reads the status on all pages anyway, so
it is more costly overall then just clearing the faults on all pages.

> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
> +{
> +	struct device *dev = &client->dev;
> +	const struct pmbus_status_category *cat;
> +	const struct pmbus_status_assoc *bit;
> +	int i, j, err, func;
> +	u8 mask;
> +
> +	static const u8 misc_status[] = {PMBUS_STATUS_CML, PMBUS_STATUS_OTHER,
> +					 PMBUS_STATUS_MFR_SPECIFIC, PMBUS_STATUS_FAN_12,
> +					 PMBUS_STATUS_FAN_34};
> +
> +	if (!client->irq)
> +		return 0;
> +
> +	for (i = 0; i < data->info->pages; i++) {
> +		func = data->info->func[i];
> +
> +		for (j = 0; j < ARRAY_SIZE(pmbus_status_flag_map); j++) {
> +			cat = &pmbus_status_flag_map[j];
> +			if (!(func & cat->func))
> +				continue;
> +			mask = 0;
> +			for (bit = cat->bits; bit->pflag; bit++)
> +				mask |= bit->pflag;
> +
> +			err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask);
> +			if (err)
> +				dev_dbg_once(dev, "Failed to set smbalert for reg 0x%02x\n",
> +					     cat->reg);
> +		}
> +
> +		for (j = 0; j < ARRAY_SIZE(misc_status); j++)
> +			pmbus_write_smbalert_mask(client, i, misc_status[j], 0xff);
> +	}
> +
> +	/* Register notifiers */
> +	err = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler, 0,
> +					"pmbus-irq", data);
> +	if (err) {
> +		dev_err(dev, "failed to request an irq %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct dentry *pmbus_debugfs_dir;	/* pmbus debugfs directory */
>  
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -3467,6 +3541,10 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info)
>  	if (ret)
>  		return ret;
>  
> +	ret = pmbus_irq_setup(client, data);
> +	if (ret)
> +		return ret;
> +
>  	ret = pmbus_init_debugfs(client, data);
>  	if (ret)
>  		dev_warn(dev, "Failed to register debugfs\n");