New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: regulator: convert to gpio_dt_spec
#37689
drivers: regulator: convert to gpio_dt_spec
#37689
Conversation
uint32_t startup_delay_us; | ||
uint32_t off_on_delay_us; | ||
gpio_pin_t gpio_pin; | ||
gpio_dt_flags_t gpio_flags; | ||
struct gpio_dt_spec enable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename to struct gpio_dt_spec gpio;
or struct gpio_dt_spec reg_gpio;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that? The devicetree property is called enable-gpios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe enable_pin
or enable_gpio
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the existing usage of struct gpio_dt_spec
, it looks like a pretty even split between including gpio
in the name and not, with an edge towards not (influenced by my merged PR's that don't #37442, #37514)
My position is that these variables are only used in functions where it is exceedingly obvious that the object is a gpio (gpio_configure_dt()
, etc), so the added value of tacking on _gpio
isn't that high. Types aren't duplicated in the names of other fields, why is it needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that? The devicetree property is called
enable-gpios
That should probably just be 'gpio'.
My position is that these variables are only used in functions where it is exceedingly obvious that the object is a gpio (
gpio_configure_dt()
, etc), so the added value of tacking on_gpio
isn't that high. Types aren't duplicated in the names of other fields, why is it needed here?
I am also fine with: onoff, offon, or disable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absent other considerations, I feel that an argument could be made for
output_enable
vsenable
. Theoe
abbreviation is IMO too abbreviated.
No, 'oe' is perfect, especially since it is part of struct driver_config
. Alternative is onoff, which is also great since it is a fixed regulator. NACK for output_enable
, enable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think output_enable
is better in this particular case. I agree that oe
is simply too abbreviated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfischer-no will you compromise to output_enable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorter names should be preferable, I would like to see oe
or oe_pin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that oe
is too abbreviated. It would not be clear to me what gpio_pin_set_dt(&cfg->oe, false);
really does, without looking at the logic/context. I am okay however with enable
, output_enable
, enable_pin
or enable_gpio
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use oe
or onoff
for struct gpio_dt_spec
inside struct driver_config
.
If that is the consensus of a larger group I am happy to update it. In the meantime I am leaving it as the name of the devicetree property, not a two letter abbreviation or a term against which true and false have no meaning. |
@stephanosio @jfischer-no I won't be making a 2am meeting to discuss a variable name. |
dev-review (sept 2, 2021): defer to next week when @jfischer-no is hopefully around to discuss. |
Convert regulator_fixed GPIO usage to utilize `struct gpio_dt_spec`. Signed-off-by: Jordan Yates <jordan.yates@data61.csiro.au>
67296fa
to
3a8f03b
Compare
Dropping dev-review label. Please add if you want to raise this again for discussion there. |
I don't feel like there has been any resolution in terms of what needs to be done. |
Convert regulator_fixed GPIO usage to utilize
struct gpio_dt_spec
.Signed-off-by: Jordan Yates jordan.yates@data61.csiro.au