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

Hi,

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:

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.

@Lee: Could you please give us a feedback?

@Ksenija: Still motivated for next round?

Regards
Stefan

-- 
Best regards,
Marek Vasut