from:"Lars\-Peter Clausen"

Re: [RFC v2 PATCH 7/7] dmaengine: xilinx_dma: Program interrupt delay timeout

2021-04-15 Thread Lars-Peter Clausen


On 4/9/21 7:56 PM, Radhey Shyam Pandey wrote:

Program IRQDelay for AXI DMA. The interrupt timeout mechanism causes
the DMA engine to generate an interrupt after the delay time period
has expired. It enables dmaengine to respond in real-time even though
interrupt coalescing is configured. It also remove the placeholder
for delay interrupt and merge it with frame completion interrupt.
Since by default interrupt delay timeout is disabled this feature
addition has no functional impact on VDMA and CDMA IP's.


In my opinion this should not come from the devicetree. This setting is 
application specific and should be configured through a runtime API.


For the VDMA there is already xilinx_vdma_channel_set_config() which 
allows to configure the maximum number of IRQs that can be coalesced and 
the IRQ delay. Something similar is probably needed for the AXIDMA.




Signed-off-by: Radhey Shyam Pandey 
---
Changes for v2:
- Read irq delay timeout value from DT.
- Merge interrupt processing for frame done and delay interrupt.
---
  drivers/dma/xilinx/xilinx_dma.c | 20 +++-
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index a2ea2d649332..0c0dc9882a01 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -173,8 +173,10 @@
  #define XILINX_DMA_MAX_TRANS_LEN_MAX  23
  #define XILINX_DMA_V2_MAX_TRANS_LEN_MAX   26
  #define XILINX_DMA_CR_COALESCE_MAXGENMASK(23, 16)
+#define XILINX_DMA_CR_DELAY_MAXGENMASK(31, 24)
  #define XILINX_DMA_CR_CYCLIC_BD_EN_MASK   BIT(4)
  #define XILINX_DMA_CR_COALESCE_SHIFT  16
+#define XILINX_DMA_CR_DELAY_SHIFT  24
  #define XILINX_DMA_BD_SOP BIT(27)
  #define XILINX_DMA_BD_EOP BIT(26)
  #define XILINX_DMA_BD_COMP_MASK   BIT(31)
@@ -410,6 +412,7 @@ struct xilinx_dma_tx_descriptor {
   * @stop_transfer: Differentiate b/w DMA IP's quiesce
   * @tdest: TDEST value for mcdma
   * @has_vflip: S2MM vertical flip
+ * @irq_delay: Interrupt delay timeout
   */
  struct xilinx_dma_chan {
struct xilinx_dma_device *xdev;
@@ -447,6 +450,7 @@ struct xilinx_dma_chan {
int (*stop_transfer)(struct xilinx_dma_chan *chan);
u16 tdest;
bool has_vflip;
+   u8 irq_delay;
  };
  
  /**

@@ -1555,6 +1559,9 @@ static void xilinx_dma_start_transfer(struct 
xilinx_dma_chan *chan)
if (chan->has_sg)
xilinx_write(chan, XILINX_DMA_REG_CURDESC,
 head_desc->async_tx.phys);
+   reg  &= ~XILINX_DMA_CR_DELAY_MAX;
+   reg  |= chan->irq_delay << XILINX_DMA_CR_DELAY_SHIFT;
+   dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
  
  	xilinx_dma_start(chan);
  
@@ -1877,15 +1884,8 @@ static irqreturn_t xilinx_dma_irq_handler(int irq, void *data)

}
}
  
-	if (status & XILINX_DMA_DMASR_DLY_CNT_IRQ) {

-   /*
-* Device takes too long to do the transfer when user requires
-* responsiveness.
-*/
-   dev_dbg(chan->dev, "Inter-packet latency too long\n");
-   }
-
-   if (status & XILINX_DMA_DMASR_FRM_CNT_IRQ) {
+   if (status & (XILINX_DMA_DMASR_FRM_CNT_IRQ |
+ XILINX_DMA_DMASR_DLY_CNT_IRQ)) {
spin_lock(>lock);
xilinx_dma_complete_descriptor(chan);
chan->idle = true;
@@ -2802,6 +2802,8 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device 
*xdev,
/* Retrieve the channel properties from the device tree */
has_dre = of_property_read_bool(node, "xlnx,include-dre");
  
+	of_property_read_u8(node, "xlnx,irq-delay", >irq_delay);

+
chan->genlock = of_property_read_bool(node, "xlnx,genlock-mode");
  
  	err = of_property_read_u32(node, "xlnx,datawidth", );

Re: [RFC v2 PATCH 5/7] dmaengine: xilinx_dma: Freeup active list based on descriptor completion bit

2021-04-15 Thread Lars-Peter Clausen


On 4/9/21 7:56 PM, Radhey Shyam Pandey wrote:

AXIDMA IP in SG mode sets completion bit to 1 when the transfer is
completed. Read this bit to move descriptor from active list to the
done list. This feature is needed when interrupt delay timeout and
IRQThreshold is enabled i.e Dly_IrqEn is triggered w/o completing
interrupt threshold.

Signed-off-by: Radhey Shyam Pandey 
---
- Check BD completion bit only for SG mode.
- Modify the logic to have early return path.
---
  drivers/dma/xilinx/xilinx_dma.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 890bf46b36e5..f2305a73cb91 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -177,6 +177,7 @@
  #define XILINX_DMA_CR_COALESCE_SHIFT  16
  #define XILINX_DMA_BD_SOP BIT(27)
  #define XILINX_DMA_BD_EOP BIT(26)
+#define XILINX_DMA_BD_COMP_MASKBIT(31)
  #define XILINX_DMA_COALESCE_MAX   255
  #define XILINX_DMA_NUM_DESCS  512
  #define XILINX_DMA_NUM_APP_WORDS  5
@@ -1683,12 +1684,18 @@ static void xilinx_dma_issue_pending(struct dma_chan 
*dchan)
  static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
  {
struct xilinx_dma_tx_descriptor *desc, *next;
+   struct xilinx_axidma_tx_segment *seg;
  
  	/* This function was invoked with lock held */

if (list_empty(>active_list))
return;
  
  	list_for_each_entry_safe(desc, next, >active_list, node) {

+   /* TODO: remove hardcoding for axidma_tx_segment */
+   seg = list_last_entry(>segments,
+ struct xilinx_axidma_tx_segment, node);
+   if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
+   break;
if (chan->has_sg && chan->xdev->dma_config->dmatype !=
XDMA_TYPE_VDMA)
desc->residue = xilinx_dma_get_residue(chan, desc);


Since not all descriptors will be completed in this function the 
`chan->idle = true;` in xilinx_dma_irq_handler() needs to be gated on 
the active_list being empty.


xilinx_dma_complete_descriptor

Re: [RFC v2 PATCH 6/7] dmaengine: xilinx_dma: Use tasklet_hi_schedule for timing critical usecase

2021-04-15 Thread Lars-Peter Clausen


On 4/9/21 7:56 PM, Radhey Shyam Pandey wrote:

Schedule tasklet with high priority to ensure that callback processing
is prioritized. It improves throughput for netdev dma clients.

Do you have specific numbers on the throughput improvement?

Re: [RFC v2 PATCH 5/7] dmaengine: xilinx_dma: Freeup active list based on descriptor completion bit

2021-04-15 Thread Lars-Peter Clausen


On 4/9/21 7:56 PM, Radhey Shyam Pandey wrote:

AXIDMA IP in SG mode sets completion bit to 1 when the transfer is
completed. Read this bit to move descriptor from active list to the
done list. This feature is needed when interrupt delay timeout and
IRQThreshold is enabled i.e Dly_IrqEn is triggered w/o completing
interrupt threshold.

Signed-off-by: Radhey Shyam Pandey 
---
- Check BD completion bit only for SG mode.
- Modify the logic to have early return path.
---
  drivers/dma/xilinx/xilinx_dma.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 890bf46b36e5..f2305a73cb91 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -177,6 +177,7 @@
  #define XILINX_DMA_CR_COALESCE_SHIFT  16
  #define XILINX_DMA_BD_SOP BIT(27)
  #define XILINX_DMA_BD_EOP BIT(26)
+#define XILINX_DMA_BD_COMP_MASKBIT(31)
  #define XILINX_DMA_COALESCE_MAX   255
  #define XILINX_DMA_NUM_DESCS  512
  #define XILINX_DMA_NUM_APP_WORDS  5
@@ -1683,12 +1684,18 @@ static void xilinx_dma_issue_pending(struct dma_chan 
*dchan)
  static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
  {
struct xilinx_dma_tx_descriptor *desc, *next;
+   struct xilinx_axidma_tx_segment *seg;
  
  	/* This function was invoked with lock held */

if (list_empty(>active_list))
return;
  
  	list_for_each_entry_safe(desc, next, >active_list, node) {

+   /* TODO: remove hardcoding for axidma_tx_segment */
+   seg = list_last_entry(>segments,
+ struct xilinx_axidma_tx_segment, node);
This needs to be fixed before this can be merged as it right now will 
break the non AXIDMA variants.

+   if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
+   break;
if (chan->has_sg && chan->xdev->dma_config->dmatype !=
XDMA_TYPE_VDMA)
desc->residue = xilinx_dma_get_residue(chan, desc);

Re: [RFC v2 PATCH 0/7] Xilinx DMA enhancements and optimization

2021-04-15 Thread Lars-Peter Clausen


On 4/9/21 7:55 PM, Radhey Shyam Pandey wrote:

Some background about the patch series: Xilinx Axi Ethernet device driver
(xilinx_axienet_main.c) currently has axi-dma code inside it. The goal
is to refactor axiethernet driver and use existing AXI DMA driver using
DMAEngine API.


This is pretty neat! Do you have the patches that modify the AXI 
Ethernet driver in a public tree somewhere, so this series can be seen 
in context?

Re: [PATCH v4 2/2] iio: temperature: add driver support for ti tmp117

2021-04-09 Thread Lars-Peter Clausen


On 4/7/21 8:21 PM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated Non-Volatile memory.
Add support for tmp117 driver in iio subsystem.

Datasheet: https://www.ti.com/lit/gpn/tmp117
Signed-off-by: Puranjay Mohan 
Reviewed-by: Andy Shevchenko 


Looks good to me, thanks.

Reviewed-by: Lars-Peter Clausen

Re: [PATCH v3 2/2] iio: temperature: add driver support for ti tmp117

2021-04-06 Thread Lars-Peter Clausen


On 4/6/21 8:28 PM, Puranjay Mohan wrote:

+
+static int tmp117_write_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int val,
+   int val2, long mask)
+{
+   struct tmp117_data *data = iio_priv(indio_dev);
+   s16 off;
+
+   switch (mask) {
+   case IIO_CHAN_INFO_CALIBBIAS:
+   off = clamp(val, -32768, 32767);
+   if (off == data->calibbias)


data->calibbias is only set in probe() and always 0. I'm not sure we 
need to cache the value. Reading it back from the device seems fine.



+   return 0;
+   return i2c_smbus_write_word_swapped(data->client,
+   TMP117_REG_TEMP_OFFSET, off);
+
+   default:
+   return -EINVAL;
+   }
+}
+

Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117

2021-04-03 Thread Lars-Peter Clausen


On 4/3/21 4:58 PM, Puranjay Mohan wrote:

On Fri, Apr 2, 2021 at 1:43 PM Lars-Peter Clausen  wrote:

On 4/1/21 11:16 AM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.
Datasheet:-https://www.ti.com/lit/gpn/tmp117

Signed-off-by: Puranjay Mohan 

Nice and clean driver. Just some comments about the CALIBBIAS.


[...]
+#define TMP117_RESOLUTION_10UC   78125

Isn't the unit here 100 uC?

it is 7.8125 milli degrees_C so 78125 x 10^-4 milli degrees_C
which is 78125 x 10^-4 x 10^3 micro degrees_C
so it becomes 78125 x 10^-1 micro degrees_C = 78125 10_microdegrees_C.
Did it in detail so I remember it in the future. I guess you thought
it as 0.78125 millidegrees_C?
Ah, I get it, it is a tenth micro degree, not tens of micro degrees, 
sorry got confused.

[...]


I think that would be quite unexpected behavior. The unit should be the
same. Here in the read function you can just return the register value.

Okay, if you feel that would be right then I will do it.
Yea, I think reading and writing in different units would be a bit 
confusing.

Just make sure to properly sign extend like for the RAW property.


+ return IIO_VAL_INT_PLUS_MICRO;
[...]
+}
+
+static int tmp117_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int val,
+ int val2, long mask)
+{
+ struct tmp117_data *data = iio_priv(indio_dev);
+ s16 off;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBBIAS:
+ off = (s16)val;

This should have some input validation to avoid writing bogus values to
the register when the value is out of range. You can either reject out
of range values or clamp them into the valid range (using the clamp()
macro).

the maximum value which this register takes is 0x, so it should
get clamped automatically when casting to s16?
I might be wrong here.
Casting will truncate the upper bits. So something like 0x12345 gets 
turned into 0x2345.



+ return i2c_smbus_write_word_swapped(data->client,
+ TMP117_REG_TEMP_OFFSET, off);
+
+ default:
+ return -EINVAL;
+ }
+}
+

[...]

Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117

2021-04-02 Thread Lars-Peter Clausen


On 4/1/21 11:36 AM, Andy Shevchenko wrote:

[...]

+   case IIO_CHAN_INFO_SCALE:
+   /* Conversion from 10s of uC to mC
+* as IIO reports temperature in mC
+*/
+   *val = TMP117_RESOLUTION_10UC / 1;
+   *val2 = (TMP117_RESOLUTION_10UC % 1) * 100;
+   return IIO_VAL_INT_PLUS_MICRO;

You use 1 many times, can you give it an appropriate name (via #define)?

#define TENTHOUSAND 1 ;)

Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117

2021-04-02 Thread Lars-Peter Clausen


On 4/1/21 11:16 AM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.
Datasheet:-https://www.ti.com/lit/gpn/tmp117

Signed-off-by: Puranjay Mohan 


Nice and clean driver. Just some comments about the CALIBBIAS.


[...]
+#define TMP117_RESOLUTION_10UC 78125

Isn't the unit here 100 uC?

+#define TMP117_DEVICE_ID   0x0117
+
+struct tmp117_data {
+   struct i2c_client *client;
+};
+
+static int tmp117_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int *val,
+   int *val2, long mask)
+{
[...]
+   case IIO_CHAN_INFO_CALIBBIAS:
+   ret = i2c_smbus_read_word_swapped(data->client,
+   TMP117_REG_TEMP_OFFSET);
+   if (ret < 0)
+   return ret;
+   *val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC)
+   / 1;
+   *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC)
+   % 1;


If I understand this right CALBBIAS is written in one unit, but read in 
another unit. E.g. if you do `echo 100 > ..._calibbias` and then `cat 
..._calibbias` you'd read a different value back.


I think that would be quite unexpected behavior. The unit should be the 
same. Here in the read function you can just return the register value. 
Just make sure to properly sign extend like for the RAW property.



+   return IIO_VAL_INT_PLUS_MICRO;
[...]
+}
+
+static int tmp117_write_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int val,
+   int val2, long mask)
+{
+   struct tmp117_data *data = iio_priv(indio_dev);
+   s16 off;
+
+   switch (mask) {
+   case IIO_CHAN_INFO_CALIBBIAS:
+   off = (s16)val;
This should have some input validation to avoid writing bogus values to 
the register when the value is out of range. You can either reject out 
of range values or clamp them into the valid range (using the clamp() 
macro).

+   return i2c_smbus_write_word_swapped(data->client,
+   TMP117_REG_TEMP_OFFSET, off);
+
+   default:
+   return -EINVAL;
+   }
+}
+

[...]

Re: [PATCH 3/3] iio: adc: ad7923: register device with devm_iio_device_register

2021-03-29 Thread Lars-Peter Clausen


On 3/28/21 11:46 PM, Lucas Stankus wrote:

Registers the device using the devm variant.
This is the final step of converting the ad7923 to only use devm routines,
meaning that the ad7923_remove() function is no longer needed to release
resources on device detach.

Signed-off-by: Lucas Stankus 


Hi,

Thanks for the patches.T his looks good, just one small comment.


---
  drivers/iio/adc/ad7923.c | 12 +---
  1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7923.c b/drivers/iio/adc/ad7923.c
index d07eaf3111ed..f7af2f194789 100644
--- a/drivers/iio/adc/ad7923.c
+++ b/drivers/iio/adc/ad7923.c
@@ -356,16 +356,7 @@ static int ad7923_probe(struct spi_device *spi)
if (ret)
return ret;
  
-	return iio_device_register(indio_dev);

-}
-
-static int ad7923_remove(struct spi_device *spi)
-{
-   struct iio_dev *indio_dev = spi_get_drvdata(spi);


This removes the last user of get_drvdata() on the SPI device. This means you 
can also remove the spi_set_drvdata() in the probe function.



-
-   iio_device_unregister(indio_dev);
-
-   return 0;
+   return devm_iio_device_register(>dev, indio_dev);
  }

Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device

2021-03-27 Thread Lars-Peter Clausen


On 3/24/21 10:10 AM, Alexandru Ardelean wrote:

On Tue, Mar 23, 2021 at 1:35 PM Jonathan Cameron
 wrote:



[..]


Continuing a bit with the original IIO buffer ioctl(), I talked to
Lars a bit over IRC.
And there was an idea/suggestion to maybe use a struct to pass more
information to the buffer FD.

So, right now the ioctl() just returns an FD.
Would it be worth to extend this to a struct?
What I'm worried about is that it opens the discussion to add more
stuff to that struct.

so now, it would be:

struct iio_buffer_ioctl_data {
 __u32 fd;
 __u32 flags;   // flags for the new FD, which maybe we
could also pass via fcntl()
}

anything else that we would need?


I have a vague recollection that is is almost always worth adding
some padding to such ioctl data coming out of the kernel.  Gives
flexibility to safely add more stuff later without userspace
failing to allocate enough space etc.

I'm curious though, because this feels backwards. I'd expect the
flags to be more useful passed into the ioctl? i.e. request
a non blocking FD?  Might want to mirror that back again of course.




The struct can be used for both. Passing flags and buffer number in and fd out.


Personally, I don't know.
I don't have any experiences on this.

So, then I'll do a change to this ioctl() to use a struct.
We can probably add some reserved space?

struct iio_buffer_ioctl_data {
 __u32 fd;
 __u32 flags;
 __u32 reserved1;
 __u32 reserved2;
}


What to make sure of when using reserved fields is to check that they are 0. 
And reject the ioctl if they are not. This is the only way to ensure that old 
applications will continue to work if the struct is updated.




Lars was giving me some articles about ioctls.
One idea was to maybe consider making them multiples of 64 bits.

But reading through one of the docs here:
  
https://www.kernel.org/doc/html/latest/driver-api/ioctl.html#interface-versions
it discourages to do interface versions.

But I guess if we plan ahead with some reserved space, it might be
somewhat fine.

I'm still a little green on this stuff.

Re: [PATCH 2/2] iio: adc: Add support for TI INA260 power monitors

2021-03-22 Thread Lars-Peter Clausen

On 3/22/21 1:11 PM, Raviteja Narayanam wrote:

-Original Message-
From: Lars-Peter Clausen 
Sent: Monday, March 22, 2021 5:22 PM
To: Raviteja Narayanam ; robh...@kernel.org;
ji...@kernel.org
Cc: Michal Simek ; pme...@pmeerw.net; linux-
i...@vger.kernel.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; git ; Shubhrajyoti Datta

Subject: Re: [PATCH 2/2] iio: adc: Add support for TI INA260 power monitors

On 3/22/21 12:43 PM, Lars-Peter Clausen wrote:

On 3/22/21 11:50 AM, Raviteja Narayanam wrote:

This driver supports software buffer mode and raw reads of ina260 iio
channels.

In software buffer mode, a kthread will capture the active
scan_elements periodically using a delay. This can produce a stream
of up to 3 channels plus a 64 bits timestamp based on the scan_elements.

Signed-off-by: Raviteja Narayanam 
Signed-off-by: Shubhrajyoti Datta 

Hi,

Thanks for that patch, it looks really good.

Looking at the datasheet the part seems to be very similar to the
ina226 from a software interface point of view. Looks like the current
and calibration register are missing and the scales are a bit different.

We already have a driver for the ina226 (adc-ina2xx) and it looks like
your driver is also inspired by it. Have you considered adding support
for the ina260 to the existing driver? This will reduce the amount of
duplicated code.

To add to that this driver has some nice new features and bug fixes that are
not in the ina2xx driver, like debug register access. But the ina2xx has also
seen a fair amount of improvements since this driver was branched off of it.
It would be nice to get the best of both in a single driver.

Thanks for the review Lars.
Yes, using ina2xx driver was the first preference but after this discussion 
(https://www.spinics.net/lists/devicetree/msg354475.html ),
we have started a new driver as ina260 was considered to be sufficiently 
different from ina2xx.

Can you elaborate where you see the significant difference? When I look 
at the code the drivers are >90% identical.

I believe pretty much all we need to support the ina260 in the existing 
driver is to add a new iio_chan_spec and ina2xx_config for it. And then 
some minor tweaks to other parts of the driver, e.g. to make calibration 
optional. I believe this can be done in ~50 lines of code without making 
the structure of the driver more cluttered.

Regards,
Raviteja N

- Lars

---
   MAINTAINERS  |   8 +
   drivers/iio/adc/Kconfig  |  12 +
   drivers/iio/adc/Makefile |   1 +
   drivers/iio/adc/ina260-adc.c | 556
+++
   4 files changed, 577 insertions(+)
   create mode 100644 drivers/iio/adc/ina260-adc.c

diff --git a/MAINTAINERS b/MAINTAINERS index
aa84121c5611..768a4b148035 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8760,6 +8760,14 @@ F:    Documentation/hwmon/ina2xx.rst
   F:    drivers/hwmon/ina2xx.c
   F:    include/linux/platform_data/ina2xx.h
   +INA260 POWER MONITOR DRIVER
+M:    Raviteja Narayanam 
+R:    Shubhrajyoti Datta 
+R:    Michal Simek 
+S:    Maintained
+F:    Documentation/devicetree/bindings/iio/adc/ti,ina260.yaml
+F:    drivers/iio/adc/ina260-adc.c
+
   INDUSTRY PACK SUBSYSTEM (IPACK)
   M:    Samuel Iglesias Gonsalvez 
   M:    Jens Taprogge  diff --git
a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
bf7d22fa4be2..f6f8fd6bd113 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -497,6 +497,18 @@ config INA2XX_ADC
     Say yes here to build support for TI INA2xx family of Power
Monitors.
     This driver is mutually exclusive with the HWMON version.
   +config INA260_ADC
+    tristate "Texas Instruments INA260 Power Monitors IIO driver"
+    depends on I2C
+    select REGMAP_I2C
+    select IIO_BUFFER
+    select IIO_KFIFO_BUF
+    help
+  Say yes here to build support for TI INA260 power monitor.
+  This driver can also be built as a module. It supports
software buffer
+  mode and raw reads of ina260 iio channels. Up to 4 channels
+are
+  supported including timestamp.
+
   config INGENIC_ADC
   tristate "Ingenic JZ47xx SoCs ADC driver"
   depends on MIPS || COMPILE_TEST diff --git
a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
5fca90ada0ec..a3bbcad64a41 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_HI8435) += hi8435.o
   obj-$(CONFIG_HX711) += hx711.o
   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
   obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
+obj-$(CONFIG_INA260_ADC) += ina260-adc.o
   obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
   obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
   obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o diff --git
a/drivers/iio/adc/ina260-adc.c b/drivers/iio/adc/ina260-adc.c new
file mode 100644 index ..7f74aa94fd31
--- /dev/null
+++ b/drivers/iio/adc/ina260-adc.c
@@ -0,0 +1,556 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * INA260 power m

Re: [PATCH 2/2] iio: adc: Add support for TI INA260 power monitors

