| Message ID | 20240423084210.191987-5-ramona.gradinariu@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for adis16545/47 | expand |
On Tue, 23 Apr 2024 11:42:09 +0300 Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > The ADIS16545 and ADIS16547 are a complete inertial system that > includes a triaxis gyroscope and a triaxis accelerometer. > The serial peripheral interface (SPI) and register structure provide a > simple interface for data collection and configuration control. > > These devices are similar to the ones already supported in the driver, > with changes in the scales, timings and the max spi speed in burst > mode. > Also, they support delta angle and delta velocity readings in burst > mode, for which support was added in the trigger handler. > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> What is Nuno's relationship to this patch? You are author and the sender which is fine, but in that case you need to make Nuno's involvement explicit. Perhaps a Co-developed-by or similar is appropriate? > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> A few comments inline. Biggest one is I'd like a clear statement of why you can't do a burst of one type, then a burst of other. My guess is that the transition is very time consuming? If so, that is fine, but you should be able to let available_scan_masks handle the disjoint channel sets. From what I recall a similar case was why that got added in the first place. The uses in optimizing for devices that 'want' to grab lots of channels was a later use case. Jonathan > > static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc) > @@ -1200,7 +1355,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) > struct adis16480 *st = iio_priv(indio_dev); > struct adis *adis = &st->adis; > struct device *dev = &adis->spi->dev; > - int ret, bit, offset, i = 0; > + int ret, bit, offset, i = 0, buff_offset = 0; > __be16 *buffer; > u32 crc; > bool valid; > @@ -1233,8 +1388,8 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) > * 16-bit responses containing the BURST_ID depending on the sclk. If > * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ, > * we have only one. To manage that variation, we use the transition from the > - * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If > - * we not find this variation in the first 4 segments, then the data should > + * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3. > + * If we not find this variation in the first 4 segments, then the data should > * not be valid. > */ > buffer = adis->buffer; > @@ -1242,7 +1397,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) > u16 curr = be16_to_cpu(buffer[offset]); > u16 next = be16_to_cpu(buffer[offset + 1]); > > - if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) { > + if (curr == st->burst_id && next != st->burst_id) { > offset++; > break; > } > @@ -1269,11 +1424,22 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) > switch (bit) { > case ADIS16480_SCAN_TEMP: > st->data[i++] = buffer[offset + 1]; > + /* > + * The temperature channel has 16-bit storage size. > + * We need to perform the padding to have the buffer > + * elements naturally aligned in case there are any > + * 32-bit storage size channels enabled which have a > + * scan index higher than the temperature channel scan > + * index. > + */ > + if (*indio_dev->active_scan_mask & > + GENMASK(ADIS16480_SCAN_DELTVEL_Z, ADIS16480_SCAN_DELTANG_X)) > + st->data[i++] = 0; I think it is harmless to always do this. If there is no data after this channel then we are writing data that won't be copied anywhere. If there is data after it is needed. So I think you can drop the condition but do add a comment on it being necessary if there is data afterwards and harmless if there isn't. > break; > case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z: > /* The lower register data is sequenced first */ > - st->data[i++] = buffer[2 * bit + offset + 3]; > - st->data[i++] = buffer[2 * bit + offset + 2]; > + st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3]; > + st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2]; > break; > } > } > @@ -1285,10 +1451,38 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) > return IRQ_HANDLED; > } > > +static int adis16480_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + u16 en; > + int ret; > + struct adis16480 *st = iio_priv(indio_dev); > + > + if (st->chip_info->has_burst_delta_data) { > + if ((*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) && > + (*scan_mask & ADIS16545_BURST_DATA_SEL_1_CHN_MASK)) > + return -EINVAL; Use available scan_masks to enforce this. That can have mutually exclusive sets like you have here. However, what stops 2 burst reads - so do them one after the other if needed. You'd still want available_scan_masks to have the subsets though but added to that the combination of the two. > + if (*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) { > + en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 0); > + st->burst_id = 0xA5A5; Give the magic value a name via a define. > + } else { > + en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 1); > + st->burst_id = 0xC3C3; > + } > + > + ret = __adis_update_bits(&st->adis, ADIS16480_REG_CONFIG, > + ADIS16545_BURST_DATA_SEL_MASK, en); > + if (ret) > + return ret; > + } > + > + return adis_update_scan_mode(indio_dev, scan_mask); > +} > > @@ -1498,6 +1692,8 @@ static int adis16480_probe(struct spi_device *spi) > if (ret) > return ret; > > + st->burst_id = ADIS16495_BURST_ID; Why this default? Probably wants an explanatory comment. > +
On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > On Tue, 23 Apr 2024 11:42:09 +0300 > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > includes a triaxis gyroscope and a triaxis accelerometer. > > The serial peripheral interface (SPI) and register structure provide a > > simple interface for data collection and configuration control. > > > > These devices are similar to the ones already supported in the driver, > > with changes in the scales, timings and the max spi speed in burst > > mode. > > Also, they support delta angle and delta velocity readings in burst > > mode, for which support was added in the trigger handler. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > What is Nuno's relationship to this patch? You are author and the sender > which is fine, but in that case you need to make Nuno's involvement explicit. > Perhaps a Co-developed-by or similar is appropriate? > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > A few comments inline. Biggest one is I'd like a clear statement of why you > can't do a burst of one type, then a burst of other. My guess is that the > transition is very time consuming? If so, that is fine, but you should be > able > to let available_scan_masks handle the disjoint channel sets. Yeah, the burst message is a special spi transfer that brings you all of the channels data at once but for the accel/gyro you need to explicitly configure the chip to either give you the "normal vs "delta" readings. Re-configuring the chip and then do another burst would destroy performance I think. We could do the manual readings as we do in adis16475 for chips not supporting burst32. But in the burst32 case those manual readings should be minimal while in here we could have to do 6 of them which could also be very time consuming... Now, why we don't use available_scan_masks is something I can't really remember but this implementation goes in line with what we have in the adis16475 driver. - Nuno Sá
On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > On Tue, 23 Apr 2024 11:42:09 +0300 > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > includes a triaxis gyroscope and a triaxis accelerometer. > > The serial peripheral interface (SPI) and register structure provide a > > simple interface for data collection and configuration control. > > > > These devices are similar to the ones already supported in the driver, > > with changes in the scales, timings and the max spi speed in burst > > mode. > > Also, they support delta angle and delta velocity readings in burst > > mode, for which support was added in the trigger handler. > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > What is Nuno's relationship to this patch? You are author and the sender > which is fine, but in that case you need to make Nuno's involvement explicit. > Perhaps a Co-developed-by or similar is appropriate? > Ah and regarding this is that I guess Ramona just modified my internal patches adding support for these parts long ago (when they were not public yet). Not sure how much did she changed but a Co-developed-by looks the way to go... - Nuno Sá >
> -----Original Message----- > From: Nuno Sá <noname.nuno@gmail.com> > Sent: Monday, April 29, 2024 10:59 AM > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > <ramona.bolboaca13@gmail.com> > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org; > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > <Nuno.Sa@analog.com> > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families > > [External] > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > On Tue, 23 Apr 2024 11:42:09 +0300 > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > The serial peripheral interface (SPI) and register structure provide a > > > simple interface for data collection and configuration control. > > > > > > These devices are similar to the ones already supported in the driver, > > > with changes in the scales, timings and the max spi speed in burst > > > mode. > > > Also, they support delta angle and delta velocity readings in burst > > > mode, for which support was added in the trigger handler. > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > What is Nuno's relationship to this patch? You are author and the sender > > which is fine, but in that case you need to make Nuno's involvement explicit. > > Perhaps a Co-developed-by or similar is appropriate? > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > A few comments inline. Biggest one is I'd like a clear statement of why you > > can't do a burst of one type, then a burst of other. My guess is that the > > transition is very time consuming? If so, that is fine, but you should be > > able > > to let available_scan_masks handle the disjoint channel sets. > > Yeah, the burst message is a special spi transfer that brings you all of the > channels data at once but for the accel/gyro you need to explicitly configure > the chip to either give you the "normal vs "delta" readings. Re-configuring the > chip and then do another burst would destroy performance I think. We could > do > the manual readings as we do in adis16475 for chips not supporting burst32. > But > in the burst32 case those manual readings should be minimal while in here we > could have to do 6 of them which could also be very time consuming... > > Now, why we don't use available_scan_masks is something I can't really > remember > but this implementation goes in line with what we have in the adis16475 > driver. > > - Nuno Sá > Thank you Nuno for all the additional explanations. Regarding the use of available_scan_masks, the idea is to have any possible combination for accel, gyro, temp and timestamp channels or delta angle, delta velocity, temp and timestamp channels. There are a lot of combinations for this and it does not seem like a good idea to write them all manually. That is why adis16480_update_scan_mode is used for checking the correct channels selection. Ramona G.
On Mon, 29 Apr 2024 13:17:42 +0000 "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > -----Original Message----- > > From: Nuno Sá <noname.nuno@gmail.com> > > Sent: Monday, April 29, 2024 10:59 AM > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > <ramona.bolboaca13@gmail.com> > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org; > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > <Nuno.Sa@analog.com> > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families > > > > [External] > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > The serial peripheral interface (SPI) and register structure provide a > > > > simple interface for data collection and configuration control. > > > > > > > > These devices are similar to the ones already supported in the driver, > > > > with changes in the scales, timings and the max spi speed in burst > > > > mode. > > > > Also, they support delta angle and delta velocity readings in burst > > > > mode, for which support was added in the trigger handler. > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > What is Nuno's relationship to this patch? You are author and the sender > > > which is fine, but in that case you need to make Nuno's involvement explicit. > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > > A few comments inline. Biggest one is I'd like a clear statement of why you > > > can't do a burst of one type, then a burst of other. My guess is that the > > > transition is very time consuming? If so, that is fine, but you should be > > > able > > > to let available_scan_masks handle the disjoint channel sets. > > > > Yeah, the burst message is a special spi transfer that brings you all of the > > channels data at once but for the accel/gyro you need to explicitly configure > > the chip to either give you the "normal vs "delta" readings. Re-configuring the > > chip and then do another burst would destroy performance I think. We could > > do > > the manual readings as we do in adis16475 for chips not supporting burst32. > > But > > in the burst32 case those manual readings should be minimal while in here we > > could have to do 6 of them which could also be very time consuming... > > > > Now, why we don't use available_scan_masks is something I can't really > > remember > > but this implementation goes in line with what we have in the adis16475 > > driver. > > > > - Nuno Sá > > > > Thank you Nuno for all the additional explanations. > Regarding the use of available_scan_masks, the idea is to have any possible > combination for accel, gyro, temp and timestamp channels or delta angle, delta > velocity, temp and timestamp channels. There are a lot of combinations for > this and it does not seem like a good idea to write them all manually. That is > why adis16480_update_scan_mode is used for checking the correct channels > selection. If you are using bursts, the data is getting read anyway - which is the main cost here. The real question becomes what are you actually saving by supporting all the combinations of the the two subsets of channels in the pollfunc? Currently you have to pick the channels out and repack them, if pushing them all looks to me like a mempcy and a single value being set (unconditionally). Then it's a question of what the overhead of the channel demux in the core is. What you pass out of the driver via iio_push_to_buffers*() is not what ends up in the buffer if you allow the IIO core to do data demuxing for you - that is enabled by providing available_scan_masks. At buffer start up the demux code computes a fairly optimal set of copies to repack the incoming data to match with what channels the consumer (here probably the kfifo on the way to userspace) is expecting. That demux adds a small overhead but it should be small as long as the channels wanted aren't pathological (i.e. every other one). Advantage is the driver ends up simpler and in the common case of turn on all the channels (why else did you buy a device with those measurements if you didn't want them!) the demux is zerocopy so effectively free which is not going to be the case for the bitmap walk and element copy in the driver. Jonathan > > Ramona G.
On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote: > On Mon, 29 Apr 2024 13:17:42 +0000 > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > -----Original Message----- > > > From: Nuno Sá <noname.nuno@gmail.com> > > > Sent: Monday, April 29, 2024 10:59 AM > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > > <ramona.bolboaca13@gmail.com> > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org; > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > > <Nuno.Sa@analog.com> > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families > > > > > > [External] > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > > The serial peripheral interface (SPI) and register structure provide a > > > > > simple interface for data collection and configuration control. > > > > > > > > > > These devices are similar to the ones already supported in the driver, > > > > > with changes in the scales, timings and the max spi speed in burst > > > > > mode. > > > > > Also, they support delta angle and delta velocity readings in burst > > > > > mode, for which support was added in the trigger handler. > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > > What is Nuno's relationship to this patch? You are author and the sender > > > > which is fine, but in that case you need to make Nuno's involvement explicit. > > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > > > A few comments inline. Biggest one is I'd like a clear statement of why you > > > > can't do a burst of one type, then a burst of other. My guess is that the > > > > transition is very time consuming? If so, that is fine, but you should be > > > > able > > > > to let available_scan_masks handle the disjoint channel sets. > > > > > > Yeah, the burst message is a special spi transfer that brings you all of the > > > channels data at once but for the accel/gyro you need to explicitly configure > > > the chip to either give you the "normal vs "delta" readings. Re-configuring the > > > chip and then do another burst would destroy performance I think. We could > > > do > > > the manual readings as we do in adis16475 for chips not supporting burst32. > > > But > > > in the burst32 case those manual readings should be minimal while in here we > > > could have to do 6 of them which could also be very time consuming... > > > > > > Now, why we don't use available_scan_masks is something I can't really > > > remember > > > but this implementation goes in line with what we have in the adis16475 > > > driver. > > > > > > - Nuno Sá > > > > > > > Thank you Nuno for all the additional explanations. > > Regarding the use of available_scan_masks, the idea is to have any possible > > combination for accel, gyro, temp and timestamp channels or delta angle, delta > > velocity, temp and timestamp channels. There are a lot of combinations for > > this and it does not seem like a good idea to write them all manually. That is > > why adis16480_update_scan_mode is used for checking the correct channels > > selection. > > If you are using bursts, the data is getting read anyway - which is the main > cost here. The real question becomes what are you actually saving by supporting all > the combinations of the the two subsets of channels in the pollfunc? > Currently you have to pick the channels out and repack them, if pushing them all > looks to me like a mempcy and a single value being set (unconditionally). > Then it's a question of what the overhead of the channel demux in the core is. > What you pass out of the driver via iio_push_to_buffers*() > is not what ends up in the buffer if you allow the IIO core to do data demuxing > for you - that is enabled by providing available_scan_masks. At buffer > start up the demux code computes a fairly optimal set of copies to repack > the incoming data to match with what channels the consumer (here probably > the kfifo on the way to userspace) is expecting. > > That demux adds a small overhead but it should be small as long > as the channels wanted aren't pathological (i.e. every other one). > > Advantage is the driver ends up simpler and in the common case of turn > on all the channels (why else did you buy a device with those measurements > if you didn't want them!) the demux is zerocopy so effectively free which > is not going to be the case for the bitmap walk and element copy in the > driver. > Maybe my younger me was smarter but reading again the validation of the scan mask code (when available_scan_masks is available), I'm not sure why we're not using them. I think that having one mask with delta values + temperature and another one with normal + temperature would be enough for what we want in here. The code in adis16480_update_scan_mode() could then be simpler I think. Now, what I'm still not following is the straight memcpy(). I may be missing something but the demux code only appears to kick in when we have compound masks resulting of multiple buffers being enabled. So I'm not seeing how we can get away without picking the channels and place them correctly in the buffer passed to IIO? What we could do in the future (for a similar device) is to maybe have a fastpath in the handler. Something like: if (bitmap_full(scan_mask, masklength)) { memcpy(iio_buff, burst + data_off, size); goto push_to_iio; } Right now we would always have to do some "manual" work as the temperature scan index does not match the position on the received burst data. Some devices with the burst32 (which I think do not exist in this driver) would also make the plain memcpy() harder. - Nuno Sá
On Thu, 02 May 2024 13:31:55 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote: > > On Mon, 29 Apr 2024 13:17:42 +0000 > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > > > -----Original Message----- > > > > From: Nuno Sá <noname.nuno@gmail.com> > > > > Sent: Monday, April 29, 2024 10:59 AM > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > > > <ramona.bolboaca13@gmail.com> > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.org; > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > > > <Nuno.Sa@analog.com> > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families > > > > > > > > [External] > > > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > > > The serial peripheral interface (SPI) and register structure provide a > > > > > > simple interface for data collection and configuration control. > > > > > > > > > > > > These devices are similar to the ones already supported in the driver, > > > > > > with changes in the scales, timings and the max spi speed in burst > > > > > > mode. > > > > > > Also, they support delta angle and delta velocity readings in burst > > > > > > mode, for which support was added in the trigger handler. > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > What is Nuno's relationship to this patch? You are author and the sender > > > > > which is fine, but in that case you need to make Nuno's involvement explicit. > > > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > > > > A few comments inline. Biggest one is I'd like a clear statement of why you > > > > > can't do a burst of one type, then a burst of other. My guess is that the > > > > > transition is very time consuming? If so, that is fine, but you should be > > > > > able > > > > > to let available_scan_masks handle the disjoint channel sets. > > > > > > > > Yeah, the burst message is a special spi transfer that brings you all of the > > > > channels data at once but for the accel/gyro you need to explicitly configure > > > > the chip to either give you the "normal vs "delta" readings. Re-configuring the > > > > chip and then do another burst would destroy performance I think. We could > > > > do > > > > the manual readings as we do in adis16475 for chips not supporting burst32. > > > > But > > > > in the burst32 case those manual readings should be minimal while in here we > > > > could have to do 6 of them which could also be very time consuming... > > > > > > > > Now, why we don't use available_scan_masks is something I can't really > > > > remember > > > > but this implementation goes in line with what we have in the adis16475 > > > > driver. > > > > > > > > - Nuno Sá > > > > > > > > > > Thank you Nuno for all the additional explanations. > > > Regarding the use of available_scan_masks, the idea is to have any possible > > > combination for accel, gyro, temp and timestamp channels or delta angle, delta > > > velocity, temp and timestamp channels. There are a lot of combinations for > > > this and it does not seem like a good idea to write them all manually. That is > > > why adis16480_update_scan_mode is used for checking the correct channels > > > selection. > > > > If you are using bursts, the data is getting read anyway - which is the main > > cost here. The real question becomes what are you actually saving by supporting all > > the combinations of the the two subsets of channels in the pollfunc? > > Currently you have to pick the channels out and repack them, if pushing them all > > looks to me like a mempcy and a single value being set (unconditionally). > > > Then it's a question of what the overhead of the channel demux in the core is. > > What you pass out of the driver via iio_push_to_buffers*() > > is not what ends up in the buffer if you allow the IIO core to do data demuxing > > for you - that is enabled by providing available_scan_masks. At buffer > > start up the demux code computes a fairly optimal set of copies to repack > > the incoming data to match with what channels the consumer (here probably > > the kfifo on the way to userspace) is expecting. > > > > That demux adds a small overhead but it should be small as long > > as the channels wanted aren't pathological (i.e. every other one). > > > > Advantage is the driver ends up simpler and in the common case of turn > > on all the channels (why else did you buy a device with those measurements > > if you didn't want them!) the demux is zerocopy so effectively free which > > is not going to be the case for the bitmap walk and element copy in the > > driver. > > > > Maybe my younger me was smarter but reading again the validation of the scan mask > code (when available_scan_masks is available), I'm not sure why we're not using them. > I think that having one mask with delta values + temperature and another one with > normal + temperature would be enough for what we want in here. The code in > adis16480_update_scan_mode() could then be simpler I think. > > Now, what I'm still not following is the straight memcpy(). I may be missing > something but the demux code only appears to kick in when we have compound masks > resulting of multiple buffers being enabled. So I'm not seeing how we can get away > without picking the channels and place them correctly in the buffer passed to IIO? It runs whenever the enabled mask requested (the channels that are enabled) is different from the active_scan_mask. It only sends channels in one direction if there is only one user but it only sends the ones enabled by that consumer. It's called unconditionally from iio_enable_buffers() That iterates over all enabled buffers (often there is only 1) and then checks if the active scan mask is the same as the one for this buffer. https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006 The setup for whether to find a superset from available_scan_masks is here https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928 Key is that if it happens to match, then we don't actually do anything in the demux. It just gets passed straight on through. That does the fast path you mention below. > What we could do in the future (for a similar device) is to maybe have a fastpath in > the handler. Something like: > > if (bitmap_full(scan_mask, masklength)) { > memcpy(iio_buff, burst + data_off, size); > goto push_to_iio; > } > > Right now we would always have to do some "manual" work as the temperature scan index > does not match the position on the received burst data. Agreed -that one is needed to shuffle the temperature to the right place. > > Some devices with the burst32 (which I think do not exist in this driver) would also > make the plain memcpy() harder. > > - Nuno Sá >
On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote: > On Thu, 02 May 2024 13:31:55 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote: > > > On Mon, 29 Apr 2024 13:17:42 +0000 > > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Nuno Sá <noname.nuno@gmail.com> > > > > > Sent: Monday, April 29, 2024 10:59 AM > > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > > > > <ramona.bolboaca13@gmail.com> > > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > > > > robh@kernel.org; > > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > > > > <Nuno.Sa@analog.com> > > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 > > > > > families > > > > > > > > > > [External] > > > > > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > > > > The serial peripheral interface (SPI) and register structure > > > > > > > provide a > > > > > > > simple interface for data collection and configuration control. > > > > > > > > > > > > > > These devices are similar to the ones already supported in the > > > > > > > driver, > > > > > > > with changes in the scales, timings and the max spi speed in burst > > > > > > > mode. > > > > > > > Also, they support delta angle and delta velocity readings in > > > > > > > burst > > > > > > > mode, for which support was added in the trigger handler. > > > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > What is Nuno's relationship to this patch? You are author and the > > > > > > sender > > > > > > which is fine, but in that case you need to make Nuno's involvement > > > > > > explicit. > > > > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > > > > > A few comments inline. Biggest one is I'd like a clear statement of > > > > > > why you > > > > > > can't do a burst of one type, then a burst of other. My guess is > > > > > > that the > > > > > > transition is very time consuming? If so, that is fine, but you > > > > > > should be > > > > > > able > > > > > > to let available_scan_masks handle the disjoint channel sets. > > > > > > > > > > Yeah, the burst message is a special spi transfer that brings you all > > > > > of the > > > > > channels data at once but for the accel/gyro you need to explicitly > > > > > configure > > > > > the chip to either give you the "normal vs "delta" readings. Re- > > > > > configuring the > > > > > chip and then do another burst would destroy performance I think. We > > > > > could > > > > > do > > > > > the manual readings as we do in adis16475 for chips not supporting > > > > > burst32. > > > > > But > > > > > in the burst32 case those manual readings should be minimal while in > > > > > here we > > > > > could have to do 6 of them which could also be very time consuming... > > > > > > > > > > Now, why we don't use available_scan_masks is something I can't really > > > > > remember > > > > > but this implementation goes in line with what we have in the > > > > > adis16475 > > > > > driver. > > > > > > > > > > - Nuno Sá > > > > > > > > > > > > > Thank you Nuno for all the additional explanations. > > > > Regarding the use of available_scan_masks, the idea is to have any > > > > possible > > > > combination for accel, gyro, temp and timestamp channels or delta angle, > > > > delta > > > > velocity, temp and timestamp channels. There are a lot of combinations > > > > for > > > > this and it does not seem like a good idea to write them all manually. > > > > That is > > > > why adis16480_update_scan_mode is used for checking the correct channels > > > > selection. > > > > > > If you are using bursts, the data is getting read anyway - which is the > > > main > > > cost here. The real question becomes what are you actually saving by > > > supporting all > > > the combinations of the the two subsets of channels in the pollfunc? > > > Currently you have to pick the channels out and repack them, if pushing > > > them all > > > looks to me like a mempcy and a single value being set (unconditionally). > > > > > Then it's a question of what the overhead of the channel demux in the core > > > is. > > > What you pass out of the driver via iio_push_to_buffers*() > > > is not what ends up in the buffer if you allow the IIO core to do data > > > demuxing > > > for you - that is enabled by providing available_scan_masks. At buffer > > > start up the demux code computes a fairly optimal set of copies to repack > > > the incoming data to match with what channels the consumer (here probably > > > the kfifo on the way to userspace) is expecting. > > > > > > That demux adds a small overhead but it should be small as long > > > as the channels wanted aren't pathological (i.e. every other one). > > > > > > Advantage is the driver ends up simpler and in the common case of turn > > > on all the channels (why else did you buy a device with those measurements > > > if you didn't want them!) the demux is zerocopy so effectively free which > > > is not going to be the case for the bitmap walk and element copy in the > > > driver. > > > > > > > Maybe my younger me was smarter but reading again the validation of the scan > > mask > > code (when available_scan_masks is available), I'm not sure why we're not > > using them. > > I think that having one mask with delta values + temperature and another one > > with > > normal + temperature would be enough for what we want in here. The code in > > adis16480_update_scan_mode() could then be simpler I think. > > > > Now, what I'm still not following is the straight memcpy(). I may be missing > > something but the demux code only appears to kick in when we have compound > > masks > > resulting of multiple buffers being enabled. So I'm not seeing how we can > > get away > > without picking the channels and place them correctly in the buffer passed > > to IIO? > > It runs whenever the enabled mask requested (the channels that are enabled) is > different from the active_scan_mask. It only sends channels in one > direction if there is only one user but it only sends the ones enabled by that > consumer. > It's called unconditionally from iio_enable_buffers() > > That iterates over all enabled buffers (often there is only 1) > > and then checks if the active scan mask is the same as the one for this > buffer. > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006 > > The setup for whether to find a superset from available_scan_masks is here > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928 > > Key is that if it happens to match, then we don't actually do anything in the > demux. > It just gets passed straight on through. That does the fast path you mention > below. Ahh got it... May failure was not realizing that iio_scan_mask_match() returns the available masks so I was assuming that the bitmap_equal() check would only differ when multiple buffers are enabled. Oh well, I think then this should work... I'm not sure it will be more performant for the case where we don't enable all the channels because the demux is a linked list which is far from being a performance friend (maybe we can do some trials with something like xarray...). Still, for this to work the buffer we pass into IIO has to be bigger than it needs to be (so bigger memset()) because, AFAIU, we will have to pass on all the scan elements and, as I said, the burst data will either have delta or normal measurements (not both). I guess the code will still look simpler so if there's no visible difference in performance I'm fine with the change. I guess Ramona can give it a try to see how it looks like. - Nuno Sá >
On Fri, 03 May 2024 08:09:29 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: Resend as the to / cc entries were garbled. No idea why! > On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote: > > On Thu, 02 May 2024 13:31:55 +0200 > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote: > > > > On Mon, 29 Apr 2024 13:17:42 +0000 > > > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Nuno Sá <noname.nuno@gmail.com> > > > > > > Sent: Monday, April 29, 2024 10:59 AM > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > > > > > <ramona.bolboaca13@gmail.com> > > > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux- > > > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > > > > > robh@kernel.org; > > > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > > > > > <Nuno.Sa@analog.com> > > > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 > > > > > > families > > > > > > > > > > > > [External] > > > > > > > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system that > > > > > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > > > > > The serial peripheral interface (SPI) and register structure > > > > > > > > provide a > > > > > > > > simple interface for data collection and configuration control. > > > > > > > > > > > > > > > > These devices are similar to the ones already supported in the > > > > > > > > driver, > > > > > > > > with changes in the scales, timings and the max spi speed in burst > > > > > > > > mode. > > > > > > > > Also, they support delta angle and delta velocity readings in > > > > > > > > burst > > > > > > > > mode, for which support was added in the trigger handler. > > > > > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > > > What is Nuno's relationship to this patch? You are author and the > > > > > > > sender > > > > > > > which is fine, but in that case you need to make Nuno's involvement > > > > > > > explicit. > > > > > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > > > > > > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com> > > > > > > > A few comments inline. Biggest one is I'd like a clear statement of > > > > > > > why you > > > > > > > can't do a burst of one type, then a burst of other. My guess is > > > > > > > that the > > > > > > > transition is very time consuming? If so, that is fine, but you > > > > > > > should be > > > > > > > able > > > > > > > to let available_scan_masks handle the disjoint channel sets. > > > > > > > > > > > > Yeah, the burst message is a special spi transfer that brings you all > > > > > > of the > > > > > > channels data at once but for the accel/gyro you need to explicitly > > > > > > configure > > > > > > the chip to either give you the "normal vs "delta" readings. Re- > > > > > > configuring the > > > > > > chip and then do another burst would destroy performance I think. We > > > > > > could > > > > > > do > > > > > > the manual readings as we do in adis16475 for chips not supporting > > > > > > burst32. > > > > > > But > > > > > > in the burst32 case those manual readings should be minimal while in > > > > > > here we > > > > > > could have to do 6 of them which could also be very time consuming... > > > > > > > > > > > > Now, why we don't use available_scan_masks is something I can't really > > > > > > remember > > > > > > but this implementation goes in line with what we have in the > > > > > > adis16475 > > > > > > driver. > > > > > > > > > > > > - Nuno Sá > > > > > > > > > > > > > > > > Thank you Nuno for all the additional explanations. > > > > > Regarding the use of available_scan_masks, the idea is to have any > > > > > possible > > > > > combination for accel, gyro, temp and timestamp channels or delta angle, > > > > > delta > > > > > velocity, temp and timestamp channels. There are a lot of combinations > > > > > for > > > > > this and it does not seem like a good idea to write them all manually. > > > > > That is > > > > > why adis16480_update_scan_mode is used for checking the correct channels > > > > > selection. > > > > > > > > If you are using bursts, the data is getting read anyway - which is the > > > > main > > > > cost here. The real question becomes what are you actually saving by > > > > supporting all > > > > the combinations of the the two subsets of channels in the pollfunc? > > > > Currently you have to pick the channels out and repack them, if pushing > > > > them all > > > > looks to me like a mempcy and a single value being set (unconditionally). > > > > > > > Then it's a question of what the overhead of the channel demux in the core > > > > is. > > > > What you pass out of the driver via iio_push_to_buffers*() > > > > is not what ends up in the buffer if you allow the IIO core to do data > > > > demuxing > > > > for you - that is enabled by providing available_scan_masks. At buffer > > > > start up the demux code computes a fairly optimal set of copies to repack > > > > the incoming data to match with what channels the consumer (here probably > > > > the kfifo on the way to userspace) is expecting. > > > > > > > > That demux adds a small overhead but it should be small as long > > > > as the channels wanted aren't pathological (i.e. every other one). > > > > > > > > Advantage is the driver ends up simpler and in the common case of turn > > > > on all the channels (why else did you buy a device with those measurements > > > > if you didn't want them!) the demux is zerocopy so effectively free which > > > > is not going to be the case for the bitmap walk and element copy in the > > > > driver. > > > > > > > > > > Maybe my younger me was smarter but reading again the validation of the scan > > > mask > > > code (when available_scan_masks is available), I'm not sure why we're not > > > using them. > > > I think that having one mask with delta values + temperature and another one > > > with > > > normal + temperature would be enough for what we want in here. The code in > > > adis16480_update_scan_mode() could then be simpler I think. > > > > > > Now, what I'm still not following is the straight memcpy(). I may be missing > > > something but the demux code only appears to kick in when we have compound > > > masks > > > resulting of multiple buffers being enabled. So I'm not seeing how we can > > > get away > > > without picking the channels and place them correctly in the buffer passed > > > to IIO? > > > > It runs whenever the enabled mask requested (the channels that are enabled) is > > different from the active_scan_mask. It only sends channels in one > > direction if there is only one user but it only sends the ones enabled by that > > consumer. > > It's called unconditionally from iio_enable_buffers() > > > > That iterates over all enabled buffers (often there is only 1) > > > > and then checks if the active scan mask is the same as the one for this > > buffer. > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006 > > > > The setup for whether to find a superset from available_scan_masks is here > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928 > > > > Key is that if it happens to match, then we don't actually do anything in the > > demux. > > It just gets passed straight on through. That does the fast path you mention > > below. > > Ahh got it... May failure was not realizing that iio_scan_mask_match() returns > the available masks so I was assuming that the bitmap_equal() check would only > differ when multiple buffers are enabled. > > Oh well, I think then this should work... I'm not sure it will be more > performant for the case where we don't enable all the channels because the demux > is a linked list which is far from being a performance friend (maybe we can do > some trials with something like xarray...). Still, for this to work the buffer > we pass into IIO has to be bigger than it needs to be (so bigger memset()) > because, AFAIU, we will have to pass on all the scan elements and, as I said, > the burst data will either have delta or normal measurements (not both). I guess > the code will still look simpler so if there's no visible difference in > performance I'm fine with the change. I guess Ramona can give it a try to see > how it looks like. Would be interesting to look at the performance of that code, but the real question is what channels do users typically enabled. If they enabled a full set (and I suspect most do) then that code doesn't nothing at all. Original thinking was that the non common case didn't really matter much. If it is worth optimizing I'd suggest a double pass (or cheat - see later) 1st pass works out number of elements, 2nd just saves them in a nice linear array. It's typically small however, so maybe just allocate a linear array big enough for the worst case. Jonathan > > - Nuno Sá > > > >
On Fri, 2024-05-03 at 09:42 +0100, Jonathan Cameron wrote: > On Fri, 03 May 2024 08:09:29 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > Resend as the to / cc entries were garbled. No idea why! > > > On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote: > > > On Thu, 02 May 2024 13:31:55 +0200 > > > Nuno Sá <noname.nuno@gmail.com> wrote: > > > > > > > On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote: > > > > > On Mon, 29 Apr 2024 13:17:42 +0000 > > > > > "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Nuno Sá <noname.nuno@gmail.com> > > > > > > > Sent: Monday, April 29, 2024 10:59 AM > > > > > > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu > > > > > > > <ramona.bolboaca13@gmail.com> > > > > > > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; > > > > > > > linux- > > > > > > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net; > > > > > > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > > > > > > robh@kernel.org; > > > > > > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno > > > > > > > <Nuno.Sa@analog.com> > > > > > > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for > > > > > > > adis16545/7 > > > > > > > families > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote: > > > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300 > > > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote: > > > > > > > > > > > > > > > > > The ADIS16545 and ADIS16547 are a complete inertial system > > > > > > > > > that > > > > > > > > > includes a triaxis gyroscope and a triaxis accelerometer. > > > > > > > > > The serial peripheral interface (SPI) and register structure > > > > > > > > > provide a > > > > > > > > > simple interface for data collection and configuration > > > > > > > > > control. > > > > > > > > > > > > > > > > > > These devices are similar to the ones already supported in the > > > > > > > > > driver, > > > > > > > > > with changes in the scales, timings and the max spi speed in > > > > > > > > > burst > > > > > > > > > mode. > > > > > > > > > Also, they support delta angle and delta velocity readings in > > > > > > > > > burst > > > > > > > > > mode, for which support was added in the trigger handler. > > > > > > > > > > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > > > > > > > > > > > > What is Nuno's relationship to this patch? You are author and > > > > > > > > the > > > > > > > > sender > > > > > > > > which is fine, but in that case you need to make Nuno's > > > > > > > > involvement > > > > > > > > explicit. > > > > > > > > Perhaps a Co-developed-by or similar is appropriate? > > > > > > > > > > > > > > > > > Signed-off-by: Ramona Gradinariu > > > > > > > > > <ramona.gradinariu@analog.com> > > > > > > > > A few comments inline. Biggest one is I'd like a clear > > > > > > > > statement of > > > > > > > > why you > > > > > > > > can't do a burst of one type, then a burst of other. My guess > > > > > > > > is > > > > > > > > that the > > > > > > > > transition is very time consuming? If so, that is fine, but you > > > > > > > > should be > > > > > > > > able > > > > > > > > to let available_scan_masks handle the disjoint channel > > > > > > > > sets. > > > > > > > > > > > > > > Yeah, the burst message is a special spi transfer that brings you > > > > > > > all > > > > > > > of the > > > > > > > channels data at once but for the accel/gyro you need to > > > > > > > explicitly > > > > > > > configure > > > > > > > the chip to either give you the "normal vs "delta" readings. Re- > > > > > > > configuring the > > > > > > > chip and then do another burst would destroy performance I think. > > > > > > > We > > > > > > > could > > > > > > > do > > > > > > > the manual readings as we do in adis16475 for chips not supporting > > > > > > > burst32. > > > > > > > But > > > > > > > in the burst32 case those manual readings should be minimal while > > > > > > > in > > > > > > > here we > > > > > > > could have to do 6 of them which could also be very time > > > > > > > consuming... > > > > > > > > > > > > > > Now, why we don't use available_scan_masks is something I can't > > > > > > > really > > > > > > > remember > > > > > > > but this implementation goes in line with what we have in the > > > > > > > adis16475 > > > > > > > driver. > > > > > > > > > > > > > > - Nuno Sá > > > > > > > > > > > > > > > > > > > Thank you Nuno for all the additional explanations. > > > > > > Regarding the use of available_scan_masks, the idea is to have any > > > > > > possible > > > > > > combination for accel, gyro, temp and timestamp channels or delta > > > > > > angle, > > > > > > delta > > > > > > velocity, temp and timestamp channels. There are a lot of > > > > > > combinations > > > > > > for > > > > > > this and it does not seem like a good idea to write them all > > > > > > manually. > > > > > > That is > > > > > > why adis16480_update_scan_mode is used for checking the correct > > > > > > channels > > > > > > selection. > > > > > > > > > > If you are using bursts, the data is getting read anyway - which is > > > > > the > > > > > main > > > > > cost here. The real question becomes what are you actually saving by > > > > > supporting all > > > > > the combinations of the the two subsets of channels in the pollfunc? > > > > > Currently you have to pick the channels out and repack them, if > > > > > pushing > > > > > them all > > > > > looks to me like a mempcy and a single value being set > > > > > (unconditionally). > > > > > > > > > Then it's a question of what the overhead of the channel demux in the > > > > > core > > > > > is. > > > > > What you pass out of the driver via iio_push_to_buffers*() > > > > > is not what ends up in the buffer if you allow the IIO core to do data > > > > > demuxing > > > > > for you - that is enabled by providing available_scan_masks. At > > > > > buffer > > > > > start up the demux code computes a fairly optimal set of copies to > > > > > repack > > > > > the incoming data to match with what channels the consumer (here > > > > > probably > > > > > the kfifo on the way to userspace) is expecting. > > > > > > > > > > That demux adds a small overhead but it should be small as long > > > > > as the channels wanted aren't pathological (i.e. every other one). > > > > > > > > > > Advantage is the driver ends up simpler and in the common case of turn > > > > > on all the channels (why else did you buy a device with those > > > > > measurements > > > > > if you didn't want them!) the demux is zerocopy so effectively free > > > > > which > > > > > is not going to be the case for the bitmap walk and element copy in > > > > > the > > > > > driver. > > > > > > > > > > > > > Maybe my younger me was smarter but reading again the validation of the > > > > scan > > > > mask > > > > code (when available_scan_masks is available), I'm not sure why we're > > > > not > > > > using them. > > > > I think that having one mask with delta values + temperature and another > > > > one > > > > with > > > > normal + temperature would be enough for what we want in here. The code > > > > in > > > > adis16480_update_scan_mode() could then be simpler I think. > > > > > > > > Now, what I'm still not following is the straight memcpy(). I may be > > > > missing > > > > something but the demux code only appears to kick in when we have > > > > compound > > > > masks > > > > resulting of multiple buffers being enabled. So I'm not seeing how we > > > > can > > > > get away > > > > without picking the channels and place them correctly in the buffer > > > > passed > > > > to IIO? > > > > > > It runs whenever the enabled mask requested (the channels that are > > > enabled) is > > > different from the active_scan_mask. It only sends channels in one > > > direction if there is only one user but it only sends the ones enabled by > > > that > > > consumer. > > > It's called unconditionally from iio_enable_buffers() > > > > > > That iterates over all enabled buffers (often there is only 1) > > > > > > and then checks if the active scan mask is the same as the one for this > > > buffer. > > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006 > > > > > > The setup for whether to find a superset from available_scan_masks is here > > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928 > > > > > > Key is that if it happens to match, then we don't actually do anything in > > > the > > > demux. > > > It just gets passed straight on through. That does the fast path you > > > mention > > > below. > > > > Ahh got it... May failure was not realizing that iio_scan_mask_match() > > returns > > the available masks so I was assuming that the bitmap_equal() check would > > only > > differ when multiple buffers are enabled. > > > > Oh well, I think then this should work... I'm not sure it will be more > > performant for the case where we don't enable all the channels because the > > demux > > is a linked list which is far from being a performance friend (maybe we can > > do > > some trials with something like xarray...). Still, for this to work the > > buffer > > we pass into IIO has to be bigger than it needs to be (so bigger memset()) > > because, AFAIU, we will have to pass on all the scan elements and, as I > > said, > > the burst data will either have delta or normal measurements (not both). I > > guess > > the code will still look simpler so if there's no visible difference in > > performance I'm fine with the change. I guess Ramona can give it a try to > > see > > how it looks like. > > Would be interesting to look at the performance of that code, but the > real question is what channels do users typically enabled. If they enabled > a full set (and I suspect most do) then that code doesn't nothing at all. > The only channel that makes me doubt is the temperature one but yeah, I would also expect most users just enable them all... > Original thinking was that the non common case didn't really matter much. > If it is worth optimizing I'd suggest a double pass (or cheat - see later) > 1st pass works out number of elements, 2nd just saves them in a nice > linear array. It's typically small however, so maybe just allocate a linear > array big enough for the worst case. > Yeah, linear array should also be fine and likely simpler. Maybe if I'm bored at some point I'll run some experiments :) - Nuno Sá
> > If you are using bursts, the data is getting read anyway - which is the main > cost here. The real question becomes what are you actually saving by supporting all > the combinations of the the two subsets of channels in the pollfunc? > Currently you have to pick the channels out and repack them, if pushing them all > looks to me like a mempcy and a single value being set (unconditionally). I did not get a chance to look at this again until now. Unfortunately, a memcpy will not work. The current implementation is as follows: /* The lower register data is sequenced first */ st->data[i++] = buffer[2 * bit + offset + 3]; st->data[i++] = buffer[2 * bit + offset + 2]; The device first sends the 16LSB, then the next 16MSB in big endian format. So then I wonder, can we keep the same implementation logic? The code is implemented in the same manner for adis16475 driver which uses the same channels data packing approach. > > Then it's a question of what the overhead of the channel demux in the core is. > What you pass out of the driver via iio_push_to_buffers*() > is not what ends up in the buffer if you allow the IIO core to do data demuxing > for you - that is enabled by providing available_scan_masks. At buffer > start up the demux code computes a fairly optimal set of copies to repack > the incoming data to match with what channels the consumer (here probably > the kfifo on the way to userspace) is expecting. > > That demux adds a small overhead but it should be small as long > as the channels wanted aren't pathological (i.e. every other one). > > Advantage is the driver ends up simpler and in the common case of turn > on all the channels (why else did you buy a device with those measurements > if you didn't want them!) the demux is zerocopy so effectively free which > is not going to be the case for the bitmap walk and element copy in the > driver. > > Jonathan >
On Wed, 22 May 2024 12:01:21 +0000 "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote: > > > > If you are using bursts, the data is getting read anyway - which is the main > > cost here. The real question becomes what are you actually saving by supporting all > > the combinations of the the two subsets of channels in the pollfunc? > > Currently you have to pick the channels out and repack them, if pushing them all > > looks to me like a mempcy and a single value being set (unconditionally). > > I did not get a chance to look at this again until now. Unfortunately, a > memcpy will not work. > The current implementation is as follows: > /* The lower register data is sequenced first */ > st->data[i++] = buffer[2 * bit + offset + 3]; > st->data[i++] = buffer[2 * bit + offset + 2]; Ah. That's horrible... :( Thanks for pointing that out! > > The device first sends the 16LSB, then the next 16MSB in big endian > format. > > So then I wonder, can we keep the same implementation logic? The code > is implemented in the same manner for adis16475 driver which uses the > same channels data packing approach. Not much choice and given the need to handle a mixed endian stream you might as well do the packing here as well. So sure, keep the code as you have it. > > > > > Then it's a question of what the overhead of the channel demux in the core is. > > What you pass out of the driver via iio_push_to_buffers*() > > is not what ends up in the buffer if you allow the IIO core to do data demuxing > > for you - that is enabled by providing available_scan_masks. At buffer > > start up the demux code computes a fairly optimal set of copies to repack > > the incoming data to match with what channels the consumer (here probably > > the kfifo on the way to userspace) is expecting. > > > > That demux adds a small overhead but it should be small as long > > as the channels wanted aren't pathological (i.e. every other one). > > > > Advantage is the driver ends up simpler and in the common case of turn > > on all the channels (why else did you buy a device with those measurements > > if you didn't want them!) the demux is zerocopy so effectively free which > > is not going to be the case for the bitmap walk and element copy in the > > driver. > > > > Jonathan > > >
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c index 4adc2244a4ef..e0020b7b5fb5 100644 --- a/drivers/iio/imu/adis16480.c +++ b/drivers/iio/imu/adis16480.c @@ -132,6 +132,10 @@ #define ADIS16480_SYNC_MODE_MSK BIT(8) #define ADIS16480_SYNC_MODE(x) FIELD_PREP(ADIS16480_SYNC_MODE_MSK, x) +#define ADIS16545_BURST_DATA_SEL_0_CHN_MASK GENMASK(5, 0) +#define ADIS16545_BURST_DATA_SEL_1_CHN_MASK GENMASK(16, 11) +#define ADIS16545_BURST_DATA_SEL_MASK BIT(8) + struct adis16480_chip_info { unsigned int num_channels; const struct iio_chan_spec *channels; @@ -147,6 +151,7 @@ struct adis16480_chip_info { const unsigned int *filter_freqs; bool has_pps_clk_mode; bool has_sleep_cnt; + bool has_burst_delta_data; const struct adis_data adis_data; }; @@ -170,6 +175,7 @@ struct adis16480 { struct clk *ext_clk; enum adis16480_clock_mode clk_mode; unsigned int clk_freq; + u16 burst_id; /* Alignment needed for the timestamp */ __be16 data[ADIS16495_BURST_MAX_DATA] __aligned(8); }; @@ -882,6 +888,23 @@ static const struct iio_chan_spec adis16485_channels[] = { ADIS16480_DELTVEL_CHANNEL_NO_SCAN(Z), }; +static const struct iio_chan_spec adis16545_channels[] = { + ADIS16480_GYRO_CHANNEL(X), + ADIS16480_GYRO_CHANNEL(Y), + ADIS16480_GYRO_CHANNEL(Z), + ADIS16480_ACCEL_CHANNEL(X), + ADIS16480_ACCEL_CHANNEL(Y), + ADIS16480_ACCEL_CHANNEL(Z), + ADIS16480_TEMP_CHANNEL(), + ADIS16480_DELTANG_CHANNEL(X), + ADIS16480_DELTANG_CHANNEL(Y), + ADIS16480_DELTANG_CHANNEL(Z), + ADIS16480_DELTVEL_CHANNEL(X), + ADIS16480_DELTVEL_CHANNEL(Y), + ADIS16480_DELTVEL_CHANNEL(Z), + IIO_CHAN_SOFT_TIMESTAMP(13), +}; + enum adis16480_variant { ADIS16375, ADIS16480, @@ -894,6 +917,12 @@ enum adis16480_variant { ADIS16497_1, ADIS16497_2, ADIS16497_3, + ADIS16545_1, + ADIS16545_2, + ADIS16545_3, + ADIS16547_1, + ADIS16547_2, + ADIS16547_3 }; #define ADIS16480_DIAG_STAT_XGYRO_FAIL 0 @@ -975,6 +1004,12 @@ static const struct adis_timeout adis16495_1_timeouts = { .self_test_ms = 20, }; +static const struct adis_timeout adis16545_timeouts = { + .reset_ms = 315, + .sw_reset_ms = 270, + .self_test_ms = 35, +}; + static const struct adis16480_chip_info adis16480_chip_info[] = { [ADIS16375] = { .channels = adis16485_channels, @@ -1176,6 +1211,126 @@ static const struct adis16480_chip_info adis16480_chip_info[] = { ADIS16495_BURST_MAX_DATA * 2, 6000000), }, + [ADIS16545_1] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 20000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(125), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 8, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(360), + .deltvel_max_val = 100, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16545, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, + [ADIS16545_2] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 18000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(450), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 8, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(720), + .deltvel_max_val = 100, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16545, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, + [ADIS16545_3] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 20000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(2000), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 8, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(2160), + .deltvel_max_val = 100, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16545, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, + [ADIS16547_1] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 20000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(125), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 40, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(360), + .deltvel_max_val = 400, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16547, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, + [ADIS16547_2] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 18000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(450), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 40, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(720), + .deltvel_max_val = 400, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16547, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, + [ADIS16547_3] = { + .channels = adis16545_channels, + .num_channels = ARRAY_SIZE(adis16545_channels), + .gyro_max_val = 20000 << 16, + .gyro_max_scale = IIO_DEGREE_TO_RAD(2000), + .accel_max_val = IIO_M_S_2_TO_G(32000 << 16), + .accel_max_scale = 40, + .temp_scale = 7000, /* 7 milli degree Celsius */ + .deltang_max_val = IIO_DEGREE_TO_RAD(2160), + .deltvel_max_val = 400, + .int_clk = 4250000, + .max_dec_rate = 4250, + .filter_freqs = adis16495_def_filter_freqs, + .has_pps_clk_mode = true, + .has_burst_delta_data = true, + /* 20 elements of 16bits */ + .adis_data = ADIS16480_DATA(16547, &adis16545_timeouts, + ADIS16495_BURST_MAX_DATA * 2, + 3200000), + }, }; static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc) @@ -1200,7 +1355,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) struct adis16480 *st = iio_priv(indio_dev); struct adis *adis = &st->adis; struct device *dev = &adis->spi->dev; - int ret, bit, offset, i = 0; + int ret, bit, offset, i = 0, buff_offset = 0; __be16 *buffer; u32 crc; bool valid; @@ -1233,8 +1388,8 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) * 16-bit responses containing the BURST_ID depending on the sclk. If * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ, * we have only one. To manage that variation, we use the transition from the - * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If - * we not find this variation in the first 4 segments, then the data should + * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3. + * If we not find this variation in the first 4 segments, then the data should * not be valid. */ buffer = adis->buffer; @@ -1242,7 +1397,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) u16 curr = be16_to_cpu(buffer[offset]); u16 next = be16_to_cpu(buffer[offset + 1]); - if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) { + if (curr == st->burst_id && next != st->burst_id) { offset++; break; } @@ -1269,11 +1424,22 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) switch (bit) { case ADIS16480_SCAN_TEMP: st->data[i++] = buffer[offset + 1]; + /* + * The temperature channel has 16-bit storage size. + * We need to perform the padding to have the buffer + * elements naturally aligned in case there are any + * 32-bit storage size channels enabled which have a + * scan index higher than the temperature channel scan + * index. + */ + if (*indio_dev->active_scan_mask & + GENMASK(ADIS16480_SCAN_DELTVEL_Z, ADIS16480_SCAN_DELTANG_X)) + st->data[i++] = 0; break; case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z: /* The lower register data is sequenced first */ - st->data[i++] = buffer[2 * bit + offset + 3]; - st->data[i++] = buffer[2 * bit + offset + 2]; + st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3]; + st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2]; break; } } @@ -1285,10 +1451,38 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p) return IRQ_HANDLED; } +static int adis16480_update_scan_mode(struct iio_dev *indio_dev, + const unsigned long *scan_mask) +{ + u16 en; + int ret; + struct adis16480 *st = iio_priv(indio_dev); + + if (st->chip_info->has_burst_delta_data) { + if ((*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) && + (*scan_mask & ADIS16545_BURST_DATA_SEL_1_CHN_MASK)) + return -EINVAL; + if (*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) { + en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 0); + st->burst_id = 0xA5A5; + } else { + en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 1); + st->burst_id = 0xC3C3; + } + + ret = __adis_update_bits(&st->adis, ADIS16480_REG_CONFIG, + ADIS16545_BURST_DATA_SEL_MASK, en); + if (ret) + return ret; + } + + return adis_update_scan_mode(indio_dev, scan_mask); +} + static const struct iio_info adis16480_info = { .read_raw = &adis16480_read_raw, .write_raw = &adis16480_write_raw, - .update_scan_mode = adis_update_scan_mode, + .update_scan_mode = &adis16480_update_scan_mode, .debugfs_reg_access = adis_debugfs_reg_access, }; @@ -1498,6 +1692,8 @@ static int adis16480_probe(struct spi_device *spi) if (ret) return ret; + st->burst_id = ADIS16495_BURST_ID; + if (st->chip_info->has_sleep_cnt) { ret = devm_add_action_or_reset(dev, adis16480_stop, indio_dev); if (ret) @@ -1571,6 +1767,12 @@ static const struct spi_device_id adis16480_ids[] = { { "adis16497-1", ADIS16497_1 }, { "adis16497-2", ADIS16497_2 }, { "adis16497-3", ADIS16497_3 }, + { "adis16545-1", ADIS16545_1 }, + { "adis16545-2", ADIS16545_2 }, + { "adis16545-3", ADIS16545_3 }, + { "adis16547-1", ADIS16547_1 }, + { "adis16547-2", ADIS16547_2 }, + { "adis16547-3", ADIS16547_3 }, { } }; MODULE_DEVICE_TABLE(spi, adis16480_ids); @@ -1587,6 +1789,12 @@ static const struct of_device_id adis16480_of_match[] = { { .compatible = "adi,adis16497-1" }, { .compatible = "adi,adis16497-2" }, { .compatible = "adi,adis16497-3" }, + { .compatible = "adi,adis16545-1" }, + { .compatible = "adi,adis16545-2" }, + { .compatible = "adi,adis16545-3" }, + { .compatible = "adi,adis16547-1" }, + { .compatible = "adi,adis16547-2" }, + { .compatible = "adi,adis16547-3" }, { }, }; MODULE_DEVICE_TABLE(of, adis16480_of_match);