Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP
Hi
I have written a driver for the DS28E17 Onewire to I2C master bridge. It acts as a slave device in the W1 subsystem and as a bus master in the I2C subsystem.
It's available here:
https://github.com/ianka/w1_ds28e17
The kernel driver is in the files w1_ds28e17, Kconfig and Makefile. The other files are test programs for the DS28E17 development kit as sold by Maxim. Please see the README for further details.
I've already sent a mail to Evgeniy Polyakov, maintainer of the W1 subsystem and Wolfram Sang, maintainer of the I2C subsystem but got no reply. Maybe due to holiday season.
It would be nice if someone with insight could give me a ping "that looks okay" or "you have to fix this or that".
I also have a question/feature request for the i2c subsystem. The DS28E17 can support I2C_M_STOP, which allows to combine write/read into a single transaction for slaves which need the intermediate stop condition – as the DS7505 from the evaluation board. Using I2C_M_STOP instead of splitting the transaction into two would save some overhead on the Onewire bus (8 bytes at 15kBaud == 4ms, of busy-looping when Onewire is bitbanged.)
However, the test for the I2C_M_STOP feature is made with I2C_FUNC_PROTOCOL_MANGLING, which also includes I2C_M_REV_DIR_ADDR, I2C_M_IGNORE_NAK, I2C_M_NO_RD_ACK. All of these *are not* supported by the DS28E17.
What's the right way to handle this on the I2C driver side? Could we have a I2C_FUNC_STOP as we have a I2C_FUNC_NOSTART?
Kind regards
Jan
Hi,
Quoted 3 lines
It's available here:
https://github.com/ianka/w1_ds28e17
You need to send patches to get anything reviewed.
Quoted 3 lines
I've already sent a mail to Evgeniy Polyakov, maintainer of the W1 subsystem and Wolfram Sang, maintainer of the I2C subsystem but got no reply. Maybe due to holiday season.
For my case, I am just extremly busy. Have a look for the pending patches for I2C here:
http://patchwork.ozlabs.org/project/linux-i2c/list/
Please also note, that a maintainer is not the only reviewer for a subsystem, the whole list is. You can try there to get people attracted to your patch. If you count on me, expect a delay of weeks.
Quoted 4 lines
I also have a question/feature request for the i2c subsystem. The DS28E17 can support
I2C_M_STOP, which allows to combine write/read into a single transaction for slaves which need the intermediate stop condition – as the DS7505 from the evaluation board. UsingI2C_M_STOP
It cannot do repeated start?
Quoted 6 lines
instead of splitting the transaction into two would save some overhead on the Onewire bus (8 bytes at 15kBaud == 4ms, of busy-looping when Onewire is bitbanged.)
What's the right way to handle this on the I2C driver side? Could we have a
I2C_FUNC_STOPas we have aI2C_FUNC_NOSTART?
We can factor it out like I2C_FUNC_NOSTART when it is proven that it really is needed.
Thanks,
Wolfram
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Am 14.07.2016 um 11:58 schrieb Wolfram Sang:
Quoted 7 lines
Quoted 4 lines
It's available here:
You need to send patches to get anything reviewed.
Is there a special procedure to follow to have those three diffs queued? Because Kconfig.diff, Makefile.diff and w1.diff are in that archive and the other diff can be easily obtained:
$ diff -u /dev/null w1_ds28e17.c >w1_ds28e17.diff
Quoted 9 lines
Quoted 3 lines
I've already sent a mail to Evgeniy Polyakov, maintainer of the W1 subsystem and Wolfram Sang, maintainer of the I2C subsystem but got no reply. Maybe due to holiday season.
For my case, I am just extremly busy. Have a look for the pending patches for I2C here:
Uh! Understood.
Quoted 5 lines
Please also note, that a maintainer is not the only reviewer for a subsystem, the whole list is. You can try there to get people attracted to your patch. If you count on me, expect a delay of weeks.
Oh, I haven't expected a review, just a ping "have noticed, please follow the procedure as follows."
As I'm not familiar with the procedures.
As for sending the information to you instead of the list, the MAINTAINERS file wasn't clear to me about this. And for the w1 subsystem, where this driver is also relevant to, there isn't a list at all.
It's not clear to me who's in charge of a driver which touches both subsystems.
Quoted 8 lines
Quoted 5 lines
I also have a question/feature request for the i2c subsystem. The DS28E17 can support
I2C_M_STOP, which allows to combine write/read into a single transaction for slaves which need the intermediate stop condition – as the DS7505 from the evaluation board. UsingI2C_M_STOPIt cannot do repeated start?
The datasheet says it can at least for a write to the pointer byte followed by a read but it doesn't seem to work.
- From my tests, it doesn't work, or it least it doesn't work the way the ds28e17 bus master does the repeated start.
Quoted 10 lines
Quoted 6 lines
instead of splitting the transaction into two would save some overhead on the Onewire bus (8 bytes at 15kBaud == 4ms, of busy-looping when Onewire is bitbanged.)
What's the right way to handle this on the I2C driver side? Could we have a
I2C_FUNC_STOPas we have aI2C_FUNC_NOSTART?We can factor it out like
I2C_FUNC_NOSTARTwhen it is proven that it really is needed.
I will check it again.
Kind regards
Jan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAleHi1IACgkQzGZqmZvWQdm4oACfYOySD0I7LYljZxD/UYfNHjCy 0S4Amwdj0ctB3wi6qJFoHuz0JweYV1eW
=ifHt
-----END PGP SIGNATURE-----
Quoted 2 lines
Is there a special procedure to follow to have those three diffs queued?
Try https://kernelnewbies.org/ or Documentation/SubmittingPatches for Kernel submission procedures.
Quoted 7 lines
As for sending the information to you instead of the list, the
MAINTAINERSfile wasn't clear to me about this. And for the w1 subsystem, where this driver is also relevant to, there isn't a list at all.It's not clear to me who's in charge of a driver which touches both subsystems.
Yes, this can be tricky at times. One tool to assist is scripts/get_maintainer.pl. Other than that, simply use all addresses mentioned in a MAINTAINERS entry for a subsystem. And if you touch multiple subsystems, just CC them all.
Quoted 5 lines
The datasheet says it can at least for a write to the pointer byte followed by a read but it doesn't seem to work.
- From my tests, it doesn't work, or it least it doesn't work the way the ds28e17 bus master does the repeated start.
It would be really good if that would work. Having a STOP betwen those messages is unsafe. It won't probably matter for your setup, but still...
Quoted 1 line
I will check it again.
Thanks!
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Am 14.07.2016 um 15:19 schrieb Wolfram Sang:
Quoted 11 lines
Quoted 6 lines
The datasheet says it can at least for a write to the pointer byte followed by a read but it doesn't seem to work.
- From my tests, it doesn't work, or it least it doesn't work the way the ds28e17 bus master does the repeated start.
It would be really good if that would work. Having a STOP betwen those messages is unsafe. It won't probably matter for your setup, but still...
I checked it again and it turned out to be a bug in my driver code, living there from my very first tries. The DS28E17 and DS7505 in conjunction support the Restart Condition.
So, please discard my request for I2C_FUNC_STOP. It's not needed.
But I have another question, this time regarding I2C_FUNC_NOSTART. The DS28E17 can do an I2C write transfer without Start Condition, to continue a previous write without Stop Condition. My driver already uses that internally to do continous writes of more than 255 bytes, the buffer limit of the DS28E17.
BUT, the DS28E17 can only do this for writes. Not for reads. Does it make sense to expose that feature and announce it as I2C_FUNC_NOSTART then?
Kind regards
Jan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAleIxUYACgkQzGZqmZvWQdnypwCgnrHFhOnGesAwn9P6qnrRA+UK yZ0An02KMy1Z+vKB8VO8Pn0XKFXP4e0t
=Tqkg
-----END PGP SIGNATURE-----
Quoted 2 lines
So, please discard my request for
I2C_FUNC_STOP. It's not needed.
Great! Thanks for looking into it again.
Quoted 3 lines
BUT, the DS28E17 can only do this for writes. Not for reads. Does it make sense to expose that feature and announce it as
I2C_FUNC_NOSTARTthen?
I would say no, since it is not generic.
You should also populate an i2c_adapter_quirks structure and limit reads to 256 (and maybe other stuff), if not done already.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Am 15.07.2016 um 13:26 schrieb Wolfram Sang:
Quoted 6 lines
I would say no, since it is not generic.
You should also populate an
i2c_adapter_quirksstructure and limit reads to 256 (and maybe other stuff), if not done already.
Done.
Next quirk with the DS28E17: It can do an arbitrary number of write messages, followed by ONE read message in one transfer. As there is no "Read Without Stop Condition".
My driver code just ignores the fact there always is a Stop Condition after a read. As the DS28E17 is hardly ever used in a multi-master setup, that should be okay.
For the combined transfer the DS28E17 also supports, my driver code checks if all of the conditions are met inside its i2c_master_transfer() function and uses that function automatically when applicable.
The current quirks flags only allows to limit the number of messages to two in general. How to announce the behaviour correctly?
Kind regards
Jan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iEYEARECAAYFAleJHPIACgkQzGZqmZvWQdkQFACfUYomuw8JYMdNqouP8sjpd6Nm U5EAoJ+U6G2gDoZbJ+lGpIAGOxiTTJ52
=SCdT
-----END PGP SIGNATURE-----
Am 15.07.2016 um 13:26 schrieb Wolfram Sang:
Quoted 6 lines
I would say no, since it is not generic.
You should also populate an
i2c_adapter_quirksstructure and limit reads to 256 (and maybe other stuff), if not done already.
I found that feature is new when trying to compile my driver on 3.16.7.
What's the correct way to test for struct i2c_adapter_quirks et al?
#ifdef I2C_AQ_COMB
...
#endif
Something else?
Kind regards
Jan
Quoted 1 line
What's the correct way to test for struct
i2c_adapter_quirkset al?
When you upstream stuff, it is for the current version only, so no check needed. You can do anything you want in your private version, of course.