2021-03-22 Thread Lars-Peter Clausen


On 3/22/21 12:43 PM, Lars-Peter Clausen wrote:

On 3/22/21 11:50 AM, Raviteja Narayanam wrote:

This driver supports software buffer mode and raw reads of ina260
iio channels.

In software buffer mode, a kthread will capture the active scan_elements
periodically using a delay. This can produce a stream of up to 3
channels plus a 64 bits timestamp based on the scan_elements.

Signed-off-by: Raviteja Narayanam 
Signed-off-by: Shubhrajyoti Datta 


Hi,

Thanks for that patch, it looks really good.

Looking at the datasheet the part seems to be very similar to the 
ina226 from a software interface point of view. Looks like the current 
and calibration register are missing and the scales are a bit different.


We already have a driver for the ina226 (adc-ina2xx) and it looks like 
your driver is also inspired by it. Have you considered adding support 
for the ina260 to the existing driver? This will reduce the amount of 
duplicated code.


To add to that this driver has some nice new features and bug fixes that 
are not in the ina2xx driver, like debug register access. But the ina2xx 
has also seen a fair amount of improvements since this driver was 
branched off of it. It would be nice to get the best of both in a single 
driver.




- Lars


---
  MAINTAINERS  |   8 +
  drivers/iio/adc/Kconfig  |  12 +
  drivers/iio/adc/Makefile |   1 +
  drivers/iio/adc/ina260-adc.c | 556 +++
  4 files changed, 577 insertions(+)
  create mode 100644 drivers/iio/adc/ina260-adc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa84121c5611..768a4b148035 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8760,6 +8760,14 @@ F:    Documentation/hwmon/ina2xx.rst
  F:    drivers/hwmon/ina2xx.c
  F:    include/linux/platform_data/ina2xx.h
  +INA260 POWER MONITOR DRIVER
+M:    Raviteja Narayanam 
+R:    Shubhrajyoti Datta 
+R:    Michal Simek 
+S:    Maintained
+F:    Documentation/devicetree/bindings/iio/adc/ti,ina260.yaml
+F:    drivers/iio/adc/ina260-adc.c
+
  INDUSTRY PACK SUBSYSTEM (IPACK)
  M:    Samuel Iglesias Gonsalvez 
  M:    Jens Taprogge 
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index bf7d22fa4be2..f6f8fd6bd113 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -497,6 +497,18 @@ config INA2XX_ADC
    Say yes here to build support for TI INA2xx family of Power 
Monitors.

    This driver is mutually exclusive with the HWMON version.
  +config INA260_ADC
+    tristate "Texas Instruments INA260 Power Monitors IIO driver"
+    depends on I2C
+    select REGMAP_I2C
+    select IIO_BUFFER
+    select IIO_KFIFO_BUF
+    help
+  Say yes here to build support for TI INA260 power monitor.
+  This driver can also be built as a module. It supports 
software buffer

+  mode and raw reads of ina260 iio channels. Up to 4 channels are
+  supported including timestamp.
+
  config INGENIC_ADC
  tristate "Ingenic JZ47xx SoCs ADC driver"
  depends on MIPS || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 5fca90ada0ec..a3bbcad64a41 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_HI8435) += hi8435.o
  obj-$(CONFIG_HX711) += hx711.o
  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
+obj-$(CONFIG_INA260_ADC) += ina260-adc.o
  obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
  obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/ina260-adc.c b/drivers/iio/adc/ina260-adc.c
new file mode 100644
index ..7f74aa94fd31
--- /dev/null
+++ b/drivers/iio/adc/ina260-adc.c
@@ -0,0 +1,556 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * INA260 power monitor driver
+ * Based on drivers/iio/adc/ina2xx-adc.c
+ *
+ * Copyright (C) 2021 Xilinx, Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* INA260 registers definition */
+#define INA260_CONFIG    0x00
+#define INA260_CURRENT    0x01
+#define INA260_VOLTAGE    0x02
+#define INA260_POWER    0x03
+#define INA260_MASK_ENABLE    0x06
+#define INA260_ALERT_LIMIT    0x07
+#define INA260_MANF_ID    0xFE
+#define INA260_DIE_ID    0xFF
+
+#define INA260_CONFIG_DEFAULT    0x6327
+
+#define INA260_CURRENT_LSB    1250
+#define INA260_VOLTAGE_LSB    1250
+#define INA260_POWER_LSB    10
+
+/* Bits */
+#define INA260_CVRF    BIT(3)
+
+#define INA260_MODE_MASK    GENMASK(2, 0)
+#define INA260_VOLT_MASK    GENMASK(8, 6)
+#define INA260_SHIFT_VOLT(val)    ((val) << 6)
+#define INA260_CURR_MASK    GENMASK(5, 3)
+#define INA260_SHIFT_CURR(val)    ((val) << 3)
+#define INA260_AVG_MASK    GENMASK(11, 9)
+#define INA260_SHIFT_AVG(val)    ((val) << 9)
+
+#define SAMPLING_PERIOD(x) ({    \
+    typeof(x) _x = (x);    \
+    (_x->config->volt_conv

Re: [PATCH 2/2] iio: adc: Add support for TI INA260 power monitors

2021-03-22 Thread Lars-Peter Clausen


On 3/22/21 11:50 AM, Raviteja Narayanam wrote:

This driver supports software buffer mode and raw reads of ina260
iio channels.

In software buffer mode, a kthread will capture the active scan_elements
periodically using a delay. This can produce a stream of up to 3
channels plus a 64 bits timestamp based on the scan_elements.

Signed-off-by: Raviteja Narayanam 
Signed-off-by: Shubhrajyoti Datta 


Hi,

Thanks for that patch, it looks really good.

Looking at the datasheet the part seems to be very similar to the ina226 
from a software interface point of view. Looks like the current and 
calibration register are missing and the scales are a bit different.


We already have a driver for the ina226 (adc-ina2xx) and it looks like 
your driver is also inspired by it. Have you considered adding support 
for the ina260 to the existing driver? This will reduce the amount of 
duplicated code.


- Lars


---
  MAINTAINERS  |   8 +
  drivers/iio/adc/Kconfig  |  12 +
  drivers/iio/adc/Makefile |   1 +
  drivers/iio/adc/ina260-adc.c | 556 +++
  4 files changed, 577 insertions(+)
  create mode 100644 drivers/iio/adc/ina260-adc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa84121c5611..768a4b148035 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8760,6 +8760,14 @@ F:   Documentation/hwmon/ina2xx.rst
  F:drivers/hwmon/ina2xx.c
  F:include/linux/platform_data/ina2xx.h
  
+INA260 POWER MONITOR DRIVER

+M: Raviteja Narayanam 
+R: Shubhrajyoti Datta 
+R: Michal Simek 
+S: Maintained
+F: Documentation/devicetree/bindings/iio/adc/ti,ina260.yaml
+F: drivers/iio/adc/ina260-adc.c
+
  INDUSTRY PACK SUBSYSTEM (IPACK)
  M:Samuel Iglesias Gonsalvez 
  M:Jens Taprogge 
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index bf7d22fa4be2..f6f8fd6bd113 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -497,6 +497,18 @@ config INA2XX_ADC
  Say yes here to build support for TI INA2xx family of Power Monitors.
  This driver is mutually exclusive with the HWMON version.
  
+config INA260_ADC

+   tristate "Texas Instruments INA260 Power Monitors IIO driver"
+   depends on I2C
+   select REGMAP_I2C
+   select IIO_BUFFER
+   select IIO_KFIFO_BUF
+   help
+ Say yes here to build support for TI INA260 power monitor.
+ This driver can also be built as a module. It supports software buffer
+ mode and raw reads of ina260 iio channels. Up to 4 channels are
+ supported including timestamp.
+
  config INGENIC_ADC
tristate "Ingenic JZ47xx SoCs ADC driver"
depends on MIPS || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 5fca90ada0ec..a3bbcad64a41 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_HI8435) += hi8435.o
  obj-$(CONFIG_HX711) += hx711.o
  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
+obj-$(CONFIG_INA260_ADC) += ina260-adc.o
  obj-$(CONFIG_INGENIC_ADC) += ingenic-adc.o
  obj-$(CONFIG_INTEL_MRFLD_ADC) += intel_mrfld_adc.o
  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/ina260-adc.c b/drivers/iio/adc/ina260-adc.c
new file mode 100644
index ..7f74aa94fd31
--- /dev/null
+++ b/drivers/iio/adc/ina260-adc.c
@@ -0,0 +1,556 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * INA260 power monitor driver
+ * Based on drivers/iio/adc/ina2xx-adc.c
+ *
+ * Copyright (C) 2021 Xilinx, Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* INA260 registers definition */
+#define INA260_CONFIG  0x00
+#define INA260_CURRENT 0x01
+#define INA260_VOLTAGE 0x02
+#define INA260_POWER   0x03
+#define INA260_MASK_ENABLE 0x06
+#define INA260_ALERT_LIMIT 0x07
+#define INA260_MANF_ID 0xFE
+#define INA260_DIE_ID  0xFF
+
+#define INA260_CONFIG_DEFAULT  0x6327
+
+#define INA260_CURRENT_LSB 1250
+#define INA260_VOLTAGE_LSB 1250
+#define INA260_POWER_LSB   10
+
+/* Bits */
+#define INA260_CVRFBIT(3)
+
+#define INA260_MODE_MASK   GENMASK(2, 0)
+#define INA260_VOLT_MASK   GENMASK(8, 6)
+#define INA260_SHIFT_VOLT(val) ((val) << 6)
+#define INA260_CURR_MASK   GENMASK(5, 3)
+#define INA260_SHIFT_CURR(val) ((val) << 3)
+#define INA260_AVG_MASKGENMASK(11, 9)
+#define INA260_SHIFT_AVG(val)  ((val) << 9)
+
+#define SAMPLING_PERIOD(x) ({  \
+   typeof(x) _x = (x); \
+   (_x->config->volt_conv_time   \
+   + _x->config->curr_conv_time) \
+   * _x->config->avgs; })
+
+static bool ina260_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+   return (reg == INA260_CONFIG) || (reg == INA260_MASK_ENABLE) ||
+   (reg == INA260_ALERT_LIMIT);
+}
+
+static bool

Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117

2021-03-20 Thread Lars-Peter Clausen


On 3/21/21 6:07 AM, Lars-Peter Clausen wrote:

On 3/20/21 7:45 AM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.

Datasheet:-https://www.ti.com/lit/gpn/tmp117

Signed-off-by: Puranjay Mohan 


This looks good to me. Just two small bits I overlooked during the 
first review, sorry for that.



+};
+
[...]
+static int tmp117_read_raw(struct iio_dev *indio_dev,
+    struct iio_chan_spec const *channel, int *val,
+    int *val2, long mask)
+{
+    struct tmp117_data *data = iio_priv(indio_dev);
+    u16 tmp, off;
+
+    switch (mask) {
+    case IIO_CHAN_INFO_RAW:
+    tmp = tmp117_read_reg(data, TMP117_REG_TEMP);
+    *val = tmp;

No need for tmp here. Just directly assign to val.


Actually thinking about this, does the current implementation work 
correctly for negative temperatures?


I think there are two options to fix it. Either cast to s16 or use the 
sign_extend32() function.


Have a look at how the tmp006 driver handles this. It is a good example, 
including the error checking. In general you should check if your I2C 
read failed and return an error in that case rather than a bogus value 
for the measurement. Same for when reading the calibration offset.


Another thing. IIO reports temperature values in milli degrees Celsius. 
I believe in the current implementation the scale is so that it will 
report in degrees Celsius instead.

Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117

2021-03-20 Thread Lars-Peter Clausen


On 3/20/21 7:45 AM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.

Datasheet:-https://www.ti.com/lit/gpn/tmp117

Signed-off-by: Puranjay Mohan 


This looks good to me. Just two small bits I overlooked during the first 
review, sorry for that.



+};
+
[...]
+static int tmp117_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int *val,
+   int *val2, long mask)
+{
+   struct tmp117_data *data = iio_priv(indio_dev);
+   u16 tmp, off;
+
+   switch (mask) {
+   case IIO_CHAN_INFO_RAW:
+   tmp = tmp117_read_reg(data, TMP117_REG_TEMP);
+   *val = tmp;

No need for tmp here. Just directly assign to val.

+   return IIO_VAL_INT;
+
+   case IIO_CHAN_INFO_CALIBBIAS:
+   off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET);
+   *val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 1000;
+   *val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 1000;
+   return IIO_VAL_INT_PLUS_MICRO;
+
+   case IIO_CHAN_INFO_SCALE:
+   *val = 0;
+   *val2 = TMP117_SCALE;
+   return IIO_VAL_INT_PLUS_NANO;
+
+   default:
+   return -EINVAL;
+   }
+}
+
+static int tmp117_write_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int val,
+   int val2, long mask)
+{
+   struct tmp117_data *data = iio_priv(indio_dev);
+   u16 off;
+
+   switch (mask) {
+   case IIO_CHAN_INFO_CALIBBIAS:
+   off = ((val * 1000) + (val2 * 10))
+   / (int32_t)TMP117_RESOLUTION;


This needs some input validation. Writing a too large or too small value 
will cause an overflow/underflow and a bogus value will be written to 
the register.


You can either reject invalid values by returning -EINVAL or clamp them 
into the right range. Up to you how you want to handle this.



+   return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off);
+
+   default:
+   return -EINVAL;
+   }
+}

Re: [PATCH 2/2] iio: temperature: add driver support for ti tmp117

2021-03-19 Thread Lars-Peter Clausen


On 3/19/21 9:30 PM, Puranjay Mohan wrote:

TMP117 is a Digital temperature sensor with integrated NV memory.

Add support for tmp117 driver in iio subsystem.

Datasheet:-https://www.ti.com/lit/gpn/tmp117

Signed-off-by: Puranjay Mohan 


Hi,

Thanks for the patch, this looks really good. I have a couple of small 
comments inline.


And one general comment. What you have implemented as 
IIO_CHAN_INFO_OFFSET should be IIO_CHAN_INFO_CALIBBIAS, since it is an 
offset for calibration. The IIO_CHAN_INFO_OFFSET property on the other 
hand is used to describe how to convert from raw data to SI units.



---
  drivers/iio/temperature/Kconfig  |  11 ++
  drivers/iio/temperature/Makefile |   1 +
  drivers/iio/temperature/tmp117.c | 196 +++
  3 files changed, 208 insertions(+)
  create mode 100644 drivers/iio/temperature/tmp117.c

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 3f11ed870..200efc880 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -86,6 +86,17 @@ config TMP007
  This driver can also be built as a module. If so, the module will
  be called tmp007.
  
+config TMP117

+   tristate "TMP117 Digital temperature sensor with integrated NV memory"
+   depends on I2C
+   help
+ If you say yes here you get support for the Texas Instruments
+ TMP117 Digital temperature sensor with integrated NV memory.
+
+ This driver can also be built as a module. If so, the module will
+ be called tmp117.
+
+
  config TSYS01
tristate "Measurement Specialties TSYS01 temperature sensor using I2C bus 
connection"
depends on I2C
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index e4e0bad5a..7f2a95ed2 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_MLX90614) += mlx90614.o
  obj-$(CONFIG_MLX90632) += mlx90632.o
  obj-$(CONFIG_TMP006) += tmp006.o
  obj-$(CONFIG_TMP007) += tmp007.o
+obj-$(CONFIG_TMP117) += tmp117.o
  obj-$(CONFIG_TSYS01) += tsys01.o
  obj-$(CONFIG_TSYS02D) += tsys02d.o
  obj-$(CONFIG_LTC2983) += ltc2983.o
diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
new file mode 100644
index 0..15cdf590e
--- /dev/null
+++ b/drivers/iio/temperature/tmp117.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * tmp117.c - Digital temperature sensor with integrated NV memory
+ *
+ * Copyright (c) 2021 Puranjay Mohan 
+ *
+ * Driver for the Texas Instruments TMP117 Temperature Sensor
+ *
+ * (7-bit I2C slave address (0x48 - 0x4B), changeable via ADD pins)
+ *
+ * Note: This driver assumes that the sensor has been calibrated beforehand.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


interrupt.h is for consumers, irq.h is for providers. Since this driver 
is only a interrupt consumer you only need irq.h.


I believe bitops.h and delay.h are also not needed by this driver.


+
+#include 
+#include 
+#include 
+
+#define TMP117_REG_TEMP0x0
+#define TMP117_REG_CFGR0x1
+#define TMP117_REG_HIGH_LIM0x2
+#define TMP117_REG_LOW_LIM 0x3
+#define TMP117_REG_EEPROM_UL   0x4
+#define TMP117_REG_EEPROM1 0x5
+#define TMP117_REG_EEPROM2 0x6
+#define TMP117_REG_EEPROM3 0x7
+#define TMP117_REG_TEMP_OFFSET 0x7


You got register 0x7 twice here.


+#define TMP117_REG_EEPROM4 0x8
+#define TMP117_REG_DEVICE_ID   0xF
+
+#define TMP117_RESOLUTION  78125   /* in tens of uCelsius*/
+#define TMP117_RESOLUTION_DIV  1000
+
+#define TMP117_DEVICE_ID   0x0117
+
+struct tmp117_data {
+   struct i2c_client *client;
+   struct mutex lock;
+};
+
+static int tmp117_read_reg(struct tmp117_data *data, u8 reg)
+{
+
Being pedantic: the newline at the top here and also in write_reg are 
not needed.

+   return i2c_smbus_read_word_swapped(data->client, reg);
+}
+
+static int tmp117_write_reg(struct tmp117_data *data, u8 reg, int val)
+{
+
+   return i2c_smbus_write_word_swapped(data->client, reg, val);
+}
+
+static int tmp117_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *channel, int *val,
+   int *val2, long mask)
+{
+   struct tmp117_data *data = iio_priv(indio_dev);
+   u16 tmp, off;
+
+   switch (mask) {
+   case IIO_CHAN_INFO_PROCESSED:
+   tmp = tmp117_read_reg(data, TMP117_REG_TEMP);
+   *val = ((int16_t)tmp * (int32_t)TMP117_RESOLUTION) / 1000;
+   *val2 = ((int16_t)tmp * (int32_t)TMP117_RESOLUTION) % 1000;
Since this is a linear transform it would be better to report RAW 
measurements and then provide the scale attribute for conversion,.

+   return

Re: A potential data race in drivers/iio/adc/berlin2-adc.ko

2021-03-18 Thread Lars-Peter Clausen


On 3/18/21 9:27 AM, Lars-Peter Clausen wrote:

On 3/18/21 9:07 AM, Pavel Andrianov wrote:

Hi,

berlin2_adc_probe [1] registers two interrupt handlers: 
berlin2_adc_irq [2]
and berlin2_adc_tsen_irq [3]. The interrupt handlers operate with the 
same data, for example, modify

priv->data with different masks:

priv->data &= BERLIN2_SM_ADC_MASK;
and
priv->data &= BERLIN2_SM_TSEN_MASK;

If the two interrupt handlers are executed simultaneously, a 
potential data race takes place. So, the question is if the situation 
is possible. For example, in the case of the handlers are executed on 
different CPU cores.


Best regards,
Pavel

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L283 

[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L239 

[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L259


Looking at the code there are two functions. berlin2_adc_tsen_read() 
and berlin2_adc_read(). These two function are take the same mutex and 
can not run concurrently. At the beginning of the protected section 
the corresponding interrupt for that function is enabled and at the 
end disabled. So at least if the hardware works correctly those two 
interrupts will never fire at the same time.


Now, if the hardware misbehaves the two interrupts could still fire at 
the same time.


- Lars

Actually thinking a bit more about this the interrupt could still fire 
after it has been disabled since there is no synchronization between the 
disable and the interrupt handler. And the handler might be queued on 
one CPU, while the disable is running on another CPU.

Re: A potential data race in drivers/iio/adc/berlin2-adc.ko

2021-03-18 Thread Lars-Peter Clausen


On 3/18/21 9:07 AM, Pavel Andrianov wrote:

Hi,

berlin2_adc_probe [1] registers two interrupt handlers: 
berlin2_adc_irq [2]
and berlin2_adc_tsen_irq [3]. The interrupt handlers operate with the 
same data, for example, modify

priv->data with different masks:

priv->data &= BERLIN2_SM_ADC_MASK;
and
priv->data &= BERLIN2_SM_TSEN_MASK;

If the two interrupt handlers are executed simultaneously, a potential 
data race takes place. So, the question is if the situation is 
possible. For example, in the case of the handlers are executed on 
different CPU cores.


Best regards,
Pavel

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L283 

[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L239 

[3] 
https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/berlin2-adc.c#L259


Looking at the code there are two functions. berlin2_adc_tsen_read() and 
berlin2_adc_read(). These two function are take the same mutex and can 
not run concurrently. At the beginning of the protected section the 
corresponding interrupt for that function is enabled and at the end 
disabled. So at least if the hardware works correctly those two 
interrupts will never fire at the same time.


Now, if the hardware misbehaves the two interrupts could still fire at 
the same time.


- Lars

Re: [PATCH v2] iio:dac:max517.c: Use devm_iio_device_register()

2021-03-14 Thread Lars-Peter Clausen


On 3/14/21 6:57 PM, Mugilraj Dhavachelvan wrote:

Use devm_iio_device_register() to avoid remove function and
drop explicit call to iio_device_unregister().

Signed-off-by: Mugilraj Dhavachelvan 

changes v1->v2:
-As sugested by Alexandru removed i2c_set_clientdata() because the 
stored
 data will be never used.


Hi,

This looks good!

Reviewed-by: Lars-Peter Clausen 

One thing process wise. I know it is tempting to send version 2 as a 
reply to version 1, but this way it is also easy for the messages to get 
lost in longer threads. At least for the IIO mailinglist we have decided 
that it is best to send new versions of a patch series as their own 
threads so that they stand own and get noticed.


- Lars


---
  drivers/iio/dac/max517.c | 10 +-
  1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/iio/dac/max517.c b/drivers/iio/dac/max517.c
index 7e01838ef4d0..00f0062a0298 100644
--- a/drivers/iio/dac/max517.c
+++ b/drivers/iio/dac/max517.c
@@ -153,7 +153,6 @@ static int max517_probe(struct i2c_client *client,
if (!indio_dev)
return -ENOMEM;
data = iio_priv(indio_dev);
-   i2c_set_clientdata(client, indio_dev);
data->client = client;
  
  	/* establish that the iio_dev is a child of the i2c device */

@@ -189,13 +188,7 @@ static int max517_probe(struct i2c_client *client,
data->vref_mv[chan] = platform_data->vref_mv[chan];
}
  
-	return iio_device_register(indio_dev);

-}
-
-static int max517_remove(struct i2c_client *client)
-{
-   iio_device_unregister(i2c_get_clientdata(client));
-   return 0;
+   return devm_iio_device_register(>dev, indio_dev);
  }
  
  static const struct i2c_device_id max517_id[] = {

@@ -214,7 +207,6 @@ static struct i2c_driver max517_driver = {
.pm = _pm_ops,
},
.probe  = max517_probe,
-   .remove = max517_remove,
.id_table   = max517_id,
  };
  module_i2c_driver(max517_driver);

Re: [PATCH 02/23] ASoC: ad1836: remove useless return

2021-03-12 Thread Lars-Peter Clausen


On 3/12/21 7:22 PM, Pierre-Louis Bossart wrote:

Cppcheck warning:

sound/soc/codecs/ad1836.c:311:9: warning: Identical condition and return 
expression 'ret', return value is always 0 [identicalConditionAfterEarlyExit]
  return ret;
 ^
sound/soc/codecs/ad1836.c:308:6: note: If condition 'ret' is true, the function 
will return/exit
  if (ret)
  ^
sound/soc/codecs/ad1836.c:311:9: note: Returning identical expression 'ret'
  return ret;
 ^

Likely copy/paste between adc and dac cases.

Signed-off-by: Pierre-Louis Bossart 
---
  sound/soc/codecs/ad1836.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/sound/soc/codecs/ad1836.c b/sound/soc/codecs/ad1836.c
index a46152560294..08a5651bed9f 100644
--- a/sound/soc/codecs/ad1836.c
+++ b/sound/soc/codecs/ad1836.c
@@ -305,8 +305,6 @@ static int ad1836_probe(struct snd_soc_component *component)
return ret;
  
  	ret = snd_soc_dapm_add_routes(dapm, ad183x_adc_routes, num_adcs);

-   if (ret)
-   return ret;
  
  	return ret;

  }


We could even go one step further and do

return snd_soc_dapm_add_routes(dapm, ad183x_adc_routes, num_adcs);

But either is fine. Thanks for cleaning this up

Re: [PATCH 01/10] spi: spi-axi-spi-engine: remove usage of delay_usecs

2021-03-09 Thread Lars-Peter Clausen


On 3/10/21 8:16 AM, Alexandru Ardelean wrote:

On Mon, 8 Mar 2021 at 18:42, Lars-Peter Clausen  wrote:

On 3/8/21 3:54 PM, Alexandru Ardelean wrote:

The 'delay_usecs' field was handled for backwards compatibility in case
there were some users that still configured SPI delay transfers with
this field.

They should all be removed by now.

Signed-off-by: Alexandru Ardelean 
---
   drivers/spi/spi-axi-spi-engine.c | 12 
   1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index af86e6d6e16b..80c3e38f5c1b 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct 
spi_engine_program *p, bool dry,
   unsigned int t;
   int delay;

- if (xfer->delay_usecs) {
- delay = xfer->delay_usecs;
- } else {
- delay = spi_delay_to_ns(>delay, xfer);
- if (delay < 0)
- return;
- delay /= 1000;
- }
+ delay = spi_delay_to_ns(>delay, xfer);
+ if (delay < 0)
+ return;

Bit of a nit, but this could be `delay <= 0` and then drop the check for
`delay == 0` below.

hmm, that's a bit debatable, because the `delay == 0` check comes
after `delay /= 1000` ;
to do what you're suggesting, it would probably need a
DIV_ROUND_UP(delay, 1000) to make sure that even sub-microsecond
delays don't become zero;


Ah, true. Lets keep the code as it is.

On the other hand you could argue that we should round up to ensure the 
delay is at least as long as requested. But that is something that 
should be changed independently from this series.

Re: [PATCH 01/10] spi: spi-axi-spi-engine: remove usage of delay_usecs

2021-03-08 Thread Lars-Peter Clausen


On 3/8/21 3:54 PM, Alexandru Ardelean wrote:

The 'delay_usecs' field was handled for backwards compatibility in case
there were some users that still configured SPI delay transfers with
this field.

They should all be removed by now.

Signed-off-by: Alexandru Ardelean 
---
  drivers/spi/spi-axi-spi-engine.c | 12 
  1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index af86e6d6e16b..80c3e38f5c1b 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct 
spi_engine_program *p, bool dry,
unsigned int t;
int delay;
  
-	if (xfer->delay_usecs) {

-   delay = xfer->delay_usecs;
-   } else {
-   delay = spi_delay_to_ns(>delay, xfer);
-   if (delay < 0)
-   return;
-   delay /= 1000;
-   }
+   delay = spi_delay_to_ns(>delay, xfer);
+   if (delay < 0)
+   return;


Bit of a nit, but this could be `delay <= 0` and then drop the check for 
`delay == 0` below.



+   delay /= 1000;
  
  	if (delay == 0)

return;

Re: [PATCH] iio: buffer: fix use-after-free for attached_buffers array

2021-03-07 Thread Lars-Peter Clausen


On 3/7/21 1:36 PM, Jonathan Cameron wrote:

On Sat,  6 Mar 2021 18:47:10 +0200
Alexandru Ardelean  wrote:


Thanks to Lars for finding this.
The free of the 'attached_buffers' array should be done as late as
possible. This change moves it to iio_buffers_put(), which looks like
the best place for it, since it takes place right before the IIO device
data is free'd.

It feels a bit wrong to do direct freeing of stuff in a _put() call
given that kind of implies nothing will happen without some reference
count dropping to 0.  We could think about renaming the function to
something like

iio_buffers_put_and_free_array() but is a bit long winded.

Otherwise, I'm fine with this but want to let it sit on list a tiny bit
longer before I take it as it's not totally trivial unlike the previous
one.


Maybe to go with naming schema of iio_device_attach_buffer() call this 
function iio_device_detach_buffers(). We grab the reference in attach, 
and drop it in detach.


- Lars

Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device

2021-02-28 Thread Lars-Peter Clausen


On 2/28/21 3:34 PM, Jonathan Cameron wrote:

On Sun, 28 Feb 2021 09:51:38 +0100
Lars-Peter Clausen  wrote:


On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

With this change, an ioctl() call is added to open a character device for a
buffer. The ioctl() number is 'i' 0x91, which follows the
IIO_GET_EVENT_FD_IOCTL ioctl.

The ioctl() will return an FD for the requested buffer index. The indexes
are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
variable).

Since there doesn't seem to be a sane way to return the FD for buffer0 to
be the same FD for the /dev/iio:deviceX, this ioctl() will return another
FD for buffer0 (or the first buffer). This duplicate FD will be able to
access the same buffer object (for buffer0) as accessing directly the
/dev/iio:deviceX chardev.

Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
index for each buffer (and the count) can be deduced from the
'/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
bufferY folders).

Used following C code to test this:
---

   #include 
   #include 
   #include 
   #include 
   #include 

   #define IIO_BUFFER_GET_FD_IOCTL  _IOWR('i', 0x91, int)

int main(int argc, char *argv[])
{
  int fd;
  int fd1;
  int ret;

  if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
  fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
  return -1;
  }

  fprintf(stderr, "Using FD %d\n", fd);

  fd1 = atoi(argv[1]);

  ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, );
  if (ret < 0) {
  fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", 
fd1, ret, errno);
  close(fd);
  return -1;
  }

  fprintf(stderr, "Got FD %d\n", fd1);

  close(fd1);
  close(fd);

  return 0;
}
---

