On 26/02/18 17:19, Andy Shevchenko wrote:
On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
On 26/02/18 14:11, Andy Shevchenko wrote:
On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
+static void madera_enable_hard_reset(struct madera *madera)
+{
+ if (madera->reset_gpio)
if (!...)
return
Could do but why bother? For such a trivial function, in my opinion
static void madera_enable_hard_reset(struct madera *madera)
{
if (madera->reset_gpio)
gpiod_set_value_cansleep(madera->reset_gpio, 0);
}
is simpler and more readable than
static void madera_enable_hard_reset(struct madera *madera)
{
if (!madera->reset_gpio)
return;
gpiod_set_value_cansleep(madera->reset_gpio, 0);
}
The rationale is that if someone wants to add more code you will not
need to take care of deeper indentation and potentially split lines.
Yes, true. It's probably unlikely here and I'm inclined to leave it
as it is because Lee already acked it.
+ gpiod_set_value_cansleep(madera->reset_gpio, 0);
+}
+
+static void madera_disable_hard_reset(struct madera *madera)
+{
+ if (madera->reset_gpio) {
Ditto.
As above, yes it would work the other way but I think for such a simple
implementation the way I have written it is more readable.
I have different opinion, but yes. It's more matter of taste with
rationale above (perhaps never happen to this code).
+ gpiod_set_value_cansleep(madera->reset_gpio, 1);
+ usleep_range(1000, 2000);
+ }
+}
+const struct of_device_id madera_of_match[] = {
+ { .compatible = "cirrus,cs47l35", .data = (void *)CS47L35 },
+ { .compatible = "cirrus,cs47l85", .data = (void *)CS47L85 },
+ { .compatible = "cirrus,cs47l90", .data = (void *)CS47L90 },
+ { .compatible = "cirrus,cs47l91", .data = (void *)CS47L91 },
+ { .compatible = "cirrus,wm1840", .data = (void *)WM1840 },
+ {},
No comma.
Seems to be personal preference. Both ways are used in the kernel and
we've always used this style so I'll leave it to Lee to decide.
This is not.
The rationale is pretty obvious, terminator must terminate. With cheap
price (no comma), we just prevent some potential weird cases (bad
patch application for example, or not very careful contributor) where
entry goes after. Compiler will fail.
Yes, ok. I see a lot of people don't do that (I searched). But if I
do a new version of this patch I'll change it.
+};
+ ret = devm_gpio_request_one(madera->dev,
+ madera->pdata.reset,
+ GPIOF_DIR_OUT |
GPIOF_INIT_LOW,
+ "madera reset");
+ if (!ret)
+ madera->reset_gpio =
gpio_to_desc(madera->pdata.reset);
Why? What's wrong with descriptors?
This is what I mean by code going stale when it's acked but then never
gets merged. Some time ago there was a reason (which I forget).
So, can we switch to descriptors?
Yes, however as this patch has been in review for nearly 1 year now, and
acked for several months, I'd really hoped we could get it merged now
and update it later.
+ dev_set_drvdata(madera->dev, madera);
+ if (dev_get_platdata(madera->dev))
What this dance for?
Are you perhaps thinking the second line is dev_get_drvdata()?
dev_get_platdata() gets a pointer to any pdata, so not related
to dev_set_drvdata().
Indeed.
+ ret = mfd_add_devices(madera->dev, PLATFORM_DEVID_NONE,
+ mfd_devs, n_devs,
+ NULL, 0, NULL);
devm_?
I can try it and see. It's scary because we can depend on our
children but maybe devm_mfd_add_devices() is safe.
It will fail in the same way. It does nothing more, than keeping a
pointer to release function and its data.
+struct madera_irqchip_pdata;
+struct madera_codec_pdata;
Why do you need platform data in new code?
Answered in a comment in another patch. We care about allowing people
to use our chips with systems that don't use devicetree/acpi. There
are also many out-of-tree systems.
a) we don't care about out of tree much;
You might not, but as a commercial company we have to.
b) there are other means to provide date w/o using platform data:
- unified device property API (including built-in device properties)
- bunch of lookup tables GPIO, regulator, PWM, etc
- fwnode graph for more complex cases with device dependencies
Basically same answer as (a)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
