Aw: Re: [PATCH v2 4/5] ahci: mediatek: add ahci driver

[Frank Wunderlich]

Hi,

thanks for first review

> Gesendet: Donnerstag, 13. August 2020 um 07:52 Uhr
> Von: "Chunfeng Yun" <chunfeng.yun at mediatek.com>
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <ahci.h>
> > +#include <scsi.h>
> > +#include <sata.h>
> > +#include <reset.h>
> > +#include <asm/io.h>
> > +#include <generic-phy.h>
> > +#include <dm/of_access.h>
> > +#include <syscon.h>
> > +#include <linux/err.h>
> > +#include <regmap.h>
>
> alphabetic sort?

at least common.h neds to be first ;) but then it looks like this:

#include <common.h>
#include <ahci.h>
#include <asm/io.h>
#include <dm.h>
#include <dm/of_access.h>
#include <generic-phy.h>
#include <linux/err.h>
#include <regmap.h>
#include <reset.h>
#include <sata.h>
#include <scsi.h>
#include <syscon.h>

> > +struct mtk_ahci_priv {
> > +	void *base;
> > +
> > +	struct regmap *mode;
> > +	struct reset_ctl axi_rst;
> > +	struct reset_ctl sw_rst;
> > +	struct reset_ctl reg_rst;
> How about using reset_ctl_bulk?

this will drop mtk_ahci_platform_resets and add this in probe, right?

struct reset_ctl_bulk rst_bulk;

    ret = reset_get_bulk(dev, &rst_bulk);
    if (!ret) {
        reset_assert_bulk(&rst_bulk);
        reset_deassert_bulk(&rst_bulk);
    } else
        dev_err(dev, "Failed to get reset: %d\n", ret);


> > +	priv->base = map_physmem(devfdt_get_addr(dev), sizeof(void *),
> > +				 MAP_NOCACHE);
> use devfdt_remap_addr_index/name instead?

have to look how these work/other drivers use this...

> > +	hpriv = malloc(sizeof(struct ahci_uc_priv));
> > +	if (!hpriv)
> > +		return -ENOMEM;
> How about put an ahci_uc_priv struct instance in mtk_ahci_priv struct,
> then no need malloc it here?

ok, i test with this :)
can memset also dropped?