Results are:
---
   # ./test 0
   Using FD 3
   Got FD 4

   # ./test 1
   Using FD 3
   Got FD 4

   # ./test 2
   Using FD 3
   Got FD 4

   # ./test 3
   Using FD 3
   Got FD 4

   # ls /sys/bus/iio/devices/iio\:device0
   buffer  buffer0  buffer1  buffer2  buffer3  dev
   in_voltage_sampling_frequency  in_voltage_scale
   in_voltage_scale_available
   name  of_node  power  scan_elements  subsystem  uevent
---

iio:device0 has some fake kfifo buffers attached to an IIO device.

For me there is one major problem with this approach. We only allow one
application to open /dev/iio:deviceX at a time. This means we can't have
different applications access different buffers of the same device. I
believe this is a circuital feature.

Thats not quite true (I think - though I've not tested it).  What we don't
allow is for multiple processes to access them in an unaware fashion.
My assumption is we can rely on fork + fd passing via appropriate sockets.


It is possible to open the chardev, get the annonfd, close the chardev
and keep the annonfd open. Then the next application can do the same and
get access to a different buffer. But this has room for race conditions
when two applications try this at the very same time.

We need to somehow address this.

I'd count this as a bug :).  It could be safely done in a particular custom
system but in general it opens a can of worm.


I'm also not much of a fan of using ioctls to create annon fds. In part
because all the standard mechanisms for access control no longer work.

The inability to trivially have multiple processes open the anon fds
without care is one of the things I like most about them.

IIO drivers and interfaces really aren't designed for multiple unaware
processes to access them.  We don't have per process controls for device
wide sysfs attributes etc.  In general, it would be hard to
do due to the complexity of modeling all the interactions between the
different interfaces (events / buffers / sysfs access) in a generic fashion.

As such, the model, in my head at least, is that we only want a single
process to ever be responsible for access control.  That process can then
assign access to children or via a deliberate action (I think passing the
anon fd over a unix socket should work for example).  The intent being
that it is also responsible for mediating access to infrastructure that
multiple child processes all want to access.

As such, having one chrdev isn't a disadvantage because only one process
should ever open it at a time.  This same process also handles the
resource / control mediation.  Therefore we should only have one file
exposed for all the standard access control mechanisms.


Hm, I see your point, but I'm not convinced.

Having to have explicit synchroni

Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device

2021-02-28 Thread Lars-Peter Clausen


On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

With this change, an ioctl() call is added to open a character device for a
buffer. The ioctl() number is 'i' 0x91, which follows the
IIO_GET_EVENT_FD_IOCTL ioctl.

The ioctl() will return an FD for the requested buffer index. The indexes
are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
variable).

Since there doesn't seem to be a sane way to return the FD for buffer0 to
be the same FD for the /dev/iio:deviceX, this ioctl() will return another
FD for buffer0 (or the first buffer). This duplicate FD will be able to
access the same buffer object (for buffer0) as accessing directly the
/dev/iio:deviceX chardev.

Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
index for each buffer (and the count) can be deduced from the
'/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
bufferY folders).

Used following C code to test this:
---

  #include 
  #include 
  #include 
  #include 
  #include 

  #define IIO_BUFFER_GET_FD_IOCTL  _IOWR('i', 0x91, int)

int main(int argc, char *argv[])
{
 int fd;
 int fd1;
 int ret;

 if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
 fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
 return -1;
 }

 fprintf(stderr, "Using FD %d\n", fd);

 fd1 = atoi(argv[1]);

 ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, );
 if (ret < 0) {
 fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", 
fd1, ret, errno);
 close(fd);
 return -1;
 }

 fprintf(stderr, "Got FD %d\n", fd1);

 close(fd1);
 close(fd);

 return 0;
}
---

Results are:
---
  # ./test 0
  Using FD 3
  Got FD 4

  # ./test 1
  Using FD 3
  Got FD 4

  # ./test 2
  Using FD 3
  Got FD 4

  # ./test 3
  Using FD 3
  Got FD 4

  # ls /sys/bus/iio/devices/iio\:device0
  buffer  buffer0  buffer1  buffer2  buffer3  dev
  in_voltage_sampling_frequency  in_voltage_scale
  in_voltage_scale_available
  name  of_node  power  scan_elements  subsystem  uevent
---

iio:device0 has some fake kfifo buffers attached to an IIO device.


For me there is one major problem with this approach. We only allow one 
application to open /dev/iio:deviceX at a time. This means we can't have 
different applications access different buffers of the same device. I 
believe this is a circuital feature.


It is possible to open the chardev, get the annonfd, close the chardev 
and keep the annonfd open. Then the next application can do the same and 
get access to a different buffer. But this has room for race conditions 
when two applications try this at the very same time.


We need to somehow address this.

I'm also not much of a fan of using ioctls to create annon fds. In part 
because all the standard mechanisms for access control no longer work.

Re: [PATCH v6 19/24] iio: buffer: introduce support for attaching more IIO buffers

2021-02-28 Thread Lars-Peter Clausen


On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

  static ssize_t iio_show_scan_index(struct device *dev,
   struct device_attribute *attr,
   char *buf)
@@ -1451,11 +1465,13 @@ static void __iio_buffer_free_sysfs_and_mask(struct 
iio_buffer *buffer)
iio_free_chan_devattr_list(>buffer_attr_list);
  }
  
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)

+int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
  {
[...]
+error_unwind_sysfs_and_mask:
+   for (; unwind_idx >= 0; unwind_idx--) {
+   buffer = iio_dev_opaque->attached_buffers[unwind_idx];
+   __iio_buffer_free_sysfs_and_mask(buffer);
+   }
+   kfree(iio_dev_opaque->attached_buffers);
+   return ret;
  }
  
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)

+void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev)
  {
[...]
+   for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
+   buffer = iio_dev_opaque->attached_buffers[i];
+   __iio_buffer_free_sysfs_and_mask(buffer);
+   }
+
+   kfree(iio_dev_opaque->attached_buffers);
  }
[...]
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 1d500ea246af..f7f785431106 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1583,7 +1583,7 @@ static void iio_dev_release(struct device *device)
iio_device_unregister_eventset(indio_dev);
iio_device_unregister_sysfs(indio_dev);
  
-	iio_buffer_put(indio_dev->buffer);

+   iio_buffers_put(indio_dev);
We do call kfree(iio_dev_opaque->attached_buffers) before we get here. I 
think we need to keep the array around, otherwise we end of up with a 
use after free.

Re: [PATCH v6 02/24] iio: kfifo: add devm_iio_kfifo_buffer_setup() helper

2021-02-28 Thread Lars-Peter Clausen


On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

