Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Tue, 3 Oct 2023 09:39:59 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Abdel Alkuor <alkuor@gmail.com>
Cc:     krzysztof.kozlowski+dt@linaro.org, bryan.odonoghue@linaro.org,
        gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, ryan.eleceng@gmail.com,
        robh+dt@kernel.org, conor+dt@kernel.org,
        devicetree@vger.kernel.org, Abdel Alkuor <abdelalkuor@geotab.com>
Subject: Re: [PATCH v9 08/14] USB: typec: tps6598x: Add interrupt support for
 TPS25750
Message-ID: <ZRu3P7Dlo7CiQfFK@kuha.fi.intel.com>
References: <20231001081134.37101-1-alkuor@gmail.com>
 <20231001081134.37101-9-alkuor@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231001081134.37101-9-alkuor@gmail.com>
Precedence: bulk

On Sun, Oct 01, 2023 at 04:11:28AM -0400, Abdel Alkuor wrote:
> From: Abdel Alkuor <abdelalkuor@geotab.com>
> 
> tps25750 event registers structure is different than tps6598x's,
> tps25750 has 11 bytes of events which are read at once where
> tps6598x has two event registers of 8 bytes each which are read
> separately. Likewise MASK event registers. Also, not all events
> are supported in both devices.
> 
> - Create a new handler to accommodate tps25750 interrupt
> - Add device data to of_device_id
> 
> Signed-off-by: Abdel Alkuor <abdelalkuor@geotab.com>

I'm sorry, but I just realised that this one also has to be in place
by the time TPS25750 is enabled in patch 4/14. Otherwise the series is
not bisectable.

I think you need to refactor the patch order a bit:

First come the patches that prepare everything that needs preparing -
introduction of struct tipd_data (without anything TPS25750 specific),
and so on.

Then you have patches for things that are specific to TPS25750 (small
stuff just merge together) if needed.

In the very last patches you finally enable TPS25750.

thanks,

