* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 20:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170112174902.j52foglkdouyz36n-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> >> > uncomfortable.
>> >> >>
>> >> >> I agree but this exactly the hardware way of working described in the
>> >> >> reference manual.
>> >> >
>> >> > IMHO that's a hw bug. This makes it for example impossible to implement
>> >> > SMBus block transfers (I think).
>> >>
>> >> This is not correct.
>> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> >> Here we have just to prepare the hardware for the 2 next pulse but the
>> >> STOP/START/ACK pulse will be generated at the right time as required
>> >> by I2C specification.
>> >> So SMBus block transfer will be possible.
>> >
>> > A block transfer consists of a byte that specifies the count of bytes
>> > yet to come. So the device sends for example:
>> >
>> > 0x01 0xab
>> >
>> > So when you read the 1 in the first byte it's already too late to set
>> > STOP to get it after the 2nd byte.
>> >
>> > Not sure I got all the required details right, though.
>>
>> Ok I understand your use case but I always think that the harware manages it.
>> If I take the above example, the I2C SMBus block read transaction will
>> be as below:
>> S Addr Wr [A] Comm [A]
>> S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>>
>> The first message is a single byte-transmission so there is no problem.
>>
>> The second message is a N-byte reception with N = 3
>>
>> When the I2C controller has finished to send the device address (S
>> Addr Rd), the ADDR flag is set and an interrupt is raised.
>> In the routine that handles ADDR event, we set ACK bit in order to
>> generate ACK pulse as soon as a data byte is received in the shift
>> register and then we clear the ADDR flag.
>> Please note that the SCL line is stretched low until ADDR flag is cleared.
>> So, as far I understand, the device could not sent any data as long as
>> the SCL line is stretched low. Right ?
>>
>> Then, as soon as the SCL line is high, the device could send the first
>> data byte (Count).
>> When this byte is received in the shift register, an ACK is
>> automatically generated as defined during adress match phase and the
>> data byte is pushed in DR (data register).
>> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
>> In the routine that handles RXNE event, as N=3, we just clear all
>> buffer interrupts in order to avoid another system preemption due to
>> RXNE event but we does not read the data in DR.
>
> In my example I want to receive a block of length 1, so only two bytes
> are read, a 1 (the length) and the data byte (0xab in my example). I
> think that as soon as you read the 1 it's already to late to schedule
> the NA after the next byte?
Not really. This 2-byte reception is also correctly managed.
Indeed, in this case, when the controller has sent the device address,
the ADDR flag is set and an interrupt is raised.
So, as long as the ADDR flag is not cleared, the SCL line is stretched
low and the device could not send any data.
During this address match phase, for a 2-byte reception, we enable
NACK and set POS bit (ACK/NACK position).
As POS=1, the NACK will be sent for the next byte which will be
received in the shift register instead of the current one.
So in this example, the next byte will be the last one.
After that, we clear the ADDR flag and the device is allowed to send data.
When the first data is received in the shift register, the RXNE flag
is set and an interrupt is raised.
As it is a 2-byte reception, we just clear all interrupts buffer to
avoid another preemption due to RXNE but we does not read DR.
Then, the second and last byte is received in the shift register.
The NACK is automatically sent by I2C controller as it was configured
to do that in the address match phase described above.
Moereover, as the first byte has not been read in DR, the BTF event
flag is set and an interrupt is raised.
Again, the SCL line is stretching low as long as data register has not
been read.
In the meantime, we set STOP bit to generate the pulse and we launch 2
consecutive read of DR to retrieve the 2 data bytes and release SCL
stretching.
In that way, NA and STOP are generated as expected even for a 2-byte reception.
Best regards,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-12 21:10 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
Linus Walleij, Patrice Chotard, Russell King, Rob Herring,
linux-i2c, Maxime Coquelin, linux-arm-kernel
In-Reply-To: <CAOAejn3tPi0fjy+t-UhMTvAq7LfdKJJdbcLw2su1-YAGDZMxew@mail.gmail.com>
On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > Hello Cedric,
> >> >
> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> >> >> > uncomfortable.
> >> >> >>
> >> >> >> I agree but this exactly the hardware way of working described in the
> >> >> >> reference manual.
> >> >> >
> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> >> > SMBus block transfers (I think).
> >> >>
> >> >> This is not correct.
> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> >> STOP/START/ACK pulse will be generated at the right time as required
> >> >> by I2C specification.
> >> >> So SMBus block transfer will be possible.
> >> >
> >> > A block transfer consists of a byte that specifies the count of bytes
> >> > yet to come. So the device sends for example:
> >> >
> >> > 0x01 0xab
> >> >
> >> > So when you read the 1 in the first byte it's already too late to set
> >> > STOP to get it after the 2nd byte.
> >> >
> >> > Not sure I got all the required details right, though.
> >>
> >> Ok I understand your use case but I always think that the harware manages it.
> >> If I take the above example, the I2C SMBus block read transaction will
> >> be as below:
> >> S Addr Wr [A] Comm [A]
> >> S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> >>
> >> The first message is a single byte-transmission so there is no problem.
> >>
> >> The second message is a N-byte reception with N = 3
> >>
> >> When the I2C controller has finished to send the device address (S
> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
> >> In the routine that handles ADDR event, we set ACK bit in order to
> >> generate ACK pulse as soon as a data byte is received in the shift
> >> register and then we clear the ADDR flag.
> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
> >> So, as far I understand, the device could not sent any data as long as
> >> the SCL line is stretched low. Right ?
> >>
> >> Then, as soon as the SCL line is high, the device could send the first
> >> data byte (Count).
> >> When this byte is received in the shift register, an ACK is
> >> automatically generated as defined during adress match phase and the
> >> data byte is pushed in DR (data register).
> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> >> In the routine that handles RXNE event, as N=3, we just clear all
> >> buffer interrupts in order to avoid another system preemption due to
> >> RXNE event but we does not read the data in DR.
> >
> > In my example I want to receive a block of length 1, so only two bytes
> > are read, a 1 (the length) and the data byte (0xab in my example). I
> > think that as soon as you read the 1 it's already to late to schedule
> > the NA after the next byte?
>
> Not really. This 2-byte reception is also correctly managed.
> Indeed, in this case, when the controller has sent the device address,
> the ADDR flag is set and an interrupt is raised.
> So, as long as the ADDR flag is not cleared, the SCL line is stretched
> low and the device could not send any data.
> During this address match phase, for a 2-byte reception, we enable
> NACK and set POS bit (ACK/NACK position).
> As POS=1, the NACK will be sent for the next byte which will be
> received in the shift register instead of the current one.
> So in this example, the next byte will be the last one.
> After that, we clear the ADDR flag and the device is allowed to send data.
I didn't follow, but if you are convinced it works that's good. I wonder
if it simplifies the driver if POS=1 is used and so ACK/NACK can be
setup later?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 21:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170112211004.z3wylc7vrubulc3x-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2017-01-12 22:10 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
>> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> > Hello Cedric,
>> >> >
>> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >> >> >> > This is surprising. I didn't recheck the manual, but that looks very
>> >> >> >> > uncomfortable.
>> >> >> >>
>> >> >> >> I agree but this exactly the hardware way of working described in the
>> >> >> >> reference manual.
>> >> >> >
>> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
>> >> >> > SMBus block transfers (I think).
>> >> >>
>> >> >> This is not correct.
>> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
>> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
>> >> >> STOP/START/ACK pulse will be generated at the right time as required
>> >> >> by I2C specification.
>> >> >> So SMBus block transfer will be possible.
>> >> >
>> >> > A block transfer consists of a byte that specifies the count of bytes
>> >> > yet to come. So the device sends for example:
>> >> >
>> >> > 0x01 0xab
>> >> >
>> >> > So when you read the 1 in the first byte it's already too late to set
>> >> > STOP to get it after the 2nd byte.
>> >> >
>> >> > Not sure I got all the required details right, though.
>> >>
>> >> Ok I understand your use case but I always think that the harware manages it.
>> >> If I take the above example, the I2C SMBus block read transaction will
>> >> be as below:
>> >> S Addr Wr [A] Comm [A]
>> >> S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
>> >>
>> >> The first message is a single byte-transmission so there is no problem.
>> >>
>> >> The second message is a N-byte reception with N = 3
>> >>
>> >> When the I2C controller has finished to send the device address (S
>> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
>> >> In the routine that handles ADDR event, we set ACK bit in order to
>> >> generate ACK pulse as soon as a data byte is received in the shift
>> >> register and then we clear the ADDR flag.
>> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
>> >> So, as far I understand, the device could not sent any data as long as
>> >> the SCL line is stretched low. Right ?
>> >>
>> >> Then, as soon as the SCL line is high, the device could send the first
>> >> data byte (Count).
>> >> When this byte is received in the shift register, an ACK is
>> >> automatically generated as defined during adress match phase and the
>> >> data byte is pushed in DR (data register).
>> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
>> >> In the routine that handles RXNE event, as N=3, we just clear all
>> >> buffer interrupts in order to avoid another system preemption due to
>> >> RXNE event but we does not read the data in DR.
>> >
>> > In my example I want to receive a block of length 1, so only two bytes
>> > are read, a 1 (the length) and the data byte (0xab in my example). I
>> > think that as soon as you read the 1 it's already to late to schedule
>> > the NA after the next byte?
>>
>> Not really. This 2-byte reception is also correctly managed.
>> Indeed, in this case, when the controller has sent the device address,
>> the ADDR flag is set and an interrupt is raised.
>> So, as long as the ADDR flag is not cleared, the SCL line is stretched
>> low and the device could not send any data.
>> During this address match phase, for a 2-byte reception, we enable
>> NACK and set POS bit (ACK/NACK position).
>> As POS=1, the NACK will be sent for the next byte which will be
>> received in the shift register instead of the current one.
>> So in this example, the next byte will be the last one.
>> After that, we clear the ADDR flag and the device is allowed to send data.
>
> I didn't follow, but if you are convinced it works that's good. I wonder
> if it simplifies the driver if POS=1 is used and so ACK/NACK can be
> setup later?
Please see below a quote from datasheet that clearly described how to handle
For 2-byte reception:
● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
● Set ACK low, set POS high
● Clear ADDR flag
● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
stretched low until a data1 is read)
● Set STOP high
● Read data 1 and 2
So we cannot set POS=1 and setup ACK/NACK later as you suggest.
Best regards,
Cedric
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-13 0:17 UTC (permalink / raw)
To: Wolfram Sang; +Cc: peda, robh+dt, mark.rutland, linux-i2c, devicetree
In-Reply-To: <20170112191514.dpusqnbqahayx6j3@ninjato>
On 13/01/2017 03:15, Wolfram Sang wrote:
> On Mon, Jan 09, 2017 at 05:02:54PM +0800, Phil Reid wrote:
>> Various muxes can aggregate multiple interrupts from each i2c bus.
>> All of the muxes with interrupt support combine the active low irq lines
>> using an internal 'and' function and generate a combined active low
>> output. The muxes do provide the ability to read a control register to
>> determine which irq is active. By making the mux an irq controller isr
>> latency can potentially be reduced by reading the status register and
>> then only calling the registered isr on that bus segment.
>>
>> As there is no irq masking on the mux irq are disabled until irq_unmask is
>> called at least once.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> Is the ack from Peda here forgotten or still missing?
>
> @peda: Once you are happy, do you want to take these patches via your
> shiny new mux-tree or do you prefer if I pick them?
>
G'day Wolfram,
I think he was hoping someone else would have a look at it based on this
feedback.
On 2017-01-04 15:13, Peter Rosin wrote:
> Irqs are not my strong point, I would prefer if someone else also had
> a look. And there are some comments below...
--
Regards
Phil Reid
^ permalink raw reply
* Re: [PATCH] i2c: i2c-cadence: Don't register the adapter until it's ready
From: Mike Looijmans @ 2017-01-13 6:40 UTC (permalink / raw)
To: Vladimir Zapolskiy, linux-i2c
Cc: soren.brinkmann, michal.simek, linux-kernel, linux-arm-kernel,
wsa
In-Reply-To: <428e8fc8-8b23-8e2c-763e-d977aaa98691@mleia.com>
On 06-01-17 22:34, Vladimir Zapolskiy wrote:
> Hello Mike,
>
> On 01/05/2017 12:47 PM, Mike Looijmans wrote:
>> The driver calls i2c_add_adapter before writing to config registers,
>> resulting in dmesg output like this, where devices fail to initialize:
>>
>> cdns-i2c ff030000.i2c: timeout waiting on completion
>> pca953x 1-0041: failed reading register
>> pca953x: probe of 1-0041 failed with error -110
>> at24 1-0050: 512 byte 24c04 EEPROM, writable, 1 bytes/write
>> cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197
>>
>> The adapter is being used before it completed the "probe". To fix
>> this, make "i2c_add_adapter" the last thing it calls in probe.
>> It also makes sense to show the adapter initialization before
>> the devices on the bus.
>
> commonly "it also" in a commit message means a change, which should be done
> separately, and this is the case here as well.
>
> Because the adapter registration i2c_add_adapter() can fail, information
> about the adapter initialization would be expected only in case of
> successful registration.
I would argue that the "info" message means "the I2C adapter is ready for
transaction now, and we'll start initializing devices on the bus". That is the
case before it calls i2c_add_adapter().
When i2c_add_adapter() runs, it will start probing devices on the bus. This
yields very confusing output, as it will output things in a reversed order:
- device X on I2C bus
- device Y on I2C bus
- cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197
This is especially confusing if there are multiple I2C adapters with muxes
behind them, the order then becomes like this:
- Device X on bus 0
- Mux A on bus 0 registering bus 2, 3 and 4
- I2C controller for bus 0
- Device Y on bus 1
- I2C controller for bus 1
- Device Z on bus 2
- etc..
> The information sent to the kernel log buffer here is quite trivial,
> probably dev_info() can be just removed, but in any case it should be
> a separate change.
Fine with me too.
>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>
> --
> With best wishes,
> Vladimir
>
Kind regards,
Mike Looijmans
System Expert
TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-13 7:26 UTC (permalink / raw)
To: M'boumba Cedric Madianga
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King, linux-i2c,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <CAOAejn1HAU5spQeXmYPH4XH0L5bGuoFgguozwMp=uLKcXDEPEw@mail.gmail.com>
Hello,
On Thu, Jan 12, 2017 at 10:28:20PM +0100, M'boumba Cedric Madianga wrote:
> Please see below a quote from datasheet that clearly described how to handle
> For 2-byte reception:
> ● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
> ● Set ACK low, set POS high
> ● Clear ADDR flag
> ● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
> stretched low until a data1 is read)
> ● Set STOP high
> ● Read data 1 and 2
The problem is that you only know that you have a 2 byte transfer after
you read the first byte (and it's a 1). (But note that this is
irrelevant for the patch as the driver doesn't claim to support this
kind of transfer.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Wolfram Sang @ 2017-01-13 8:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: M'boumba Cedric Madianga, Rob Herring, Maxime Coquelin,
Alexandre Torgue, Linus Walleij, Patrice Chotard, Russell King,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170113072650.orx6vl2orqfakcuk-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 194 bytes --]
> (But note that this is irrelevant for the patch as the driver doesn't
> claim to support this kind of transfer.)
Yes, I wanted to mention that, too.
I'd think the series is good to go in?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: i2c-cadence: Don't register the adapter until it's ready
From: Wolfram Sang @ 2017-01-13 8:37 UTC (permalink / raw)
To: Mike Looijmans
Cc: Vladimir Zapolskiy, linux-i2c, linux-kernel, linux-arm-kernel,
soren.brinkmann, michal.simek
In-Reply-To: <469bf72b-70e6-24d2-99b1-79b6f0a635bb@topic.nl>
[-- Attachment #1: Type: text/plain, Size: 912 bytes --]
> I would argue that the "info" message means "the I2C adapter is ready for
> transaction now, and we'll start initializing devices on the bus". That is
> the case before it calls i2c_add_adapter().
I know what you mean, but i2c_add_adapter does more, and it can fail
because the adapter is *not* ready to transfer. Seeing the success
message before is also confusing.
> When i2c_add_adapter() runs, it will start probing devices on the bus. This
> yields very confusing output, as it will output things in a reversed order:
>
> - device X on I2C bus
> - device Y on I2C bus
> - cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197
I agree. That being said, somewhen I started working on moving such
messages into the core to save string space and have consistent output.
Then, we can print at the proper time.
So, until then, we should be consistent with the other driver, I'd say.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-13 8:45 UTC (permalink / raw)
To: Wolfram Sang
Cc: M'boumba Cedric Madianga, Rob Herring, Maxime Coquelin,
Alexandre Torgue, Linus Walleij, Patrice Chotard, Russell King,
linux-i2c, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170113082903.GA1446@katana>
On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
>
> > (But note that this is irrelevant for the patch as the driver doesn't
> > claim to support this kind of transfer.)
>
> Yes, I wanted to mention that, too.
>
> I'd think the series is good to go in?
AFAICT there are some unaddressed comments that Cedrics claimed to fix
before our discussion was dominated by block transfers.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Peter Rosin @ 2017-01-13 9:11 UTC (permalink / raw)
To: Phil Reid, wsa, robh+dt, mark.rutland, linux-i2c, devicetree
In-Reply-To: <1483952576-5308-4-git-send-email-preid@electromag.com.au>
On 2017-01-09 10:02, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> latency can potentially be reduced by reading the status register and
> then only calling the registered isr on that bus segment.
>
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
>
I had a second reading of this patch. I'm still no master-of-irqs, though.
Anyway, I have some questions below. I guess it mostly shows that I don't
really know what I'm talking about here...
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 127 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index bbf088e..84fc767 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -41,14 +41,19 @@
> #include <linux/i2c.h>
> #include <linux/i2c-mux.h>
> #include <linux/i2c/pca954x.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/of_irq.h>
> #include <linux/pm.h>
> #include <linux/slab.h>
>
> #define PCA954X_MAX_NCHANS 8
>
> +#define PCA954X_IRQ_OFFSET 4
> +
> enum pca_type {
> pca_9540,
> pca_9542,
> @@ -63,6 +68,7 @@ enum pca_type {
> struct chip_desc {
> u8 nchans;
> u8 enable; /* used for muxes only */
> + u8 has_irq;
> enum muxtype {
> pca954x_ismux = 0,
> pca954x_isswi
> @@ -75,6 +81,9 @@ struct pca954x {
> u8 last_chan; /* last register value */
> u8 deselect;
> struct i2c_client *client;
> +
> + struct irq_domain *irq;
> + unsigned int irq_mask;
> };
>
> /* Provide specs for the PCA954x types we know about */
> @@ -87,19 +96,23 @@ struct pca954x {
> [pca_9542] = {
> .nchans = 2,
> .enable = 0x4,
> + .has_irq = 1,
> .muxtype = pca954x_ismux,
> },
> [pca_9543] = {
> .nchans = 2,
> + .has_irq = 1,
> .muxtype = pca954x_isswi,
> },
> [pca_9544] = {
> .nchans = 4,
> .enable = 0x4,
> + .has_irq = 1,
> .muxtype = pca954x_ismux,
> },
> [pca_9545] = {
> .nchans = 4,
> + .has_irq = 1,
> .muxtype = pca954x_isswi,
> },
> [pca_9547] = {
> @@ -222,6 +235,102 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
> return pca954x_reg_write(muxc->parent, client, data->last_chan);
> }
>
> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> +{
> + struct pca954x *data = dev_id;
> + unsigned int child_irq;
> + int ret, i, handled;
> +
> + ret = i2c_smbus_read_byte(data->client);
> + if (ret < 0)
> + return IRQ_NONE;
> +
> + for (i = 0; i < data->chip->nchans; i++) {
> + if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
> + child_irq = irq_linear_revmap(data->irq, i);
> + handle_nested_irq(child_irq);
> + handled++;
> + }
> + }
> + return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void pca954x_irq_mask(struct irq_data *idata)
> +{
> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
> + unsigned int pos = idata->hwirq;
> +
> + data->irq_mask &= ~BIT(pos);
> + if (!data->irq_mask)
> + disable_irq(data->client->irq);
> +}
> +
> +static void pca954x_irq_unmask(struct irq_data *idata)
> +{
> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
> + unsigned int pos = idata->hwirq;
> +
> + if (!data->irq_mask)
> + enable_irq(data->client->irq);
> + data->irq_mask |= BIT(pos);
> +}
I assume the irq core makes sure that .irq_mask and .irq_unmask may not
be called concurrently?
> +
> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +{
> + if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static struct irq_chip pca954x_irq_chip = {
> + .name = "i2c-mux-pca954x",
> + .irq_mask = pca954x_irq_mask,
> + .irq_unmask = pca954x_irq_unmask,
> + .irq_set_type = pca954x_irq_set_type,
> +};
> +
> +static int pca954x_irq_setup(struct i2c_mux_core *muxc)
> +{
> + struct pca954x *data = i2c_mux_priv(muxc);
> + struct i2c_client *client = data->client;
> + int c, err, irq;
> +
> + if (!data->chip->has_irq || client->irq <= 0)
> + return 0;
I assume "client->irq <= 0" means that users not specifying any interrupts
continue to behave as they use to, right?
BTW, what does client->irq == 0 represent?
Cheers,
peda
> +
> + data->irq = irq_domain_add_linear(client->dev.of_node,
> + data->chip->nchans,
> + &irq_domain_simple_ops, data);
> + if (!data->irq)
> + return -ENODEV;
> +
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_create_mapping(data->irq, c);
> + irq_set_chip_data(irq, data);
> + irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> + handle_simple_irq);
> + }
> +
> + err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
> + pca954x_irq_handler,
> + IRQF_ONESHOT | IRQF_SHARED,
> + "pca954x", data);
> + if (err)
> + goto err_req_irq;
> +
> + disable_irq(data->client->irq);
> +
> + return 0;
> +err_req_irq:
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_find_mapping(data->irq, c);
> + irq_dispose_mapping(irq);
> + }
> + irq_domain_remove(data->irq);
> +
> + return err;
> +}
> +
> /*
> * I2C init/probing/exit functions
> */
> @@ -286,6 +395,10 @@ static int pca954x_probe(struct i2c_client *client,
> idle_disconnect_dt = of_node &&
> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>
> + ret = pca954x_irq_setup(muxc);
> + if (ret)
> + goto fail_del_adapters;
> +
> /* Now create an adapter for each channel */
> for (num = 0; num < data->chip->nchans; num++) {
> bool idle_disconnect_pd = false;
> @@ -311,7 +424,7 @@ static int pca954x_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "failed to register multiplexed adapter"
> " %d as bus %d\n", num, force);
> - goto virt_reg_failed;
> + goto fail_del_adapters;
> }
> }
>
> @@ -322,7 +435,7 @@ static int pca954x_probe(struct i2c_client *client,
>
> return 0;
>
> -virt_reg_failed:
> +fail_del_adapters:
> i2c_mux_del_adapters(muxc);
> return ret;
> }
> @@ -330,6 +443,16 @@ static int pca954x_probe(struct i2c_client *client,
> static int pca954x_remove(struct i2c_client *client)
> {
> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> + struct pca954x *data = i2c_mux_priv(muxc);
> + int c, irq;
> +
> + if (data->irq) {
> + for (c = 0; c < data->chip->nchans; c++) {
> + irq = irq_find_mapping(data->irq, c);
> + irq_dispose_mapping(irq);
> + }
> + irq_domain_remove(data->irq);
> + }
>
> i2c_mux_del_adapters(muxc);
> return 0;
>
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Ville Syrjälä @ 2017-01-13 9:26 UTC (permalink / raw)
To: Hans de Goede
Cc: Jarkko Nikula, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <2ecf7b65-98aa-696b-f399-1f2d15d0ea65@redhat.com>
On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote:
> Hi,
>
> On 02-01-17 15:12, Ville Syrjälä wrote:
> > On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
> >> The punit on baytrail / cherrytrail systems is not only accessed through
> >> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
> >> the punit against simultaneous accesses and 2 functions to lock / unlock
> >> this mutex.
> >
> > I'm not sure which part of punit you're actually trying to protect
> > here. Some specific registers?
>
> The theory I'm going by is that for certain actions / certain requests
> we send to the punit, the punit needs to access the (axp288) pmic, to
> change (or enable / disable) certain voltages.
At least for cpu/display/gt voltages that shouldn't really be the case.
The vcc/vnn/vgg rails are controlled via svid, not i2c.
It also feels quite hand wavy since the punit could do whatever at
any time AFAIK. Eg. if there's some thermal event or something the
punit might kick into action. So trying to protect this from the OS
side might not be able to avoid these problems entirely. It feels like
there really should be some kind of shared hardware/firmware mutex
with the punit to arbitrate access to the i2c bus.
> So it needs to access the pmic i2c bus, but in some cases the kernel
> is accessing this itself (e.g. for battery monitoring) and is holding
> the punit i2c bus semaphore. At least with CPU-core C-state transitions,
> this seems to be happening, if I do read i2c transfers on the pmic
> i2c bus repeatedly without blocking the CPU from entering C6 (*) while
> accessing the i2c bus my cherrytrail tablet will freeze in 10 - 30
> seconds.
>
> The findings of one of the users commenting in:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=155241
>
> Seem to indicate a similar problem with the i915 driver doing
> power-management while the i2c-designware-baytrail code is holding
> the punit i2c bus semaphore. One would hope that the punit would be
> smart enough to simply wait for the semaphore to get released in that
> case, but at least for the C6 CPU core transition it seems that allowing
> that to happen while holding the semaphore causes a hard crash of the
> SoC. So I guess that for explicit requests the punit code assumes that
> the OS is not holding the semaphore.
>
> Regards,
>
> Hans
>
>
>
> *) which powers off most of the core, so likely causes interaction with
> the pmic
>
>
> >
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++
> >> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
> >> 2 files changed, 32 insertions(+)
> >>
> >> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
> >> index b41ee16..02963bd 100644
> >> --- a/arch/x86/include/asm/iosf_mbi.h
> >> +++ b/arch/x86/include/asm/iosf_mbi.h
> >> @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
> >> */
> >> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
> >>
> >> +/**
> >> + * iosf_mbi_punit_lock() - Lock the punit mutex
> >> + *
> >> + * This function must be called before accessing the punit or the pmic, be it
> >> + * through iosf_mbi_* or through other means.
> >> + *
> >> + * This function locks a mutex, as such it may sleep.
> >> + */
> >> +void iosf_mbi_punit_lock(void);
> >> +
> >> +/**
> >> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
> >> + */
> >> +void iosf_mbi_punit_unlock(void);
> >> +
> >> #else /* CONFIG_IOSF_MBI is not enabled */
> >> static inline
> >> bool iosf_mbi_available(void)
> >> @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
> >> WARN(1, "IOSF_MBI driver not available");
> >> return -EPERM;
> >> }
> >> +
> >> +static inline void iosf_mbi_punit_lock(void) {}
> >> +static inline void iosf_mbi_punit_unlock(void) {}
> >> +
> >> #endif /* CONFIG_IOSF_MBI */
> >>
> >> #endif /* IOSF_MBI_SYMS_H */
> >> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
> >> index edf2c54..75d8135 100644
> >> --- a/arch/x86/platform/intel/iosf_mbi.c
> >> +++ b/arch/x86/platform/intel/iosf_mbi.c
> >> @@ -34,6 +34,7 @@
> >>
> >> static struct pci_dev *mbi_pdev;
> >> static DEFINE_SPINLOCK(iosf_mbi_lock);
> >> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> >>
> >> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> >> {
> >> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
> >> }
> >> EXPORT_SYMBOL(iosf_mbi_available);
> >>
> >> +void iosf_mbi_punit_lock(void)
> >> +{
> >> + mutex_lock(&iosf_mbi_punit_mutex);
> >> +}
> >> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
> >> +
> >> +void iosf_mbi_punit_unlock(void)
> >> +{
> >> + mutex_unlock(&iosf_mbi_punit_mutex);
> >> +}
> >> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
> >> +
> >> #ifdef CONFIG_IOSF_MBI_DEBUG
> >> static u32 dbg_mdr;
> >> static u32 dbg_mcr;
> >> --
> >> 2.9.3
> >
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-13 9:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Patrice Chotard, Russell King, linux-i2c,
devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <20170113084513.wvuxkanrlmgz3ang@pengutronix.de>
Ok so I am going to send the v9 asap.
Thanks
2017-01-13 9:45 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
>>
>> > (But note that this is irrelevant for the patch as the driver doesn't
>> > claim to support this kind of transfer.)
>>
>> Yes, I wanted to mention that, too.
>>
>> I'd think the series is good to go in?
>
> AFAICT there are some unaddressed comments that Cedrics claimed to fix
> before our discussion was dominated by block transfers.
>
> Best regards
> Uwe
>
>
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] i2c: core: don't try to OF populate DDC i2c buses
From: Lucas Stach @ 2017-01-13 10:14 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Vladimir Zapolskiy, patchwork-lst, linux-i2c, kernel
In-Reply-To: <20161211221622.GJ2552@katana>
Am Sonntag, den 11.12.2016, 23:16 +0100 schrieb Wolfram Sang:
> Lucas,
>
> > Shallow review of DTS files and dpaux drivers let me say that the change
> > below has no regressions (the change is untested):
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 3e6fe82..f91ade1 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > aux->ddc.class = I2C_CLASS_DDC;
> > aux->ddc.owner = THIS_MODULE;
> > aux->ddc.dev.parent = aux->dev;
> > - aux->ddc.dev.of_node = aux->dev->of_node;
> >
> > strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> > sizeof(aux->ddc.name));
>
> Is this an acceptable approach? It makes sense to me from an I2C PoV.
>
> I have to say that I second Vladimir's arguments. There shouldn't be any
> other I2C devices on DDC, but surely there is somebody somewhere hacking
> this bus to do something.
Yes, I've convinced myself that this is the better way.
Please drop this patch.
Regards,
Lucas
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Jarkko Nikula @ 2017-01-13 10:34 UTC (permalink / raw)
To: Ville Syrjälä, Hans de Goede
Cc: russianneuromancer @ ya . ru, intel-gfx, linux-i2c, Len Brown
In-Reply-To: <20170113092644.GD31595@intel.com>
On 01/13/2017 11:26 AM, Ville Syrjälä wrote:
> It also feels quite hand wavy since the punit could do whatever at
> any time AFAIK. Eg. if there's some thermal event or something the
> punit might kick into action. So trying to protect this from the OS
> side might not be able to avoid these problems entirely. It feels like
> there really should be some kind of shared hardware/firmware mutex
> with the punit to arbitrate access to the i2c bus.
>
There is an HW semaphore for I2C access. It is implemented in
drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans
is adding support for Cherrytrail into it.
--
Jarkko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Ville Syrjälä @ 2017-01-13 10:51 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Hans de Goede, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <3e46500b-8e22-6aa1-8142-97bcbd5d5571@linux.intel.com>
On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote:
> On 01/13/2017 11:26 AM, Ville Syrjälä wrote:
> > It also feels quite hand wavy since the punit could do whatever at
> > any time AFAIK. Eg. if there's some thermal event or something the
> > punit might kick into action. So trying to protect this from the OS
> > side might not be able to avoid these problems entirely. It feels like
> > there really should be some kind of shared hardware/firmware mutex
> > with the punit to arbitrate access to the i2c bus.
> >
> There is an HW semaphore for I2C access. It is implemented in
> drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans
> is adding support for Cherrytrail into it.
Then why do we need anything else?
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Jarkko Nikula @ 2017-01-13 11:12 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Hans de Goede, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <20170113105127.GH31595@intel.com>
On 01/13/2017 12:51 PM, Ville Syrjälä wrote:
> On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote:
>> On 01/13/2017 11:26 AM, Ville Syrjälä wrote:
>>> It also feels quite hand wavy since the punit could do whatever at
>>> any time AFAIK. Eg. if there's some thermal event or something the
>>> punit might kick into action. So trying to protect this from the OS
>>> side might not be able to avoid these problems entirely. It feels like
>>> there really should be some kind of shared hardware/firmware mutex
>>> with the punit to arbitrate access to the i2c bus.
>>>
>> There is an HW semaphore for I2C access. It is implemented in
>> drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans
>> is adding support for Cherrytrail into it.
>
> Then why do we need anything else?
>
From this patch: "The punit on baytrail / cherrytrail systems is not
only accessed through the iosf_mbi functions, but also by the i915 code."
--
Jarkko
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Ville Syrjälä @ 2017-01-13 12:20 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Hans de Goede, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <d504f816-96f6-a745-e73c-eb0ea1bd8620@linux.intel.com>
On Fri, Jan 13, 2017 at 01:12:15PM +0200, Jarkko Nikula wrote:
> On 01/13/2017 12:51 PM, Ville Syrjälä wrote:
> > On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote:
> >> On 01/13/2017 11:26 AM, Ville Syrjälä wrote:
> >>> It also feels quite hand wavy since the punit could do whatever at
> >>> any time AFAIK. Eg. if there's some thermal event or something the
> >>> punit might kick into action. So trying to protect this from the OS
> >>> side might not be able to avoid these problems entirely. It feels like
> >>> there really should be some kind of shared hardware/firmware mutex
> >>> with the punit to arbitrate access to the i2c bus.
> >>>
> >> There is an HW semaphore for I2C access. It is implemented in
> >> drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans
> >> is adding support for Cherrytrail into it.
> >
> > Then why do we need anything else?
> >
> From this patch: "The punit on baytrail / cherrytrail systems is not
> only accessed through the iosf_mbi functions, but also by the i915 code."
I don't see how that's relevant at all. Multiple things accessing the
punit concurrently should be perfectly fine as long as they don't frob
the same registers.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [PATCH] i2c: i2c-cadence: Don't register the adapter until it's ready
From: Mike Looijmans @ 2017-01-13 14:52 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-arm-kernel, linux-kernel, michal.simek, linux-i2c,
Vladimir Zapolskiy, soren.brinkmann
In-Reply-To: <20170113083727.GB1446@katana>
On 13-01-17 09:37, Wolfram Sang wrote:
>
>> I would argue that the "info" message means "the I2C adapter is ready for
>> transaction now, and we'll start initializing devices on the bus". That is
>> the case before it calls i2c_add_adapter().
>
> I know what you mean, but i2c_add_adapter does more, and it can fail
> because the adapter is *not* ready to transfer. Seeing the success
> message before is also confusing.
>
>> When i2c_add_adapter() runs, it will start probing devices on the bus. This
>> yields very confusing output, as it will output things in a reversed order:
>>
>> - device X on I2C bus
>> - device Y on I2C bus
>> - cdns-i2c ff030000.i2c: 100 kHz mmio ff030000 irq 197
>
> I agree. That being said, somewhen I started working on moving such
> messages into the core to save string space and have consistent output.
> Then, we can print at the proper time.
>
> So, until then, we should be consistent with the other driver, I'd say.
Makes sense.
I'll create a v2 patch to just move the i2c_add_adapter to after writing the
configuration registers, and leave the dmesg output as is.
Thanks for reviewing,
Mike.
Kind regards,
Mike Looijmans
System Expert
TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com
Please consider the environment before printing this e-mail
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-13 15:43 UTC (permalink / raw)
To: Peter Rosin, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <383ee3cb-b452-259f-486f-682ea9526708-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 13/01/2017 17:11, Peter Rosin wrote:
> On 2017-01-09 10:02, Phil Reid wrote:
>> Various muxes can aggregate multiple interrupts from each i2c bus.
>> All of the muxes with interrupt support combine the active low irq lines
>> using an internal 'and' function and generate a combined active low
>> output. The muxes do provide the ability to read a control register to
>> determine which irq is active. By making the mux an irq controller isr
>> latency can potentially be reduced by reading the status register and
>> then only calling the registered isr on that bus segment.
>>
>> As there is no irq masking on the mux irq are disabled until irq_unmask is
>> called at least once.
>>
>
> I had a second reading of this patch. I'm still no master-of-irqs, though.
> Anyway, I have some questions below. I guess it mostly shows that I don't
> really know what I'm talking about here...
I'm no expert either...
>
>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>> ---
>> drivers/i2c/muxes/i2c-mux-pca954x.c | 127 +++++++++++++++++++++++++++++++++++-
>> 1 file changed, 125 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index bbf088e..84fc767 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -41,14 +41,19 @@
>> #include <linux/i2c.h>
>> #include <linux/i2c-mux.h>
>> #include <linux/i2c/pca954x.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> #include <linux/pm.h>
>> #include <linux/slab.h>
>>
>> #define PCA954X_MAX_NCHANS 8
>>
>> +#define PCA954X_IRQ_OFFSET 4
>> +
>> enum pca_type {
>> pca_9540,
>> pca_9542,
>> @@ -63,6 +68,7 @@ enum pca_type {
>> struct chip_desc {
>> u8 nchans;
>> u8 enable; /* used for muxes only */
>> + u8 has_irq;
>> enum muxtype {
>> pca954x_ismux = 0,
>> pca954x_isswi
>> @@ -75,6 +81,9 @@ struct pca954x {
>> u8 last_chan; /* last register value */
>> u8 deselect;
>> struct i2c_client *client;
>> +
>> + struct irq_domain *irq;
>> + unsigned int irq_mask;
>> };
>>
>> /* Provide specs for the PCA954x types we know about */
>> @@ -87,19 +96,23 @@ struct pca954x {
>> [pca_9542] = {
>> .nchans = 2,
>> .enable = 0x4,
>> + .has_irq = 1,
>> .muxtype = pca954x_ismux,
>> },
>> [pca_9543] = {
>> .nchans = 2,
>> + .has_irq = 1,
>> .muxtype = pca954x_isswi,
>> },
>> [pca_9544] = {
>> .nchans = 4,
>> .enable = 0x4,
>> + .has_irq = 1,
>> .muxtype = pca954x_ismux,
>> },
>> [pca_9545] = {
>> .nchans = 4,
>> + .has_irq = 1,
>> .muxtype = pca954x_isswi,
>> },
>> [pca_9547] = {
>> @@ -222,6 +235,102 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>> return pca954x_reg_write(muxc->parent, client, data->last_chan);
>> }
>>
>> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>> +{
>> + struct pca954x *data = dev_id;
>> + unsigned int child_irq;
>> + int ret, i, handled;
>> +
>> + ret = i2c_smbus_read_byte(data->client);
>> + if (ret < 0)
>> + return IRQ_NONE;
>> +
>> + for (i = 0; i < data->chip->nchans; i++) {
>> + if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
>> + child_irq = irq_linear_revmap(data->irq, i);
>> + handle_nested_irq(child_irq);
>> + handled++;
>> + }
>> + }
>> + return handled ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +static void pca954x_irq_mask(struct irq_data *idata)
>> +{
>> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> + unsigned int pos = idata->hwirq;
>> +
>> + data->irq_mask &= ~BIT(pos);
>> + if (!data->irq_mask)
>> + disable_irq(data->client->irq);
>> +}
>> +
>> +static void pca954x_irq_unmask(struct irq_data *idata)
>> +{
>> + struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> + unsigned int pos = idata->hwirq;
>> +
>> + if (!data->irq_mask)
>> + enable_irq(data->client->irq);
>> + data->irq_mask |= BIT(pos);
>> +}
>
> I assume the irq core makes sure that .irq_mask and .irq_unmask may not
> be called concurrently?
I also wasn't 100% sure about this myself.
Looking again at other drivers and the core irq code I'm still not sure.
But I think on review a lock of some kind is needed.
I think a spin_lock_irqsave would do it.
Will test next week and post a new version.
>
>> +
>> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
>> +{
>> + if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
>> + return -EINVAL;
>> + return 0;
>> +}
>> +
>> +static struct irq_chip pca954x_irq_chip = {
>> + .name = "i2c-mux-pca954x",
>> + .irq_mask = pca954x_irq_mask,
>> + .irq_unmask = pca954x_irq_unmask,
>> + .irq_set_type = pca954x_irq_set_type,
>> +};
>> +
>> +static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>> +{
>> + struct pca954x *data = i2c_mux_priv(muxc);
>> + struct i2c_client *client = data->client;
>> + int c, err, irq;
>> +
>> + if (!data->chip->has_irq || client->irq <= 0)
>> + return 0;
>
> I assume "client->irq <= 0" means that users not specifying any interrupts
> continue to behave as they use to, right?
>
> BTW, what does client->irq == 0 represent?
This one I'm fairly confident on. Initially I had "client->irq < 0"
Which caused problems with an i2c mux that doesn't have irq's defined in the dt.
This commit seems to confirm that.
commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned")
And various patches converting to "<= 0" checks.
eg:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/364630.html
>
>
>
>> +
>> + data->irq = irq_domain_add_linear(client->dev.of_node,
>> + data->chip->nchans,
>> + &irq_domain_simple_ops, data);
>> + if (!data->irq)
>> + return -ENODEV;
>> +
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_create_mapping(data->irq, c);
>> + irq_set_chip_data(irq, data);
>> + irq_set_chip_and_handler(irq, &pca954x_irq_chip,
>> + handle_simple_irq);
>> + }
>> +
>> + err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
>> + pca954x_irq_handler,
>> + IRQF_ONESHOT | IRQF_SHARED,
>> + "pca954x", data);
>> + if (err)
>> + goto err_req_irq;
>> +
>> + disable_irq(data->client->irq);
>> +
>> + return 0;
>> +err_req_irq:
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_find_mapping(data->irq, c);
>> + irq_dispose_mapping(irq);
>> + }
>> + irq_domain_remove(data->irq);
>> +
>> + return err;
>> +}
>> +
>> /*
>> * I2C init/probing/exit functions
>> */
>> @@ -286,6 +395,10 @@ static int pca954x_probe(struct i2c_client *client,
>> idle_disconnect_dt = of_node &&
>> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>>
>> + ret = pca954x_irq_setup(muxc);
>> + if (ret)
>> + goto fail_del_adapters;
>> +
>> /* Now create an adapter for each channel */
>> for (num = 0; num < data->chip->nchans; num++) {
>> bool idle_disconnect_pd = false;
>> @@ -311,7 +424,7 @@ static int pca954x_probe(struct i2c_client *client,
>> dev_err(&client->dev,
>> "failed to register multiplexed adapter"
>> " %d as bus %d\n", num, force);
>> - goto virt_reg_failed;
>> + goto fail_del_adapters;
>> }
>> }
>>
>> @@ -322,7 +435,7 @@ static int pca954x_probe(struct i2c_client *client,
>>
>> return 0;
>>
>> -virt_reg_failed:
>> +fail_del_adapters:
>> i2c_mux_del_adapters(muxc);
>> return ret;
>> }
>> @@ -330,6 +443,16 @@ static int pca954x_probe(struct i2c_client *client,
>> static int pca954x_remove(struct i2c_client *client)
>> {
>> struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>> + struct pca954x *data = i2c_mux_priv(muxc);
>> + int c, irq;
>> +
>> + if (data->irq) {
>> + for (c = 0; c < data->chip->nchans; c++) {
>> + irq = irq_find_mapping(data->irq, c);
>> + irq_dispose_mapping(irq);
>> + }
>> + irq_domain_remove(data->irq);
>> + }
>>
>> i2c_mux_del_adapters(muxc);
>> return 0;
>>
>
>
>
--
Regards
Phil Reid
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-13 16:06 UTC (permalink / raw)
To: Ville Syrjälä
Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c,
Len Brown
In-Reply-To: <20170113092644.GD31595@intel.com>
Hi,
On 01/13/2017 10:26 AM, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-01-17 15:12, Ville Syrjälä wrote:
>>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
>>>> The punit on baytrail / cherrytrail systems is not only accessed through
>>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
>>>> the punit against simultaneous accesses and 2 functions to lock / unlock
>>>> this mutex.
>>>
>>> I'm not sure which part of punit you're actually trying to protect
>>> here. Some specific registers?
>>
>> The theory I'm going by is that for certain actions / certain requests
>> we send to the punit, the punit needs to access the (axp288) pmic, to
>> change (or enable / disable) certain voltages.
>
> At least for cpu/display/gt voltages that shouldn't really be the case.
> The vcc/vnn/vgg rails are controlled via svid, not i2c.
Are you sure? The ax288 pmic does not have a svid interface, only
an i2c interface, and AFAICT its buck DCDC converters are used to
feed all of these.
> It also feels quite hand wavy since the punit could do whatever at
> any time AFAIK. Eg. if there's some thermal event or something the
> punit might kick into action. So trying to protect this from the OS
> side might not be able to avoid these problems entirely. It feels like
> there really should be some kind of shared hardware/firmware mutex
> with the punit to arbitrate access to the i2c bus.
Right, and there is such a mutex (which only gets used on systems
with an axp288 pmic...) and we are taking this mutex before starting
an i2c transaction on the pmic i2c bus. But this does not seem to be
enough. It seems the the punit does not check the mutex before
certain OS / host triggered actions. I guess it expects the host to
do this itself.
Please see my new (non RFC) version of this series I've posted.
There are at least 2 problems when relying solely on the punit
pmic i2c bus sempaphore:
1) CPU C1 <-> C6 transations happening while the pmic i2c bus
is being accessed by the host cause the system to hang
2) i915 (runtime) suspend resume fails every other attempt
with timeouts when trying to get a forcewake lock inn i915,
often followed by a system freeze shortly after this.
My non RFC version of this patch-set fixes both.
So summarizing yes you are right that there should be some
hardware mutex (there is, and we are already taking it), but
unfortunately that does not seem to be enough, when explicitly
requesting some power state transation, while another driver
is acccessing the pmic i2c bus bad things happen. Adding
some exclusion mechanism here seems to be necessary.
Note that the i2c acccess vs i915 forcewake issues was
first reported by an user who was trying my patches for
fixing pmic i2c access on cht (fixing the semaphore code on
cht) and then noticed a problem with the i915 driver as soon
as the pmic i2c bus was used. He has also confirmed that the
new non RFC version of my patches fix this.
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Ville Syrjälä @ 2017-01-13 16:30 UTC (permalink / raw)
To: Hans de Goede
Cc: Jarkko Nikula, Len Brown, Jani Nikula,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
In-Reply-To: <a213718a-b4e9-09a0-a97f-69eb5e892151@redhat.com>
On Fri, Jan 13, 2017 at 05:06:52PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/13/2017 10:26 AM, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 02-01-17 15:12, Ville Syrjälä wrote:
> >>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote:
> >>>> The punit on baytrail / cherrytrail systems is not only accessed through
> >>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
> >>>> the punit against simultaneous accesses and 2 functions to lock / unlock
> >>>> this mutex.
> >>>
> >>> I'm not sure which part of punit you're actually trying to protect
> >>> here. Some specific registers?
> >>
> >> The theory I'm going by is that for certain actions / certain requests
> >> we send to the punit, the punit needs to access the (axp288) pmic, to
> >> change (or enable / disable) certain voltages.
> >
> > At least for cpu/display/gt voltages that shouldn't really be the case.
> > The vcc/vnn/vgg rails are controlled via svid, not i2c.
>
> Are you sure? The ax288 pmic does not have a svid interface, only
> an i2c interface, and AFAICT its buck DCDC converters are used to
> feed all of these.
Yes, looks like you're right. I guess someone didn't want to spend three
pins for svid.
>
> > It also feels quite hand wavy since the punit could do whatever at
> > any time AFAIK. Eg. if there's some thermal event or something the
> > punit might kick into action. So trying to protect this from the OS
> > side might not be able to avoid these problems entirely. It feels like
> > there really should be some kind of shared hardware/firmware mutex
> > with the punit to arbitrate access to the i2c bus.
>
> Right, and there is such a mutex (which only gets used on systems
> with an axp288 pmic...) and we are taking this mutex before starting
> an i2c transaction on the pmic i2c bus. But this does not seem to be
> enough. It seems the the punit does not check the mutex before
> certain OS / host triggered actions. I guess it expects the host to
> do this itself.
>
> Please see my new (non RFC) version of this series I've posted.
>
> There are at least 2 problems when relying solely on the punit
> pmic i2c bus sempaphore:
>
> 1) CPU C1 <-> C6 transations happening while the pmic i2c bus
> is being accessed by the host cause the system to hang
> 2) i915 (runtime) suspend resume fails every other attempt
> with timeouts when trying to get a forcewake lock inn i915,
> often followed by a system freeze shortly after this.
Hmm. But forcewake works at other times? That seems quite strange.
Runtime suspend itself shouldn't really do much, and if we're still
poking at the the hw then we haven't really even suspended anything
yet, so having failing forcewake doesn't sounds at all good.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* Re: [PATCH v2 1/2] iio: distance: srf08: add trivial DT binding
From: Rob Herring @ 2017-01-13 17:18 UTC (permalink / raw)
To: Andreas Klinger
Cc: jic23, knaack.h, lars, pmeerw, linux-iio, linux-kernel, ktsai,
wsa, pawel.moll, mark.rutland, ijc+devicetree, galak, trivial,
mranostay, linux-i2c, devicetree
In-Reply-To: <20170110184744.GA15520@andreas>
On Tue, Jan 10, 2017 at 07:47:44PM +0100, Andreas Klinger wrote:
> Add DT binding for devantech,srf08
> Add vendor devantech to vendor list
>
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
> Documentation/devicetree/bindings/i2c/trivial-devices.txt | 1 +
> Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
> 2 files changed, 2 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support
From: Wolfram Sang @ 2017-01-13 18:54 UTC (permalink / raw)
To: Phil Reid
Cc: Peter Rosin, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1284b1c8-bb90-fbd9-53a2-0c36f2597b29-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 814 bytes --]
> >BTW, what does client->irq == 0 represent?
>
> This one I'm fairly confident on. Initially I had "client->irq < 0"
> Which caused problems with an i2c mux that doesn't have irq's defined in the dt.
>
> This commit seems to confirm that.
> commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned")
In the Linux Kernel, 0 means no irq so one can write:
if (!irq) ...
There used to be NO_IRQ per arch but this is converted over, although it
takes a lot of time... If there is a HW IRQ0, it needs to be remapped.
But since we rely on irq_desc these days and not irq numbers, this is
easy.
At least this was the transition a few years ago, but I don't think
something essential has changed. Please enlighten me, if so.
Regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger
From: Jonathan Cameron @ 2017-01-14 12:17 UTC (permalink / raw)
To: Andreas Klinger, knaack.h-Mmb7MZpHnFY,
lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ktsai-GubuWUlQtMwciDkP5Hr2oA,
wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, trivial-DgEjT+Ai2ygdnm+yROfE0A,
mranostay-Re5JQEeQqe8AvxtiuMwx3w,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170110184815.GA15532@andreas>
On 10/01/17 18:48, Andreas Klinger wrote:
> This is the IIO driver for devantech srf08 ultrasonic ranger which can be
> used to measure the distances to an object.
>
> The sensor supports I2C with some registers.
>
> Supported Features include:
> - read the distance in ranging mode in centimeters
> - output of the driver is directly the read value
> - together with the scale the driver delivers the distance in meters
> - only the first echo of the nearest object is delivered
> - set max gain register; userspace enters analogue value
> - set range registers; userspace enters range in millimeters in 43 mm steps
>
> Features not supported by this driver:
> - ranging mode in inches or in microseconds
> - ANN mode
> - change I2C address through this driver
> - light sensor
>
> The driver was added in the directory "proximity" of the iio subsystem
> in absence of another directory named "distance".
> There is also a new submenu "distance"
Hi Andreas,
Sorry it took me a while to get to this!
I'd not bother with the new submenu. Perhaps we should rename the
proximity menu to proximity/distance.
We already the lightening detector in there which is definitely not
measuring proximity in the convetional sense!
Anyhow, the actual code is fine, but we need to think about how the
userspace ABI fits within the wider IIO ABI. Naming and approaches
that make sense in a single class of drivers can end up meaining
very different things for other drivers. Various suggestions inline.
Jonathan
>
> Signed-off-by: Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> ---
> drivers/iio/proximity/Kconfig | 15 ++
> drivers/iio/proximity/Makefile | 1 +
> drivers/iio/proximity/srf08.c | 362 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 378 insertions(+)
> create mode 100644 drivers/iio/proximity/srf08.c
>
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index ef4c73db5b53..7b10a137702b 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -46,3 +46,18 @@ config SX9500
> module will be called sx9500.
>
> endmenu
> +
> +menu "Distance sensors"
> +
> +config SRF08
> + tristate "Devantech SRF08 ultrasonic ranger sensor"
> + depends on I2C
> + help
> + Say Y here to build a driver for Devantech SRF08 ultrasonic
> + ranger sensor. This driver can be used to measure the distance
> + of objects.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called srf08.
> +
> +endmenu
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 9aadd9a8ee99..e914c2a5dd49 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -5,4 +5,5 @@
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AS3935) += as3935.o
> obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> +obj-$(CONFIG_SRF08) += srf08.o
> obj-$(CONFIG_SX9500) += sx9500.o
> diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
> new file mode 100644
> index 000000000000..f38c74ed0933
> --- /dev/null
> +++ b/drivers/iio/proximity/srf08.c
> @@ -0,0 +1,362 @@
> +/*
> + * srf08.c - Support for Devantech SRF08 ultrasonic ranger
> + *
> + * Copyright (c) 2016 Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * For details about the device see:
> + * http://www.robot-electronics.co.uk/htm/srf08tech.html
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +/* registers of SRF08 device */
> +#define SRF08_WRITE_COMMAND 0x00 /* Command Register */
> +#define SRF08_WRITE_MAX_GAIN 0x01 /* Max Gain Register: 0 .. 31 */
> +#define SRF08_WRITE_RANGE 0x02 /* Range Register: 0 .. 255 */
> +#define SRF08_READ_SW_REVISION 0x00 /* Software Revision */
> +#define SRF08_READ_LIGHT 0x01 /* Light Sensor during last echo */
> +#define SRF08_READ_ECHO_1_HIGH 0x02 /* Range of first echo received */
> +#define SRF08_READ_ECHO_1_LOW 0x03 /* Range of first echo received */
> +
> +#define SRF08_CMD_RANGING_CM 0x51 /* Ranging Mode - Result in cm */
> +
> +#define SRF08_DEFAULT_GAIN 1025 /* max. analogue value of Gain */
> +#define SRF08_DEFAULT_RANGE 11008 /* max. value of Range in mm */
> +
> +struct srf08_data {
> + struct i2c_client *client;
> + int gain; /* Max Gain */
> + int range_mm; /* Range in mm */
> + struct mutex lock;
> +};
> +
> +static const int srf08_gain[] = {
> + 94, 97, 100, 103, 107, 110, 114, 118,
> + 123, 128, 133, 139, 145, 152, 159, 168,
> + 177, 187, 199, 212, 227, 245, 265, 288,
> + 317, 352, 395, 450, 524, 626, 777, 1025 };
> +
> +static int srf08_read_ranging(struct srf08_data *data)
> +{
> + struct i2c_client *client = data->client;
> + int ret, i;
> +
> + mutex_lock(&data->lock);
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + SRF08_WRITE_COMMAND, SRF08_CMD_RANGING_CM);
> + if (ret < 0) {
> + dev_err(&client->dev, "write command - err: %d\n", ret);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + /*
> + * normally after 65 ms the device should have the read value
> + * we round it up to 100 ms
I'd suggest this should be adapted so that it takes advantage of knowing
roughly how long it is going to take as the 'range' maximum is changed.
So perhaps in the basic case, sleep for 65 msecs, then poll at 5msec
intervals. If we know it's going to be a lot faster, then poll it from
an earlier time.
> + *
> + * we read here until a correct version number shows up as
> + * suggested by the documentation
> + */
> + for (i = 0; i < 5; i++) {
> + ret = i2c_smbus_read_byte_data(data->client,
> + SRF08_READ_SW_REVISION);
> +
> + /* check if a valid version number is read */
> + if (ret < 255 && ret > 0)
> + break;
> + msleep(20);
> + }
> +
> + if (ret >= 255 || ret <= 0) {
> + dev_err(&client->dev, "device not ready\n");
> + mutex_unlock(&data->lock);
> + return -EIO;
> + }
> +
> + ret = i2c_smbus_read_word_swapped(data->client,
> + SRF08_READ_ECHO_1_HIGH);
> + if (ret < 0) {
> + dev_err(&client->dev, "cannot read distance: ret=%d\n", ret);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + mutex_unlock(&data->lock);
> +
> + return ret;
> +}
> +
> +static int srf08_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct srf08_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (channel->type != IIO_DISTANCE)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = srf08_read_ranging(data);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + /* 1 LSB is 1 cm */
> + *val = 0;
> + *val2 = 10000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t srf08_show_range_mm_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int i, len = 0;
> +
> + for (i = 0; i < 256; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len,
> + "%d ", (i + 1) * 43);
> +
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(range_mm_available, S_IRUGO,
> + srf08_show_range_mm_available, NULL, 0);
> +
> +static ssize_t srf08_show_range_mm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct srf08_data *data = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%d\n", data->range_mm);
> +}
> +
> +/*
> + * set the range of the sensor to an even multiple of 43 mm
> + * which corresponds to 1 LSB in the register
> + *
> + * register value corresponding range
> + * 0x00 43 mm
> + * 0x01 86 mm
> + * 0x02 129 mm
> + * ...
> + * 0xFF 11008 mm
> + */
> +static ssize_t srf08_write_range_mm(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct srf08_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int ret;
> + unsigned int val, mod;
> + u8 regval;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + ret = val / 43 - 1;
> + mod = val % 43;
> +
> + if (mod || (ret < 0) || (ret > 255))
> + return -EINVAL;
> +
> + regval = ret;
> +
> + mutex_lock(&data->lock);
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + SRF08_WRITE_RANGE, regval);
> + if (ret < 0) {
> + dev_err(&client->dev, "write_range - err: %d\n", ret);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + data->range_mm = val;
> +
> + mutex_unlock(&data->lock);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(range_mm, S_IRUGO | S_IWUSR,
> + srf08_show_range_mm, srf08_write_range_mm, 0);
> +
> +static ssize_t srf08_show_gain_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int i, len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> + len += sprintf(buf + len, "%d ", srf08_gain[i]);
> +
> + len += sprintf(buf + len, "\n");
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(gain_available, S_IRUGO,
> + srf08_show_gain_available, NULL, 0);
> +
> +static ssize_t srf08_show_gain(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct srf08_data *data = iio_priv(indio_dev);
> + int len;
> +
> + len = sprintf(buf, "%d\n", data->gain);
> +
> + return len;
> +}
> +
> +static ssize_t srf08_write_gain(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct srf08_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int ret, i;
> + unsigned int val;
> + u8 regval;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> + if (val == srf08_gain[i]) {
> + regval = i;
> + break;
> + }
> +
> + if (i >= ARRAY_SIZE(srf08_gain))
> + return -EINVAL;
> +
> + mutex_lock(&data->lock);
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + SRF08_WRITE_MAX_GAIN, regval);
> + if (ret < 0) {
> + dev_err(&client->dev, "write_gain - err: %d\n", ret);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + data->gain = val;
> +
> + mutex_unlock(&data->lock);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
> + srf08_show_gain, srf08_write_gain, 0);
> +
> +static struct attribute *srf08_attributes[] = {
> + &iio_dev_attr_range_mm.dev_attr.attr,
> + &iio_dev_attr_range_mm_available.dev_attr.attr,
> + &iio_dev_attr_gain.dev_attr.attr,
> + &iio_dev_attr_gain_available.dev_attr.attr,
Hmm. Custom attributes always give us issues. The primary point of IIO
is to enforce (more or less) standard interfaces.
If you do need to add something new then that is fine (and I do think
you need to here!).
They need to be formally proposed as an addition to the ABI with
docs in /Documentation/ABI/testing/sysfs-bus-iio*
Once we take one driver using it it becomes part of our ABI that
userspace will need to handle, hence we consider these very
carefully.
My gut feeling would be that gain needs to be more specific as it's
a term that can mean very different things.. Here we are talking
about an amplifier on a signal that we are then looking at the timing
of. It might otherwise be interpretted as another term for what
we term 'scale' in IIO.
So what to call it... Perhaps afegain for Analog front end gain?
We might want to add this to the core supported attrs, but lets
not do so until we see if we have this on a number of devices.
The description would need to make it explicit that this gain is
for cases where we aren't measuring the magnitude of what is
being amplified.
For the range, it's an interesting one. Again the term range could
mean too many things within the wider ABI. We need to make it more
specific.
Actually reading the datasheet, I think this is fundamentally about the
maximum sampling frequency rather than directly about the range.
The only reason you'd reduce the range is to speed that up. It doesn't
improve the resolution, the device simply answers quicker.
So I'd support this as sampling_frequency. You could then use
the the iio_info_mask_*_available and relevant callback to provide
info on what it then restricts the possible output values to
(rather than controlling it directly).
> + NULL,
> +};
> +
> +static const struct attribute_group srf08_attribute_group = {
> + .attrs = srf08_attributes,
> +};
> +
> +static const struct iio_chan_spec srf08_channels[] = {
> + {
> + .type = IIO_DISTANCE,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + },
> +};
> +
> +static const struct iio_info srf08_info = {
> + .read_raw = srf08_read_raw,
> + .attrs = &srf08_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int srf08_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *indio_dev;
> + struct srf08_data *data;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_READ_WORD_DATA))
> + return -ENODEV;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + /*
> + * set default values of device here
> + * these values are already set on the hardware after power on
> + */
> + data->gain = SRF08_DEFAULT_GAIN;
> + data->range_mm = SRF08_DEFAULT_RANGE;
We should be a little careful with assumptions about the device having
just been powered on. The driver might simply have been removed and
reprobed. So I'd sugest rewriting them whatever to be sure we have
what we expect. Either that or if they can be read back, then just
always retrieve them from the device.
> +
> + indio_dev->name = dev_name(&client->dev);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &srf08_info;
> + indio_dev->channels = srf08_channels;
> + indio_dev->num_channels = ARRAY_SIZE(srf08_channels);
> +
> + mutex_init(&data->lock);
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id srf08_id[] = {
> + { "srf08", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, srf08_id);
> +
> +static struct i2c_driver srf08_driver = {
> + .driver = {
> + .name = "srf08",
> + },
> + .probe = srf08_probe,
> + .id_table = srf08_id,
> +};
> +module_i2c_driver(srf08_driver);
> +
> +MODULE_AUTHOR("Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>");
> +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> +MODULE_LICENSE("GPL");
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger
From: Andreas Klinger @ 2017-01-14 17:48 UTC (permalink / raw)
To: Jonathan Cameron
Cc: knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
pmeerw-jW+XmwGofnusTnJN9+BGXg, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, ktsai-GubuWUlQtMwciDkP5Hr2oA,
wsa-z923LK4zBo2bacvFa/9K2g, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, trivial-DgEjT+Ai2ygdnm+yROfE0A,
mranostay-Re5JQEeQqe8AvxtiuMwx3w,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4099feac-959c-2b5b-a21a-3b111098af39-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Hi Jonathan,
see comments below.
Andreas
Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> schrieb am Sat, 14. Jan 12:17:
> On 10/01/17 18:48, Andreas Klinger wrote:
> > This is the IIO driver for devantech srf08 ultrasonic ranger which can be
> > used to measure the distances to an object.
> >
> > The sensor supports I2C with some registers.
> >
> > Supported Features include:
> > - read the distance in ranging mode in centimeters
> > - output of the driver is directly the read value
> > - together with the scale the driver delivers the distance in meters
> > - only the first echo of the nearest object is delivered
> > - set max gain register; userspace enters analogue value
> > - set range registers; userspace enters range in millimeters in 43 mm steps
> >
> > Features not supported by this driver:
> > - ranging mode in inches or in microseconds
> > - ANN mode
> > - change I2C address through this driver
> > - light sensor
> >
> > The driver was added in the directory "proximity" of the iio subsystem
> > in absence of another directory named "distance".
> > There is also a new submenu "distance"
> Hi Andreas,
>
> Sorry it took me a while to get to this!
>
> I'd not bother with the new submenu. Perhaps we should rename the
> proximity menu to proximity/distance.
>
> We already the lightening detector in there which is definitely not
> measuring proximity in the convetional sense!
>
> Anyhow, the actual code is fine, but we need to think about how the
> userspace ABI fits within the wider IIO ABI. Naming and approaches
> that make sense in a single class of drivers can end up meaining
> very different things for other drivers. Various suggestions inline.
>
> Jonathan
> >
> > Signed-off-by: Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> > ---
> > drivers/iio/proximity/Kconfig | 15 ++
> > drivers/iio/proximity/Makefile | 1 +
> > drivers/iio/proximity/srf08.c | 362 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 378 insertions(+)
> > create mode 100644 drivers/iio/proximity/srf08.c
> >
> > diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> > index ef4c73db5b53..7b10a137702b 100644
> > --- a/drivers/iio/proximity/Kconfig
> > +++ b/drivers/iio/proximity/Kconfig
> > @@ -46,3 +46,18 @@ config SX9500
> > module will be called sx9500.
> >
> > endmenu
> > +
> > +menu "Distance sensors"
> > +
> > +config SRF08
> > + tristate "Devantech SRF08 ultrasonic ranger sensor"
> > + depends on I2C
> > + help
> > + Say Y here to build a driver for Devantech SRF08 ultrasonic
> > + ranger sensor. This driver can be used to measure the distance
> > + of objects.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called srf08.
> > +
> > +endmenu
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 9aadd9a8ee99..e914c2a5dd49 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,4 +5,5 @@
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AS3935) += as3935.o
> > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> > +obj-$(CONFIG_SRF08) += srf08.o
> > obj-$(CONFIG_SX9500) += sx9500.o
> > diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
> > new file mode 100644
> > index 000000000000..f38c74ed0933
> > --- /dev/null
> > +++ b/drivers/iio/proximity/srf08.c
> > @@ -0,0 +1,362 @@
> > +/*
> > + * srf08.c - Support for Devantech SRF08 ultrasonic ranger
> > + *
> > + * Copyright (c) 2016 Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
> > + *
> > + * This file is subject to the terms and conditions of version 2 of
> > + * the GNU General Public License. See the file COPYING in the main
> > + * directory of this archive for more details.
> > + *
> > + * For details about the device see:
> > + * http://www.robot-electronics.co.uk/htm/srf08tech.html
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/bitops.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +/* registers of SRF08 device */
> > +#define SRF08_WRITE_COMMAND 0x00 /* Command Register */
> > +#define SRF08_WRITE_MAX_GAIN 0x01 /* Max Gain Register: 0 .. 31 */
> > +#define SRF08_WRITE_RANGE 0x02 /* Range Register: 0 .. 255 */
> > +#define SRF08_READ_SW_REVISION 0x00 /* Software Revision */
> > +#define SRF08_READ_LIGHT 0x01 /* Light Sensor during last echo */
> > +#define SRF08_READ_ECHO_1_HIGH 0x02 /* Range of first echo received */
> > +#define SRF08_READ_ECHO_1_LOW 0x03 /* Range of first echo received */
> > +
> > +#define SRF08_CMD_RANGING_CM 0x51 /* Ranging Mode - Result in cm */
> > +
> > +#define SRF08_DEFAULT_GAIN 1025 /* max. analogue value of Gain */
> > +#define SRF08_DEFAULT_RANGE 11008 /* max. value of Range in mm */
> > +
> > +struct srf08_data {
> > + struct i2c_client *client;
> > + int gain; /* Max Gain */
> > + int range_mm; /* Range in mm */
> > + struct mutex lock;
> > +};
> > +
> > +static const int srf08_gain[] = {
> > + 94, 97, 100, 103, 107, 110, 114, 118,
> > + 123, 128, 133, 139, 145, 152, 159, 168,
> > + 177, 187, 199, 212, 227, 245, 265, 288,
> > + 317, 352, 395, 450, 524, 626, 777, 1025 };
> > +
> > +static int srf08_read_ranging(struct srf08_data *data)
> > +{
> > + struct i2c_client *client = data->client;
> > + int ret, i;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_COMMAND, SRF08_CMD_RANGING_CM);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write command - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + /*
> > + * normally after 65 ms the device should have the read value
> > + * we round it up to 100 ms
> I'd suggest this should be adapted so that it takes advantage of knowing
> roughly how long it is going to take as the 'range' maximum is changed.
> So perhaps in the basic case, sleep for 65 msecs, then poll at 5msec
> intervals. If we know it's going to be a lot faster, then poll it from
> an earlier time.
> > + *
> > + * we read here until a correct version number shows up as
> > + * suggested by the documentation
> > + */
> > + for (i = 0; i < 5; i++) {
> > + ret = i2c_smbus_read_byte_data(data->client,
> > + SRF08_READ_SW_REVISION);
> > +
> > + /* check if a valid version number is read */
> > + if (ret < 255 && ret > 0)
> > + break;
> > + msleep(20);
> > + }
> > +
> > + if (ret >= 255 || ret <= 0) {
> > + dev_err(&client->dev, "device not ready\n");
> > + mutex_unlock(&data->lock);
> > + return -EIO;
> > + }
> > +
> > + ret = i2c_smbus_read_word_swapped(data->client,
> > + SRF08_READ_ECHO_1_HIGH);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "cannot read distance: ret=%d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int srf08_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + if (channel->type != IIO_DISTANCE)
> > + return -EINVAL;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = srf08_read_ranging(data);
> > + if (ret < 0)
> > + return ret;
> > + *val = ret;
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /* 1 LSB is 1 cm */
> > + *val = 0;
> > + *val2 = 10000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static ssize_t srf08_show_range_mm_available(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int i, len = 0;
> > +
> > + for (i = 0; i < 256; i++)
> > + len += scnprintf(buf + len, PAGE_SIZE - len,
> > + "%d ", (i + 1) * 43);
> > +
> > + buf[len - 1] = '\n';
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(range_mm_available, S_IRUGO,
> > + srf08_show_range_mm_available, NULL, 0);
> > +
> > +static ssize_t srf08_show_range_mm(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > +
> > + return sprintf(buf, "%d\n", data->range_mm);
> > +}
> > +
> > +/*
> > + * set the range of the sensor to an even multiple of 43 mm
> > + * which corresponds to 1 LSB in the register
> > + *
> > + * register value corresponding range
> > + * 0x00 43 mm
> > + * 0x01 86 mm
> > + * 0x02 129 mm
> > + * ...
> > + * 0xFF 11008 mm
> > + */
> > +static ssize_t srf08_write_range_mm(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + int ret;
> > + unsigned int val, mod;
> > + u8 regval;
> > +
> > + ret = kstrtouint(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ret = val / 43 - 1;
> > + mod = val % 43;
> > +
> > + if (mod || (ret < 0) || (ret > 255))
> > + return -EINVAL;
> > +
> > + regval = ret;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_RANGE, regval);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write_range - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + data->range_mm = val;
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(range_mm, S_IRUGO | S_IWUSR,
> > + srf08_show_range_mm, srf08_write_range_mm, 0);
> > +
> > +static ssize_t srf08_show_gain_available(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int i, len = 0;
> > +
> > + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> > + len += sprintf(buf + len, "%d ", srf08_gain[i]);
> > +
> > + len += sprintf(buf + len, "\n");
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(gain_available, S_IRUGO,
> > + srf08_show_gain_available, NULL, 0);
> > +
> > +static ssize_t srf08_show_gain(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + int len;
> > +
> > + len = sprintf(buf, "%d\n", data->gain);
> > +
> > + return len;
> > +}
> > +
> > +static ssize_t srf08_write_gain(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct srf08_data *data = iio_priv(indio_dev);
> > + struct i2c_client *client = data->client;
> > + int ret, i;
> > + unsigned int val;
> > + u8 regval;
> > +
> > + ret = kstrtouint(buf, 10, &val);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
> > + if (val == srf08_gain[i]) {
> > + regval = i;
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(srf08_gain))
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->lock);
> > +
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + SRF08_WRITE_MAX_GAIN, regval);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "write_gain - err: %d\n", ret);
> > + mutex_unlock(&data->lock);
> > + return ret;
> > + }
> > +
> > + data->gain = val;
> > +
> > + mutex_unlock(&data->lock);
> > +
> > + return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
> > + srf08_show_gain, srf08_write_gain, 0);
> > +
> > +static struct attribute *srf08_attributes[] = {
> > + &iio_dev_attr_range_mm.dev_attr.attr,
> > + &iio_dev_attr_range_mm_available.dev_attr.attr,
> > + &iio_dev_attr_gain.dev_attr.attr,
> > + &iio_dev_attr_gain_available.dev_attr.attr,
> Hmm. Custom attributes always give us issues. The primary point of IIO
> is to enforce (more or less) standard interfaces.
>
> If you do need to add something new then that is fine (and I do think
> you need to here!).
>
> They need to be formally proposed as an addition to the ABI with
> docs in /Documentation/ABI/testing/sysfs-bus-iio*
>
> Once we take one driver using it it becomes part of our ABI that
> userspace will need to handle, hence we consider these very
> carefully.
>
> My gut feeling would be that gain needs to be more specific as it's
> a term that can mean very different things.. Here we are talking
> about an amplifier on a signal that we are then looking at the timing
> of. It might otherwise be interpretted as another term for what
> we term 'scale' in IIO.
>
> So what to call it... Perhaps afegain for Analog front end gain?
> We might want to add this to the core supported attrs, but lets
> not do so until we see if we have this on a number of devices.
>
In /Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 there is also a
gain used in a similar situation and it's called there "sensor_sensitivity"
What it we also use this name here?
> The description would need to make it explicit that this gain is
> for cases where we aren't measuring the magnitude of what is
> being amplified.
>
> For the range, it's an interesting one. Again the term range could
> mean too many things within the wider ABI. We need to make it more
> specific.
>
> Actually reading the datasheet, I think this is fundamentally about the
> maximum sampling frequency rather than directly about the range.
> The only reason you'd reduce the range is to speed that up. It doesn't
> improve the resolution, the device simply answers quicker.
>
> So I'd support this as sampling_frequency. You could then use
> the the iio_info_mask_*_available and relevant callback to provide
> info on what it then restricts the possible output values to
> (rather than controlling it directly).
>
By changing the range one cannot influence the sampling frequency directly. I
have seen on the oszilloscope that the telegrams arrive almost at the same time
with different settings of range and the same gain.
Only if the gain is also adjusted the sensor works faster and a higher frequency
can be used. But the gain is also used to adjust the sensitivity of the sensor.
What about calling it "sensor_domain" or "sensor_max_range"?
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group srf08_attribute_group = {
> > + .attrs = srf08_attributes,
> > +};
> > +
> > +static const struct iio_chan_spec srf08_channels[] = {
> > + {
> > + .type = IIO_DISTANCE,
> > + .info_mask_separate =
> > + BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE),
> > + },
> > +};
> > +
> > +static const struct iio_info srf08_info = {
> > + .read_raw = srf08_read_raw,
> > + .attrs = &srf08_attribute_group,
> > + .driver_module = THIS_MODULE,
> > +};
> > +
> > +static int srf08_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct srf08_data *data;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_READ_WORD_DATA))
> > + return -ENODEV;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + i2c_set_clientdata(client, indio_dev);
> > + data->client = client;
> > +
> > + /*
> > + * set default values of device here
> > + * these values are already set on the hardware after power on
> > + */
> > + data->gain = SRF08_DEFAULT_GAIN;
> > + data->range_mm = SRF08_DEFAULT_RANGE;
> We should be a little careful with assumptions about the device having
> just been powered on. The driver might simply have been removed and
> reprobed. So I'd sugest rewriting them whatever to be sure we have
> what we expect. Either that or if they can be read back, then just
> always retrieve them from the device.
You are right.
Then i'll set the default value at the sensor, because it cannot be read.
> > +
> > + indio_dev->name = dev_name(&client->dev);
> > + indio_dev->dev.parent = &client->dev;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->info = &srf08_info;
> > + indio_dev->channels = srf08_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(srf08_channels);
> > +
> > + mutex_init(&data->lock);
> > +
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct i2c_device_id srf08_id[] = {
> > + { "srf08", 0 },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, srf08_id);
> > +
> > +static struct i2c_driver srf08_driver = {
> > + .driver = {
> > + .name = "srf08",
> > + },
> > + .probe = srf08_probe,
> > + .id_table = srf08_id,
> > +};
> > +module_i2c_driver(srf08_driver);
> > +
> > +MODULE_AUTHOR("Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>");
> > +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> > +MODULE_LICENSE("GPL");
> >
>
--
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox