Dear Christoph Thank you for your comments. > -----Original Message----- > From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi- > owner@xxxxxxxxxxxxxxx] On Behalf Of Christoph Hellwig > Sent: Tuesday, November 28, 2017 11:25 PM > To: 김기웅 > Cc: linux-scsi@xxxxxxxxxxxxxxx; Martin K. Petersen; cpgs@xxxxxxxxxxx; > HeonGwang Chu; 김부진; YOUNGEUN PARK > Subject: Re: [PATCH 2/2] scsi: ufs: add Exynos-specific driver > > On Tue, Nov 28, 2017 at 02:36:31PM +0900, 김기웅 wrote: > > This driver is to use UFS devices on Exynos SoC and has been already > > used for many years for commercial products. > > So why do you only submit it only now? The 1st author of this didn't complete submitting it. And then following products has been coming and I need time to organize this. > > > +/* Helper for UFS CAL interface */ > > +static inline int ufs_init_cal(struct exynos_ufs *ufs, int idx, > > + struct platform_device *pdev) > > +{ > > + return 0; > > +} > > + > > +static inline int ufs_pre_link(struct exynos_ufs *ufs) { > > + return 0; > > +} > > + > > +static inline int ufs_post_link(struct exynos_ufs *ufs) { > > + return 0; > > +} > > + > > +static inline int ufs_pre_gear_change(struct exynos_ufs *ufs, > > + struct uic_pwr_mode *pmd) > > +{ > > + return 0; > > +} > > + > > +static inline int ufs_post_gear_change(struct exynos_ufs *ufs) { > > + return 0; > > +} > > + > > +static inline int ufs_post_h8_enter(struct exynos_ufs *ufs) { > > + return 0; > > +} > > + > > +static inline int ufs_pre_h8_exit(struct exynos_ufs *ufs) { > > + return 0; > > +} > > These are all dummys, please rmeove them. > > > +#ifndef __EXYNOS_UFS_VS_DEBUG__ > > Please don't have ifdef code that isn't Kconfig selectable. Okay. > > > +#ifndef __EXYNOS_UFS_MMIO_FUNC__ > > +#define __EXYNOS_UFS_MMIO_FUNC__ > > +#define EXYNOS_UFS_MMIO_FUNC(name) \ > > +static inline void name##_writel(struct exynos_ufs *ufs, u32 val, u32 > reg) \ > > +{ \ > > + writel(val, ufs->reg_##name + reg); \ > > +} \ > > + \ > > +static inline u32 name##_readl(struct exynos_ufs *ufs, u32 reg) > \ > > +{ \ > > + return readl(ufs->reg_##name + reg); \ > > +} > > + > > +EXYNOS_UFS_MMIO_FUNC(hci); > > +EXYNOS_UFS_MMIO_FUNC(unipro); > > Please remove this macro magic. Could you explain an intention of this comment? Because this driver needs MMIO-based accesses for multi regions? For example, is this kind of a rule not to use macro? > > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > > index 1332e544da92..1afd5ac9707c 100644 > > --- a/drivers/scsi/ufs/ufshcd.h > > +++ b/drivers/scsi/ufs/ufshcd.h > > @@ -308,6 +308,7 @@ struct ufs_hba_variant_ops { > > int (*setup_clocks)(struct ufs_hba *, bool, > > enum ufs_notify_change_status); > > int (*setup_regulators)(struct ufs_hba *, bool); > > + void (*host_reset)(struct ufs_hba *); > > int (*hce_enable_notify)(struct ufs_hba *, > > enum ufs_notify_change_status); > > int (*link_startup_notify)(struct ufs_hba *, > > New ufs core methods should be added in a separate patch with a good > description, and also with actual callers using them. Okay.