> ---
> Changes in v9:
>   - Move of_device_id to its original place
>   - Move device data structs to the top of of_device_id
>   - Use device_get_match_data to get device data
> Changes in v8:
>   - Populate of_device_id with device data
>   - Change tps->cb to tps->data
>   - Assign matched data to tps data
> Changes in v7:
>   - Add driver name to commit subject
>   - Create tps25750 interrupt handler
> Changes in v6:
>   - Create tipd callbacks factory 
> Changes in v5:
>   - Incorporating tps25750 into tps6598x driver
> 
>  drivers/usb/typec/tipd/core.c | 96 +++++++++++++++++++++++++++++++----
>  1 file changed, 87 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 32e42798688f..52dc1cc16bed 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -18,6 +18,7 @@
>  #include <linux/usb/role.h>
>  #include <linux/workqueue.h>
>  #include <linux/firmware.h>
> +#include <linux/of_device.h>
>  
>  #include "tps6598x.h"
>  #include "trace.h"
> @@ -101,6 +102,10 @@ static const char *const modes[] = {
>  /* Unrecognized commands will be replaced with "!CMD" */
>  #define INVALID_CMD(_cmd_)		(_cmd_ == 0x444d4321)
>  
> +struct tipd_data {
> +	irq_handler_t irq_handler;
> +};
> +
>  struct tps6598x {
>  	struct device *dev;
>  	struct regmap *regmap;
> @@ -118,9 +123,11 @@ struct tps6598x {
>  	enum power_supply_usb_type usb_type;
>  
>  	int wakeup;
> +	u32 status; /* status reg */
>  	u16 pwr_status;
>  	struct delayed_work	wq_poll;
> -	irq_handler_t irq_handler;
> +
> +	const struct tipd_data *data;
>  };
>  
>  static enum power_supply_property tps6598x_psy_props[] = {
> @@ -545,6 +552,64 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  	return IRQ_NONE;
>  }
>  
> +static bool tps6598x_has_role_changed(struct tps6598x *tps, u32 status)
> +{
> +	status ^= tps->status;
> +
> +	return status & (TPS_STATUS_PORTROLE | TPS_STATUS_DATAROLE);
> +}
> +
> +static irqreturn_t tps25750_interrupt(int irq, void *data)
> +{
> +	struct tps6598x *tps = data;
> +	u64 event[2] = { };
> +	u32 status;
> +	int ret;
> +
> +	mutex_lock(&tps->lock);
> +
> +	ret = tps6598x_block_read(tps, TPS_REG_INT_EVENT1, event, 11);
> +	if (ret) {
> +		dev_err(tps->dev, "%s: failed to read events\n", __func__);
> +		goto err_unlock;
> +	}
> +
> +	if (!(event[0] | event[1]))
> +		goto err_unlock;
> +
> +	if (!tps6598x_read_status(tps, &status))
> +		goto err_clear_ints;
> +
> +	if ((event[0] | event[1]) & TPS_REG_INT_POWER_STATUS_UPDATE)
> +		if (!tps6598x_read_power_status(tps))
> +			goto err_clear_ints;
> +
> +	if ((event[0] | event[1]) & TPS_REG_INT_DATA_STATUS_UPDATE)
> +		if (!tps6598x_read_data_status(tps))
> +			goto err_clear_ints;
> +
> +	/*
> +	 * data/port roles could be updated independently after
> +	 * a plug event. Therefore, we need to check
> +	 * for pr/dr status change to set TypeC dr/pr accordingly.
> +	 */
> +	if ((event[0] | event[1]) & TPS_REG_INT_PLUG_EVENT ||
> +	    tps6598x_has_role_changed(tps, status))
> +		tps6598x_handle_plug_event(tps, status);
> +
> +	tps->status = status;
> +
> +err_clear_ints:
> +	tps6598x_block_write(tps, TPS_REG_INT_CLEAR1, event, 11);
> +
> +err_unlock:
> +	mutex_unlock(&tps->lock);
> +
> +	if (event[0] | event[1])
> +		return IRQ_HANDLED;
> +	return IRQ_NONE;
> +}
> +
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> @@ -600,7 +665,7 @@ static void tps6598x_poll_work(struct work_struct *work)
>  	struct tps6598x *tps = container_of(to_delayed_work(work),
>  					    struct tps6598x, wq_poll);
>  
> -	tps->irq_handler(0, tps);
> +	tps->data->irq_handler(0, tps);
>  	queue_delayed_work(system_power_efficient_wq,
>  			   &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>  }
> @@ -969,7 +1034,6 @@ static int tps25750_apply_patch(struct tps6598x *tps)
>  
>  static int tps6598x_probe(struct i2c_client *client)
>  {
> -	irq_handler_t irq_handler = tps6598x_interrupt;
>  	struct device_node *np = client->dev.of_node;
>  	struct typec_capability typec_cap = { };
>  	struct tps6598x *tps;
> @@ -1017,7 +1081,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  			APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
>  			APPLE_CD_REG_INT_PLUG_EVENT;
>  
> -		irq_handler = cd321x_interrupt;
>  	} else {
>  		/* Enable power status, data status and plug event interrupts */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> @@ -1025,7 +1088,10 @@ static int tps6598x_probe(struct i2c_client *client)
>  			TPS_REG_INT_PLUG_EVENT;
>  	}
>  
> -	tps->irq_handler = irq_handler;
> +	tps->data = device_get_match_data(tps->dev);
> +	if (!tps->data)
> +		return -EINVAL;
> +
>  	/* Make sure the controller has application firmware running */
>  	ret = tps6598x_check_mode(tps);
>  	if (ret < 0)
> @@ -1125,7 +1191,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  	if (client->irq) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> -						irq_handler,
> +						tps->data->irq_handler,
>  						IRQF_SHARED | IRQF_ONESHOT,
>  						dev_name(&client->dev), tps);
>  	} else {
> @@ -1231,10 +1297,22 @@ static const struct dev_pm_ops tps6598x_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(tps6598x_suspend, tps6598x_resume)
>  };
>  
> +static const struct tipd_data cd321x_data = {
> +	.irq_handler = cd321x_interrupt,
> +};
> +
> +static const struct tipd_data tps6598x_data = {
> +	.irq_handler = tps6598x_interrupt,
> +};
> +
> +static const struct tipd_data tps25750_data = {
> +	.irq_handler = tps25750_interrupt,
> +};
> +
>  static const struct of_device_id tps6598x_of_match[] = {
> -	{ .compatible = "ti,tps6598x", },
> -	{ .compatible = "apple,cd321x", },
> -	{ .compatible = "ti,tps25750", },
> +	{ .compatible = "ti,tps6598x", &tps6598x_data},
> +	{ .compatible = "apple,cd321x", &cd321x_data},
> +	{ .compatible = "ti,tps25750", &tps25750_data},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, tps6598x_of_match);
> -- 
> 2.34.1

-- 
heikki