[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250316110237.0b558248@jic23-huawei>
Date: Sun, 16 Mar 2025 11:02:37 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
<lars@...afoo.de>, Nuno Sa <nuno.sa@...log.com>, David Lechner
<dlechner@...libre.com>, Javier Carrasco <javier.carrasco.cruz@...il.com>,
Olivier Moysan <olivier.moysan@...s.st.com>, Guillaume Stols
<gstols@...libre.com>, Dumitru Ceclan <mitrutzceclan@...il.com>, Trevor
Gamblin <tgamblin@...libre.com>, Matteo Martelli
<matteomartelli3@...il.com>, Alisa-Dariana Roman <alisadariana@...il.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, João
Paulo Gonçalves <joao.goncalves@...adex.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v7 06/10] iio: adc: Support ROHM BD79124 ADC
On Thu, 13 Mar 2025 09:19:03 +0200
Matti Vaittinen <mazziesaccount@...il.com> wrote:
> The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
> an automatic measurement mode, with an alarm interrupt for out-of-window
> measurements. The window is configurable for each channel.
>
> The I2C protocol for manual start of the measurement and data reading is
> somewhat peculiar. It requires the master to do clock stretching after
> sending the I2C slave-address until the slave has captured the data.
> Needless to say this is not well suopported by the I2C controllers.
>
> Thus the driver does not support the BD79124's manual measurement mode
Given you are going to be doing a v8 and I'm bored on a train, so utterly
trivial comments that you get as a frequent contributor as things to
consider for future patches. (I'm sure it's just what you always wanted
:)
In theory should be imperative though I don't care as much as some.
Hence, do not support the....
> but implements the measurements using automatic measurement mode relying
> on the BD79124's ability of storing latest measurements into register.
>
> The driver does also support configuring the threshold events for
> detecting the out-of-window events.
Trivial editorial comment: that 'does' is not providing anything use
in modern English (might have done in the past, no idea!)
"Also support configure the threshold..."
>
> The BD79124 keeps asserting IRQ for as long as the measured voltage is
> out of the configured window. Thus the driver masks the received event
> for a fixed duration (1 second) when an event is handled. This prevents
> the user-space from choking on the events
>
> The ADC input pins can be also configured as general purpose outputs.
> Those pins which don't have corresponding ADC channel node in the
> device-tree will be controllable as GPO.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
A few trivial things inline.
Jonathan
> +/*
> + * The high and low limits as well as the recent result values are stored in
> + * the same way in 2 consequent registers. The first register contains 4 bits
> + * of the value. These bits are stored in the high bits [7:4] of register, but
> + * they represent the low bits [3:0] of the value.
> + * The value bits [11:4] are stored in the next regoster.
> + *
> + * Conver the integer to register format and write it using rmw cycle.
Convert?
> + */
> +static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
> + unsigned int val)
..
> +static int bd79124_read_event_config(struct iio_dev *iio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct bd79124_data *data = iio_priv(iio_dev);
> +
> + if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
> + return -EINVAL;
> +
> + return (data->alarm_monitored[chan->channel] & BIT(dir));
Drop the outer brackets as not adding anything.
> +}
> +static int bd79124_probe(struct i2c_client *i2c)
> +{
...
> + data->gpio_valid_mask = gpio_pins;
> + data->gc = bd79124gpo_chip;
> + data->gc.parent = dev;
> + devm_mutex_init(dev, &data->mutex);
ret = devm_mutex_init(dev, &data->mutex);
if (ret)
return ret;
It is very unlikely to fail so no point in papering over failing
to register the cleanup.
> +
> + ret = devm_gpiochip_add_data(dev, &data->gc, data);
> + if (ret)
> + return dev_err_probe(dev, ret, "gpio init Failed\n");
> +
> + if (i2c->irq > 0) {
> + ret = devm_request_threaded_irq(dev, i2c->irq,
> + bd79124_irq_handler, &bd79124_event_handler,
> + IRQF_ONESHOT, "adc-thresh-alert", iio_dev);
> + if (ret)
> + return dev_err_probe(data->dev, ret,
> + "Failed to register IRQ\n");
> + }
> +
> + return devm_iio_device_register(data->dev, iio_dev);
> +}
Powered by blists - more mailing lists