Re: [PATCH 1/5] ata: sata_dwc_460ex: use device_property_present()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Incomplete refactoring: missed identical `of_property_present` pattern.
- [Low] Direct usage of `device_property_present()` without including `<linux/property.h>`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `clear_interrupt_bit()` ignores its `bit` argument and accidentally clears all pending interrupts.
--

commit 3c350702aea5f359b0516233c160076946c26124
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: use device_property_present()

There's no need to use np here when dev is enough. Follows the similar
pattern elsewhere where of APIs are replaced with device ones,
especially when using platform_device.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..2bcb3ac5ae211 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1180,7 +1180,7 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	}
>  
>  #ifdef CONFIG_SATA_DWC_OLD_DMA
> -	if (!of_property_present(np, "dmas")) {
> +	if (!device_property_present(dev, "dmas")) {

[Severity: Low]
Does this require including linux/property.h? The build currently succeeds
due to transitive includes, but it might be safer to include the header
directly to prevent future build breakages if the include chain changes.

[Severity: Low]
Is the intention to replace all of_property_present usages? Looking at
sata_dwc_dma_get_channel() in the same file, it still uses the old function:

	if (!of_property_present(dev->of_node, "dmas"))

Should this be updated as well for consistency?

[Severity: High]
This is a pre-existing issue, but clear_interrupt_bit() seems to ignore its
bit argument and accidentally clears all pending interrupts:

static void clear_interrupt_bit(struct sata_dwc_device *hsdev, u32 bit)
{
	sata_dwc_writel(&hsdev->sata_dwc_regs->intpr,
			sata_dwc_readl(&hsdev->sata_dwc_regs->intpr));
}

Because it reads the entire intpr register and writes it back unmodified,
does this acknowledge and clear all pending interrupts in the write-1-to-clear
register instead of just the requested bit? This could drop device
interrupts when multiple interrupts are asserted simultaneously.

>  		err = sata_dwc_dma_init_old(ofdev, hsdev);
>  		if (err)
>  			return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630202415.234463-1-rosenp@gmail.com?part=1