Re: [PATCH 4/6] spi: altera: use regmap instead of direct mmio register access

[Xu Yilun <yilun.xu@xxxxxxxxx>]

On Thu, Jun 11, 2020 at 12:02:11PM +0100, Mark Brown wrote:
> On Thu, Jun 11, 2020 at 11:25:09AM +0800, Xu Yilun wrote:
> 
> > +	if (pdata && pdata->use_parent_regmap) {
> > +		hw->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +		if (!hw->regmap) {
> > +			dev_err(&pdev->dev, "get regmap failed\n");
> > +			goto exit;
> > +		}
> > +		hw->base = pdata->regoff;
> 
> This seems very confused - there's some abstraction problem here.  This
> looks like the driver should know about whatever is causing it to use
> the parent regmap, it shouldn't just be "use the parent regmap" directly
> since that is too much of an implementation detail.  This new feature

Our usecase is that, we have the PCIE card which integrates this
spi-altera soft IP. So It seems reasonable we reuse this driver. But the
IP registers are not directly accessed by MMIO. It is accessed via an
indirect access interfaces, SW need to operate on the indirect access
interfaces to RW the spi-altera registers. like the following:

+------+   +------------+   +------------+
| PCIE |---| indirect   |---| spi-altera |
+------+   | access     |   +------------+
           | interfaces |
           +------------+
           | SPI params |
           +------------+

So we think of creating regmap to abstract the actually register accessing
detail. The parent device driver creates the regmap of indirect access,
and it creates the spi-altera platform device as child. Spi-altera
driver could just get the regmap from parent, don't have to care about
the indirect access detail.

It seems your concern is how to gracefully let spi-altera driver get the
regmap. or not using it. Since our platform doesn't enable device tree
support, seems the only way to talk to platform device is the
platform_data.

Do you have any suggestion on that?


I think the driver may need to figure out the role of the device in
system, whether it is a subdev of other device (like MFD? Many mfd subdev
driver will get parent regmap by default), or it is an independent mmio
device. But I'm not sure how to do it in right way.

> also seems like a separate change which should be in a separate patch,
> the changelog only talked about converting to use regmap which I'd have
> expected to be a straight 1:1 replacement of non-regmap stuff with
> regmap stuff.

Understood. I should make a patch to do 1:1 replacement of regmap first,
then a second patch to handle the parent regmap.