Hi, thanks for first review
> Gesendet: Donnerstag, 13. August 2020 um 07:52 Uhr > Von: "Chunfeng Yun" <[email protected]> > > +#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?
