Re: [PATCH RFC v3 2/2] memory: pl353: Add driver for arm pl353 static memory controller

[Arnd Bergmann <arnd@arndb.de>]

On Monday 21 July 2014, punnaiah choudary kalluri wrote:
> On Mon, Jul 7, 2014 at 10:15 PM, punnaiah choudary kalluri
> <punnaia@xilinx.com> wrote:
> > On Thu, Jul 3, 2014 at 4:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Friday 27 June 2014 08:32:42 Punnaiah Choudary Kalluri wrote:
> >>
> >>> +/**
> >>> + * struct pl353_smc_data - Private smc driver structure
> >>> + * @devclk:          Pointer to the peripheral clock
> >>> + * @aperclk:         Pointer to the APER clock
> >>> + */
> >>> +struct pl353_smc_data {
> >>> +     struct clk              *memclk;
> >>> +     struct clk              *aclk;
> >>> +};
> >>> +
> >>> +/* SMC virtual register base */
> >>> +static void __iomem *pl353_smc_base;
> >>
> >> Drivers should not assume that there is only a single instance of the
> >> hardware in the system. Please remove this global variable and
> >> change the code to pass around a pointer to a structure containing
> >> it.
> >
> > Since, the pl353 nand driver has dependency with this driver for configuring the
> > bus width,operational parameters and ecc specific settings.
> > So, i didnt find any mechanism for passing this instance from other driver other
> > than exporting this instance.
> > http://lists.infradead.org/pipermail/linux-mtd/2014-April/053432.html
> >
> > Please suggest the better option if you want it to be not global.

I would expect the nand device to be a child of the smc device. In this case,
the nand driver can just pass a pointer to its own 'struct device', and the
smc driver can find its data using dev_get_drvdata(dev->parent).

> >>> +/**
> >>> + * pl353_smc_set_buswidth - Set memory buswidth
> >>> + * @bw:      Memory buswidth (8 | 16)
> >>> + * Return: 0 on success or negative errno.
> >>> + */
> >>> +int pl353_smc_set_buswidth(unsigned int bw)
> >>> +{
> >>> +
> >>> +     if (bw != PL353_SMC_MEM_WIDTH_8  && bw != PL353_SMC_MEM_WIDTH_16)
> >>> +             return -EINVAL;
> >>> +
> >>> +     writel(bw, pl353_smc_base + PL353_SMC_SET_OPMODE_OFFS);
> >>> +     writel(PL353_SMC_DC_UPT_NAND_REGS, pl353_smc_base +
> >>> +            PL353_SMC_DIRECT_CMD_OFFS);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(pl353_smc_set_buswidth);
> >>
> >> Why is this an exported interface? Wouldn't that setting just be
> >> determined from DT when the device is probed?
> >
> > Nand driver rely on auto bus width detection mechanism for identifying
> > the bus width using onfi parameter page settings. So, once the driver
> > finds the bus width, updates accordingly using this API

This should at least pass the device pointer then.


	Arnd