Re: [PATCH V7 3/7] ata: ahci_tegra: Update initialization sequence

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

On Mon, Feb 12, 2018 at 10:56:42PM +0530, Preetham Chandru Ramchandra wrote:
> From: Preetham Ramchandra <pchandru@nvidia.com>
> 
> Update the controller initialization sequence and move
> t124 specifics to tegra124_ahci_init.
> 
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> ---
> v7:
> * moveed tegra124_ahci_soc_data definition to be
>   just above the of_device_id table.
> ---
>  drivers/ata/ahci_tegra.c | 288 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 223 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
> index 3a62eb246d80..7ffe8a97447a 100644
> --- a/drivers/ata/ahci_tegra.c
> +++ b/drivers/ata/ahci_tegra.c
[...]
> @@ -99,6 +159,14 @@ static const struct sata_pad_calibration tegra124_pad_calibration[] = {
>  	{0x14, 0x0e, 0x1a, 0x0e},
>  };
>  
> +struct tegra_ahci_ops {
> +	int (*init)(struct ahci_host_priv *hpriv);
> +};
> +
> +struct tegra_ahci_soc {
> +	struct tegra_ahci_ops	ops;
> +};
> +
>  struct tegra_ahci_priv {
>  	struct platform_device	   *pdev;
>  	void __iomem		   *sata_regs;
> @@ -108,8 +176,53 @@ struct tegra_ahci_priv {
>  	/* Needs special handling, cannot use ahci_platform */
>  	struct clk		   *sata_clk;
>  	struct regulator_bulk_data supplies[5];
> +	struct tegra_ahci_soc	   *soc_data;

This should be const. I also think the _data suffix is unnecessary.

> @@ -145,7 +258,6 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
>  
>  disable_regulators:
>  	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
> -
>  	return ret;
>  }

Unneeded whitespace change.

> @@ -179,78 +290,114 @@ static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>  		return ret;
>  	}
>  
> +	/*
> +	 * Program the following SATA IPFS registers
> +	 * to allow SW accesses to SATA's MMIO register range.
> +	 */

You can also use the full width of a line for comments. No need to wrap
at arbitrary points.

> @@ -285,8 +432,17 @@ static const struct ata_port_info ahci_tegra_port_info = {
>  	.port_ops	= &ahci_tegra_port_ops,
>  };
>  
> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
> +	.ops = {
> +		.init = tegra124_ahci_init,
> +	},
> +};
> +

Again, I don't think there's a need for _data. Also, you've made this
const, so tegra->soc(_data) should also be const. One rule of thumb is
that if you have const in one place, then you need to make it const
everywhere.

>  static const struct of_device_id tegra_ahci_of_match[] = {
> -	{ .compatible = "nvidia,tegra124-ahci" },
> +	{
> +		.compatible = "nvidia,tegra124-ahci",
> +		.data = &tegra124_ahci_soc_data
> +	},
>  	{}
>  };

of_device_id.data is const as well, which is why this compiles properly.

>  MODULE_DEVICE_TABLE(of, tegra_ahci_of_match);
> @@ -313,6 +469,8 @@ static int tegra_ahci_probe(struct platform_device *pdev)
>  	hpriv->plat_data = tegra;
>  
>  	tegra->pdev = pdev;
> +	tegra->soc_data =
> +	    (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);

And if you make tegra->soc(_data) const, then there's no need for this
cast, you can simply assign directly:

	tegra->soc = of_device_get_match_data(&pdev->dev);

void * will be automatically cast to any other pointer. The only reason
why you need the cast above is to avoid the compiler from warning about
the const qualifier being cast away.

Once const, always const.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]