Page MenuHomeFreeBSD

Add documentation for fdt_pinctrl driver
ClosedPublic

Authored by gonzo on Feb 7 2018, 6:11 AM.

Details

Summary
  • Add techical overview of the driver to section 4
  • Add API description to section 9

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gonzo created this revision.Feb 7 2018, 6:11 AM
gonzo retitled this revision from Add documentation for fdt_pinctrl driver Add techical overview of the driver to section 4 Add API description to section 9 to Add documentation for fdt_pinctrl driverAdd techical overview of the driver to section 4Add API description to section 9.Feb 7 2018, 6:13 AM
gonzo added reviewers: ian, imp, andrew, manu, mmel.
wblock added a subscriber: wblock.Feb 9 2018, 9:25 PM
wblock added inline comments.
share/man/man4/fdt_pinctrl.4
37 ↗(On Diff #38987)

s/single/a single/

39 ↗(On Diff #38987)

Serial comma and articles:

pin, ball, or pad) to connect a signal from one of the SoC internal
41 ↗(On Diff #38987)
For example, based on the actual device design, a single SoC chip
42 ↗(On Diff #38987)
pin can perform any of these roles: SPI clock, I2C
43 ↗(On Diff #38987)

Serial comma: s/ping/ping,/

44 ↗(On Diff #38987)

Definitely need some articles here. Maybe:

Function selection is performed by the pinmux controller, a SoC
45 ↗(On Diff #38987)

The last part of this is unclear. Is the pinmux controller just a set of registers, or is it controlled by a set of registers?

46 ↗(On Diff #38987)

Don't need the "s" on registers: s/registers/register/

47 ↗(On Diff #38987)

Needs an article: s/on/on the/

51 ↗(On Diff #38987)
based systems, the pinmux controller is represented by a node in
52 ↗(On Diff #38987)
the device tree.
77 ↗(On Diff #38987)

Articles, and use "might" for probability instead of "may" which implies permission:

Depending on the state the device is in (active, idle) it might
79 ↗(On Diff #38987)

Article:

Each configuration is described by setting the pinctrl-N
81 ↗(On Diff #38987)

Article:

nodes of the pinmux controller node.
84 ↗(On Diff #38987)

Article:

pinctrl-0 is a default configuration that is applied in the
87 ↗(On Diff #38987)

Needs a comma for a pause:

In addition to referring to pin configurations by index, they
88 ↗(On Diff #38987)

Article and spelling correction:

can be referred to by name if the pinctrl-names property is set.
89 ↗(On Diff #38987)

I suggest avoiding possessives whenever possible because they so frequently go wrong.

The value of pinctrl-names is a list of strings with names for each pinctrl-N
95 ↗(On Diff #38987)

Should this word "names" be here? Seems like the sentence should end after the second function (with added article):

Client devices can request a specific configuration using
.Xr fdt_pinctrl_configure 9 
​and
​.Xr fdt_pinctrl_configure_by_name 9 .

108 ↗(On Diff #38987)

Article:

The pinctrl driver should implement FDT_PINCTRL_CONFIGURE
110 ↗(On Diff #38987)

Serial comma:

calling fdt_pinctrl_register function, and call
112 ↗(On Diff #38987)

Avoid contractions:

to configure pins for all enabled devices (devices where
113 ↗(On Diff #38987)
the "status" property is not set to "disabled").
share/man/man9/fdt_pinctrl.9
49 ↗(On Diff #38987)

No "s" on pins ("pin configurations" is the plural), articles:

provides an API for manipulating I/O pin configurations on
51 ↗(On Diff #38987)

This is complex. Maybe:

On the controller side, the standard newbus probe and
attach methods are implemented.
This driver also implements the
54 ↗(On Diff #38987)
method, in which it calls the
56 ↗(On Diff #38987)

Break the sentence here:

function to register itself as a pinmux controller.

and add another line afterwards:

Then
58 ↗(On Diff #38987)
is used to walk the device tree and configure pins specified by the pinctrl-0
60 ↗(On Diff #38987)
If a client device requires a pin configuration change at some
61 ↗(On Diff #38987)
point of its lifecycle, it uses the
67 ↗(On Diff #38987)

I suggest these "The function" parts can be removed. In other words, change:

The function fdt_pinctrl_configure is used

to just

fdt_pinctrl_configure is used

This might make some people Very Concerned that the sentence does not start with a capital letter, but it does start with a proper name, the name of the function. And not all names start with capital letters.

An alternate approach is to move the word "function" after the function name:

The fdt_pinctrl_configure function is used

but I still think that is redundant and adds very little or nothing to the content.

71 ↗(On Diff #38987)
to request a pin configuration
72 ↗(On Diff #38987)

Probably don't need to quote this, earlier ones were not quoted. Also remove a possessive.

described by the pinctrl-N property with index
75 ↗(On Diff #38987)

See above, note at line 67.

79 ↗(On Diff #38987)

Article:

to request the pin configuration with name
82 ↗(On Diff #38987)

As above.

91 ↗(On Diff #38987)

Spelling:

identifies each descendant of the pinctrl
92 ↗(On Diff #38987)

Break the sentence:

node.
The pinctrl node is a pin configuration
101 ↗(On Diff #38987)

As above.

106 ↗(On Diff #38987)

This is really hard to understand and needs to be broken into two or three sentences. Maybe:

.Fn fdt_pinctrl_configure_tree
walks through enabled devices in the device tree.
If the pinctrl-0 property contains references
to child nodes of the specified pinctrl device,
their pins are configured.
gonzo retitled this revision from Add documentation for fdt_pinctrl driverAdd techical overview of the driver to section 4Add API description to section 9 to Add documentation for fdt_pinctrl driver.Feb 17 2018, 6:34 AM
gonzo edited the summary of this revision. (Show Details)
gonzo marked 40 inline comments as done.Feb 17 2018, 7:20 AM

Fixed. Thanks for edits Warren.

share/man/man4/fdt_pinctrl.4
45 ↗(On Diff #38987)

Controlled. It's more than just a set of registers. I changed to "usually controlled by a ..."

gonzo updated this revision to Diff 39410.Feb 17 2018, 7:22 AM
gonzo marked an inline comment as done.

Implement wblock@ edits

gonzo added a comment.Feb 26 2018, 8:41 PM

Hi Warren,

Is current version OK to be committed?

Thanks

gonzo updated this revision to Diff 39778.Feb 27 2018, 12:11 AM

Add information that fdt_pinctrl_register can be called
mutiple type to handle more than one pins property type

Hi Warren,
Is current version OK to be committed?
Thanks

The man page looks good to me. Please remember to bump .Dd before commit. Thanks!

ian accepted this revision.Mar 3 2018, 12:09 AM
This revision is now accepted and ready to land.Mar 3 2018, 12:09 AM
manu accepted this revision.Mar 3 2018, 12:22 AM
This revision was automatically updated to reflect the committed changes.