Received-SPF: pass (google.com: domain of linux-kernel+bounces-198334-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 2 Jun 2024 16:47:47 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Dimitri Fedrau <dima.fedrau@gmail.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Andrew Hepp
 <andrew.hepp@ahepp.dev>, Marcelo Schmitt <marcelo.schmitt1@gmail.com>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] iio: temperature: mcp9600: add threshold events
 support
Message-ID: <20240602164747.355772f7@jic23-huawei>
In-Reply-To: <20240526055234.736862-1-dima.fedrau@gmail.com>
References: <20240526055234.736862-1-dima.fedrau@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Sun, 26 May 2024 07:52:34 +0200
Dimitri Fedrau <dima.fedrau@gmail.com> wrote:

> The device has four programmable temperature alert outputs which can be
> used to monitor hot or cold-junction temperatures and detect falling and
> rising temperatures. It supports up to 255 degree celsius programmable
> hysteresis. Each alert can be individually configured by setting following
> options in the associated alert configuration register:
> - monitor hot or cold junction temperature
> - monitor rising or falling temperature
> - set comparator or interrupt mode
> - set output polarity
> - enable alert
> 
> This patch binds alert outputs to iio events:
> - alert1: hot junction, rising temperature
> - alert2: hot junction, falling temperature
> - alert3: cold junction, rising temperature
> - alert4: cold junction, falling temperature
> 
> All outputs are set in comparator mode and polarity depends on interrupt
> configuration.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
Hi Dimitri

I'm afraid I forgot to check you were handling the case of missing
interrupts in previous reviews.  The handling doesn't need to be
comprehensive but given the driver previously didn't support events
it seems likely people might not have wired them.

I think you should definitely handle 'no interrupts'
but it is probably fine to not bother with complexity of only some
of them wired unless a user actually shows up who cares about that.

See below for more on this.

Jonathan


> ---
> 
> Changes in V2:
>   - Remove pretty printing patches from series
>   - Add patch for providing index for both channels(ABI change)!
>     Suggested by Jonathan, hope this okay.
>   - Remove formatting in a precursor patch
>   - Add lock documentation
>   - Add define MCP9600_TEMP_SCALE_NUM and use it instead of MICRO. MICRO is
>     type unsigned long which we have to cast to int when using
>     multiplication or division, because we are handling negative values.
>   - Use switch statement in mcp9600_write_thresh
>   - Replaced generic interrupt handler with four separate interrupt handler
>   - Use one lock instead of four
>   - Added error check for mcp9600_probe_alerts
> 
> Changes in V3:
>   - Remove patch for providing index for both channels(ABI change)!
>   - Treat hysteresis as offset and remove the lock, since dependency
>     between hysteresis and threshold doesn't exist anymore.
>   - Use helper function for handling alerts to avoid code duplication
>   - Scale max,min defines for hot and cold junction temperature to micro
> 
> Changes in V4:
>   - Added version number of patch
> 
> ---
>  drivers/iio/temperature/mcp9600.c | 319 ++++++++++++++++++++++++++++++
>  1 file changed, 319 insertions(+)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 46845804292b..1f34f0bab22f 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -6,28 +6,90 @@
>   * Author: <andrew.hepp@ahepp.dev>
>   */
>  
>
> +
>  static const struct iio_chan_spec mcp9600_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.address = MCP9600_HOT_JUNCTION,
>  		.info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.event_spec = mcp9600_events,
> +		.num_event_specs = ARRAY_SIZE(mcp9600_events),
>  	},
>  	{
>  		.type = IIO_TEMP,
> @@ -36,6 +98,8 @@ static const struct iio_chan_spec mcp9600_channels[] = {
>  		.modified = 1,
>  		.info_mask_separate =
>  			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +		.event_spec = mcp9600_events,
> +		.num_event_specs = ARRAY_SIZE(mcp9600_events),
>  	},
>  };

What if the irq isn't wired?  We still get all the interfaces but
I assume the don't do anything.

Usual trick is to pick between two sets of channels depending on whether
the interrupts are present.

Here is is fiddly because of the 4 separate alert lines.
I think a compromise would be to have the driver only use interrupts
if all 4 are present.  If less than that are present don't register
channels with the event_spec etc.

That will require you to check for all the interrupts before starting
to register any of them which is a little messy but not too bad.


>  
> @@ -79,10 +143,260 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
>  	}
>  }

>
> +
> +static irqreturn_t mcp9600_alert1_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
Trivial: Shouldn't need the local variable. Just pass private in directly
to your mcp9600_alert_handler() 

> +
> +	return mcp9600_alert_handler(indio_dev, MCP9600_ALERT1, IIO_NO_MOD,
> +				     IIO_EV_DIR_RISING);
> +}
> +
> +static irqreturn_t mcp9600_alert2_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	return mcp9600_alert_handler(indio_dev, MCP9600_ALERT2, IIO_NO_MOD,
> +				     IIO_EV_DIR_FALLING);
> +}
> +
> +static irqreturn_t mcp9600_alert3_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	return mcp9600_alert_handler(indio_dev, MCP9600_ALERT3,
> +				     IIO_MOD_TEMP_AMBIENT, IIO_EV_DIR_RISING);
> +}
> +
> +static irqreturn_t mcp9600_alert4_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +
> +	return mcp9600_alert_handler(indio_dev, MCP9600_ALERT4,
> +				     IIO_MOD_TEMP_AMBIENT, IIO_EV_DIR_FALLING);
> +}