Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Jan Kandziora]

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

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

Hi,

> It's available here:
> 
> 	https://github.com/ianka/w1_ds28e17

You need to send patches to get anything reviewed.

> 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.

> 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

It cannot do repeated start?

> 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_STOP as we have a I2C_FUNC_NOSTART?

We can factor it out like I2C_FUNC_NOSTART when it is proven that it
really is needed.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Jan Kandziora]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 14.07.2016 um 11:58 schrieb Wolfram Sang:
> 
>> It's available here:
>> 
>> https://github.com/ianka/w1_ds28e17
> 
> 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


>> 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/
> 
Uh! Understood.


> 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.


>> 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
> 
> It 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.


>> 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_STOP as we have a I2C_FUNC_NOSTART?
> 
> We can factor it out like I2C_FUNC_NOSTART when 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-----

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]


> Is there a special procedure to follow to have those three diffs queued?

Try https://kernelnewbies.org/ or Documentation/SubmittingPatches for
Kernel submission 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.

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.

> 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 will check it again.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Jan Kandziora]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 14.07.2016 um 15:19 schrieb Wolfram Sang:
> 
>> 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-----

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]


> So, please discard my request for I2C_FUNC_STOP. It's not needed.

Great! Thanks for looking into it again.

> 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?

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Jan Kandziora]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 15.07.2016 um 13:26 schrieb Wolfram Sang:
> 
> 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.
> 
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-----

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Jan Kandziora]

Am 15.07.2016 um 13:26 schrieb Wolfram Sang:
>
> 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.
>
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

Re: Introducing a kernel driver for the DS28E17 Onewire to I2C master bridge; Feature request: introduce I2C_FUNC_STOP

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

> What's the correct way to test for struct i2c_adapter_quirks et 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]