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

[Vinod Koul <vkoul@kernel.org>]

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@intel.com>
> ---
>  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

> +	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 ?

> 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!

-- 
~Vinod