On Mon, Feb 26, 2018 at 7:06 PM, Richard Fitzgerald
[off-list ref] wrote:

On 26/02/18 14:11, Andy Shevchenko wrote:

quoted

On Mon, Feb 26, 2018 at 3:05 PM, Richard Fitzgerald
[off-list ref] wrote:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+               gpiod_set_value_cansleep(madera->reset_gpio, 1);
+               usleep_range(1000, 2000);
+       }
+}

quoted

quoted

+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 },

quoted

+       {},

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.

quoted

quoted

+};

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+struct madera_irqchip_pdata;
+struct madera_codec_pdata;

quoted

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

-- 
With Best Regards,
Andy Shevchenko