Re: [PATCH v2 6/7] ata: Add support for the Tegra124 SATA controller

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

On Wed, Jun 18, 2014 at 05:23:25PM +0300, Mikko Perttunen wrote:
[...]
> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
[...]
> +#include <linux/ahci_platform.h>
> +#include <linux/reset.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/tegra-powergate.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/tegra-soc.h>
> +#include "ahci.h"
> +
> +#define SATA_CONFIGURATION_0				0x180
> +#define		SATA_CONFIGURATION_EN_FPCI		BIT(0)

This, and the register/field definitions below look weirdly indented,
but I guess that's mostly a matter of taste.

> +#define T_SATA0_CHX_PHY_CTRL1_GEN1			0x690
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK	0xff
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT	0
> +#define		T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK	(0xff<<8)

Perhaps single spaces around "<<"? The same for other occurrences below.

> +struct sata_pad_calibration {
> +	u8 gen1_tx_amp, gen1_tx_peak, gen2_tx_amp, gen2_tx_peak;
> +};

I think a more idiomatic way would be to put the fields in a structure
declaration on separate lines.

> +static const struct sata_pad_calibration tegra124_pad_calibration[] = {
> +	{0x18, 0x04, 0x18, 0x0a},

Maybe spaces after '{' and before '}'?

> +static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
> +				    tegra->supplies);
> +	if (ret)
> +		return ret;
> +
> +	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
> +		tegra->sata_clk, tegra->sata_rst);

These could be aligned with TEGRA_POWERGATE_SATA.

> +	ret = clk_prepare_enable(tegra->cml1_clk);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}
> +
> +	ret = clk_prepare_enable(tegra->plle_clk);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->cml1_clk);
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}
> +
> +	reset_control_deassert(tegra->sata_cold_rst);
> +	reset_control_deassert(tegra->sata_oob_rst);
> +
> +	ret = phy_power_on(tegra->padctl_phy);
> +	if (ret) {
> +		clk_disable_unprepare(tegra->plle_clk);
> +		clk_disable_unprepare(tegra->cml1_clk);
> +		clk_disable_unprepare(tegra->sata_oob_clk);
> +		goto disable_power;
> +	}

These error paths might be more readable if you did unwind them similar
to how you did with sata_clk below.

> +static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
> +{
> +	struct tegra_ahci_priv *tegra = hpriv->plat_data;
> +	int ret;
> +	unsigned int val;
> +	struct sata_pad_calibration calib;
> +
> +	ret = tegra_ahci_power_on(hpriv);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to power on ahci controller: %d\n", ret);

s/ahci/AHCI/

> +		return ret;
> +	}
> +
> +	val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
> +	val |= SATA_CONFIGURATION_EN_FPCI;
> +	writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
> +
> +	/* Pad calibration */
> +
> +	ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
> +	if (ret) {
> +		dev_err(&tegra->pdev->dev,
> +			"failed to read sata calibration fuse: %d\n", ret);

s/sata/SATA/

> +static struct ata_port_operations ahci_tegra_port_ops = {
> +	.inherits	= &ahci_ops,
> +	.host_stop	= tegra_ahci_host_stop,
> +};
> +
> +static const struct ata_port_info ahci_tegra_port_info = {
> +	.flags		= AHCI_FLAG_COMMON,
> +	.pio_mask	= ATA_PIO4,
> +	.udma_mask	= ATA_UDMA6,
> +	.port_ops	= &ahci_tegra_port_ops,
> +};

I prefer these to not be arbitrarily indented, but Tejun seemed to
indicate he did, so it's ultimately his call.

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

Technically there's no need for the comma at the end here. Usually you'd
put one here so that if somebody ever were to add a new entry after this
the diff wouldn't need to include the line that you only add a comma to.
But in this particular case the last entry is used as a sentinel, so
leaving away the comma is actually useful as a hint that entries should
be added in front rather than at the end.

> +	hpriv->mmio = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 0));

Perhaps a temporary variable for the result of platform_get_resource()
would help make this look less cluttered.

> +	if (IS_ERR(hpriv->mmio)) {
> +		dev_err(&pdev->dev, "Failed to get AHCI IO memory\n");

This isn't necessary. devm_ioremap_resource() will already output an
error message.

> +	tegra->sata_regs = devm_ioremap_resource(&pdev->dev,
> +		platform_get_resource(pdev, IORESOURCE_MEM, 1));
> +	if (IS_ERR(tegra->sata_regs)) {
> +		dev_err(&pdev->dev, "Failed to get SATA IO memory\n");

Same here.

> +	tegra->padctl_phy = devm_phy_get(&pdev->dev, "sata-phy");
> +	if (IS_ERR(tegra->padctl_phy)) {
> +		dev_err(&pdev->dev, "Failed to get phy\n");

s/phy/PHY/

> +	ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra->supplies),
> +					tegra->supplies);

The trailing argument here isn't aligned as in other places.

> +static struct platform_driver tegra_ahci_driver = {
> +	.probe = tegra_ahci_probe,
> +	.remove = ata_platform_remove_one,
> +	.driver = {
> +		.name = "tegra-ahci",
> +		.owner = THIS_MODULE,

This isn't really needed anymore, module_platform_driver will end up
setting this for you anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]