On 26/02/18 17:19, Andy Shevchenko wrote:

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

quoted

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

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.

Yes, true. It's probably unlikely here and I'm inclined to leave it
as it is because Lee already acked it.

quoted

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

quoted

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

quoted

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.

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.

quoted

quoted

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?

quoted

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.

quoted

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

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

quoted

+struct madera_irqchip_pdata;
+struct madera_codec_pdata;

quoted

quoted

Why do you need platform data in new code?

quoted

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)