2023-07-20 10:41:32

by Sergey Shtylyov

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

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

> From: Nikita Shubin <[email protected]>
>
> - Add OF ID match table
> - Drop ep93xx_chip_revision and use soc_device_match instead
>
> Signed-off-by: Nikita Shubin <[email protected]>
> ---
> 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