Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 21 Nov 2020 17:33:30 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Ye Xiang <xiang.ye@intel.com>
Cc:     jikos@kernel.org, srinivas.pandruvada@linux.intel.com,
        linux-input@vger.kernel.org, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/4] iio: hid-sensor-trigger: Use iio->trig instead
 trig field internal structure
Message-ID: <20201121173330.3afd1087@archlinux>
In-Reply-To: <20201119100331.2594-4-xiang.ye@intel.com>
References: <20201119100331.2594-1-xiang.ye@intel.com>
        <20201119100331.2594-4-xiang.ye@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 19 Nov 2020 18:03:30 +0800
Ye Xiang <xiang.ye@intel.com> wrote:

> Use iio->trig instead of attrb->trig as parameter of iio_trigger_unregister
> and iio_trigger_free. This allows one HID sensor driver to create
> multiple iio devices. In this case common attributes are shared and
> there can be one instance for the structure containing common attributes
> for all iio devices.
> 
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>
Whilst indio_dev->trig is set to the local trigger at startup,
I'm not seeing the validate callbacks that will be needed to ensure it 
is still set to that trigger when you remove the driver.

Specifically validate_trigger callback for all the hid sensor devices.

It is entirely possible that should be set and these devices can only
use the hid sensor trigger, but I don't think it currently is.

Thus this patch is going to cause some interesting crashes.
Note this is the reason all IIO drivers have to carry an extra copy
of the trig pointer.

So you'll have to do something a bit more interesting to pass that
trigger to the relevant devices that share it.

Arguably if they are actually sharing the trigger, then we shouldn't really
have separate IIO devices in the first place as we loose the
connection between the timing of the data across the different channels.

Jonathan

> ---
>  drivers/iio/common/hid-sensors/hid-sensor-trigger.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index 30340abcbc8d..bb5e7c8ff15b 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -236,8 +236,8 @@ void hid_sensor_remove_trigger(struct iio_dev *indio_dev,
>  	pm_runtime_put_noidle(&attrb->pdev->dev);
>  
>  	cancel_work_sync(&attrb->work);
> -	iio_trigger_unregister(attrb->trigger);
> -	iio_trigger_free(attrb->trigger);
> +	iio_trigger_unregister(indio_dev->trig);
> +	iio_trigger_free(indio_dev->trig);

>  	iio_triggered_buffer_cleanup(indio_dev);
>  }
>  EXPORT_SYMBOL(hid_sensor_remove_trigger);