On Wed, 31 Mar 2021 10:29:51 +0300 Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote: > On Sun, Mar 14, 2021 at 8:17 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > > Note this doesn't support everything the chip can do as we ignore > > window mode for now (in window / out of window). > > > > * Given the chip doesn't have any way of disabling the threshold > > pins, use disable_irq() etc to mask them except when we actually > > want them enabled (previously events were always enabled). > > Note there are race conditions, but using the current state from > > the status register and disabling interrupts across changes in > > type of event should mean those races result in interrupts, > > but no events to userspace. > > > > * Correctly reflect that there is one threshold line per channel. > > > > * Only take notice of rising edge. If anyone wants the other edge > > then they should set the other threshold (they are available for > > rising and falling directions). This isn't perfect but it makes > > it a lot simpler. > > > > * If insufficient interrupts are specified in firnware, don't support > > events. > > > > * Adaptive events use the same pos/neg values of thrMD as non adaptive > > ones. > > > > Tested against qemu based emulation. > > > > Overall looks ok. > A few minor issues with the device-matching and OF. > And maybe with the interrupt handling. Hopefully answers below address those two. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > Link: https://lore.kernel.org/r/20210207154623.433442-13-jic23@xxxxxxxxxx > > --- > > drivers/staging/iio/cdc/ad7150.c | 258 ++++++++++++++++++------------- > > 1 file changed, 153 insertions(+), 105 deletions(-) > > > > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c > > index f8183c540d5c..24be97456c03 100644 > > --- a/drivers/staging/iio/cdc/ad7150.c > > +++ b/drivers/staging/iio/cdc/ad7150.c > > @@ -11,6 +11,7 @@ > > #include <linux/kernel.h> > > #include <linux/slab.h> > > #include <linux/i2c.h> > > +#include <linux/irq.h> > > #include <linux/module.h> > > > > #include <linux/iio/iio.h> > > @@ -60,8 +61,6 @@ enum { > > /** > > * struct ad7150_chip_info - instance specific chip data > > * @client: i2c client for this device > > - * @current_event: device always has one type of event enabled. > > - * This element stores the event code of the current one. > > * @threshold: thresholds for simple capacitance value events > > * @thresh_sensitivity: threshold for simple capacitance offset > > * from 'average' value. > > @@ -70,21 +69,23 @@ enum { > > * value jumps to current value. Note made up of two fields, > > * 3:0 are for timeout receding - applies if below lower threshold > > * 7:4 are for timeout approaching - applies if above upper threshold > > - * @old_state: store state from previous event, allowing confirmation > > - * of new condition. > > - * @conversion_mode: the current conversion mode. > > * @state_lock: ensure consistent state of this structure wrt the > > * hardware. > > + * @interrupts: one or two interrupt numbers depending on device type. > > + * @int_enabled: is a given interrupt currently enabled. > > + * @type: threshold type > > + * @dir: threshold direction > > */ > > struct ad7150_chip_info { > > struct i2c_client *client; > > - u64 current_event; > > u16 threshold[2][2]; > > u8 thresh_sensitivity[2][2]; > > u8 thresh_timeout[2][2]; > > - int old_state; > > - char *conversion_mode; > > struct mutex state_lock; > > + int interrupts[2]; > > + bool int_enabled[2]; > > + enum iio_event_type type; > > + enum iio_event_direction dir; > > }; > > > > /* > > @@ -158,8 +159,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev, > > switch (type) { > > case IIO_EV_TYPE_THRESH_ADAPTIVE: > > if (dir == IIO_EV_DIR_RISING) > > - return !thrfixed && (threshtype == 0x3); > > - return !thrfixed && (threshtype == 0x2); > > + return !thrfixed && (threshtype == 0x1); > > + return !thrfixed && (threshtype == 0x0); > > case IIO_EV_TYPE_THRESH: > > if (dir == IIO_EV_DIR_RISING) > > return thrfixed && (threshtype == 0x1); > > @@ -179,11 +180,9 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev, > > { > > struct ad7150_chip_info *chip = iio_priv(indio_dev); > > int rising = (dir == IIO_EV_DIR_RISING); > > - u64 event_code; > > > > - event_code = IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, chan, type, dir); > > - > > - if (event_code != chip->current_event) > > + /* Only update value live, if parameter is in use */ > > + if ((type != chip->type) || (dir != chip->dir)) > > return 0; > > > > switch (type) { > > @@ -231,52 +230,91 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev, > > int ret; > > struct ad7150_chip_info *chip = iio_priv(indio_dev); > > int rising = (dir == IIO_EV_DIR_RISING); > > - u64 event_code; > > - > > - /* Something must always be turned on */ > > - if (!state) > > - return -EINVAL; > > > > - event_code = IIO_UNMOD_EVENT_CODE(chan->type, chan->channel, type, dir); > > - if (event_code == chip->current_event) > > + /* > > + * There is only a single shared control and no on chip > > + * interrupt disables for the two interrupt lines. > > + * So, enabling will switch the events configured to enable > > + * whatever was most recently requested and if necessary enable_irq() > > + * the interrupt and any disable will disable_irq() for that > > + * channels interrupt. > > + */ > > + if (!state) { > > + if ((chip->int_enabled[chan->channel]) && > > + (type == chip->type) && (dir == chip->dir)) { > > + disable_irq(chip->interrupts[chan->channel]); > > + chip->int_enabled[chan->channel] = false; > > + } > > return 0; > > + } > > + > > mutex_lock(&chip->state_lock); > > - ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG); > > - if (ret < 0) > > - goto error_ret; > > + if ((type != chip->type) || (dir != chip->dir)) { > > > > - cfg = ret & ~((0x03 << 5) | BIT(7)); > > + /* > > + * Need to temporarily disable both interrupts if > > + * enabled - this is to avoid races around changing > > + * config and thresholds. > > + * Note enable/disable_irq() are reference counted so > > + * no need to check if already enabled. > > + */ > > + disable_irq(chip->interrupts[0]); > > + disable_irq(chip->interrupts[1]); > > > > - switch (type) { > > - case IIO_EV_TYPE_THRESH_ADAPTIVE: > > - adaptive = 1; > > - if (rising) > > - thresh_type = 0x3; > > - else > > - thresh_type = 0x2; > > - break; > > - case IIO_EV_TYPE_THRESH: > > - adaptive = 0; > > - if (rising) > > - thresh_type = 0x1; > > - else > > - thresh_type = 0x0; > > - break; > > - default: > > - ret = -EINVAL; > > - goto error_ret; > > - } > > + ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG); > > + if (ret < 0) > > + goto error_ret; > > > > - cfg |= (!adaptive << 7) | (thresh_type << 5); > > + cfg = ret & ~((0x03 << 5) | BIT(7)); > > > > - ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg); > > - if (ret < 0) > > - goto error_ret; > > + switch (type) { > > + case IIO_EV_TYPE_THRESH_ADAPTIVE: > > + adaptive = 1; > > + if (rising) > > + thresh_type = 0x1; > > + else > > + thresh_type = 0x0; > > + break; > > + case IIO_EV_TYPE_THRESH: > > + adaptive = 0; > > + if (rising) > > + thresh_type = 0x1; > > + else > > + thresh_type = 0x0; > > this can be ignored; it's a minor nit; > the if (rising) {} else {} block looks duplicate; > it could be moved after the switch() block; If you are happy with rest I'll just roll that in whilst applying. > > > + break; > > + default: > > + ret = -EINVAL; > > + goto error_ret; > > + } > > > > - chip->current_event = event_code; > > + cfg |= (!adaptive << 7) | (thresh_type << 5); > > + > > + ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg); > > + if (ret < 0) > > + goto error_ret; > > + > > + /* > > + * There is a potential race condition here, but not easy > > + * to close given we can't disable the interrupt at the > > + * chip side of things. Rely on the status bit. > > + */ > > + chip->type = type; > > + chip->dir = dir; > > + > > + /* update control attributes */ > > + ret = ad7150_write_event_params(indio_dev, chan->channel, type, > > + dir); > > + if (ret) > > + goto error_ret; > > + /* reenable any irq's we disabled whilst changing mode */ > > + enable_irq(chip->interrupts[0]); > > + enable_irq(chip->interrupts[1]); > > i'm wondering if we need to set 'chip->int_enabled[ for 0 & 1 ] = true;' > tbh i am not sure if there is a chance of mismatch between > 'int_enabled' and the actual enable_irq() > > maybe it might make sense to add an ad7150_{enable/disable}_irq() > that checks the per-channel 'int_enabled' field; Ah. I'm playing a bit of a game here. (there is a comment at the irq_disable() calls that was meant to indicate what was going on). There is a difference between when chip->int_enabled[] is changed in conjunction with enable_irq()/disable_irq() and when enable_irq()/disable_irq() are change on their own. The intent is that int_enabled[] reflects non-transient state of whether the interrupt is enabled - I don't want to update it during transient states. We are fine to do the disable_irq() calls on irqs that aren't enabled because disable_irq() calls can be safely nested. Only when you have more enable_irq() calls (including startup if it's auto enabled) than you have disable_irq() calls will the interrupt be enabled. https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L771 https://elixir.bootlin.com/linux/latest/source/include/linux/irqdesc.h#L28 The depth field in irq_desc is used for this. > > > > + } > > + if (!chip->int_enabled[chan->channel]) { > > + enable_irq(chip->interrupts[chan->channel]); > > + chip->int_enabled[chan->channel] = true; > > + } > > > > - /* update control attributes */ > > - ret = ad7150_write_event_params(indio_dev, chan->channel, type, dir); > > error_ret: > > mutex_unlock(&chip->state_lock); > > > > @@ -434,59 +472,44 @@ static const struct iio_chan_spec ad7151_channels_no_irq[] = { > > AD7150_CAPACITANCE_CHAN_NO_IRQ(0), > > }; > > > > -static irqreturn_t ad7150_event_handler(int irq, void *private) > > +static irqreturn_t __ad7150_event_handler(void *private, u8 status_mask, > > + int channel) > > { > > struct iio_dev *indio_dev = private; > > struct ad7150_chip_info *chip = iio_priv(indio_dev); > > - u8 int_status; > > s64 timestamp = iio_get_time_ns(indio_dev); > > - int ret; > > + int int_status; > > > > - ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS); > > - if (ret < 0) > > + int_status = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS); > > + if (int_status < 0) > > return IRQ_HANDLED; > > > > - int_status = ret; > > - > > - if ((int_status & AD7150_STATUS_OUT1) && > > - !(chip->old_state & AD7150_STATUS_OUT1)) > > - iio_push_event(indio_dev, > > - IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, > > - 0, > > - IIO_EV_TYPE_THRESH, > > - IIO_EV_DIR_RISING), > > - timestamp); > > - else if ((!(int_status & AD7150_STATUS_OUT1)) && > > - (chip->old_state & AD7150_STATUS_OUT1)) > > - iio_push_event(indio_dev, > > - IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, > > - 0, > > - IIO_EV_TYPE_THRESH, > > - IIO_EV_DIR_FALLING), > > - timestamp); > > - > > - if ((int_status & AD7150_STATUS_OUT2) && > > - !(chip->old_state & AD7150_STATUS_OUT2)) > > - iio_push_event(indio_dev, > > - IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, > > - 1, > > - IIO_EV_TYPE_THRESH, > > - IIO_EV_DIR_RISING), > > - timestamp); > > - else if ((!(int_status & AD7150_STATUS_OUT2)) && > > - (chip->old_state & AD7150_STATUS_OUT2)) > > - iio_push_event(indio_dev, > > - IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, > > - 1, > > - IIO_EV_TYPE_THRESH, > > - IIO_EV_DIR_FALLING), > > - timestamp); > > - /* store the status to avoid repushing same events */ > > - chip->old_state = int_status; > > + /* > > + * There are race conditions around enabling and disabling that > > + * could easily land us here with a spurious interrupt. > > + * Just eat it if so. > > + */ > > + if (!(int_status & status_mask)) > > + return IRQ_HANDLED; > > + > > + iio_push_event(indio_dev, > > + IIO_UNMOD_EVENT_CODE(IIO_CAPACITANCE, channel, > > + chip->type, chip->dir), > > + timestamp); > > > > return IRQ_HANDLED; > > } > > > > +static irqreturn_t ad7150_event_handler_ch1(int irq, void *private) > > +{ > > + return __ad7150_event_handler(private, AD7150_STATUS_OUT1, 0); > > +} > > + > > +static irqreturn_t ad7150_event_handler_ch2(int irq, void *private) > > +{ > > + return __ad7150_event_handler(private, AD7150_STATUS_OUT2, 1); > > +} > > + > > static IIO_CONST_ATTR(in_capacitance_thresh_adaptive_timeout_available, > > "[0 0.01 0.15]"); > > > > @@ -533,12 +556,44 @@ static int ad7150_probe(struct i2c_client *client, > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > - if (client->irq) { > > + chip->interrupts[0] = fwnode_irq_get(dev_fwnode(&client->dev), 0); > > + if (chip->interrupts[0] < 0) > > + return chip->interrupts[0]; > > + if (id->driver_data == AD7150) { > > checking 'id->driver_data' will be flaky after the of_match_table is added; It's not, though the path is a bit obscure and only works if you don't use probe_new() - if using probe_new() you to call i2c_match_id() directly. I remember raising this in a review I did a few years back and chasing it down. I just sanity checked and it still works ;) So, even if we do an of_based registration the following happens of_i2c_register_device() -> of_i2c_get_board_info() // gets the of_node pointer -> i2c_new_client_device(adap, &info); .. some time later the i2c_bus_type.probe() gets called for the devices we added and -> i2c_device_probe() -> driver->probe(client, i2c_match_id(driver->id_table, client)) -> i2c_match_id() //matches against id table and hence gets us the driver_data. > either we defer the of_match_table patch, or we update it somehow; > maybe a chip_info struct is an idea; > otherwise we need to do void pointer to int conversions [which look nasty]; > > i'm also seeing that there is a 'type' field inside of_device_id that > is underused, but looks useful for cases like this; > i'm also a bit paranoid to use it; because it may not be compatible > with acpi_device_id We can refactor this at some point, but I don't think it is necessary to move the driver out of staging as a lot of drivers are doing things the same way we do it here. > > > + chip->interrupts[1] = fwnode_irq_get(dev_fwnode(&client->dev), 1); > > + if (chip->interrupts[1] < 0) > > + return chip->interrupts[1]; > > + } > > + if (chip->interrupts[0] && > > + (id->driver_data == AD7151 || chip->interrupts[1])) { > > + irq_set_status_flags(chip->interrupts[0], IRQ_NOAUTOEN); > > + ret = devm_request_threaded_irq(&client->dev, > > + chip->interrupts[0], > > + NULL, > > + &ad7150_event_handler_ch1, > > + IRQF_TRIGGER_RISING | > > + IRQF_ONESHOT, > > + "ad7150_irq1", > > + indio_dev); > > + if (ret) > > + return ret; > > + > > indio_dev->info = &ad7150_info; > > switch (id->driver_data) { > > case AD7150: > > indio_dev->channels = ad7150_channels; > > indio_dev->num_channels = ARRAY_SIZE(ad7150_channels); > > + irq_set_status_flags(chip->interrupts[1], IRQ_NOAUTOEN); > > + ret = devm_request_threaded_irq(&client->dev, > > + chip->interrupts[1], > > + NULL, > > + &ad7150_event_handler_ch2, > > + IRQF_TRIGGER_RISING | > > + IRQF_ONESHOT, > > + "ad7150_irq2", > > + indio_dev); > > + if (ret) > > + return ret; > > break; > > case AD7151: > > indio_dev->channels = ad7151_channels; > > @@ -548,25 +603,18 @@ static int ad7150_probe(struct i2c_client *client, > > return -EINVAL; > > } > > > > - ret = devm_request_threaded_irq(&client->dev, client->irq, > > - NULL, > > - &ad7150_event_handler, > > - IRQF_TRIGGER_RISING | > > - IRQF_ONESHOT, > > - "ad7150_irq1", > > - indio_dev); > > - if (ret) > > - return ret; > > } else { > > indio_dev->info = &ad7150_info_no_irq; > > switch (id->driver_data) { > > case AD7150: > > indio_dev->channels = ad7150_channels_no_irq; > > - indio_dev->num_channels = ARRAY_SIZE(ad7150_channels_no_irq); > > + indio_dev->num_channels = > > + ARRAY_SIZE(ad7150_channels_no_irq); > > break; > > case AD7151: > > indio_dev->channels = ad7151_channels_no_irq; > > - indio_dev->num_channels = ARRAY_SIZE(ad7151_channels_no_irq); > > + indio_dev->num_channels = > > + ARRAY_SIZE(ad7151_channels_no_irq); > > break; > > default: > > return -EINVAL; > > -- > > 2.30.2 > >