+int devm_iio_kfifo_buffer_setup(struct device *dev,
+   struct iio_dev *indio_dev,
+   int mode_flags,
+   const struct iio_buffer_setup_ops *setup_ops)
+{
+   struct iio_buffer *buffer;
+
+   if (mode_flags)
+   mode_flags &= kfifo_access_funcs.modes;


The if seems unnecessary. If it is 0 and we AND it with something it is 
still 0.

Re: [PATCH v6 20/24] iio: buffer: add ioctl() to support opening extra buffers for IIO device

2021-02-28 Thread Lars-Peter Clausen


On 2/15/21 11:40 AM, Alexandru Ardelean wrote:

[...]
  /**
   * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
   * @indio_dev: The IIO device
@@ -1343,6 +1371,96 @@ static void 
iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
  }
  
[...]

+static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long 
arg)
+{
+   struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+   int __user *ival = (int __user *)arg;
+   struct iio_dev_buffer_pair *ib;
+   struct iio_buffer *buffer;
+   int fd, idx, ret;
+
+   if (copy_from_user(, ival, sizeof(idx)))
+   return -EFAULT;


If we only want to pass an int, we can pass that directly, no need to 
pass it as a pointer.


int fd = arg;


+
+   if (idx >= iio_dev_opaque->attached_buffers_cnt)
+   return -ENODEV;
+
+   iio_device_get(indio_dev);
+
+   buffer = iio_dev_opaque->attached_buffers[idx];
+
+   if (test_and_set_bit(IIO_BUSY_BIT_POS, >flags)) {
+   ret = -EBUSY;
+   goto error_iio_dev_put;
+   }
+
+   ib = kzalloc(sizeof(*ib), GFP_KERNEL);
+   if (!ib) {
+   ret = -ENOMEM;
+   goto error_clear_busy_bit;
+   }
+
+   ib->indio_dev = indio_dev;
+   ib->buffer = buffer;
+
+   fd = anon_inode_getfd("iio:buffer", _buffer_chrdev_fileops,
+ ib, O_RDWR | O_CLOEXEC);


I wonder if we need to allow to pass flags, like e.g. O_NONBLOCK.

Something like 
https://elixir.bootlin.com/linux/latest/source/fs/signalfd.c#L288



+   if (fd < 0) {
+   ret = fd;
+   goto error_free_ib;
+   }
+
+   if (copy_to_user(ival, , sizeof(fd))) {
+   put_unused_fd(fd);
+   ret = -EFAULT;
+   goto error_free_ib;
+   }


Here we copy back the fd, but also return it. Just return is probably 
enough.



+
+   return fd;
+
+error_free_ib:
+   kfree(ib);
+error_clear_busy_bit:
+   clear_bit(IIO_BUSY_BIT_POS, >flags);
+error_iio_dev_put:
+   iio_device_put(indio_dev);
+   return ret;
+}
[...]
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index b6ebc04af3e7..32addd5e790e 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -9,6 +9,7 @@
   * @event_interface:  event chrdevs associated with interrupt lines
   * @attached_buffers: array of buffers statically attached by the 
driver
   * @attached_buffers_cnt: number of buffers in the array of statically 
attached buffers
+ * @buffer_ioctl_handler:  ioctl() handler for this IIO device's buffer 
interface
   * @buffer_list:  list of all buffers currently attached
   * @channel_attr_list:keep track of automatically created 
channel
   *attributes
@@ -28,6 +29,7 @@ struct iio_dev_opaque {
struct iio_event_interface  *event_interface;
struct iio_buffer   **attached_buffers;
unsigned intattached_buffers_cnt;
+   struct iio_ioctl_handler*buffer_ioctl_handler;


Can we just embedded this struct so we do not have to 
allocate/deallocate it?



struct list_headbuffer_list;
struct list_headchannel_attr_list;
struct attribute_group  chan_attr_group;

Re: [PATCH v2 5/5] iio: dac: ad5686: Add PWM as a trigger source

2021-02-23 Thread Lars-Peter Clausen


On 2/18/21 3:05 PM, Jonathan Cameron wrote:

On Wed, 17 Feb 2021 10:34:38 +0200
Alexandru Ardelean  wrote:


From: Mircea Caprioru 

A PWM signal will be used as a trigger source to have a deterministic
sampling frequency since this family of DAC has no hardware interrupt
source.

This feature is made optional however, as there are some board setups where
this isn't used.


So this is taking a very generic setup, but then implementing it
as a bit of a hack within the driver.

It's effectively a PWM connected up to an instance
of iio/triggers/iio-trig-interrupt.c

Now, I've not looked at that trigger driver for a while, so you may well
need to figure out how to add a binding to instantiate it.
(looks like no one has used it since board file days, or via instantiation
from another driver).

It's a slightly odd corner case as what it reflects is that we have
an interrupt available that is intended to drive some sort of data
capture or output (it's a trigger signal) - but exactly what is done
is a runtime configurable.  In this particular case that interrupt
is hooked up to a PWM and we also want to represent that.

The fact it's being driven via a PWM is interesting but we should be
able to extend that trigger driver to optionally accept a pwm provider
and if it has one provide frequency control.

Binding might look something like the following..

interrupt-trigger {
interrupts = <>;
pwms = < 0 4000 PWM_POLARITY_INVERTED>;   
};

@Rob, what do you think of this odd beast?

So all in all, this generic facility needs a generic implementation, not
one buried in a driver.

Another open question here is whether you really can't just use an hrtimer
to get similar precision?  Way back at the dawn of time in IIO we had
code to use the RTC periodic ticks as a trigger with the theory that they
would give very precise and even timing.  In the end it turned out that
hrtimers worked just as well (and RTCs drivers emulated the periodic
ticks via hrtimers, dropping their use of the hardware periodic timers).

The way this DAC works is that it has a "latch" pin and some shadow 
registers. The way this is supposed to be used is that you update the 
shadow registers and then when the there is a rising edge on the latch 
pin all the shadow register values are transferred to DAC output registers.


This means if you hook up a periodic signal like a PWM or clock to the 
latch pin you can generate very precise waveforms that have much lower 
jitter than when using a hrtimer since there is no variable interrupt 
latency for the update step itself. This is useful when generating 
periodic signals.


But you could for example also use a GPIO to update multiple discrete 
DACs at the same time.


This is not specific to this particular chip. There are quite a few ADI 
(and probably from other vendors) precision DACs that have this 
functionality. I agree that this should be a some sort of generic 
trigger helper module.


Now for the implementation since there is a direct connection between 
the PWM and the DAC I think it makes sense to describe this connection 
in the DT. After all if there is no connection this will not work.


As for the interrupt, most PWM controllers do have the ability to 
generate an IRQ by themselves once per period. There should be not need 
for a hardware loopback. Unfortunately the PWM framework does not have a 
mechanism yet to expose those IRQs and register a callback.


A similar feature btw exists for many of the ADCs and we did have this 
special Blackfin PWM trigger[1] back in the day to support this. The 
bfin PWM trigger driver essentially implements what I'm describing 
above, but without using the PWM framework.


- Lars

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/iio/trigger/iio-trig-bfin-timer.c?h=v3.15

Re: Adding custom functional callbacks to IIO driver

2021-02-12 Thread Lars-Peter Clausen


On 2/12/21 12:07 PM, Anand Ashok Dumbre wrote:

Hello,

I have an IIO adc driver that measures temperatures and voltages on the SOC.
There are other kernel modules interested in the temperature and voltage event 
information.

Would using a custom callback registration mechanism be advisable?
Is there something similar done already in IIO that can be leveraged?


Hi,

Have a look at the IIO consumer API that allows other kernel modules to 
subscribe to the output of an IIO device. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/iio/consumer.h 



- Lars

Re: [PATCH v2] dt-bindings: iio: dac: Fix AD5686 references

2021-02-02 Thread Lars-Peter Clausen


On 2/2/21 10:55 PM, Rob Herring wrote:

The example and filename use 'adi,ad5686', but the schema doesn't
document it. The AD5686 is also a SPI interface variant while all the
documented variants have an I2C interface. So let's update all the
references to AD5686 to AD5696.

Cc: Lars-Peter Clausen 
Cc: Michael Hennerich 
Cc: Jonathan Cameron 
Cc: Peter Meerwald-Stadler 
Cc: Michael Auchter 
Cc: linux-...@vger.kernel.org
Signed-off-by: Rob Herring 


Acked-by: Lars-Peter Clausen 

Thanks Rob.


---
v2:
- Rename instead of adding AD5686

  .../iio/dac/{adi,ad5686.yaml => adi,ad5696.yaml}   | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)
  rename Documentation/devicetree/bindings/iio/dac/{adi,ad5686.yaml => 
adi,ad5696.yaml} (77%)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml 
b/Documentation/devicetree/bindings/iio/dac/adi,ad5696.yaml
similarity index 77%
rename from Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml
rename to Documentation/devicetree/bindings/iio/dac/adi,ad5696.yaml
index 8065228e5df8..56b0cda0f30a 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5696.yaml
@@ -1,16 +1,16 @@
  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
  %YAML 1.2
  ---
-$id: http://devicetree.org/schemas/iio/dac/adi,ad5686.yaml#
+$id: http://devicetree.org/schemas/iio/dac/adi,ad5696.yaml#
  $schema: http://devicetree.org/meta-schemas/core.yaml#
  
-title: Analog Devices AD5686 and similar multi-channel DACs

+title: Analog Devices AD5696 and similar multi-channel DACs
  
  maintainers:

- Michael Auchter 
  
  description: |

-  Binding for Analog Devices AD5686 and similar multi-channel DACs
+  Binding for Analog Devices AD5696 and similar multi-channel DACs
  
  properties:

compatible:
@@ -48,8 +48,8 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
  
-  ad5686: dac@0 {

-compatible = "adi,ad5686";
+  ad5696: dac@0 {
+compatible = "adi,ad5696";
  reg = <0>;
  vcc-supply = <_vref>;
};

Re: [PATCH] dt-bindings: iio: dac: Add missing ad5686 compatible strings

2021-02-02 Thread Lars-Peter Clausen


On 2/2/21 7:14 PM, Rob Herring wrote:

The example uses 'adi,ad5686', but the schema fails to document it. Given
the filename and there is a similar part AD5686, let's just add the
compatible strings including the 'r' variant.


There are two variants of this chip. One with a SPI interface and one 
with a I2C interface. This binding document only describes the I2C 
variants. But the ad5686 is a SPI variant.


I think this is a typo and we should replace ad5686 with ad5696, 
including the document name.




Cc: Lars-Peter Clausen 
Cc: Michael Hennerich 
Cc: Jonathan Cameron 
Cc: Peter Meerwald-Stadler 
Cc: Michael Auchter 
Cc: linux-...@vger.kernel.org
Signed-off-by: Rob Herring 
---
  Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml 
b/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml
index 8065228e5df8..190919291828 100644
--- a/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5686.yaml
@@ -19,6 +19,8 @@ properties:
- adi,ad5338r
- adi,ad5671r
- adi,ad5675r
+  - adi,ad5686
+  - adi,ad5686r
- adi,ad5691r
- adi,ad5692r
- adi,ad5693

Re: dmaengine : xilinx_dma two issues

2021-01-11 Thread Lars-Peter Clausen


On 1/11/21 10:32 AM, Michal Simek wrote:

Hi Lars,

On 10. 01. 21 16:43, Lars-Peter Clausen wrote:

On 1/10/21 4:16 PM, Paul Thomas wrote:

On Fri, Jan 8, 2021 at 1:36 PM Radhey Shyam Pandey
 wrote:

-Original Message-
From: Paul Thomas 
Sent: Friday, January 8, 2021 9:27 PM
To: Radhey Shyam Pandey 
Cc: Dan Williams ; Vinod Koul
; Michal Simek ; Matthew Murrian
; Romain Perier
; Krzysztof Kozlowski ; Marc
Ferland ; Sebastian von Ohr
; dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel ; dave.ji...@intel.com; Shravya Kumbham
; git 
Subject: Re: dmaengine : xilinx_dma two issues

Hi All,

On Fri, Jan 8, 2021 at 2:13 AM Radhey Shyam Pandey 
wrote:

-Original Message-
From: Radhey Shyam Pandey
Sent: Monday, January 4, 2021 10:50 AM
To: Paul Thomas ; Dan Williams
; Vinod Koul ; Michal
Simek ; Matthew Murrian
; Romain Perier
; Krzysztof Kozlowski ;
Marc Ferland ; Sebastian von Ohr
; dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel ; Shravya Kumbham ; git

Subject: RE: dmaengine : xilinx_dma two issues


-Original Message-
From: Paul Thomas 
Sent: Monday, December 28, 2020 10:14 AM
To: Dan Williams ; Vinod Koul
; Michal Simek ; Radhey
Shyam Pandey ; Matthew Murrian
; Romain Perier

;

Krzysztof Kozlowski ; Marc Ferland
; Sebastian von Ohr ;
dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel 
Subject: dmaengine : xilinx_dma two issues

Hello,

I'm trying to get the 5.10 kernel up and running for our system,
and I'm running into a couple of issues with xilinx_dma.

+ (Xilinx mailing list)

Thanks for bringing the issues to our notice. Replies inline.


First, commit 14ccf0aab46e 'dmaengine: xilinx_dma: In dma channel
probe fix node order dependency' breaks our usage. Before this
commit a

call to:

dma_request_chan(_dev->dev, "axi_dma_0"); returns fine, but
after that commit it returns -19. The reason for this seems to be
that the only channel that is setup is channel 1 (chan->id is 1 in

xilinx_dma_chan_probe()).

However in
of_dma_xilinx_xlate() chan_id is gets set to 0 (int chan_id =
dma_spec-

args[0];), which causes the:

!xdev->chan[chan_id]
test to fail in of_dma_xilinx_xlate()

What is the channel number passed in dmaclient DT?

Is this a question for me?

Yes, please also share the dmaclient DT client node. Need to see
channel number passed to dmas property. Something like below-

dmas = <& axi_dma_0 1>
dma-names = "axi_dma_0"

OK, I think I need to revisit this and clean it up some. Currently In
the driver (a custom iio adc driver) it is hard coded:
dma_request_chan(_dev->dev, "axi_dma_0");

However, the DT also has the entries (currently unused by the driver):
  dmas = <_dma_0 0>;
  dma-names = "axi_dma_0";

I'll go back and clean up our driver to do something like
adi-axi-adc.c does:

  if (!device_property_present(dev, "dmas"))
  return 0;

  if (device_property_read_string(dev, "dma-names", _name))
  dma_name = "axi_dma_0";

Should the dmas node get used by the driver? I see the second argument
is: '0' for write/tx and '1' for read/rx channel. So I should be
setting this to 1 like this?
  dmas = <_dma_0 1>;
  dma-names = "axi_dma_0";

But where does that field get used?

This got broken in "dmaengine: xilinx_dma: In dma channel probe fix node
order dependency"
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=14ccf0aab46e1888e2f45b6e995c621c70b32651>.
Before if there was only one channel that channel was always at index 0.
Regardless of whether the channel was RX or TX. But after that change
the RX channel is always at offset 1, regardless of whether the DMA has
one or two channels. This is a breakage in ABI.

If you have the choice I'd recommend to not use the Xilinx DMA, it gets
broken pretty much every other release.

I expect that you are talking about Xilinx releases and I hope that this
has changed over times when most of changes are upstreamed already. The
patch above you are referencing has been applied by Vinod and he is
checking patches a lot. If there is a problem and any breakage it needs
to be fixed. And bugs happen all the time and we have a way how to work
with it.


I don't know if it has gotten better. When I upgrade to a new release 
what takes up most of the time is figuring out why the Xilinx DMA 
doesn't work anymore. Its been like this for years.



If you see there any issue please report them and let's fix them and
continue on this topic from technical point of view.
In connection to this problem what are you suggesting? Just revert this
patch or fix ordering differently? Would be good to provide your
suggestion and fix it.


Reverting would re-introduce the issue the patch was supposed to fix.

The would have been to use index 0 for the channel if there is only one 
channel. If there are two channels use 0 for T

Re: dmaengine : xilinx_dma two issues

2021-01-10 Thread Lars-Peter Clausen

On 1/10/21 4:16 PM, Paul Thomas wrote:

On Fri, Jan 8, 2021 at 1:36 PM Radhey Shyam Pandey  wrote:

-Original Message-
From: Paul Thomas 
Sent: Friday, January 8, 2021 9:27 PM
To: Radhey Shyam Pandey 
Cc: Dan Williams ; Vinod Koul
; Michal Simek ; Matthew Murrian
; Romain Perier
; Krzysztof Kozlowski ; Marc
Ferland ; Sebastian von Ohr
; dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel ; dave.ji...@intel.com; Shravya Kumbham
; git 
Subject: Re: dmaengine : xilinx_dma two issues

Hi All,

On Fri, Jan 8, 2021 at 2:13 AM Radhey Shyam Pandey 
wrote:

-Original Message-
From: Radhey Shyam Pandey
Sent: Monday, January 4, 2021 10:50 AM
To: Paul Thomas ; Dan Williams
; Vinod Koul ; Michal
Simek ; Matthew Murrian
; Romain Perier
; Krzysztof Kozlowski ;
Marc Ferland ; Sebastian von Ohr
; dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel ; Shravya Kumbham ; git

Subject: RE: dmaengine : xilinx_dma two issues

-Original Message-
From: Paul Thomas 
Sent: Monday, December 28, 2020 10:14 AM
To: Dan Williams ; Vinod Koul
; Michal Simek ; Radhey
Shyam Pandey ; Matthew Murrian
; Romain Perier

;

Krzysztof Kozlowski ; Marc Ferland
; Sebastian von Ohr ;
dmaeng...@vger.kernel.org; Linux ARM ; linux-kernel 
Subject: dmaengine : xilinx_dma two issues

Hello,

I'm trying to get the 5.10 kernel up and running for our system,
and I'm running into a couple of issues with xilinx_dma.

+ (Xilinx mailing list)

Thanks for bringing the issues to our notice. Replies inline.

First, commit 14ccf0aab46e 'dmaengine: xilinx_dma: In dma channel
probe fix node order dependency' breaks our usage. Before this
commit a

call to:

dma_request_chan(_dev->dev, "axi_dma_0"); returns fine, but
after that commit it returns -19. The reason for this seems to be
that the only channel that is setup is channel 1 (chan->id is 1 in

xilinx_dma_chan_probe()).

However in
of_dma_xilinx_xlate() chan_id is gets set to 0 (int chan_id =
dma_spec-

args[0];), which causes the:

!xdev->chan[chan_id]
test to fail in of_dma_xilinx_xlate()

What is the channel number passed in dmaclient DT?

Is this a question for me?

Yes, please also share the dmaclient DT client node. Need to see
channel number passed to dmas property. Something like below-

dmas = <& axi_dma_0 1>
dma-names = "axi_dma_0"

OK, I think I need to revisit this and clean it up some. Currently In
the driver (a custom iio adc driver) it is hard coded:
dma_request_chan(_dev->dev, "axi_dma_0");

However, the DT also has the entries (currently unused by the driver):
 dmas = <_dma_0 0>;
 dma-names = "axi_dma_0";

I'll go back and clean up our driver to do something like adi-axi-adc.c does:

 if (!device_property_present(dev, "dmas"))
 return 0;

 if (device_property_read_string(dev, "dma-names", _name))
 dma_name = "axi_dma_0";

Should the dmas node get used by the driver? I see the second argument
is: '0' for write/tx and '1' for read/rx channel. So I should be
setting this to 1 like this?
 dmas = <_dma_0 1>;
 dma-names = "axi_dma_0";

But where does that field get used?

This got broken in "dmaengine: xilinx_dma: In dma channel probe fix node 
order dependency" 
. 
Before if there was only one channel that channel was always at index 0. 
Regardless of whether the channel was RX or TX. But after that change 
the RX channel is always at offset 1, regardless of whether the DMA has 
one or two channels. This is a breakage in ABI.

If you have the choice I'd recommend to not use the Xilinx DMA, it gets 
broken pretty much every other release.

- Lars

Re: Haswell audio no longer working with new Catpt driver (was: sound)

2020-12-31 Thread Lars-Peter Clausen


On 12/31/20 9:33 AM, Greg Kroah-Hartman wrote:

On Wed, Dec 30, 2020 at 07:10:16PM +0100, Christian Labisch wrote:

Update :

I've just tested the kernel 5.10.4 from ELRepo.
Unfortunately nothing changed - still no sound.

Ah, sad.  Can you run 'git bisect' between 5.9 and 5.10 to determine the
commit that caused the problem?


The problem is that one driver was replaced with another driver. git 
bisect wont really help to narrow down why the new driver does not work.


Christian can you run the alsa-info.sh[1] script on your system and send 
back the result?


You say sound is not working, can you clarify that a bit? Is no sound 
card registered? Is it registered but output stays silent?


- Lars

[1] https://www.alsa-project.org/wiki/AlsaInfo

Re: [RFC PATCH v2 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors

2020-12-30 Thread Lars-Peter Clausen


On 12/30/20 2:41 PM, Jonathan Cameron wrote:

On Thu, 24 Dec 2020 03:19:21 +
Jyoti Bhayana  wrote:


+   /*
+* The seconds field in the sensor interval in SCMI is 16 bits long
+* Therefore seconds  = 1/Hz <= 0x. As floating point calculations 
are
+* discouraged in the kernel driver code, to calculate the scale factor 
(sf)
+* (1* 100 * sf)/uHz <= 0x. Therefore, sf <= (uHz * 
0x)/100
+*  To calculate the multiplier,we convert the sf into char string  and
+*  count the number of characters
+*/
+
+   mult = scnprintf(buf, 32, "%llu", ((u64)uHz * 0x) / UHZ_PER_HZ) - 1;

use sizeof(buf) instead of having 32 again.

Since this is just interested in the number of characters and not the 
string itself I believe it is possible to just call sprintf with NULL 
instead of a buffer. It will then still return the number of characters, 
but not print anything.


But maybe providing a ilog10() helper is the better approach.

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 4:18 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 15:57:02 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 3:24 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 14:16:48 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 12:06 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 11:59:23 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 10:55 AM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.


I wonder, do we also have to align size to be a multiple of PAGE_SIZE
to avoid leaking unrelated data?

Hm, a good question.  Basically the PCM buffer size itself shouldn't
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed.  I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.

What I meant was that most of the APIs that we use to allocate memory
work on a PAGE_SIZE granularity. I.e. if you request a buffer that
where the size is not a multiple of PAGE_SIZE internally they will
still allocate a buffer that is a multiple of PAGE_SIZE and mark the
unused bytes as reserved.

But I believe that is not the case gen_pool_dma_alloc(). It will
happily allocate those extra bytes to some other allocation request.

That we need to zero out the reserved bytes even for those other APIs
is a very good additional point!

I looked at this a few years ago and I'm pretty sure that we cleared
out the allocated area, but I can't find that anymore in the current
code. Which is not so great I guess.

IIRC, we used GFP_ZERO in the past for the normal page allocations,
but this was dropped as it's no longer supported or so.

Also, we clear out the PCM buffer in hw_params call, but this is for
the requested size, not the actual allocated size, hence the padding
bytes will remain uncleared.

Ah! That memset() in hw_params is new.

So I believe it's safer to add an extra memset() like my test patch.

Yea, we definitely want that.

Do we care about leaking audio samples from a previous
application. I.e. application 'A' allocates a buffer plays back some
data and then closes the device again. Application 'B' then opens the
same audio devices allocates a slightly smaller buffer, so that it
still uses the same number of pages. The buffer from the previous
allocation get reused, but the remainder of the last page wont get
cleared in hw_params().

That's true.  On the second though, it might be better to extend that
memset() in hw_params to assure clearing the whole allocated buffer.
We can check runtime->dma_buffer_p->bytes for the actual size.

Also, in the PCM memory allocator, we make sure that the allocation is
performed for page size.


diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 47b155a49226..6aabad070abf 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -755,8 +755,15 @@ static int snd_pcm_hw_params(struct snd_pcm_substream 
*substream,
runtime->boundary *= 2;
/* clear the buffer for avoiding possible kernel info leaks */
-   if (runtime->dma_area && !substream->ops->copy_user)
-   memset(runtime->dma_area, 0, runtime->dma_bytes);
+   if (runtime->dma_area && !substream->ops->copy_user) {
+   size_t size;
+
+   if (runtime->dma_buffer_p)
+   size = runtime->dma_buffer_p->bytes;
+   else
+   size = runtime->dma_bytes;

I'm not sure.

Not all drivers use snd_pcm_lib_malloc_pages() and
runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.

The runtime->dma_buffer_p->bytes is assured to be page-aligned by the
change in pcm_memory.c in this patch.  But it's true that non-standard
allocations won't cover the whole pages...


On the other hand if it is mmap-able, the underlying buffer must be a
multiple of PAGE_SIZE. So a simple memset(..., PAGE_ALIGN(size))
should work.

But we'd risk breaking drivers that do not reserve the remainder of
the page and use it for something else.

Maybe what we need is a check that runtime->dma_area is page aligned
and runtime->dma_bytes is a multiple of PAGE_SIZE. With a warning at
first and then turn this into a error a year later or so.

OK, how about the following instead?
Just check SNDRV_PCM_INFO_MMAP in runtime->info; if this is set, the
buffer size must be aligned with the page size, and we are safe to
extend the size to clear.

So the revised fix is much simpler, something like below.


I think this will work for the leaking data issue.

But it will not help with the original issue that 
gen_pool_dma_alloc_align() does not reserve the remainde

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 3:24 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 14:16:48 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 12:06 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 11:59:23 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 10:55 AM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.


I wonder, do we also have to align size to be a multiple of PAGE_SIZE
to avoid leaking unrelated data?

Hm, a good question.  Basically the PCM buffer size itself shouldn't
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed.  I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.

What I meant was that most of the APIs that we use to allocate memory
work on a PAGE_SIZE granularity. I.e. if you request a buffer that
where the size is not a multiple of PAGE_SIZE internally they will
still allocate a buffer that is a multiple of PAGE_SIZE and mark the
unused bytes as reserved.

But I believe that is not the case gen_pool_dma_alloc(). It will
happily allocate those extra bytes to some other allocation request.

That we need to zero out the reserved bytes even for those other APIs
is a very good additional point!

I looked at this a few years ago and I'm pretty sure that we cleared
out the allocated area, but I can't find that anymore in the current
code. Which is not so great I guess.

IIRC, we used GFP_ZERO in the past for the normal page allocations,
but this was dropped as it's no longer supported or so.

Also, we clear out the PCM buffer in hw_params call, but this is for
the requested size, not the actual allocated size, hence the padding
bytes will remain uncleared.

Ah! That memset() in hw_params is new.

So I believe it's safer to add an extra memset() like my test patch.

Yea, we definitely want that.

Do we care about leaking audio samples from a previous
application. I.e. application 'A' allocates a buffer plays back some
data and then closes the device again. Application 'B' then opens the
same audio devices allocates a slightly smaller buffer, so that it
still uses the same number of pages. The buffer from the previous
allocation get reused, but the remainder of the last page wont get
cleared in hw_params().

That's true.  On the second though, it might be better to extend that
memset() in hw_params to assure clearing the whole allocated buffer.
We can check runtime->dma_buffer_p->bytes for the actual size.

Also, in the PCM memory allocator, we make sure that the allocation is
performed for page size.


diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 47b155a49226..6aabad070abf 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -755,8 +755,15 @@ static int snd_pcm_hw_params(struct snd_pcm_substream 
*substream,
runtime->boundary *= 2;
  
  	/* clear the buffer for avoiding possible kernel info leaks */

-   if (runtime->dma_area && !substream->ops->copy_user)
-   memset(runtime->dma_area, 0, runtime->dma_bytes);
+   if (runtime->dma_area && !substream->ops->copy_user) {
+   size_t size;
+
+   if (runtime->dma_buffer_p)
+   size = runtime->dma_buffer_p->bytes;
+   else
+   size = runtime->dma_bytes;


I'm not sure.

Not all drivers use snd_pcm_lib_malloc_pages() and 
runtime->dma_buffer_p->bytes might not be a multiple of PAGE_SIZE.


On the other hand if it is mmap-able, the underlying buffer must be a 
multiple of PAGE_SIZE. So a simple memset(..., PAGE_ALIGN(size)) should 
work.


But we'd risk breaking drivers that do not reserve the remainder of the 
page and use it for something else.


Maybe what we need is a check that runtime->dma_area is page aligned and 
runtime->dma_bytes is a multiple of PAGE_SIZE. With a warning at first 
and then turn this into a error a year later or so.



+   memset(runtime->dma_area, 0, size);
+   }
  
  	snd_pcm_timer_resolution_change(substream);

snd_pcm_set_state(substream, SNDRV_PCM_STATE_SETUP);

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 12:06 PM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 11:59:23 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 10:55 AM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.


I wonder, do we also have to align size to be a multiple of PAGE_SIZE
to avoid leaking unrelated data?

Hm, a good question.  Basically the PCM buffer size itself shouldn't
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed.  I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.

What I meant was that most of the APIs that we use to allocate memory
work on a PAGE_SIZE granularity. I.e. if you request a buffer that
where the size is not a multiple of PAGE_SIZE internally they will
still allocate a buffer that is a multiple of PAGE_SIZE and mark the
unused bytes as reserved.

But I believe that is not the case gen_pool_dma_alloc(). It will
happily allocate those extra bytes to some other allocation request.

That we need to zero out the reserved bytes even for those other APIs
is a very good additional point!

I looked at this a few years ago and I'm pretty sure that we cleared
out the allocated area, but I can't find that anymore in the current
code. Which is not so great I guess.

IIRC, we used GFP_ZERO in the past for the normal page allocations,
but this was dropped as it's no longer supported or so.

Also, we clear out the PCM buffer in hw_params call, but this is for
the requested size, not the actual allocated size, hence the padding
bytes will remain uncleared.

Ah! That memset() in hw_params is new.


So I believe it's safer to add an extra memset() like my test patch.


Yea, we definitely want that.

Do we care about leaking audio samples from a previous application. I.e. 
application 'A' allocates a buffer plays back some data and then closes 
the device again. Application 'B' then opens the same audio devices 
allocates a slightly smaller buffer, so that it still uses the same 
number of pages. The buffer from the previous allocation get reused, but 
the remainder of the last page wont get cleared in hw_params().

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 10:55 AM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.


I wonder, do we also have to align size to be a multiple of PAGE_SIZE
to avoid leaking unrelated data?

Hm, a good question.  Basically the PCM buffer size itself shouldn't
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed.  I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.


What I meant was that most of the APIs that we use to allocate memory 
work on a PAGE_SIZE granularity. I.e. if you request a buffer that where 
the size is not a multiple of PAGE_SIZE internally they will still 
allocate a buffer that is a multiple of PAGE_SIZE and mark the unused 
bytes as reserved.


But I believe that is not the case gen_pool_dma_alloc(). It will happily 
allocate those extra bytes to some other allocation request.


That we need to zero out the reserved bytes even for those other APIs is 
a very good additional point!


I looked at this a few years ago and I'm pretty sure that we cleared out 
the allocated area, but I can't find that anymore in the current code. 
Which is not so great I guess.

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 11:14 AM, Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:55:42 +0100,
Takashi Iwai wrote:

On Thu, 17 Dec 2020 10:43:45 +0100,
Lars-Peter Clausen wrote:

On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.


I wonder, do we also have to align size to be a multiple of PAGE_SIZE
to avoid leaking unrelated data?

Hm, a good question.  Basically the PCM buffer size itself shouldn't
be influenced by that (i.e. no hw-constraint or such is needed), but
the padding should be cleared indeed.  I somehow left those to the
allocator side, but maybe it's safer to clear the whole buffer in
sound/core/memalloc.c commonly.

That said, something like below (totally untested).
We might pass the pass-aligned size to dmab->bytes field instead of
keeping the original value, too.


We'd need this for those APIs that also pass the size to the free() 
function. Like dma_free_coherent() and free_pages_exact(), but maybe 
those round up internally as well.


I had a quick look and I could not find any place were the code relies 
on the requested buffer size being stored in dmab->bytes. In fact we 
already reuse the buffer if  there is an allocated buffer that is larger 
than the requested buffer (See snd_pcm_lib_malloc_pages), so this should 
be OK.





Takashi

---
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -126,6 +126,7 @@ static inline gfp_t snd_mem_get_gfp_flags(const struct 
device *dev,
  int snd_dma_alloc_pages(int type, struct device *device, size_t size,
struct snd_dma_buffer *dmab)
  {
+   size_t orig_size = size;
gfp_t gfp;
  
  	if (WARN_ON(!size))

@@ -133,6 +134,7 @@ int snd_dma_alloc_pages(int type, struct device *device, 
size_t size,
if (WARN_ON(!dmab))
return -ENXIO;
  
+	size = PAGE_ALIGN(size);

dmab->dev.type = type;
dmab->dev.dev = device;
dmab->bytes = 0;
@@ -177,7 +179,8 @@ int snd_dma_alloc_pages(int type, struct device *device, 
size_t size,
}
if (! dmab->area)
return -ENOMEM;
-   dmab->bytes = size;
+   memset(dmab->area, 0, size);
+   dmab->bytes = orig_size;
return 0;
  }
  EXPORT_SYMBOL(snd_dma_alloc_pages);

Re: [PATCH v1 ] ALSA: core: memalloc: add page alignment for iram

2020-12-17 Thread Lars-Peter Clausen


On 12/17/20 5:15 PM, Robin Gong wrote:

Since mmap for userspace is based on page alignment, add page alignment
for iram alloc from pool, otherwise, some good data located in the same
page of dmab->area maybe touched wrongly by userspace like pulseaudio.

I wonder, do we also have to align size to be a multiple of PAGE_SIZE to 
avoid leaking unrelated data?

Re: [PATCH V2] iio: adc: ad7476: Add LTC2314-14 support

2020-12-16 Thread Lars-Peter Clausen


On 12/16/20 9:23 AM, Mircea Caprioru wrote:

[...]
Changelog v2
- fix conflict with ADS7868 device in chip_info_tbl
[...]
[ID_ADS7868] = {
.channel[0] = ADS786X_CHAN(8),
.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),


Hi Mircea,

I think this is still missing a } here



+   [ID_LTC2314_14] = {
+   .channel[0] = AD7940_CHAN(14),
+   .channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
},
  };

Re: [PATCH v2 3/3] Input: adp5589-keys - add basic devicetree support

2020-11-24 Thread Lars-Peter Clausen


On 11/24/20 9:22 AM, Alexandru Ardelean wrote:

error = devm_add_action_or_reset(>dev, adp5589_clear_config,
@@ -1078,6 +1098,13 @@ static int __maybe_unused adp5589_resume(struct device 
*dev)
  
  static SIMPLE_DEV_PM_OPS(adp5589_dev_pm_ops, adp5589_suspend, adp5589_resume);
  
+static const struct of_device_id adp5589_of_match[] = {

+   { .compatible = "adi,adp5585", .data = 
_chip_info_tbl[ADP5585_01] },
+   { .compatible = "adi,adp5585-02", .data = 
_chip_info_tbl[ADP5585_02] },
+   { .compatible = "adi,adp5589", .data = _chip_info_tbl[ADP5589] 
},


I think we need to add these to 
Documentation/devicetree/bindings/trivial-devices.yaml

Re: [Cocci] Proposal for a new checkpatch check; matching _set_drvdata() & _get_drvdata()

2020-11-20 Thread Lars-Peter Clausen


On 11/20/20 12:54 PM, Alexandru Ardelean wrote:

On Fri, Nov 20, 2020 at 12:47 PM Julia Lawall  wrote:



On Thu, 19 Nov 2020, Joe Perches wrote:


On Thu, 2020-11-19 at 17:16 +0200, Andy Shevchenko wrote:

On Thu, Nov 19, 2020 at 4:09 PM Alexandru Ardelean
 wrote:

Hey,

So, I stumbled on a new check that could be added to checkpatch.
Since it's in Perl, I'm reluctant to try it.

Seems many drivers got to a point where they now call (let's say)
spi_set_drvdata(), but never access that information via
spi_get_drvdata().
Reasons for this seem to be:
1. They got converted to device-managed functions and there is no
longer a remove hook to require the _get_drvdata() access
2. They look like they were copied from a driver that had a
_set_drvdata() and when the code got finalized, the _set_drvdata() was
omitted

There are a few false positives that I can notice at a quick look,
like the data being set via some xxx_set_drvdata() and retrieved via a
dev_get_drvdata().

I can say quite a few. And this makes a difference.
So, basically all drivers that are using PM callbacks would rather use
dev_get_drvdata() rather than bus specific.


I think checkpatch reporting these as well would be acceptable simply
from a reviewability perspective.

I did a shell script to quickly check these. See below.
It's pretty badly written but it is enough for me to gather a list.
And I wrote it in 5 minutes :P
I initially noticed this in some IIO drivers, and then I suspected
that this may be more widespread.

It seems more suitable for coccinelle.

To me as well.

To me as well, since it seems to involve nonlocal information.

I'm not sure to understand the original shell script. Is there
something interesting about pci_set_drvdata?

Ah, it's a stupid script I wrote in 5 minutes, so I did not bother to
make things smart.
In the text-matching I did in shell, there are some entries that come
from comments and docs.
It's only about 3-4 entries, so I just did a visual/manual ignore.

In essence:
The script searches for all strings that contain _set_drvdata.
The separators are whitespace.
It creates a list of all  _set_drvdata functions.
For each _set_drvdata function:
 It checks all files that have a _set_drvdata entry, but no
_get_drvdata

I piped this output into a file and started manually checking the drivers.
There is one [I forget which function] that is _set_drvdata() but
equivalent is _drvdata()

As Andy said, some precautions must be taken in places where
_set_drvdata() is called but dev_get_drvdata() is used.
Cases like PM suspend/resume calls.
And there may be some cases outside this context.


Doing something like this with coccinelle is fairly easy.

But I'd be very cautious about putting such a script into the kernel. It 
will result in too many false positive drive-by patches. Such a script 
will not detect cases such as:


 * Driver is split over multiple files. One file does 
..._set_drvdata(), another does ..._get_drvdata().


 * Framework uses drvdata to exchange data with the driver. E.g driver 
is expected to call set_drvdata() and then the framework uses 
get_drvdata() to retrieve the data. This is not a very good pattern, but 
there are some palces int he kernel where this is used. I believe for 
example V4L2 uses this.


- Lars

Re: [PATCH] iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack

2020-11-12 Thread Lars-Peter Clausen


On 11/12/20 11:14 AM, Alexandru Ardelean wrote:

On Thu, Nov 12, 2020 at 11:55 AM Lars-Peter Clausen  wrote:

On 11/12/20 10:10 AM, Alexandru Ardelean wrote:

From: Lars-Peter Clausen 

Use a heap allocated memory for the SPI transfer buffer. Using stack memory
can corrupt stack memory when using DMA on some systems.

This change adds 4 bytes at the end of the current DMA buffer, which will
be used by the trigger handler.
This is required because the first 4 bytes are reserved for register data.

Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
Signed-off-by: Lars-Peter Clausen 
Signed-off-by: Alexandru Ardelean 
---
   drivers/iio/adc/ad_sigma_delta.c   | 4 ++--
   include/linux/iio/adc/ad_sigma_delta.h | 2 +-
   2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 86039e9ecaca..33297f26508a 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -395,11 +395,11 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
   struct iio_poll_func *pf = p;
   struct iio_dev *indio_dev = pf->indio_dev;
   struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+ uint8_t *data = _delta->data[4];
   unsigned int reg_size;
   unsigned int data_reg;
- uint8_t data[16];

- memset(data, 0x00, 16);
+ memset(data, 0x00, 4);

Younger me didn't know what he was doing, this is wrong. We need the
extra space for the padding and timestamp.

We also can't put the beginning of the buffer at an 4 byte offset since
it needs to be 8 byte aligned for the timestamp.

I'll correct this.
I was re-spinning this out of some old patches and discussions on this
that I have.
So, then this becomes 24 bytes? Or 16?

Something like:
uint8_t data[24] cacheline_aligned;

uint8_t *data = _delta->data[8];


Yes.

Re: [PATCH] iio: ad_sigma_delta: Don't put SPI transfer buffer on the stack

2020-11-12 Thread Lars-Peter Clausen


On 11/12/20 10:10 AM, Alexandru Ardelean wrote:

From: Lars-Peter Clausen 

Use a heap allocated memory for the SPI transfer buffer. Using stack memory
can corrupt stack memory when using DMA on some systems.

This change adds 4 bytes at the end of the current DMA buffer, which will
be used by the trigger handler.
This is required because the first 4 bytes are reserved for register data.

Fixes: af3008485ea03 ("iio:adc: Add common code for ADI Sigma Delta devices")
Signed-off-by: Lars-Peter Clausen 
Signed-off-by: Alexandru Ardelean 
---
  drivers/iio/adc/ad_sigma_delta.c   | 4 ++--
  include/linux/iio/adc/ad_sigma_delta.h | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 86039e9ecaca..33297f26508a 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -395,11 +395,11 @@ static irqreturn_t ad_sd_trigger_handler(int irq, void *p)
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
+   uint8_t *data = _delta->data[4];
unsigned int reg_size;
unsigned int data_reg;
-   uint8_t data[16];
  
-	memset(data, 0x00, 16);

+   memset(data, 0x00, 4);


Younger me didn't know what he was doing, this is wrong. We need the 
extra space for the padding and timestamp.


We also can't put the beginning of the buffer at an 4 byte offset since 
it needs to be 8 byte aligned for the timestamp.


  
  	reg_size = indio_dev->channels[0].scan_type.realbits +

indio_dev->channels[0].scan_type.shift;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h 
b/include/linux/iio/adc/ad_sigma_delta.h
index a3a838dcf8e4..ac4ac4752c62 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -80,7 +80,7 @@ struct ad_sigma_delta {
 * DMA (thus cache coherency maintenance) requires the
 * transfer buffers to live in their own cache lines.
 */
-   uint8_t data[4] cacheline_aligned;
+   uint8_t data[8] cacheline_aligned;
  };
  
  static inline int ad_sigma_delta_set_channel(struct ad_sigma_delta *sd,

Re: [PATCH] dt-bindings: clock: adi,axi-clkgen: convert old binding to yaml format

2020-10-11 Thread Lars-Peter Clausen

On 10/8/20 11:28 AM, Ardelean, Alexandru wrote:

-Original Message-
From: Rob Herring 
Sent: Tuesday, October 6, 2020 11:25 PM
To: Ardelean, Alexandru 
Cc: linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; Hennerich, Michael
; l...@metafoo.de; sb...@kernel.org;
mturque...@baylibre.com; m...@kernel.org
Subject: Re: [PATCH] dt-bindings: clock: adi,axi-clkgen: convert old binding to
yaml format

On Thu, Oct 01, 2020 at 11:50:35AM +0300, Alexandru Ardelean wrote:

This change converts the old binding for the AXI clkgen driver to a
yaml format.

As maintainers, added:
  - Lars-Peter Clausen  - as original author of driver &
binding

Do you have permission for relicensing? The default was GPL-2.0.

I talked to Michael Hennerich [he's cc-ed], and we have permission from his 
side.
I think Lars would need to provide permission as well, as the author.
If we won't have a reply from him [after by some time-frame] I'll leave it as 
GPL-2.0.
I'm a bit clumsy about licensing in general; and I don't care about it all that 
much.

I guess you could argue that the yaml description is original work. 
Either way

Acked-by: Lars-Peter Clausen

Re: [PATCH v3 1/2] MAINTAINERS: Consolidate Analog Devices IIO entries and remove Beniamin Bia

2020-09-06 Thread Lars-Peter Clausen


On 9/6/20 4:06 PM, Jonathan Cameron wrote:

On Thu,  3 Sep 2020 20:19:25 +0200
Krzysztof Kozlowski  wrote:


Emails to Beniamin Bia bounce with no such address so remove him from
maintainers.  After this removal, many entries for Analog Devices Inc
IIO drivers look exactly the same so consolidate them.

Suggested-by: Andy Shevchenko 
Suggested-by: Jonathan Cameron 
Cc: Michael Hennerich 
Cc: Jonathan Cameron 
Cc: linux-iio 
Signed-off-by: Krzysztof Kozlowski 
Reviewed-by: Andy Shevchenko 

As I'd assume a more specific binding always overrides a catch all,
this has the effect of giving Lars and Michael responsibility
for a few things they didn't previously cover.  If the two
of them are fine with it, than that's good, but I'd ideally
like an Ack from Lars.


Acked-by: Lars-Peter Clausen 

I think I left a ticket with ADI when I left to update the maintainers 
entries and replace me with somebody else, must have gotten lost :)

Re: [PATCH] drivers/dma/dma-jz4780: Fix race condition between probe and irq handler

2020-08-20 Thread Lars-Peter Clausen


On 8/20/20 1:59 PM, Paul Cercueil wrote:

Hi,

Le dim. 16 août 2020 à 12:52, madhuparnabhowmi...@gmail.com a écrit :

From: Madhuparna Bhowmik 

In probe IRQ is requested before zchan->id is initialized which can be
read in the irq handler. Hence, shift request irq and enable clock after
other initializations complete. Here, enable clock part is not part of
the race, it is just shifted down after request_irq to keep the error
path same as before.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Madhuparna Bhowmik 


I don't think there is a race at all, the interrupt handler won't be 
called before the DMA is registered.


From a purely formal verification perspective there is a bug. The 
interrupt could fire if i.e. the hardware is buggy or something. In 
general it is a good idea to not request the IRQ until all the resources 
that are used in the interrupt handler are properly set up. Even if you 
know that in practice the interrupt will never fire this early.

Re: pcm|dmaengine|imx-sdma race condition on i.MX6

2020-08-19 Thread Lars-Peter Clausen


On 8/19/20 1:08 PM, Lars-Peter Clausen wrote:

On 8/17/20 9:28 AM, Benjamin Bara - SKIDATA wrote:
We think this is not an i.MX6-specific problem, but a problem of the 
DMAengine usage from the PCM.
In case of a XRUN, the DMA channel is never closed but first a 
SNDRV_PCM_TRIGGER_STOP next a

SNDRV_PCM_TRIGGER_START is triggered.
The SNDRV_PCM_TRIGGER_STOP simply executes a 
dmaengine_terminate_async() [1]

but does not await the termination by calling dmaengine_synchronize(),
which is required as stated by the docu [2].
Anyways, we are not able to fix it in the pcm_dmaengine layer either 
at the end of
SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete interrupt 
handler)
or at the beginning of SNDRV_PCM_TRIGGER_START (called from a PCM 
ioctl),

since the dmaengine_synchronize() requires a non-atomic context.


I think this might be an sdma specific problem after all. 
dmaengine_terminate_async() will issue a request to stop the DMA. But 
it is still safe to issue the next transfer, even without calling 
dmaengine_synchronize(). The DMA should start the new transfer at its 
earliest convenience in that case.


dmaegine_synchronize() is so that the consumer has a guarantee that 
the DMA is finished using the resources (e.g. the memory buffers) 
associated with the DMA transfer so it can safely free them. 


You can think of dmaengine_terminate_async() and 
dmaengine_issue_pending() as adding operations to a command queue. The 
DMA is responsible that the operations are executed in the same order 
that they were added to the queue and to make sure that their execution 
does not conflict.


dmaegine_synchronize() is for external consumers to wait until all 
operations in the command queue have been completed.

Re: pcm|dmaengine|imx-sdma race condition on i.MX6

2020-08-19 Thread Lars-Peter Clausen


On 8/17/20 9:28 AM, Benjamin Bara - SKIDATA wrote:

We think this is not an i.MX6-specific problem, but a problem of the DMAengine 
usage from the PCM.
In case of a XRUN, the DMA channel is never closed but first a 
SNDRV_PCM_TRIGGER_STOP next a
SNDRV_PCM_TRIGGER_START is triggered.
The SNDRV_PCM_TRIGGER_STOP simply executes a dmaengine_terminate_async() [1]
but does not await the termination by calling dmaengine_synchronize(),
which is required as stated by the docu [2].
Anyways, we are not able to fix it in the pcm_dmaengine layer either at the end 
of
SNDRV_PCM_TRIGGER_STOP (called from the DMA on complete interrupt handler)
or at the beginning of SNDRV_PCM_TRIGGER_START (called from a PCM ioctl),
since the dmaengine_synchronize() requires a non-atomic context.


I think this might be an sdma specific problem after all. 
dmaengine_terminate_async() will issue a request to stop the DMA. But it 
is still safe to issue the next transfer, even without calling 
dmaengine_synchronize(). The DMA should start the new transfer at its 
earliest convenience in that case.


dmaegine_synchronize() is so that the consumer has a guarantee that the 
DMA is finished using the resources (e.g. the memory buffers) associated 
with the DMA transfer so it can safely free them.




Based on my understanding, most of the DMA implementations don't even implement 
device_synchronize
and if they do, it might not really be necessary since the terminate_all 
operation is synchron.
There are a lot of buggy DMAengine drivers :) Pretty much all of them 
need device_synchronize() to get this right.


With the i.MX6, it looks a bit different:
Since [4], the terminate_all operation really schedules a worker which waits 
the required ~1ms and
then does the context freeing.
Now, the ioctl(SNDRV_PCM_IOCTL_PREPARE) and the following 
ioctl(SNDRV_PCM_IOCTL_READI_FRAMES),
which are called from US to handle/recover from a XRUN, are in a race with the 
terminate_worker.
If the terminate_worker finishes earlier, everything is fine.
Otherwise, the sdma_prep_dma_cyclic() is called, sets up everything and
as soon as it is scheduled out to wait for data, the terminate_worker is 
scheduled and kills it.
In this case, we wait in [5] until the timeout is reached and return with -EIO.

Based on our understanding, there exist two different fixing approaches:
We thought that the pcm_dmaengine should handle this by either synchronizing 
the DMA on a trigger or
terminating it synchronously.
However, as we are in an atomic context, we either have to give up the atomic 
context of the PCM
to finish the termination or we have to design a synchronous terminate variant 
which is callable
from an atomic context.

For the first option, which is potentially more performant, we have to leave 
the atomic PCM context
and we are not sure if we are allowed to.
For the second option, we would have to divide the dma_device terminate_all 
into an atomic sync and
an async one, which would align with the dmaengine API, giving it the option to 
ensure termination
in an atomic context.
Based on my understanding, most of them are synchronous anyways, for the 
currently async ones we
would have to implement busy waits.
However, with this approach, we reach the WARN_ON [6] inside of an atomic 
context,
indicating we might not do the right thing.


I don't know how feasible this is to implement in the SDMA dmaengine 
driver. But I think what is should do is to have some flag to indicate 
if a terminate is in progress. If a new transfer is issued while 
terminate is in progress the transfer should go on a list. Once 
terminate finishes it should check the list and start the transfer if 
there are any on the list.


- Lars

Re: [PATCH] iio: trigger: sysfs: Disable irqs before calling iio_trigger_poll()

2020-08-13 Thread Lars-Peter Clausen


On 8/12/20 1:01 PM, Christian Eggers wrote:

Hi Lars

On Monday, 3 August 2020, 08:52:54 CEST, Lars-Peter Clausen wrote:

On 8/3/20 8:44 AM, Christian Eggers wrote:

...
is my patch sufficient, or would you prefer a different solution?

The code in normal upstream is correct, there is no need to patch it
since iio_sysfs_trigger_work() always runs with IRQs disabled.


Are you using a non-upstream kernel? Maybe a RT kernel?

I use v5.4.-rt

That explains it. Have a look at
0200-irqwork-push-most-work-into-softirq-context.patch.

The right fix for this issue is to add the following snippet to the RT
patchset.

diff --git a/drivers/iio/trigger/iio-trig-sysfs.c
b/drivers/iio/trigger/iio-trig-sysfs.c
--- a/drivers/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/iio/trigger/iio-trig-sysfs.c
@@ -161,6 +161,7 @@ static int iio_sysfs_trigger_probe(int id)
   iio_trigger_set_drvdata(t->trig, t);

   init_irq_work(>work, iio_sysfs_trigger_work);
+t->work.flags = IRQ_WORK_HARD_IRQ;

   ret = iio_trigger_register(t->trig);
   if (ret)

I can confirm that this works for iio-trig-sysfs on 5.4.54-rt32. Currently I
do not use iio-trig-hrtimer, but if I remember correctly, the problem was also
present there.


Similar story, I think. On mainline hrtimers run in hardirq mode by 
default, whereas in RT they run in softirq mode by default. So we 
haven't see the issue in mainline.


To fix this we need to explicitly specify that the IIO hrtimer always 
needs to run in hardirq mode by using the HRTIMER_MODE_REL_HARD flag. 
I'll send a patch.



Do you want to apply your patch for mainline? In contrast to v5.4,
IRQ_WORK_HARD_IRQ is already available there (moved to smp_types.h).
Unfortunately I cannot test it on mainline for now, as my BSP stuff is not
ported yet.


Sounds like a plan :)

Re: [PATCH v5 2/2] iio: light: as73211: New driver

2020-08-04 Thread Lars-Peter Clausen


On 8/4/20 9:40 AM, Christian Eggers wrote:

On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote:

Thanks for an update, my comments below.

Thanks for the review. Please see below for my questions.

Best regards
Christian


On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers  wrote:

Datasheet:
https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-
b302-c2fd-e30a-c98df87616df

Do we need the UUID after the document file name?

I have send AMS an inquiry. Not sure whether I will get an answer. I will wait
a few days until sending v6.


+#define AS73211_OFFSET_TEMP (-66.9)
+#define AS73211_SCALE_TEMP  0.05

In the kernel we don't do float arithmetic. How these are being used?

Does this restriction also apply for compile time constants? I am quite
sure that all calculations using these defines will be evaluated at compile
time. If found a number of other places where probably the same is done:

find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep -v 
"\/\*.*[0-9]\.[0-9]"


I believe it is implementation defined. The compiler is free to generate 
floating math and do the conversion at runtime. Although it is probably 
safe to assume that no reasonable compiler will do this for your code. 
If only we had constexpr in C, then there was a way to make it 
guaranteed that the conversion happens during compile time.


But I agree with you, it would be nice to have a cleaner way of 
declaring fixed point numbers without having to pay attention to how 
many 0s you have to put after the least significant digit.

Re: [PATCH] iio: trigger: sysfs: Disable irqs before calling iio_trigger_poll()

2020-08-03 Thread Lars-Peter Clausen


On 8/3/20 8:44 AM, Christian Eggers wrote:

Hi Lars,

On Monday, 3 August 2020, 08:37:43 CEST, Lars-Peter Clausen wrote:

The sysfs IIO trigger uses irq_work to schedule the iio_trigger_poll()
and the promise of irq_work is that the callback will run in hard IRQ
context. That's the whole point of it.

irq_work_run_list(), which shows up in your callgraph, has as
BUG_ON(!irqs_disabled())[1], so we should never even get to calling
iio_trigger_poll() if IRQs where not disabled at this point. That's the
same condition that triggers the WARN_ON() in __handle_irq_event_percpu.

is my patch sufficient, or would you prefer a different solution?
The code in normal upstream is correct, there is no need to patch it 
since iio_sysfs_trigger_work() always runs with IRQs disabled.



Are you using a non-upstream kernel? Maybe a RT kernel?

I use v5.4.-rt


That explains it. Have a look at 
0200-irqwork-push-most-work-into-softirq-context.patch.


The right fix for this issue is to add the following snippet to the RT 
patchset.


diff --git a/drivers/iio/trigger/iio-trig-sysfs.c 
b/drivers/iio/trigger/iio-trig-sysfs.c

--- a/drivers/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/iio/trigger/iio-trig-sysfs.c
@@ -161,6 +161,7 @@ static int iio_sysfs_trigger_probe(int id)
 iio_trigger_set_drvdata(t->trig, t);

 init_irq_work(>work, iio_sysfs_trigger_work);
+    t->work.flags = IRQ_WORK_HARD_IRQ;

 ret = iio_trigger_register(t->trig);
 if (ret)

Re: [PATCH] iio: trigger: sysfs: Disable irqs before calling iio_trigger_poll()

2020-08-03 Thread Lars-Peter Clausen


On 8/3/20 7:16 AM, Christian Eggers wrote:

On Saturday, 1 August 2020, 18:02:34 CEST, Jonathan Cameron wrote:

On Mon, 27 Jul 2020 16:57:13 +0200

Christian Eggers  wrote:

iio_trigger_poll() calls generic_handle_irq(). This function expects to
be run with local IRQs disabled.

Was there an error or warning that lead to this patch?

[   17.448466] 000: [ cut here ]
[   17.448481] 000: WARNING: CPU: 0 PID: 9 at kernel/irq/handle.c:152 
__handle_irq_event_percpu+0x55/0xae
[   17.448511] 000: irq 236 handler irq_default_primary_handler+0x1/0x4 enabled 
interrupts
[   17.448526] 000: Modules linked in: bridge stp llc usb_f_ncm u_ether 
libcomposite sd_mod configfs cdc_acm usb_storage scsi_mod ci_hdrc_imx ci_hdrc 
st_magn_spi ulpi st_sensors_spi ehci_hcd regmap_spi tcpm roles st_magn_i2c 
typec st_sensors_i2c udc_core st_magn as73211 st_sensors imx_thermal usb49xx 
usbcore industrialio_triggered_buffer rtc_rv3028 kfifo_buf at24 usb_common 
nls_base i2c_dev usbmisc_imx phy_mxs_usb anatop_regulator imx2_wdt imx_fan 
spidev leds_pwm leds_gpio led_class iio_trig_sysfs imx6sx_adc industrialio 
fixed at25 spi_imx spi_bitbang imx_napi dev imx_sdma virt_dma nfsv3 nfs lockd 
grace sunrpc ksz9477_i2c ksz9477 tag_ksz ksz_common dsa_core phylink regmap_i2c 
i2c_imx i2c_core fec ptp pps_core micrel
[   17.448712] 000: CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.4.47-rt28+ 
#446
[   17.448723] 000: Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[   17.448738] 000: [] (unwind_backtrace) from [] 
(show_stack+0xb/0xc)
[   17.448754] 000: [] (show_stack) from [] 
(__warn+0x7b/0x8c)
[   17.448772] 000: [] (__warn) from [] 
(warn_slowpath_fmt+0x31/0x50)
[   17.448787] 000: [] (warn_slowpath_fmt) from [] 
(__handle_irq_event_percpu+0x55/0xae)
[   17.448807] 000: [] (__handle_irq_event_percpu) from [] 
(handle_irq_event_percpu+0x19/0x40)
[   17.448823] 000: [] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x3f/0x5c)
[   17.448839] 000: [] (handle_irq_event) from [] 
(handle_simple_irq+0x67/0x6a)
[   17.448854] 000: [] (handle_simple_irq) from [] 
(generic_handle_irq+0xd/0x16)
[   17.448870] 000: [] (generic_handle_irq) from [] 
(iio_trigger_poll+0x33/0x44 [industrialio])
[   17.448962] 000: [] (iio_trigger_poll [industrialio]) from 
[] (irq_work_run_list+0x43/0x66)
[   17.449010] 000: [] (irq_work_run_list) from [] 
(run_timer_softirq+0x7/0x3c)
[   17.449029] 000: [] (run_timer_softirq) from [] 
(__do_softirq+0x10f/0x160)
[   17.449045] 000: [] (__do_softirq) from [] 
(run_ksoftirqd+0x19/0x2c)
[   17.449061] 000: [] (run_ksoftirqd) from [] 
(smpboot_thread_fn+0x13b/0x140)
[   17.449078] 000: [] (smpboot_thread_fn) from [] 
(kthread+0xa3/0xac)
[   17.449095] 000: [] (kthread) from [] 
(ret_from_fork+0x11/0x20)
[   17.449110] 000: Exception stack(0xc2063fb0 to 0xc2063ff8)
[   17.449119] 000: 3fa0:   
 
[   17.449130] 000: 3fc0:       
 
[   17.449139] 000: 3fe0:     0013 
[   17.449146] 000: ---[ end trace 0002 ]---




Or can you point to what call in generic_handle_irq is making the
assumption that we are breaking?

Given this is using the irq_work framework I'm wondering if this is
a more general problem?

If I understand correctly, the kernel temporarily disables hardware interrupts
while hardware irq handlers are run. In case of the iio-trig-hrtim and 
iio-trig-sysfs
interrupts, __handle_irq_event_percpu() is not called from a hardware irq
(where interrupts would be disabled), but from software.


The sysfs IIO trigger uses irq_work to schedule the iio_trigger_poll() 
and the promise of irq_work is that the callback will run in hard IRQ 
context. That's the whole point of it.


irq_work_run_list(), which shows up in your callgraph, has as 
BUG_ON(!irqs_disabled())[1], so we should never even get to calling 
iio_trigger_poll() if IRQs where not disabled at this point. That's the 
same condition that triggers the WARN_ON() in __handle_irq_event_percpu.


Are you using a non-upstream kernel? Maybe a RT kernel?

- Lars

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq_work.c#n163

Re: [PATCH] iio: trigger: sysfs: Disable irqs before calling iio_trigger_poll()

2020-08-02 Thread Lars-Peter Clausen


On 8/1/20 6:02 PM, Jonathan Cameron wrote:

On Mon, 27 Jul 2020 16:57:13 +0200
Christian Eggers  wrote:


iio_trigger_poll() calls generic_handle_irq(). This function expects to
be run with local IRQs disabled.

Was there an error or warning that lead to this patch?
Or can you point to what call in generic_handle_irq is making the
assumption that we are breaking?

Given this is using the irq_work framework I'm wondering if this is
a more general problem?

Basically more info please!


There is this series https://lkml.org/lkml/2020/3/6/433 which causes 
generic_handle_irq() to issue an warning if it is called with IRQs on, 
for a IRQ controller that can't handle it.


But I'm not convinced this applies to the IIO code, since this is a 
purely virtual interrupt and is not interfering with any interrupt 
controller hardware.

Re: [PATCH] iio: trigger: Staticise stub functions

2020-07-18 Thread Lars-Peter Clausen


On 7/18/20 6:25 PM, Jonathan Cameron wrote:

On Tue, 14 Jul 2020 17:24:56 +0300
Alexandru Ardelean  wrote:


From: Lars-Peter Clausen 

Make sure that the trigger function stubs are all static inline. Otherwise
we'll get linker errors due to multiple definitions of the same function.

Fixes f8c6f4e9a40d4: ("iio: trigger: Staticise stub functions")
Signed-off-by: Lars-Peter Clausen 
Signed-off-by: Alexandru Ardelean 

I'm curious on what the actual build error is?  Static functions should
result in independent implementations in each C file that includes
them. Inline is normally considered a hint.  Hence what am I missing?


It's a bad commit message, my fault. This should have been

Make sure that the trigger function stubs are all static inline. 
Otherwise we might see compiler warnings about declared but unused 
functions.

Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC

2020-07-16 Thread Lars-Peter Clausen


On 7/16/20 9:27 AM, Cristian Pop wrote:

Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)


Very sneaky way of introducing a iio-gpio-proxy driver to be able to 
access GPIOs through libiio ;). I'm not really a fan of the whole idea.


But either way I think this needs a better description of what 1-bit 
converters are and how they are used.




Signed-off-by: Cristian Pop 
---
  drivers/iio/addac/one-bit-adc-dac.c | 229 
  1 file changed, 229 insertions(+)
  create mode 100644 drivers/iio/addac/one-bit-adc-dac.c

diff --git a/drivers/iio/addac/one-bit-adc-dac.c 
b/drivers/iio/addac/one-bit-adc-dac.c
new file mode 100644
index ..8e2a8a09fedb
--- /dev/null
+++ b/drivers/iio/addac/one-bit-adc-dac.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices ONE_BIT_ADC_DAC
+ * Digital to Analog Converters driver
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+enum ch_direction {
+   CH_IN,
+   CH_OUT,
+};
+
+struct one_bit_adc_dac_state {
+   struct platform_device  *pdev;
+   struct gpio_descs   *in_gpio_descs;
+   struct gpio_descs   *out_gpio_descs;
+};
+
+ #define ONE_BIT_ADC_DAC_CHANNEL(idx, direction)   \
+   {   \
+   .type = IIO_VOLTAGE,\
+   .indexed = 1,   \
+   .channel = idx, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
+   .output = direction,\
+   }
+
+static int one_bit_adc_dac_read_raw(struct iio_dev *indio_dev,
+   const struct iio_chan_spec *chan, int *val, int *val2, long info)
+{
+   struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+   int in_num_ch = 0, out_num_ch = 0;
+   int channel = chan->channel;
+
+   if (st->in_gpio_descs)
+   in_num_ch = st->in_gpio_descs->ndescs;
+
+   if (st->out_gpio_descs)
+   out_num_ch = st->out_gpio_descs->ndescs;
+
+   switch (info) {
+   case IIO_CHAN_INFO_RAW:
+   if (channel < in_num_ch) {
+   *val = gpiod_get_value_cansleep(
+   st->in_gpio_descs->desc[channel]);
+   } else {
+   channel -= in_num_ch;
+   *val = gpiod_get_value_cansleep(
+   st->out_gpio_descs->desc[channel]);
+   }
+   return IIO_VAL_INT;
+   default:
+   return -EINVAL;
+   }
+}
+
+static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan,
+   int val,
+   int val2,
+   long info)
+{
+   struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+   int in_num_ch = 0, out_num_ch = 0;
+   int channel = chan->channel;
+
+   if (st->in_gpio_descs)
+   in_num_ch = st->in_gpio_descs->ndescs;
+
+   if (st->out_gpio_descs)
+   out_num_ch = st->out_gpio_descs->ndescs;
+
+   switch (info) {
+   case IIO_CHAN_INFO_RAW:
+   if (channel < in_num_ch) {
+   gpiod_set_value_cansleep(
+   st->in_gpio_descs->desc[channel], val);


How can we set a value on an input GPIO?


+   } else {
+   channel -= in_num_ch;
+   gpiod_set_value_cansleep(
+   st->out_gpio_descs->desc[channel], val);
+   }
+
+   return 0;
+   default:
+   return -EINVAL;
+   }
+}
+
+static const struct iio_info one_bit_adc_dac_info = {
+   .read_raw = _bit_adc_dac_read_raw,
+   .write_raw = _bit_adc_dac_write_raw,
+};
+
+static int one_bit_adc_dac_set_ch(struct iio_dev *indio_dev,
+   struct iio_chan_spec *channels,
+   const char *propname,
+   int num_ch,
+   enum ch_direction direction,
+   int offset)
+{
+   struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
+   const char **gpio_names;
+   int ret, i;
+
+   if (num_ch <= 0)
+   return 0;
+
+   gpio_names = devm_kcalloc(indio_dev->dev.parent,
+   num_ch,
+   sizeof(char *),
sizeof(*gpio_names). It might be better to use normal kcalloc, kfree 
here since you only use it in this function.

+   GFP_KERNEL);
+   if (!gpio_names)
+   return -ENOMEM;
+
+

Re: [PATCH v3 06/13] iio: imu: inv_icm42600: add temperature sensor support

2020-06-14 Thread Lars-Peter Clausen


On 6/8/20 10:42 PM, Jean-Baptiste Maneyrol wrote:

+   case IIO_CHAN_INFO_PROCESSED:
+   ret = iio_device_claim_direct_mode(indio_dev);
+   if (ret)
+   return ret;
+   ret = inv_icm42600_temp_read(st, );
+   iio_device_release_direct_mode(indio_dev);
+   if (ret)
+   return ret;
+   *val = temp;
+   return IIO_VAL_INT;
+   case IIO_CHAN_INFO_SCALE:
+   *val = 483;
+   *val2 = 91787;
+   return IIO_VAL_INT_PLUS_MICRO;
+   case IIO_CHAN_INFO_OFFSET:
+   *val = 25000;
+   return IIO_VAL_INT;


If the data is returned processed there is no need to specify scale and 
offset.


But since the transformation to turn the data into standard units is a 
simple linear transform the preferred way to handle this is to return 
RAW data and specify scale and offset.

Re: [PATCH 0/2] iio: adc: Add a current from voltage driver

2020-05-16 Thread Lars-Peter Clausen


On 5/16/20 4:26 AM, Jonathan Bakker wrote:

In the discussion around adding the GP2A002 light driver, there came
up the question of what to do when a system emulates a current ADC
by using a voltage ADC and a resistor.  Rather than adding it on
a per-driver basis, it was suggested(1) to add a minimal IIO driver
to support this situation.

The new driver is fairly simple - it simply takes a voltage ADC and
a resistor value in ohms exposed as the scale and outputs a current.

It has been tested on a first-gen Galaxy S device which has the above
mentioned GP2A002 chip connected to the voltage ADC resistor complex.

1) https://lore.kernel.org/linux-iio/20200202150843.762c6897@archlinux/


Hi,

There is afe/iio-rescale.c, which I think already implements this 
functionality.


- Lars

Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers

2020-05-11 Thread Lars-Peter Clausen


On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:

On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:

[External]

On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:

On Mon, 2020-05-11 at 13:03 +, Ardelean, Alexandru wrote:

[External]

On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:

[External]

On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:

On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:

[External]

On Sat, 9 May 2020 10:52:14 +0200
Lars-Peter Clausen  wrote:


On 5/8/20 3:53 PM, Alexandru Ardelean wrote:

[...]
What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
This is because the 'buffer->dev.parent = _dev->dev'.
But I do feel this is correct.
So, now I don't know whether to leave it like that or symlink to
shorter
versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
The reason for naming the IIO buffer devices to 'iio:bufferX:Y'
is
mostly to make the names unique. It would have looked weird to
do
'/dev/buffer1' if I would have named the buffer devices
'bufferX'.

So, now I'm thinking of whether all this is acceptable.
Or what is acceptable?
Should I symlink 'iio:device3/iio:buffer3:0' ->
'iio:device3/buffer0'?
What else should I consider moving forward?
What means forward?
Where did I leave my beer?

Looking at how the /dev/ devices are named I think we can provide
a
name
that is different from the dev_name() of the device. Have a look
at
device_get_devnode() in drivers/base/core.c. We should be able to
provide the name for the chardev through the devnode() callback.

While we are at this, do we want to move the new devices into an
iio
subfolder? So iio/buffer0:0 instead of iio:buffer0:0?

Possibly on the folder.  I can't for the life of me remember why I
decided
not to do that the first time around - I'll leave it at the
mysterious "it may turn out to be harder than you'd think..."
Hopefully not ;)

