Re: [PATCH] ata: sata_rcar: Add RCAR Gen2 SATA PHY support

[Mark Rutland <mark.rutland@arm.com>]

On Fri, Oct 11, 2013 at 11:54:52AM +0100, Valentine wrote:
> On 10/11/2013 01:41 PM, Mark Rutland wrote:
> > On Fri, Oct 11, 2013 at 02:00:35AM +0100, Simon Horman wrote:
> >> [ CCed devicetree@vger.kernel.org as this involves DT compatibility strings ]
> >
> > Cheers!
> >
> 
> Hi Mark,
> 
> >>
> >> On Thu, Oct 10, 2013 at 11:08:03PM +0400, Valentine Barshak wrote:
> >>> RCAR Gen2 SoC has a different phy which is not compatible with
> >>> the older H1/M1 versions. This adds OF/platform device table
> >>> and PHY initialization callbacks for H2/M2 (Gen2) SoC.
> >
> > Is the PHY combined with the rest of the controller, or are they
> > logically separate components in the SoC? I note that the Calxeda
> > Highbank SATA controller driver treats the phy and the controller as
> > separate entities, and describes the way the two are attached (though
> > the driver handles both). Would a similar approach work here?
> >
> 
> It seems to be described in the docs as a single entity with the rest of 
> the controller. The phy registers are in the same address space block. 
> We don't need to describe how the phy is attached to the SATA 
> controller. In the future we may need some phy-specific configuration in 
> the device tree. For example, to describe how clock generator is 
> connected to the controller. I think this could be done with optional 
> properties added to the SATA node if needed.

As they're tightly coupled and share the same register block, I guess
describing them together is ok. Is that clock example hypothetical, or
something we _will_ need in future?

> 
> > [...]
> >
> >>> +static struct of_device_id sata_rcar_match[] = {
> >>> +	{
> >>> +		.compatible = "renesas,rcar-sata",
> >>> +		.data = (void *)RCAR_SATA
> >>> +	},
> >>> +	{
> >>> +		.compatible = "renesas,sata-r8a7790",
> >>> +		.data = (void *)RCAR_GEN2_SATA
> >>> +	},
> >>> +	{
> >>> +		.compatible = "renesas,sata-r8a7791",
> >>> +		.data = (void *)RCAR_GEN2_SATA
> >>> +	},
> >>> +	{},
> >
> > These bindings will need documentation. A grep of any of these in
> > mainline's Documentation/devicetree shows up nothing (not even the
> > existing "renesas,rcar-sata" string used by the driver.
> 
> Indeed.
> Since we need to adjust rcar-sata bindings and add documentation for it, 
> which is not really related to Gen2 phy support, looks like it will be a 
> separate patch.

If you're adding strings, there should be documentation. While it may be
a separate patch, we should _not_ add compatible strings to the kernel
without documentation. I'd like to see the documentation patch first.

Thanks,
Mark.