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

[Chunfeng Yun]

On Thu, 2020-08-13 at 09:33 +0200, Frank Wunderlich wrote:
> 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?
Both are ok for me

> 
> 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);
add {} for dev_err

> 
> 
> > > +	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?
Yes, device core will set it to zero in alloc_priv()