Re: [PATCH v2] ata: sata_mv: enable SoC SATA LED presence indication

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/09/25 23:28, Carlos Junior wrote:
> Enable the SoC SATA LED controller in mv_soc_enable_leds().
> 
> Set the LED controller enable and device presence multiplexing bits
> in the SoC SATA host-controller register. This enables SATA device
> presence indication on boards using the SoC SATA controller.
> 
> Signed-off-by: Carlos Junior <cjuniorfox@gmail.com>
> ---
> Changes in v2:
> - Do not initialize the SoC SATA LED blink bit; leave it to the
>   existing NCQ LED blink handling.
> 
>  drivers/ata/sata_mv.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index d260f04e6..52e33485b 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -252,6 +252,7 @@ enum {
>  	SOC_LED_CTRL_BLINK	= (1 << 0),	/* Active LED blink */
>  	SOC_LED_CTRL_ACT_PRESENCE = (1 << 2),	/* Multiplex dev presence */
>  						/*  with dev activity LED */
> +	SOC_LED_CTRL_ENABLE	= (1 << 3),	/* Enable SATA LED control */
>  
>  	/* Shadow block registers */
>  	SHD_BLK			= 0x100,
> @@ -3382,12 +3383,20 @@ static void mv6_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
>  	writel(m2, port_mmio + PHY_MODE2);
>  }
>  
> -/* TODO: use the generic LED interface to configure the SATA Presence */
> -/* & Acitivy LEDs on the board */
> +/*
> + * Enable the SoC SATA LED controller and device presence
> + * indication on the board's SATA LEDs.
> + */
>  static void mv_soc_enable_leds(struct mv_host_priv *hpriv,
>  				      void __iomem *mmio)
>  {
> -	return;
> +	void __iomem *hc_mmio;
> +
> +	hc_mmio = mv_hc_base(mmio, 0);

This can be done in a single line with the declaration of the variable:

	void __iomem *hc_mmio = mv_hc_base(mmio, 0);

> +
> +	writel(SOC_LED_CTRL_ENABLE |
> +		SOC_LED_CTRL_ACT_PRESENCE,

No need to have this on 2 lines:

	writel(SOC_LED_CTRL_ENABLE | SOC_LED_CTRL_ACT_PRESENCE,
	       hc_mmio + SOC_LED_CTRL);

is nicer. And you can also get rid of the local variable:

	writel(SOC_LED_CTRL_ENABLE | SOC_LED_CTRL_ACT_PRESENCE,
	       mv_hc_base(mmio, 0) + SOC_LED_CTRL);

But if you prefer keeping the variable for readability, that's fine too.

With these nits fixed, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> +		hc_mmio + SOC_LED_CTRL);
>  }
>  
>  static void mv_soc_read_preamp(struct mv_host_priv *hpriv, int idx,


-- 
Damien Le Moal
Western Digital Research