I was also thinking about the /dev/iio subfolder while doing this.
I can copy that from /dev/input
They seem to do it already.
I don't know how difficult it would be. But it looks like a good
precedent.

All you have to do is return "iio/..." from the devnode() callback.

I admit I did not look closely into drivers/input/input.c before
mentioning
this
as as good precedent.

But, I looks like /dev/inpput is a class.
While IIO devices are a bus_type devices.
Should we start implementing an IIO class? or?

What I should have highlighted [before] with this, is that there is no
devnode()
callback for the bus_type [type].

But there is one in device_type :)

Many thanks :)
That worked nicely.

I now have:

root@analog:~# ls /dev/iio/*
/dev/iio/iio:device0  /dev/iio/iio:device1

/dev/iio/device3:
buffer0  buffer1  buffer2  buffer3

/dev/iio/device4:
buffer0


It looks like I can shift these around as needed.
This is just an experiment.
I managed to move the iio devices under /dev/iio, though probably the IIO
devices will still be around as /dev/iio:deviceX for legacy reasons.

Two things remain unresolved.
1. The name of the IIO buffer device.

root@analog:/sys/bus/iio/devices# ls iio\:device3/
buffer  in_voltage0_test_mode   name
events  in_voltage1_test_mode   of_node
iio:buffer:3:0  in_voltage_sampling_frequency   power
iio:buffer:3:1  in_voltage_scalescan_elements
iio:buffer:3:2  in_voltage_scale_available  subsystem
iio:buffer:3:3  in_voltage_test_mode_available  uevent


