RE: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support

[Wan Mohamad, Wan Ahmad Zainie]

Hi Vinod.

Thanks for the review.

> -----Original Message-----
> From: Vinod Koul <vkoul@xxxxxxxxxx>
> Sent: Monday, July 13, 2020 1:40 PM
> To: Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
> Cc: kishon@xxxxxx; robh+dt@xxxxxxxxxx; Shevchenko, Andriy
> <andriy.shevchenko@xxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; MP, Sureshkumar
> <sureshkumar.mp@xxxxxxxxx>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@xxxxxxxxx>
> Subject: Re: [PATCH v6 2/2] phy: intel: Add Keem Bay eMMC PHY support
> 
> On 02-07-20, 08:09, Wan Ahmad Zainie wrote:
> > Add support for eMMC PHY on Intel Keem Bay SoC.
> >
> > Signed-off-by: Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
> > ---
> >  drivers/phy/intel/Kconfig            |  12 +
> >  drivers/phy/intel/Makefile           |   1 +
> >  drivers/phy/intel/phy-keembay-emmc.c | 314
> > +++++++++++++++++++++++++++
> >  3 files changed, 327 insertions(+)
> >  create mode 100644 drivers/phy/intel/phy-keembay-emmc.c
> >
> > diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
> > index 7b47682a4e0e..8ddda4fb95d2 100644
> > --- a/drivers/phy/intel/Kconfig
> > +++ b/drivers/phy/intel/Kconfig
> > @@ -22,3 +22,15 @@ config PHY_INTEL_EMMC
> >  	select GENERIC_PHY
> >  	help
> >  	  Enable this to support the Intel EMMC PHY
> > +
> > +config PHY_KEEMBAY_EMMC
> 
> Pls keep this in alphabetical sort

PHY_INTEL_ followed by PHY_KEEMBAY_ is alphabetically sorted.
Could you please help to clarify?

> 
> > +	tristate "Intel Keem Bay EMMC PHY driver"
> > +	depends on ARM64 || COMPILE_TEST
> 
> Intel and ARM64, aha, fun times!

ð

> 
> > +	depends on OF && HAS_IOMEM
> > +	select GENERIC_PHY
> > +	select REGMAP_MMIO
> > +	help
> > +	  Choose this option if you have an Intel Keem Bay SoC.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called phy-keembay-emmc.
> 
> phy-keembay-emmc.ko ?

Is it a must? I saw few Kconfig files omit .ko.
I can fix this in next version.

> 
> > diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
> > index 233d530dadde..6566334e7b77 100644
> > --- a/drivers/phy/intel/Makefile
> > +++ b/drivers/phy/intel/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.o
> >  obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
> > +obj-$(CONFIG_PHY_KEEMBAY_EMMC)		+= phy-keembay-
> emmc.o
> 
> here as well
> 
> > +/* eMMC/SD/SDIO core/phy configuration registers */
> > +#define PHY_CFG_0		0x24
> > +#define  SEL_DLY_TXCLK_MASK	BIT(29)
> > +#define  SEL_DLY_TXCLK(x)	(((x) << 29) & SEL_DLY_TXCLK_MASK)
> > +#define  OTAP_DLY_ENA_MASK	BIT(27)
> > +#define  OTAP_DLY_ENA(x)	(((x) << 27) & OTAP_DLY_ENA_MASK)
> > +#define  OTAP_DLY_SEL_MASK	GENMASK(26, 23)
> > +#define  OTAP_DLY_SEL(x)	(((x) << 23) & OTAP_DLY_SEL_MASK)
> 
> why not a generic helper to do (x) << ffs(reg - 1) & reg ?
> You can skip defining for each register that way!

Is it something like this following?
#define maskval(mask, val)     (((val) << (ffs(mask) - 1)) & mask)

> 
> --
> ~Vinod