Re: [PATCH v3 26/42] ata: pata_ep93xx: add device tree support

[Sergey Shtylyov <s.shtylyov@xxxxxx>]

On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:

> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> 
> - Add OF ID match table
> - Drop ep93xx_chip_revision and use soc_device_match instead
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> ---
>  drivers/ata/pata_ep93xx.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index c6e043e05d43..a88824dfc5fa 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
[...]
> @@ -910,6 +912,12 @@ static struct ata_port_operations ep93xx_pata_port_ops = {
>  	.port_start		= ep93xx_pata_port_start,
>  };
>  
> +static const struct soc_device_attribute ep93xx_soc_table[] = {
> +	{ .revision = "E1", .data = (void *)ATA_UDMA3 },
> +	{ .revision = "E2", .data = (void *)ATA_UDMA4 },
> +	{ /* sentinel */ }
> +};
> +
>  static int ep93xx_pata_probe(struct platform_device *pdev)
>  {
>  	struct ep93xx_pata_data *drv_data;
> @@ -939,7 +947,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  
>  	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
>  	if (!drv_data) {
> -		err = -ENXIO;
> +		err = -ENOMEM;
>  		goto err_rel_gpio;
>  	}
>  

   Hm, deserves its own patch. And even for this one, you should've documented it
in the patch secription...

> @@ -976,12 +984,11 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  	 * so this driver supports only UDMA modes.
>  	 */
>  	if (drv_data->dma_rx_channel && drv_data->dma_tx_channel) {
> -		int chip_rev = ep93xx_chip_revision();
> +		const struct soc_device_attribute *match;
>  
> -		if (chip_rev == EP93XX_CHIP_REV_E1)
> -			ap->udma_mask = ATA_UDMA3;
> -		else if (chip_rev == EP93XX_CHIP_REV_E2)
> -			ap->udma_mask = ATA_UDMA4;
> +		match = soc_device_match(ep93xx_soc_table);
> +		if (match)
> +			ap->udma_mask = (unsigned int) match->data;
>  		else
>  			ap->udma_mask = ATA_UDMA2;
>  	}

   This one also looks as it could have been done separately -- before the DT
conversion?

[...]

MBR, Sergey