Right now, each buffer device is named 'iio:buffer:X:Y'.
One suggesttion was  'iio:deviceX:bufferY'
I'm suspecting the latter is preferred as when you sort the folders, buffers
come right after the iio:deviceX folders in /sys/bus/iio/devices.

I don't feel it matters much the device name of the IIO buffer if we symlink it
to a shorter form.
  
I'm guessing, we symlink these devices to short-hand 'bufferY' folders in each

'iio:deviceX'?


I think that would be a bit excessive. Only for the legacy buffer we 
need to have a symlink.



[...]
2. I know this is [still] stupid now; but any suggestions one how to symlink
/dev/iio:device3 -> /dev/iio/device3/buffer0 ?

Does not seem to be possible. Userspace will have to take care of it. 
This means we need to keep legacy devices in /dev/ and only new buffers 
in /dev/iio/.

Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers

2020-05-11 Thread Lars-Peter Clausen


On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:

On Mon, 2020-05-11 at 13:03 +, Ardelean, Alexandru wrote:

[External]

On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:

[External]

On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:

On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:

[External]

On Sat, 9 May 2020 10:52:14 +0200
Lars-Peter Clausen  wrote:


On 5/8/20 3:53 PM, Alexandru Ardelean wrote:

[...]
What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
This is because the 'buffer->dev.parent = _dev->dev'.
But I do feel this is correct.
So, now I don't know whether to leave it like that or symlink to
shorter
versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
mostly to make the names unique. It would have looked weird to do
'/dev/buffer1' if I would have named the buffer devices 'bufferX'.

So, now I'm thinking of whether all this is acceptable.
Or what is acceptable?
Should I symlink 'iio:device3/iio:buffer3:0' ->
'iio:device3/buffer0'?
What else should I consider moving forward?
What means forward?
Where did I leave my beer?

Looking at how the /dev/ devices are named I think we can provide a
name
that is different from the dev_name() of the device. Have a look at
device_get_devnode() in drivers/base/core.c. We should be able to
provide the name for the chardev through the devnode() callback.

While we are at this, do we want to move the new devices into an iio
subfolder? So iio/buffer0:0 instead of iio:buffer0:0?

Possibly on the folder.  I can't for the life of me remember why I
decided
not to do that the first time around - I'll leave it at the
mysterious "it may turn out to be harder than you'd think..."
Hopefully not ;)

I was also thinking about the /dev/iio subfolder while doing this.
I can copy that from /dev/input
They seem to do it already.
I don't know how difficult it would be. But it looks like a good
precedent.

All you have to do is return "iio/..." from the devnode() callback.

I admit I did not look closely into drivers/input/input.c before mentioning
this
as as good precedent.

But, I looks like /dev/inpput is a class.
While IIO devices are a bus_type devices.
Should we start implementing an IIO class? or?

What I should have highlighted [before] with this, is that there is no devnode()
callback for the bus_type [type].

But there is one in device_type :)

Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers

2020-05-11 Thread Lars-Peter Clausen


On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:

On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:

[External]

On Sat, 9 May 2020 10:52:14 +0200
Lars-Peter Clausen  wrote:


On 5/8/20 3:53 PM, Alexandru Ardelean wrote:

[...]
What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
This is because the 'buffer->dev.parent = _dev->dev'.
But I do feel this is correct.
So, now I don't know whether to leave it like that or symlink to shorter
versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
mostly to make the names unique. It would have looked weird to do
'/dev/buffer1' if I would have named the buffer devices 'bufferX'.

So, now I'm thinking of whether all this is acceptable.
Or what is acceptable?
Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
What else should I consider moving forward?
What means forward?
Where did I leave my beer?

Looking at how the /dev/ devices are named I think we can provide a name
that is different from the dev_name() of the device. Have a look at
device_get_devnode() in drivers/base/core.c. We should be able to
provide the name for the chardev through the devnode() callback.

While we are at this, do we want to move the new devices into an iio
subfolder? So iio/buffer0:0 instead of iio:buffer0:0?

Possibly on the folder.  I can't for the life of me remember why I decided
not to do that the first time around - I'll leave it at the
mysterious "it may turn out to be harder than you'd think..."
Hopefully not ;)

I was also thinking about the /dev/iio subfolder while doing this.
I can copy that from /dev/input
They seem to do it already.
I don't know how difficult it would be. But it looks like a good precedent.


All you have to do is return "iio/..." from the devnode() callback.



My concern regarding going to use stuff from core [like device_get_devnode()] is
that it seems to bypass some layers of kernel.
If I do 'git grep device_get_devnode', I get:

