Re: [PATCH v3 1/4] mfd: mxs-lradc: Add support for mxs-lradc MFD

Hi,

On Tue, Aug 2, 2016 at 6:35 PM, Stefan Wahren [off-list ref] wrote:

Hi,

quoted

Marek Vasut [off-list ref] hat am 14. Juli 2016 um 17:38 geschrieben:


On 07/13/2016 02:49 PM, Lee Jones wrote:

quoted

On Fri, 01 Jul 2016, Harald Geyer wrote:

quoted

Hi Ksenija!

Ksenija Stanojević writes:

quoted

On Tue, Jun 28, 2016 at 6:28 PM, Lee Jones [off-list ref] wrote:

quoted

quoted

+static int mxs_lradc_add_device(struct platform_device *pdev,
+                             struct mxs_lradc *lradc, char *name, int
i)
+{
+     struct mfd_cell *cell;
+
+     cell = &lradc->cells[i];
+     cell->name = name;
+     cell->platform_data = lradc;
+     cell->pdata_size = sizeof(*lradc);
+
+     return devm_mfd_add_devices(&pdev->dev, -1, cell, 1, NULL, 0,
NULL);
+}

Please don't roll your own API.

Use 'struct mfd_cell' like everyone else does.

It has been suggested in previous reviews to use separate function to
register mfd device, and to make mfd_cell allocate dynamically because
struc mxs-lradc is allocated dynamically.
But I can revrse changes and make mfd_cells allocate staticaly
wthout separate function.

I think making mfd_cells members of struct mxs-lradc will address all
review comments.

No, please don't do that either.

It'd be nice if you explained in detail why not. Otherwise this is just
empty splat.

since there is no reply, here is my guess:

Sorry for the delay, I'm currently working on it. I will post another
version soon.

static const struct mfd_cell mxs_lradc_devs[] = {
        {
                .name = DRIVER_NAME_ADC,
        },
        {
                .name = DRIVER_NAME_TS,
        },
};

But i'm not sure if we need of_compatible defined here. The intension of this
patch series is to keep the DT binding.

I think it needs .resources because in next version DEFINE_RES_IRQ_NAMED
will be used.

@Lee: Could you please give us a feedback?

@Ksenija: Still motivated for next round?

Regards
Stefan

quoted

--
Best regards,
Marek Vasut

Regards,
Ksenija