drivers/base/core.c:name = device_get_devnode(dev, , ,
, );
drivers/base/core.c: * device_get_devnode - path of device node file
drivers/base/core.c:const char *device_get_devnode(struct device *dev,
drivers/base/devtmpfs.c:req.name = device_get_devnode(dev, ,
, , );
drivers/base/devtmpfs.c:req.name = device_get_devnode(dev, NULL, NULL,
NULL, );
include/linux/device.h:extern const char *device_get_devnode(struct device *dev,
(END)

So, basically, most uses of device_get_devnode() are in core code, and I feel
that this may be sanctioned somewhere by some core people, if I do it.
I could be wrong, but if you disagree, I'll take your word for it.
You are not supposed to use the function itself, you should implement 
the devnode() callback for the IIO bus, which is then used by the core 
device_get_devnode() function.

Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers

2020-05-09 Thread Lars-Peter Clausen


On 5/8/20 3:53 PM, Alexandru Ardelean wrote:

[...]
What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
This is because the 'buffer->dev.parent = _dev->dev'.
But I do feel this is correct.
So, now I don't know whether to leave it like that or symlink to shorter
versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
mostly to make the names unique. It would have looked weird to do
'/dev/buffer1' if I would have named the buffer devices 'bufferX'.

So, now I'm thinking of whether all this is acceptable.
Or what is acceptable?
Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
What else should I consider moving forward?
What means forward?
Where did I leave my beer?


Looking at how the /dev/ devices are named I think we can provide a name 
that is different from the dev_name() of the device. Have a look at 
device_get_devnode() in drivers/base/core.c. We should be able to 
provide the name for the chardev through the devnode() callback.


While we are at this, do we want to move the new devices into an iio 
subfolder? So iio/buffer0:0 instead of iio:buffer0:0?

Re: [PATCH][RFC] iio: core: add a class hierarchy on iio device lock

2019-10-15 Thread Lars-Peter Clausen

On 10/14/19 5:59 PM, Olivier MOYSAN wrote:
> Hello Jonathan,
> 
> Thanks for your comment.
> 
> On 10/12/19 10:57 AM, Jonathan Cameron wrote:
>> On Fri, 11 Oct 2019 17:13:14 +0200
>> Olivier Moysan  wrote:
>>
>>> The aim of this patch is to correct a recursive locking warning,
>>> detected when setting CONFIG_PROVE_LOCKING flag (as shown in message below).
>>> This message was initially triggered by the following call sequence
>>> in stm32-dfsdm-adc.c driver, when using IIO hardware consumer interface.
>>>
>>> in stm32_dfsdm_read_raw()
>>> iio_device_claim_direct_mode
>>> mutex_lock(_dev->mlock);  -> lock on 
>>> dfsdm device
>>> iio_hw_consumer_enable
>>> iio_update_buffers
>>> mutex_lock(_dev->mlock);  -> lock on hw 
>>> consumer device
>> Hmm.  I'm not sure I follow the logic.  That lock is
>> for one thing and one thing only, preventing access
>> to the iio device that are unsafe when it is running
>> in a buffered mode.  We shouldn't be in a position where
>> we both say don't do this if we are in buffered mode, + enter
>> buffered mode whilst doing this, or we need special functions
>> for entering buffering mode if in this state.  We are in
>> some sense combining internal driver logic with overall
>> IIO states.  IIO shouldn't care that the device is using
>> the same methods under the hood for buffered and non
>> buffered operations.
>>
>> I can't really recall how this driver works.   Is it actually
>> possible to have multiple hw_consumers at the same time?
>>
>> So do we end up with multiple buffers registered and have
>> to demux out to the read_raw + the actual buffered path?
>> Given we have a bit of code saying grab one sample, I'm
>> going to guess we don't...
>>
>> If so, the vast majority of the buffer setup code in IIO
>> is irrelevant here and we just need to call a few of
>> the callbacks from this driver directly... (I think
>> though I haven't chased through every corner.
>>
>> I'd rather avoid introducing this nesting for a corner
>> case that makes no 'semantic' sense in IIO as it leaves us
>> in two separate states at the same time that the driver
>> is trying to make mutually exclusive.  We can't both
>> not be in buffered mode, and in buffered mode.
>>
>> Thanks and good luck with this nasty corner!
>>
>> Jonathan
>>
> Here I consider the following use case:
> A single conversion is performed. The dfsdm (filter) is chained with a 
> front-end, which can be an ADC or a sensor. So we have two IIO devices, 
> the dfsdm and its front-end handled through the hw consumer interface.
> 
> You are right. There is something wrong here, in buffered/non-buffered 
> mode mixing.
> iio_hw_consumer_enable() call is used to enable the front-end device. 
> But this interface is intended for buffered mode.
> So this is not coherent with the expected single conversion mode, 
> indeed. Another interface is required to manage the front-end device. I 
> have a poor knowledge of iio framework, but it seems to me that there is 
> no interface to manage this.
> 
> My understanding regarding mlock, is that it is used to protect the 
> state of the iio device.
> I we want to do a conversion from the chained devices, I think we need 
> to activate the first device
> and keep it performing conversion, as long as the second device has done 
> its conversion.
> We need to protect both devices, and we should have to do it in a nested 
> way.
> So, I guess that anyway, nested mutexes would be required in this case.
>

Others like regmap have solved this by having a lockclass per instance.
Although that is not ideal either since it will slow down lockdep.

See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/regmap.h#n629

Re: [PATCH] dmaengine: axi-dmac: simple device_config operation implemented

2019-10-15 Thread Lars-Peter Clausen

On 10/15/19 12:43 PM, Vinod Koul wrote:
> On 15-10-19, 07:05, Ardelean, Alexandru wrote:
>> On Mon, 2019-10-14 at 12:31 +0530, Vinod Koul wrote:
>>> [External]
>>>
>>
>> Hey,
>>
>>> On 13-09-19, 17:54, Alexandru Ardelean wrote:
 From: Rodrigo Alencar 

 dmaengine_slave_config is called by dmaengine_pcm_hw_params when using
 axi-i2s with axi-dmac. If device_config is NULL, -ENOSYS  is returned,
 which breaks the snd_pcm_hw_params function.
 This is a fix for the error:
>>>
>>> and what is that?
>>>
 $ aplay -D plughw:ADAU1761 /usr/share/sounds/alsa/Front_Center.wav
 Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit
 Little Endian, Rate 48000 Hz, Mono
 axi-i2s 43c2.axi-i2s: ASoC: 43c2.axi-i2s hw params failed: -38
>>
>> Error is above this line [code -38].
> 
> Right and it would help explaining a bit more on the error!
> 
>>
 aplay: set_params:1403: Unable to install hw params:
 ACCESS:  RW_INTERLEAVED
 FORMAT:  S16_LE
 SUBFORMAT:  STD
 SAMPLE_BITS: 16
 FRAME_BITS: 16
 CHANNELS: 1
 RATE: 48000
 PERIOD_TIME: 125000
 PERIOD_SIZE: 6000
 PERIOD_BYTES: 12000
 PERIODS: 4
 BUFFER_TIME: 50
 BUFFER_SIZE: 24000
 BUFFER_BYTES: 48000
 TICK_TIME: 0

 Signed-off-by: Rodrigo Alencar 
 Signed-off-by: Alexandru Ardelean 
 ---

 Note: Fixes tag not added intentionally.

  drivers/dma/dma-axi-dmac.c | 16 
  1 file changed, 16 insertions(+)

 diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
 index a0ee404b736e..ab2677343202 100644
 --- a/drivers/dma/dma-axi-dmac.c
 +++ b/drivers/dma/dma-axi-dmac.c
 @@ -564,6 +564,21 @@ static struct dma_async_tx_descriptor
 *axi_dmac_prep_slave_sg(
return vchan_tx_prep(>vchan, >vdesc, flags);
  }
  
 +static int axi_dmac_device_config(struct dma_chan *c,
 +  struct dma_slave_config *slave_config)
 +{
 +  struct axi_dmac_chan *chan = to_axi_dmac_chan(c);
 +  struct axi_dmac *dmac = chan_to_axi_dmac(chan);
 +
 +  /* no configuration required, a sanity check is done instead */
 +  if (slave_config->direction != chan->direction) {
>>>
>>>  slave_config->direction is a deprecated field, pls dont use that
>>
>> ack
>> any alternative recommendations of what to do in this case?

iirc direction is checked when the channel is requested, there should be
no need to check it again.

>> i can take a look, but if you have something on-the-top-of-your-head, i'm
>> open to suggestions
>> we can also just drop this completely and let userspace fail
> 
> Yeah it is tricky, this should be ideally implemented properly.
> 
 +  dev_err(dmac->dma_dev.dev, "Direction not supported by this
 DMA Channel");
 +  return -EINVAL;
>>>
>>> So you intent to support slave dma but do not use dma_slave_config.. how
>>> are you getting the slave address and other details?
>>
>> This DMA controller is a bit special.
>> It gets synthesized in FPGA, so the configuration is fixed and cannot be
>> changed at runtime. Maybe later we would allow/implement this
>> functionality, but this is a question for my HDL colleagues.
>>
>> Two things are done (in this order):
>> 1. For some paramters, axi_dmac_parse_chan_dt() is used to determine things
>> from device-tree; as it's an FPGA core, things are synthesized once and
>> cannot change (yet)
>> 2. For other parameters, the axi_dmac_detect_caps() is used to guess some
>> of them at probe time, by doing some reg reads/writes
> 
> So the question for you hw folks is how would a controller work with
> multiple slave devices, do they need to synthesize it everytime?
> 
> Rather than that why cant they make the peripheral addresses
> programmable so that you dont need updating fpga everytime!

The DMA has a direct connection to the peripheral and the peripheral
data port is not connected to the general purpose memory interconnect.
So you can't write to it by an MMIO address and  there is no address
that needs to be configured. For an FPGA based design this is quite a
good solution in terms of resource usage, performance and simplicity. A
direct connection requires less resources than connection it to the
central memory interconnect, while at the same time having lower latency
and not eating up any additional bandwidth on the central memory connect.

So slave config in this case is a noop and all it can do is verify that
the requested configuration matches the available configuration.

Re: [PATCH 1/1] iio: core: Fix fractional format generation

2019-08-21 Thread Lars-Peter Clausen

On 8/21/19 5:50 PM, Alexander Stein wrote:
> In case the result is -0.3252 tmp0 is 0 after the div_s64_rem, so tmp0 is
> non-negative which results in an output of 0.3252.
> Fix this by explicitly handling the negative sign ourselves.

Hi,

Thanks for you patch. Some comments inline.

> 
> Signed-off-by: Alexander Stein 
> ---
>  drivers/iio/industrialio-core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 245b5844028d..18350c1959ae 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -568,6 +568,7 @@ static ssize_t __iio_format_value(char *buf, size_t len, 
> unsigned int type,
>  {
>   unsigned long long tmp;
>   int tmp0, tmp1;
> + const char *sign = vals[0] < 0 ? "-" : "";
>   bool scale_db = false;
>  
>   switch (type) {
> @@ -593,11 +594,11 @@ static ssize_t __iio_format_value(char *buf, size_t 
> len, unsigned int type,
>   tmp = div_s64((s64)vals[0] * 10LL, vals[1]);
>   tmp1 = vals[1];
>   tmp0 = (int)div_s64_rem(tmp, 10, );
> - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
> + return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
> abs(tmp1));

I think this breaks the case where vals[1] is negative, but vals[0] is
positive. Maybe we can use a similar approach as for
IIO_VAL_INT_PLUS_NANO. Maybe even put this into a small helper function.

>   case IIO_VAL_FRACTIONAL_LOG2:
>   tmp = shift_right((s64)vals[0] * 10LL, vals[1]);
>   tmp0 = (int)div_s64_rem(tmp, 10LL, );
> - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
> + return snprintf(buf, len, "%s%u.%09u", sign, abs(tmp0), 
> abs(tmp1));
>   case IIO_VAL_INT_MULTIPLE:
>   {
>   int i;
>

Re: static analysis bug report in drivers/staging/iio/dac/ad5380.c

2019-08-15 Thread Lars-Peter Clausen

On 8/15/19 12:21 PM, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity Scan has detected a potential assignment
> bug in ad5380.c:
> 
> 217case IIO_CHAN_INFO_CALIBBIAS:
> 218ret = regmap_read(st->regmap,
> AD5380_REG_OFFSET(chan->address),
> 219val);
> 220if (ret)
> 221return ret;
> 222*val >>= chan->scan_type.shift;
> 
> CID 43178 (#1 of 1): Unused value (UNUSED_VALUE)assigned_pointer:
> Assigning value from val - (1 << chan->scan_type.realbits) / 2 to val
> here, but that stored value is not used.
> 
> 223val -= (1 << chan->scan_type.realbits) / 2;
> 224return IIO_VAL_INT;
> 
> val is a pointer and so updating it before a return is probably not the
> intention.  I suspect the intention was probably:
> 
>  *val -= (1 << chan->scan_type.realbits) / 2;
> 
> However, I'm not confident about this as the following case has:
> 
> 225case IIO_CHAN_INFO_SCALE:
> 226*val = 2 * st->vref;
> 227*val2 = chan->scan_type.realbits;
> 228return IIO_VAL_FRACTIONAL_LOG2;
> 
> which may imply the update maybe to *val2 instead, e.g.:
> 
>   *val2 -= (1 << chan->scan_type.realbits) / 2;
> 
> Any ideas?

Updating changing val to *val is the right fix in this case.

Re: [PATCH] staging: iio: ad7780: update voltage on read

2018-10-25 Thread Lars-Peter Clausen

On 10/25/2018 04:55 PM, Himanshu Jha wrote:
> On Thu, Oct 25, 2018 at 11:26:36AM -0300, Renato Lui Geh wrote:
>> Hi,
>>
>> Thanks for the quick review. :)
>>
>>> But please create one patch per issue and do not put unrelated changes into
>>> the same patch.
>>
>> Should I resend this patch as a patchset containing the two changes?
> 
> Yes! "One patch per change policy"
> 
>>> Also your mail client seems to have replaced tabs in the patch with spaces,
>>> this means the patch will not apply cleanly. Check the
>>> Documentation/email-clients.txt file for some hints how to configure your
>>> mail client so it will not break patches.
>>
>> From my end my original email patch appears to have tabs instead of
>> spaces. I redownloaded my email and vim shows that the indentation has
>> the ^I tab characters. But when downloading your reply it was converted
>> to spaces. Am I missing something?
> 
> Your patch applies fine.
> 
> I think the problem is on Lars end due to Thunderbird.

Yeah, looks like I need to read email-clients.txt...

I think it is mis-rendered on my side due to the "Content-Type: ...
format=flowed" in the header.

Re: [PATCH] staging: iio: ad7780: update voltage on read

2018-10-25 Thread Lars-Peter Clausen

On 10/25/2018 04:55 PM, Himanshu Jha wrote:
> On Thu, Oct 25, 2018 at 11:26:36AM -0300, Renato Lui Geh wrote:
>> Hi,
>>
>> Thanks for the quick review. :)
>>
>>> But please create one patch per issue and do not put unrelated changes into
>>> the same patch.
>>
>> Should I resend this patch as a patchset containing the two changes?
> 
> Yes! "One patch per change policy"
> 
>>> Also your mail client seems to have replaced tabs in the patch with spaces,
>>> this means the patch will not apply cleanly. Check the
>>> Documentation/email-clients.txt file for some hints how to configure your
>>> mail client so it will not break patches.
>>
>> From my end my original email patch appears to have tabs instead of
>> spaces. I redownloaded my email and vim shows that the indentation has
>> the ^I tab characters. But when downloading your reply it was converted
>> to spaces. Am I missing something?
> 
> Your patch applies fine.
> 
> I think the problem is on Lars end due to Thunderbird.

Yeah, looks like I need to read email-clients.txt...

I think it is mis-rendered on my side due to the "Content-Type: ...
format=flowed" in the header.

Re: [PATCH] staging: iio: ad7780: update voltage on read

2018-10-25 Thread Lars-Peter Clausen

On 10/25/2018 03:32 PM, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output.
> This patch fixes two issues.
> 
> - The driver read an outdated value set at initialization. It now
> updates its voltage on read.
> - Variable val subtracted an uninitialized value on
> IIO_CHAN_INFO_OFFSET. This was fixed by assiging the correct value
> instead.
> 
> Signed-off-by: Renato Lui Geh 

Hi,

Thanks for the patch, this looks good.

But please create one patch per issue and do not put unrelated changes into
the same patch.

Also your mail client seems to have replaced tabs in the patch with spaces,
this means the patch will not apply cleanly. Check the
Documentation/email-clients.txt file for some hints how to configure your
mail client so it will not break patches.

- Lars

> ---
> drivers/staging/iio/adc/ad7780.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..06700fe554a2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,16 +87,20 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>    long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> +    int voltage_uv = 0;
> 
> switch (m) {
> case IIO_CHAN_INFO_RAW:
>     return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> case IIO_CHAN_INFO_SCALE:
> +    voltage_uv = regulator_get_voltage(st->reg);
> +    if (voltage_uv)
> +    st->int_vref_mv = voltage_uv/1000;
>     *val = st->int_vref_mv * st->gain;
>     *val2 = chan->scan_type.realbits - 1;
>     return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> -    *val -= (1 << (chan->scan_type.realbits - 1));
> +    *val = -(1 << (chan->scan_type.realbits - 1));
>     return IIO_VAL_INT;
> }
>

Re: [PATCH] staging: iio: ad7780: update voltage on read

2018-10-25 Thread Lars-Peter Clausen

On 10/25/2018 03:32 PM, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output.
> This patch fixes two issues.
> 
> - The driver read an outdated value set at initialization. It now
> updates its voltage on read.
> - Variable val subtracted an uninitialized value on
> IIO_CHAN_INFO_OFFSET. This was fixed by assiging the correct value
> instead.
> 
> Signed-off-by: Renato Lui Geh 

Hi,

Thanks for the patch, this looks good.

But please create one patch per issue and do not put unrelated changes into
the same patch.

Also your mail client seems to have replaced tabs in the patch with spaces,
this means the patch will not apply cleanly. Check the
Documentation/email-clients.txt file for some hints how to configure your
mail client so it will not break patches.

- Lars

> ---
> drivers/staging/iio/adc/ad7780.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..06700fe554a2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,16 +87,20 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>    long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> +    int voltage_uv = 0;
> 
> switch (m) {
> case IIO_CHAN_INFO_RAW:
>     return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> case IIO_CHAN_INFO_SCALE:
> +    voltage_uv = regulator_get_voltage(st->reg);
> +    if (voltage_uv)
> +    st->int_vref_mv = voltage_uv/1000;
>     *val = st->int_vref_mv * st->gain;
>     *val2 = chan->scan_type.realbits - 1;
>     return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> -    *val -= (1 << (chan->scan_type.realbits - 1));
> +    *val = -(1 << (chan->scan_type.realbits - 1));
>     return IIO_VAL_INT;
> }
>

Re: [PATCH 1/2] staging: iio: ad7606: Move out of staging

2018-10-18 Thread Lars-Peter Clausen

On 10/18/2018 02:55 PM, Dan Carpenter wrote:
> On Thu, Oct 18, 2018 at 12:10:32PM +0300, Stefan Popa wrote:
>> +static int ad7606_read_samples(struct ad7606_state *st)
>> +{
>> +unsigned int num = st->chip_info->num_channels;
>> +u16 *data = st->data;
>> +int ret;
>> +
>> +/*
>> + * The frstdata signal is set to high while and after reading the sample
>> + * of the first channel and low for all other channels. This can be used
>> + * to check that the incoming data is correctly aligned. During normal
>> + * operation the data should never become unaligned, but some glitch or
>> + * electrostatic discharge might cause an extra read or clock cycle.
>> + * Monitoring the frstdata signal allows to recover from such failure
>> + * situations.
>> + */
>> +
>> +if (st->gpio_frstdata) {
>> +ret = st->bops->read_block(st->dev, 1, data);
>> +if (ret)
>> +return ret;
>> +
>> +if (!gpiod_get_value(st->gpio_frstdata)) {
> 
> This check should maybe be:
> 
>   if (gpiod_get_value(st->gpio_frstdata) <= 0) {
> 
> (Or possibly not, I don't know the code very well).

gpiod_get_value() is only allowed to return either 0 or 1.

Re: [PATCH 1/2] staging: iio: ad7606: Move out of staging

2018-10-18 Thread Lars-Peter Clausen

On 10/18/2018 02:55 PM, Dan Carpenter wrote:
> On Thu, Oct 18, 2018 at 12:10:32PM +0300, Stefan Popa wrote:
>> +static int ad7606_read_samples(struct ad7606_state *st)
>> +{
>> +unsigned int num = st->chip_info->num_channels;
>> +u16 *data = st->data;
>> +int ret;
>> +
>> +/*
>> + * The frstdata signal is set to high while and after reading the sample
>> + * of the first channel and low for all other channels. This can be used
>> + * to check that the incoming data is correctly aligned. During normal
>> + * operation the data should never become unaligned, but some glitch or
>> + * electrostatic discharge might cause an extra read or clock cycle.
>> + * Monitoring the frstdata signal allows to recover from such failure
>> + * situations.
>> + */
>> +
>> +if (st->gpio_frstdata) {
>> +ret = st->bops->read_block(st->dev, 1, data);
>> +if (ret)
>> +return ret;
>> +
>> +if (!gpiod_get_value(st->gpio_frstdata)) {
> 
> This check should maybe be:
> 
>   if (gpiod_get_value(st->gpio_frstdata) <= 0) {
> 
> (Or possibly not, I don't know the code very well).

gpiod_get_value() is only allowed to return either 0 or 1.

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-18 Thread Lars-Peter Clausen

On 10/18/2018 09:28 AM, Phil Reid wrote:
[...]
>> +    chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
>> +    if (IS_ERR(chip->rdwr_pin)) {
>> +    ret = PTR_ERR(chip->rdwr_pin);
>> +    dev_err(_dev->dev, "Failed to request rdwr GPIO: %d\n",
>> +    ret);
>>   return ret;
>>   }
>> -    gpio_direction_input(chip->rdwr_pin);
> 
> The RD/WR pin is an input to the AD78xx. So this doesn't make sense being
> GPIOD_IN.

One thing at a time. This patch is a straight forward conversion to the GPIO
descriptor interface. It keeps the existing semantics of the driver as they are.

Now these semantics are obviously wrong and should be fixed but that should
be a separate patch from changing the interface.

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-18 Thread Lars-Peter Clausen

On 10/18/2018 09:28 AM, Phil Reid wrote:
[...]
>> +    chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
>> +    if (IS_ERR(chip->rdwr_pin)) {
>> +    ret = PTR_ERR(chip->rdwr_pin);
>> +    dev_err(_dev->dev, "Failed to request rdwr GPIO: %d\n",
>> +    ret);
>>   return ret;
>>   }
>> -    gpio_direction_input(chip->rdwr_pin);
> 
> The RD/WR pin is an input to the AD78xx. So this doesn't make sense being
> GPIOD_IN.

One thing at a time. This patch is a straight forward conversion to the GPIO
descriptor interface. It keeps the existing semantics of the driver as they are.

Now these semantics are obviously wrong and should be fixed but that should
be a separate patch from changing the interface.

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-17 Thread Lars-Peter Clausen

On 10/17/2018 04:47 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Acked-by: Lars-Peter Clausen 

Thanks.

> ---
> Changes in v2:
>  - Correct the error messages as pin number being showed
>has now been replaced by error code.
> ---
>  drivers/staging/iio/adc/ad7816.c | 80 ++--
>  1 file changed, 34 insertions(+), 46 deletions(-)
>

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-17 Thread Lars-Peter Clausen

On 10/17/2018 04:47 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Acked-by: Lars-Peter Clausen 

Thanks.

> ---
> Changes in v2:
>  - Correct the error messages as pin number being showed
>has now been replaced by error code.
> ---
>  drivers/staging/iio/adc/ad7816.c | 80 ++--
>  1 file changed, 34 insertions(+), 46 deletions(-)
>

Re: [PATCH] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-16 Thread Lars-Peter Clausen

On 10/16/2018 04:46 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Hi,

Thanks for the patch, this looks good.

One thing about the error messages though.

> + chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
> + if (IS_ERR(chip->rdwr_pin)) {
> + ret = PTR_ERR(chip->rdwr_pin);
>   dev_err(_dev->dev, "Fail to request rdwr gpio PIN %d.\n",
> - chip->rdwr_pin);
> + ret);

This previously showed the pin number which has now been replaced with the
error code. The message doesn't make that much sense semantically anymore.
Maybe replace it with something like

"Failed to request rdwr GPIO: %d\n", ret

>   return ret;

Re: [PATCH] staging: iio: ad7816: Switch to the gpio descriptor interface

2018-10-16 Thread Lars-Peter Clausen

On 10/16/2018 04:46 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Hi,

Thanks for the patch, this looks good.

One thing about the error messages though.

> + chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
> + if (IS_ERR(chip->rdwr_pin)) {
> + ret = PTR_ERR(chip->rdwr_pin);
>   dev_err(_dev->dev, "Fail to request rdwr gpio PIN %d.\n",
> - chip->rdwr_pin);
> + ret);

This previously showed the pin number which has now been replaced with the
error code. The message doesn't make that much sense semantically anymore.
Maybe replace it with something like

"Failed to request rdwr GPIO: %d\n", ret

>   return ret;

Re: [PATCH] iio: adc: Fix potential integer overflow

2018-09-24 Thread Lars-Peter Clausen

On 09/24/2018 07:18 PM, Lars-Peter Clausen wrote:
> On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
>> On Tue, 18 Sep 2018 07:53:14 -0500
>> "Gustavo A. R. Silva"  wrote:
>>
>>> Cast factor to s64 in order to give the compiler complete information
>>> about the proper arithmetic to use and avoid a potential integer
>>> overflow. Notice that such variable is being used in a context
>>> that expects an expression of type s64 (64 bits, signed).
>>>
>>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>>> Signed-off-by: Gustavo A. R. Silva 
>>> ---
>>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/adc/qcom-vadc-common.c 
>>> b/drivers/iio/adc/qcom-vadc-common.c
>>> index dcd7fb5..e360e27 100644
>>> --- a/drivers/iio/adc/qcom-vadc-common.c
>>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 
>>> adc_code,
>>> voltage = div64_s64(voltage, data->full_scale_code_volt);
>>> if (voltage > 0) {
>>> voltage *= prescale->den;
>>> -   temp = prescale->num * factor;
>>> +   temp = prescale->num * (s64)factor;
>> So factor is an unsigned int so could be 32 bits.  In reality it only
>> takes a small set of values between 1 and 1000
>>
>> Maximum numerator is 10 so a maximum of 10,000.
>>
>> Hence this is a false positive, be it one that would be very hard
>> for a static checker to identify.
> 
> I think the reason why it complains is because temp is s64. So it infers
> that the idea was that the result of the multiplication can be larger
> than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

"lager than 32 bit"

> 
>>
>> So that moves it from a fix to a warning suppression change.
>> I have no problem with those, but description needs to reflect that.
> 
> Maybe just change the type of temp to u32. There is also
> mul_u64_u32_div() which could be used here to further simplify things.
>

Re: [PATCH] iio: adc: Fix potential integer overflow

2018-09-24 Thread Lars-Peter Clausen

On 09/24/2018 07:18 PM, Lars-Peter Clausen wrote:
> On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
>> On Tue, 18 Sep 2018 07:53:14 -0500
>> "Gustavo A. R. Silva"  wrote:
>>
>>> Cast factor to s64 in order to give the compiler complete information
>>> about the proper arithmetic to use and avoid a potential integer
>>> overflow. Notice that such variable is being used in a context
>>> that expects an expression of type s64 (64 bits, signed).
>>>
>>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>>> Signed-off-by: Gustavo A. R. Silva 
>>> ---
>>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/adc/qcom-vadc-common.c 
>>> b/drivers/iio/adc/qcom-vadc-common.c
>>> index dcd7fb5..e360e27 100644
>>> --- a/drivers/iio/adc/qcom-vadc-common.c
>>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 
>>> adc_code,
>>> voltage = div64_s64(voltage, data->full_scale_code_volt);
>>> if (voltage > 0) {
>>> voltage *= prescale->den;
>>> -   temp = prescale->num * factor;
>>> +   temp = prescale->num * (s64)factor;
>> So factor is an unsigned int so could be 32 bits.  In reality it only
>> takes a small set of values between 1 and 1000
>>
>> Maximum numerator is 10 so a maximum of 10,000.
>>
>> Hence this is a false positive, be it one that would be very hard
>> for a static checker to identify.
> 
> I think the reason why it complains is because temp is s64. So it infers
> that the idea was that the result of the multiplication can be larger
> than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

"lager than 32 bit"

> 
>>
>> So that moves it from a fix to a warning suppression change.
>> I have no problem with those, but description needs to reflect that.
> 
> Maybe just change the type of temp to u32. There is also
> mul_u64_u32_div() which could be used here to further simplify things.
>

Re: [PATCH] iio: adc: Fix potential integer overflow

2018-09-24 Thread Lars-Peter Clausen

On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 07:53:14 -0500
> "Gustavo A. R. Silva"  wrote:
> 
>> Cast factor to s64 in order to give the compiler complete information
>> about the proper arithmetic to use and avoid a potential integer
>> overflow. Notice that such variable is being used in a context
>> that expects an expression of type s64 (64 bits, signed).
>>
>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>> Signed-off-by: Gustavo A. R. Silva 
>> ---
>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/qcom-vadc-common.c 
>> b/drivers/iio/adc/qcom-vadc-common.c
>> index dcd7fb5..e360e27 100644
>> --- a/drivers/iio/adc/qcom-vadc-common.c
>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 
>> adc_code,
>>  voltage = div64_s64(voltage, data->full_scale_code_volt);
>>  if (voltage > 0) {
>>  voltage *= prescale->den;
>> -temp = prescale->num * factor;
>> +temp = prescale->num * (s64)factor;
> So factor is an unsigned int so could be 32 bits.  In reality it only
> takes a small set of values between 1 and 1000
> 
> Maximum numerator is 10 so a maximum of 10,000.
> 
> Hence this is a false positive, be it one that would be very hard
> for a static checker to identify.

I think the reason why it complains is because temp is s64. So it infers
that the idea was that the result of the multiplication can be larger
than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

> 
> So that moves it from a fix to a warning suppression change.
> I have no problem with those, but description needs to reflect that.

Maybe just change the type of temp to u32. There is also
mul_u64_u32_div() which could be used here to further simplify things.

Re: [PATCH] iio: adc: Fix potential integer overflow

2018-09-24 Thread Lars-Peter Clausen

On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 07:53:14 -0500
> "Gustavo A. R. Silva"  wrote:
> 
>> Cast factor to s64 in order to give the compiler complete information
>> about the proper arithmetic to use and avoid a potential integer
>> overflow. Notice that such variable is being used in a context
>> that expects an expression of type s64 (64 bits, signed).
>>
>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>> Signed-off-by: Gustavo A. R. Silva 
>> ---
>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/qcom-vadc-common.c 
>> b/drivers/iio/adc/qcom-vadc-common.c
>> index dcd7fb5..e360e27 100644
>> --- a/drivers/iio/adc/qcom-vadc-common.c
>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 
>> adc_code,
>>  voltage = div64_s64(voltage, data->full_scale_code_volt);
>>  if (voltage > 0) {
>>  voltage *= prescale->den;
>> -temp = prescale->num * factor;
>> +temp = prescale->num * (s64)factor;
> So factor is an unsigned int so could be 32 bits.  In reality it only
> takes a small set of values between 1 and 1000
> 
> Maximum numerator is 10 so a maximum of 10,000.
> 
> Hence this is a false positive, be it one that would be very hard
> for a static checker to identify.

I think the reason why it complains is because temp is s64. So it infers
that the idea was that the result of the multiplication can be larger
than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

> 
> So that moves it from a fix to a warning suppression change.
> I have no problem with those, but description needs to reflect that.

Maybe just change the type of temp to u32. There is also
mul_u64_u32_div() which could be used here to further simplify things.

Re: [PATCH 1/1] axi-i2s: set period size register

2018-08-27 Thread Lars-Peter Clausen

On 08/27/2018 06:22 PM, Luca Ceresoli wrote:
> Hi,
> 
> thanks for your feedback.
> 
> [Adding Michal Simek (Xilinx maintainer) in Cc]
> 
> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>> The default value of the PERIOD_LEN register is 0 and results in
>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>
>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>> is active.
>>>
>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>> known. This way TLAST is emitted once per period, and the DMA raises
>>> interrupts correctly.
>>
>> The patch looks OK. But I'd prefer not to merge it if possible.
>>
>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>> to be to unreliable and we've abandoned support for it. One of the more
>> critical issues was that you can't abort a DMA transfer. That means when
>> audio capture is stopped the DMA will halt, but not complete the current
>> transfer. Then when the next audio capture start the DMA will continue with
>> the previous transfer. The observed effect of this was that the system would
>> just crash randomly (Presumably due to memory corruption).
> 
> Strange. I have done many capture experiments with arecord and didn't
> run into such bad issues. I only have a much less serious problem
> (garbage or old samples in the first few buffers), but no crashes.
> 
> Michal, are you aware of these problems?
> 
>> Have you considered using the ADI AXI-DMAC? That should work just fine.
> 
> Not until today, because AXI-DMA is working here.
> 
> I'd like to better understand what's going on before changing an IP that
> is working. Do you have additional details about your setup? How do you
> run your tests?

This was 4-5 years ago. A AXI-DMA with both TX and RX connected to the
AXI-I2S.

It might be that back then I didn't have buffer prealloc enabled, so a
new DMA buffer gets allocated for each transfer. Then you end up with
use after free and the DMA overwriting freed (and maybe reused) memory.

It was bad enough that it was a lot easier to add PL330 support to the
I2S peripheral. Not using Xilinx DMA for anything anymore has saved me
from a lot of headache.

Re: [PATCH 1/1] axi-i2s: set period size register

2018-08-27 Thread Lars-Peter Clausen

On 08/27/2018 06:22 PM, Luca Ceresoli wrote:
> Hi,
> 
> thanks for your feedback.
> 
> [Adding Michal Simek (Xilinx maintainer) in Cc]
> 
> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>> The default value of the PERIOD_LEN register is 0 and results in
>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>
>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>> is active.
>>>
>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>> known. This way TLAST is emitted once per period, and the DMA raises
>>> interrupts correctly.
>>
>> The patch looks OK. But I'd prefer not to merge it if possible.
>>
>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>> to be to unreliable and we've abandoned support for it. One of the more
>> critical issues was that you can't abort a DMA transfer. That means when
>> audio capture is stopped the DMA will halt, but not complete the current
>> transfer. Then when the next audio capture start the DMA will continue with
>> the previous transfer. The observed effect of this was that the system would
>> just crash randomly (Presumably due to memory corruption).
> 
> Strange. I have done many capture experiments with arecord and didn't
> run into such bad issues. I only have a much less serious problem
> (garbage or old samples in the first few buffers), but no crashes.
> 
> Michal, are you aware of these problems?
> 
>> Have you considered using the ADI AXI-DMAC? That should work just fine.
> 
> Not until today, because AXI-DMA is working here.
> 
> I'd like to better understand what's going on before changing an IP that
> is working. Do you have additional details about your setup? How do you
> run your tests?

This was 4-5 years ago. A AXI-DMA with both TX and RX connected to the
AXI-I2S.

It might be that back then I didn't have buffer prealloc enabled, so a
new DMA buffer gets allocated for each transfer. Then you end up with
use after free and the DMA overwriting freed (and maybe reused) memory.

It was bad enough that it was a lot easier to add PL330 support to the
I2S peripheral. Not using Xilinx DMA for anything anymore has saved me
from a lot of headache.

Re: [PATCH 1/1] axi-i2s: set period size register

2018-08-27 Thread Lars-Peter Clausen

On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
> The default value of the PERIOD_LEN register is 0 and results in
> axi-i2s keeping TLAST always asserted in its AXI Stream output.
> 
> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
> DMA generating an interrupt flood and ALSA produce a corrupted
> recording. This is because AXI-DMA raises an interrupt whenever TLAST
> is active.
> 
> Fix by setting the PERIOD_LEN register as soon as the period is
> known. This way TLAST is emitted once per period, and the DMA raises
> interrupts correctly.

The patch looks OK. But I'd prefer not to merge it if possible.

We've done some early experiments with the Xilinx AXI-DMA, but it turned out
to be to unreliable and we've abandoned support for it. One of the more
critical issues was that you can't abort a DMA transfer. That means when
audio capture is stopped the DMA will halt, but not complete the current
transfer. Then when the next audio capture start the DMA will continue with
the previous transfer. The observed effect of this was that the system would
just crash randomly (Presumably due to memory corruption).

Have you considered using the ADI AXI-DMAC? That should work just fine.

> 
> Signed-off-by: Luca Ceresoli 
> ---
>  sound/soc/adi/axi-i2s.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/adi/axi-i2s.c b/sound/soc/adi/axi-i2s.c
> index 4c23381727a1..af581a313a40 100644
> --- a/sound/soc/adi/axi-i2s.c
> +++ b/sound/soc/adi/axi-i2s.c
> @@ -24,6 +24,7 @@
>  #define AXI_I2S_REG_CTRL 0x04
>  #define AXI_I2S_REG_CLK_CTRL 0x08
>  #define AXI_I2S_REG_STATUS   0x10
> +#define AXI_I2S_REG_PERIOD_LEN   0x18
>  
>  #define AXI_I2S_REG_RX_FIFO  0x28
>  #define AXI_I2S_REG_TX_FIFO  0x2C
> @@ -101,6 +102,17 @@ static int axi_i2s_hw_params(struct snd_pcm_substream 
> *substream,
>   return 0;
>  }
>  
> +static int axi_i2s_prepare(struct snd_pcm_substream *substream,
> +struct snd_soc_dai *dai)
> +{
> + struct axi_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> + unsigned int period_bytes = snd_pcm_lib_period_bytes(substream);
> +
> + /* adi_i2s counts 32-bit words, thus divide bytes by 4 */
> + return regmap_write(i2s->regmap, AXI_I2S_REG_PERIOD_LEN,
> + (period_bytes / 4) - 1);
> +}
> +
>  static int axi_i2s_startup(struct snd_pcm_substream *substream,
>   struct snd_soc_dai *dai)
>  {
> @@ -147,6 +159,7 @@ static const struct snd_soc_dai_ops axi_i2s_dai_ops = {
>   .shutdown = axi_i2s_shutdown,
>   .trigger = axi_i2s_trigger,
>   .hw_params = axi_i2s_hw_params,
> + .prepare = axi_i2s_prepare,
>  };
>  
>  static struct snd_soc_dai_driver axi_i2s_dai = {
> @@ -175,7 +188,7 @@ static const struct regmap_config axi_i2s_regmap_config = 
> {
>   .reg_bits = 32,
>   .reg_stride = 4,
>   .val_bits = 32,
> - .max_register = AXI_I2S_REG_STATUS,
> + .max_register = AXI_I2S_REG_PERIOD_LEN,
>  };
>  
>  static int axi_i2s_probe(struct platform_device *pdev)
>

Re: [PATCH 1/1] axi-i2s: set period size register

2018-08-27 Thread Lars-Peter Clausen

On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
> The default value of the PERIOD_LEN register is 0 and results in
> axi-i2s keeping TLAST always asserted in its AXI Stream output.
> 
> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
> DMA generating an interrupt flood and ALSA produce a corrupted
> recording. This is because AXI-DMA raises an interrupt whenever TLAST
> is active.
> 
> Fix by setting the PERIOD_LEN register as soon as the period is
> known. This way TLAST is emitted once per period, and the DMA raises
> interrupts correctly.

The patch looks OK. But I'd prefer not to merge it if possible.

We've done some early experiments with the Xilinx AXI-DMA, but it turned out
to be to unreliable and we've abandoned support for it. One of the more
critical issues was that you can't abort a DMA transfer. That means when
audio capture is stopped the DMA will halt, but not complete the current
transfer. Then when the next audio capture start the DMA will continue with
the previous transfer. The observed effect of this was that the system would
just crash randomly (Presumably due to memory corruption).

Have you considered using the ADI AXI-DMAC? That should work just fine.

> 
> Signed-off-by: Luca Ceresoli 
> ---
>  sound/soc/adi/axi-i2s.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/adi/axi-i2s.c b/sound/soc/adi/axi-i2s.c
> index 4c23381727a1..af581a313a40 100644
> --- a/sound/soc/adi/axi-i2s.c
> +++ b/sound/soc/adi/axi-i2s.c
> @@ -24,6 +24,7 @@
>  #define AXI_I2S_REG_CTRL 0x04
>  #define AXI_I2S_REG_CLK_CTRL 0x08
>  #define AXI_I2S_REG_STATUS   0x10
> +#define AXI_I2S_REG_PERIOD_LEN   0x18
>  
>  #define AXI_I2S_REG_RX_FIFO  0x28
>  #define AXI_I2S_REG_TX_FIFO  0x2C
> @@ -101,6 +102,17 @@ static int axi_i2s_hw_params(struct snd_pcm_substream 
> *substream,
>   return 0;
>  }
>  
> +static int axi_i2s_prepare(struct snd_pcm_substream *substream,
> +struct snd_soc_dai *dai)
> +{
> + struct axi_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> + unsigned int period_bytes = snd_pcm_lib_period_bytes(substream);
> +
> + /* adi_i2s counts 32-bit words, thus divide bytes by 4 */
> + return regmap_write(i2s->regmap, AXI_I2S_REG_PERIOD_LEN,
> + (period_bytes / 4) - 1);
> +}
> +
>  static int axi_i2s_startup(struct snd_pcm_substream *substream,
>   struct snd_soc_dai *dai)
>  {
> @@ -147,6 +159,7 @@ static const struct snd_soc_dai_ops axi_i2s_dai_ops = {
>   .shutdown = axi_i2s_shutdown,
>   .trigger = axi_i2s_trigger,
>   .hw_params = axi_i2s_hw_params,
> + .prepare = axi_i2s_prepare,
>  };
>  
>  static struct snd_soc_dai_driver axi_i2s_dai = {
> @@ -175,7 +188,7 @@ static const struct regmap_config axi_i2s_regmap_config = 
> {
>   .reg_bits = 32,
>   .reg_stride = 4,
>   .val_bits = 32,
> - .max_register = AXI_I2S_REG_STATUS,
> + .max_register = AXI_I2S_REG_PERIOD_LEN,
>  };
>  
>  static int axi_i2s_probe(struct platform_device *pdev)
>

Re: [PATCH 1/3] iio: adxl372: Provide validate_trigger and validate_device callbacks

2018-08-20 Thread Lars-Peter Clausen

On 08/20/2018 04:53 PM, Stefan Popa wrote:
> This patch provides a validate_device callback for the trigger which makes
> sure that other devices are rejected.
> 
> Signed-off-by: Stefan Popa  ---
>  drivers/iio/accel/adxl372.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index d2fdc75..5a039ba 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -762,11 +762,24 @@ static int adxl372_dready_trig_set_state(struct 
> iio_trigger *trig,
>   return adxl372_set_interrupts(st, mask, 0);
>  }
>  
> +static int adxl372_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct adxl372_state *st = iio_priv(indio_dev);
> +
> + if (st->dready_trig != trig)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
>  static const struct iio_trigger_ops adxl372_trigger_ops = {
> + .validate_device = _trigger_validate_own_device,
>   .set_trigger_state = adxl372_dready_trig_set_state,
>  };
>  
>  static const struct iio_info adxl372_info = {
> + .validate_trigger = _validate_trigger,

I wonder, if the device only works with the trigger and the trigger only
works with the device should we actually register a trigger?

Seems to be just extra hassle when setting up the device without any extra
benefits.

Re: [PATCH 1/3] iio: adxl372: Provide validate_trigger and validate_device callbacks

2018-08-20 Thread Lars-Peter Clausen

On 08/20/2018 04:53 PM, Stefan Popa wrote:
> This patch provides a validate_device callback for the trigger which makes
> sure that other devices are rejected.
> 
> Signed-off-by: Stefan Popa  ---
>  drivers/iio/accel/adxl372.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index d2fdc75..5a039ba 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -762,11 +762,24 @@ static int adxl372_dready_trig_set_state(struct 
> iio_trigger *trig,
>   return adxl372_set_interrupts(st, mask, 0);
>  }
>  
> +static int adxl372_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct adxl372_state *st = iio_priv(indio_dev);
> +
> + if (st->dready_trig != trig)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
>  static const struct iio_trigger_ops adxl372_trigger_ops = {
> + .validate_device = _trigger_validate_own_device,
>   .set_trigger_state = adxl372_dready_trig_set_state,
>  };
>  
>  static const struct iio_info adxl372_info = {
> + .validate_trigger = _validate_trigger,

I wonder, if the device only works with the trigger and the trigger only
works with the device should we actually register a trigger?

Seems to be just extra hassle when setting up the device without any extra
benefits.

Re: [alsa-devel] [PATCH] ASoC: adav80x: mark expected switch fall-through

2018-08-09 Thread Lars-Peter Clausen

On 08/09/2018 11:30 AM, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 02:19:33PM -0500, Gustavo A. R. Silva wrote:
> 
>> @@ -648,6 +648,7 @@ static int adav80x_set_pll(struct snd_soc_component 
>> *component, int pll_id,
>>  pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
>>  break;
>>  }
>> +/* fall through */
>>  default:
>>  return -EINVAL;
>>  }
> 
> Are you *positive* this is a deliberate fall through?

It is. But it might make sense to re-order the code to look like

case 5400:
if (source != ADAV80X_PLL_SRC_XIN)
return -EINVAL;

pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
break;

It is just as many lines added when adding the /* fall through */, but it
makes it more obvious what is going on.

Either way:

Acked-by: Lars-Peter Clausen

Re: [alsa-devel] [PATCH] ASoC: adav80x: mark expected switch fall-through

2018-08-09 Thread Lars-Peter Clausen

On 08/09/2018 11:30 AM, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 02:19:33PM -0500, Gustavo A. R. Silva wrote:
> 
>> @@ -648,6 +648,7 @@ static int adav80x_set_pll(struct snd_soc_component 
>> *component, int pll_id,
>>  pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
>>  break;
>>  }
>> +/* fall through */
>>  default:
>>  return -EINVAL;
>>  }
> 
> Are you *positive* this is a deliberate fall through?

It is. But it might make sense to re-order the code to look like

case 5400:
if (source != ADAV80X_PLL_SRC_XIN)
return -EINVAL;

pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLLDIV;
break;

It is just as many lines added when adding the /* fall through */, but it
makes it more obvious what is going on.

Either way:

Acked-by: Lars-Peter Clausen

Re: [alsa-devel] [PATCH] ASoC: adau1761: Mark expected switch fall-though

2018-08-09 Thread Lars-Peter Clausen

On 08/09/2018 11:31 AM, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 02:22:13PM -0500, Gustavo A. R. Silva wrote:
> 
>> @@ -518,7 +518,8 @@ static int adau1761_setup_digmic_jackdetect(struct 
>> snd_soc_component *component)
>>  ARRAY_SIZE(adau1761_jack_detect_controls));
>>  if (ret)
>>  return ret;
>> -case ADAU1761_DIGMIC_JACKDET_PIN_MODE_NONE: /* fallthrough */
>> +/* fall through */
>> +case ADAU1761_DIGMIC_JACKDET_PIN_MODE_NONE:
>>  ret = snd_soc_dapm_add_routes(dapm, adau1761_no_dmic_routes,
>>  ARRAY_SIZE(adau1761_no_dmic_routes));
>>  if (ret)
> 
> Again, are you *sure* here?

Well, it says so in the comment next to it :)

Acked-by: Lars-Peter Clausen

Re: [alsa-devel] [PATCH] ASoC: adau1761: Mark expected switch fall-though

2018-08-09 Thread Lars-Peter Clausen

On 08/09/2018 11:31 AM, Mark Brown wrote:
> On Wed, Aug 08, 2018 at 02:22:13PM -0500, Gustavo A. R. Silva wrote:
> 
>> @@ -518,7 +518,8 @@ static int adau1761_setup_digmic_jackdetect(struct 
>> snd_soc_component *component)
>>  ARRAY_SIZE(adau1761_jack_detect_controls));
>>  if (ret)
>>  return ret;
>> -case ADAU1761_DIGMIC_JACKDET_PIN_MODE_NONE: /* fallthrough */
>> +/* fall through */
>> +case ADAU1761_DIGMIC_JACKDET_PIN_MODE_NONE:
>>  ret = snd_soc_dapm_add_routes(dapm, adau1761_no_dmic_routes,
>>  ARRAY_SIZE(adau1761_no_dmic_routes));
>>  if (ret)
> 
> Again, are you *sure* here?

Well, it says so in the comment next to it :)

Acked-by: Lars-Peter Clausen

Re: [PATCH 00/46] Use dmaenginem_async_device_register to simplify code

2018-08-03 Thread Lars-Peter Clausen

On 08/03/2018 09:19 AM, Huang Shijie wrote:
> All the patches are using dmaenginem_async_device_register to simplify code
> except the last one:
>   dmaengine: add COMPILE_TEST for the drivers
> 
> I use the last one to do the compiler test.  
> There are still 20 drivers which do not use the 
> dmaenginem_async_device_register.
> Let me take a rest, if this patch set is accepted, I will do the rest.

Lots of race conditions in this series. The DMA device needs to be removed
before any of the resources it uses are disabled/released.

As a rule of thumb you can only convert something to a managed
allocation/reregistration if it is the last action in remove.

Re: [PATCH 00/46] Use dmaenginem_async_device_register to simplify code

2018-08-03 Thread Lars-Peter Clausen

On 08/03/2018 09:19 AM, Huang Shijie wrote:
> All the patches are using dmaenginem_async_device_register to simplify code
> except the last one:
>   dmaengine: add COMPILE_TEST for the drivers
> 
> I use the last one to do the compiler test.  
> There are still 20 drivers which do not use the 
> dmaenginem_async_device_register.
> Let me take a rest, if this patch set is accepted, I will do the rest.

Lots of race conditions in this series. The DMA device needs to be removed
before any of the resources it uses are disabled/released.

As a rule of thumb you can only convert something to a managed
allocation/reregistration if it is the last action in remove.

Re: [PATCH 3/4] iio: adc: xilinx: Check for return values in clk related functions

2018-07-19 Thread Lars-Peter Clausen

On 07/18/2018 01:12 PM, Manish Narani wrote:
> This patch adds check for return values from clock related functions.
> This was reported by static code analysis tool.

This patch seems to do something else.

> 
> Signed-off-by: Manish Narani 
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 24 ++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c 
> b/drivers/iio/adc/xilinx-xadc-core.c
> index 248cffa..47eb364 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -322,6 +322,7 @@ static irqreturn_t xadc_zynq_interrupt_handler(int irq, 
> void *devid)
>  
>  #define XADC_ZYNQ_TCK_RATE_MAX 5000
>  #define XADC_ZYNQ_IGAP_DEFAULT 20
> +#define XADC_ZYNQ_PCAP_RATE_MAX 2
>  
>  static int xadc_zynq_setup(struct platform_device *pdev,
>   struct iio_dev *indio_dev, int irq)
> @@ -332,6 +333,7 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>   unsigned int div;
>   unsigned int igap;
>   unsigned int tck_rate;
> + int ret;
>  
>   /* TODO: Figure out how to make igap and tck_rate configurable */
>   igap = XADC_ZYNQ_IGAP_DEFAULT;
> @@ -341,6 +343,13 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>  
>   pcap_rate = clk_get_rate(xadc->clk);
>  
> + if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> + ret = clk_set_rate(xadc->clk,
> +(unsigned long)XADC_ZYNQ_PCAP_RATE_MAX);
> + if (ret)
> + return ret;
> + }
> +
>   if (tck_rate > pcap_rate / 2) {
>   div = 2;
>   } else {
> @@ -366,6 +375,12 @@ static int xadc_zynq_setup(struct platform_device *pdev,
>   XADC_ZYNQ_CFG_REDGE | XADC_ZYNQ_CFG_WEDGE |
>   tck_div | XADC_ZYNQ_CFG_IGAP(igap));
>  
> + if (pcap_rate > XADC_ZYNQ_PCAP_RATE_MAX) {
> + ret = clk_set_rate(xadc->clk, pcap_rate);
> + if (ret)
> + return ret;
> + }
> +
>   return 0;
>  }
>  
> @@ -887,6 +902,9 @@ static int xadc_write_raw(struct iio_dev *indio_dev,
>   unsigned long clk_rate = xadc_get_dclk_rate(xadc);
>   unsigned int div;
>  
> + if (!clk_rate)
> + return -EINVAL;
> +
>   if (info != IIO_CHAN_INFO_SAMP_FREQ)
>   return -EINVAL;
>  
> @@ -1239,8 +1257,10 @@ static int xadc_probe(struct platform_device *pdev)
>   goto err_free_irq;
>  
>   /* Disable all alarms */
> - xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> - XADC_CONF1_ALARM_MASK);
> + ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
> +   XADC_CONF1_ALARM_MASK);
> + if (ret)
> + goto err_free_irq;
>  
>   /* Set thresholds to min/max */
>   for (i = 0; i < 16; i++) {
>

1 2 3 4 5 6 7 8 9 10 >

1 - 100 of 2555 matches

Mail list logo